[Asterisk-code-review] rtp: Add support for transport-cc in receiver direction. (...asterisk[16])

[Kevin Harwell]

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

Change subject: rtp: Add support for transport-cc in receiver direction.
......................................................................


Patch Set 1: Code-Review-1

(5 comments)

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

https://gerrit.asterisk.org/#/c/11315/1/res/res_rtp_asterisk.c@6357 
PS1, Line 6357: 	*status_vector_chunk |= (status << (16 - 2 - (14 - *status_vector_chunk_bits)));
Can you add a comment clarifying this calculation more? I'm sure it works, but at quick glance it seems like it bypasses the first two bits of the chunk minus the type and status/symbol size. I could be getting calc order precedence wrong too, so maybe more parens to make it more explicit/obvious?


https://gerrit.asterisk.org/#/c/11315/1/res/res_rtp_asterisk.c@6366 
PS1, Line 6366: 	*status_vector_chunk_bits = 14;
              : 	*status_vector_chunk = (1 << 15) | (1 << 14);
I know this is in the draft, but maybe a small comment about the the magic number here, and why it's init the way it is. Or can put comment below in "...._feedback_produce" where they are also initialized.


https://gerrit.asterisk.org/#/c/11315/1/res/res_rtp_asterisk.c@6451 
PS1, Line 6451: 	 * vector chunk or a run length chunk. The code tries to be as efficient as people to
s/people/possible


https://gerrit.asterisk.org/#/c/11315/1/res/res_rtp_asterisk.c@6526 
PS1, Line 6526:                 put_unaligned_uint16(rtcpheader + packet_len, htons((0 << 15) | (run_length_chunk_status << 13) | run_length_chunk_count));
This line begins with spaces instead of tabs.


https://gerrit.asterisk.org/#/c/11315/1/res/res_rtp_asterisk.c@6600 
PS1, Line 6600: 
Is the "instance" already locked or does the transport need to be locked, so there is not collision when reading (above) vs. writing (here) to the vector?



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11315
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I654a2cff5bd5554ab94457a14f70adb71f574afc
Gerrit-Change-Number: 11315
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 30 Apr 2019 20:14:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190430/ad571c7a/attachment.html>