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

[Lorenzo Miniero]

Lorenzo Miniero has posted comments on this change. ( https://gerrit.asterisk.org/4521 )

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


Patch Set 2:

(8 comments)

> (9 comments)

Just addressed all comments (sorry if this took a while). How should I push the changes after I make them in order to avoid the mess I caused a couple of days ago? Will a

   git review -d 4521

ensure I don't cause trouble? Thanks.

https://gerrit.asterisk.org/#/c/4521/2/codecs/codec_speex.c
File codecs/codec_speex.c:

Line 101: 	int quality, default_quality;
> These should go on separate lines, we don't combine variables like this.
Ack.


Line 365: 	if(rtcp_report->reception_report_count > 0) {
> You can get rid of this entire if block by just returning early if it's 0
Ok, will do.


Line 369: 			int percent = (fraction_lost*100)/256;
> Explain this calculation with a comment.
That's derived from the format of fraction lost in report blocks (RFC3550), I'll add the comment.


PS2, Line 372: 			ast_verb(3, "Fraction lost changed: %d --> %d percent loss\n", fraction_lost, percent);
             : 			/* Handle change */
             : 			speex_encoder_ctl(tmp->speex, SPEEX_GET_BITRATE, &bitrate);
             : 			ast_verb(3, "Current bitrate: %d\n", bitrate);
             : 			ast_verb(3, "Current quality: %d/%d\n", tmp->quality, tmp->default_quality);
> I think these verbose messages are useful for testing but shouldn't be in a
Yes, I only added them to be sure I was doing the right thing. Is there any way I can define them as optional debug messages instead, and in case how can I enable them for testing?


Line 377: 			/* FIXME BADLY Very ugly example of how this could be handled: probably sucks */
> I'm not versed enough really to know if it's bad or not but for the purpose
It's a very dumb approach, as I believe an effective way of handling feedback should actually take into account a bit of history, if things are improving, getting worse, remaining steady, etc., all things that may lead to a different decision. I really only wanted to showcase a very simple logic that could be used as a result of incoming feedback, as the availability of such feedback is the main purpose of this patch.

Ack on the space for ifs, whiles, etc. (I personally never do that and that's why they're missing, sorry about that).


https://gerrit.asterisk.org/#/c/4521/2/main/channel.c
File main/channel.c:

Line 4359: 			if(ast_channel_writetrans(chan))
> Space after if and enclose in { }
Ack.


Line 4360: 				ast_translate(ast_channel_writetrans(chan), f, 0);
> It should be documented in translate.h that ast_translate can now accept th
Will do.


https://gerrit.asterisk.org/#/c/4521/2/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

Line 4334: 				report_block = rtp->f.data.ptr + rtp->f.datalen + sizeof(struct ast_rtp_rtcp_report_block *);
> I don't understand why this adds sizeof(struct ast_rtp_rtcp_report_block *)
I think it was because from my tests sizeof(struct ast_rtp_rtcp_report) would not include the size of the pointer to ast_rtp_rtcp_report_block (possibly because, sinze report blocks are optional, the array size is 0):

    struct ast_rtp_rtcp_report_block *report_block[0];

Since we do have a report block to copy, and that the property in ast_rtp_rtcp_report is a pointer and not the struct itself, we reserve space for the pointer, copy the data after that, and then set the pointer to where we just copied the data. A bit convoluted but I couldn't find any other way to share this object as a pointer instead.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd6aa77fb4a7ff546c6025900fc2baf332c31857
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Lorenzo Miniero <lminiero at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Lorenzo Miniero <lminiero at gmail.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes