[Asterisk-code-review] media: Add experimental support for RTCP feedback. (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/4614 )

Change subject: media: Add experimental support for RTCP feedback.
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

> Just as a comment, before merging you may want to hide the part that modifies the Speex bitrate according to the feedback behind a configuration option or something like this, so that only users interested in experimenting with it are affected. In fact, as explained the existing logic is very basic and untested, and only there as a placeholder example for more effective logics that may be implemented instead.

I can't speak much to the whether the logic is correct with regards to the bitrate, but if you feel it is potentially wrong and hasn't been tested I'd say either remove it or I'd be fine with wrapping it in an #ifdef 0. Or maybe better would be to comment the code along with an explanation.

https://gerrit.asterisk.org/#/c/4614/1/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

PS1, Line 4334: 				memcpy(report_block, rtcp_report->report_block[0], sizeof(struct ast_rtp_rtcp_report_block));
Although it appears we are not currently handling more than one report at this time, should this be "rtcp_report->report_block[report_counter - 1]" for if/when we do?


PS1, Line 4335: 				rtcp_report2 = (struct ast_rtp_rtcp_report *)rtp->f.data.ptr;
              : 				rtcp_report2->report_block[0] = report_block;
Is this code needed (I could be misreading it)? It appears to get the rtcp_report that was stored and then set the first report block, but hasn't this already been done above when the block was originally built?


-- 
To view, visit https://gerrit.asterisk.org/4614
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd6aa77fb4a7ff546c6025900fc2baf332c31857
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Lorenzo Miniero <lminiero at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Lorenzo Miniero <lminiero at gmail.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-HasComments: Yes