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

[Joshua Colp]

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

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


Patch Set 2: Code-Review-1

(9 comments)

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.


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


Line 368: 		if(fraction_lost != tmp->fraction_lost) {
Same here.


Line 369: 			int percent = (fraction_lost*100)/256;
Explain this calculation with a 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 final patch, maybe as debug if we can have an identifier so we can track different instances of the transcoding path


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 purposes of testing it works.

Just as an fyi - for if statements like this our coding guidelines go for a space after the if like:

if (percent < 10) {


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 { }


Line 4360: 				ast_translate(ast_channel_writetrans(chan), f, 0);
It should be documented in translate.h that ast_translate can now accept the AST_FRAME_RTCP type.


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 *)

Can't you just start at rtp->f.data.ptr + rtp->f.datalen since that will be at the precise end of ast_rtp_rtcp_report?


-- 
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: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes