[Asterisk-code-review] res rtp asterisk: Add support for sending NACK requests. (asterisk[master])

[Matthew Fredrickson]

Matthew Fredrickson has posted comments on this change. ( https://gerrit.asterisk.org/9225 )

Change subject: res_rtp_asterisk: Add support for sending NACK requests.
......................................................................


Patch Set 4: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/9225/4/main/data_buffer.c
File main/data_buffer.c:

https://gerrit.asterisk.org/#/c/9225/4/main/data_buffer.c@297
PS4, Line 297: 			if (buffer->cache_count < CACHED_PAYLOAD_MAX
Since it looks like you're doing the cache population with the same code twice (both in ast_data_buffer_remove and ast_data_buffer_remove_head), would it be prudent to make it a function?


https://gerrit.asterisk.org/#/c/9225/4/main/data_buffer.c@324
PS4, Line 324: 		if (buffer->cache_count < CACHED_PAYLOAD_MAX
Same here - unless I read it wrong, the next few lines of adding an item to the cached_payloads list are the same between this and the above function.


https://gerrit.asterisk.org/#/c/9225/4/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/#/c/9225/4/res/res_rtp_asterisk.c@4402
PS4, Line 4402: 	if (res == 0 || res == 1) {
This is probably silly, but unless we're actually using both failure values, I'd prefer to stick to something simpler as far as failure response.  So return 0 for both failure conditions, otherwise res > 0 is success.  If you want to qualify between both types of errors, you could add an ast_debug message at error time within ast_rtcp_generate_report or ast_rtcp_generate_sdes


https://gerrit.asterisk.org/#/c/9225/4/res/res_rtp_asterisk.c@4411
PS4, Line 4411: 	if (res == 0 || res == 1) {
Same here.


https://gerrit.asterisk.org/#/c/9225/4/res/res_rtp_asterisk.c@4419
PS4, Line 4419: 		ast_rtp_instance_get_remote_address(instance, &remote_address);
Why do we have to update remote_address here?



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idab644b08a1593659c92cda64132ccc203fe991d
Gerrit-Change-Number: 9225
Gerrit-PatchSet: 4
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 11:04:20 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180628/af4001c6/attachment.html>