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

[Joshua Colp]

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

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


Patch Set 2: Code-Review-1

(5 comments)

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

https://gerrit.asterisk.org/#/c/9224/2/res/res_rtp_asterisk.c@3626
PS2, Line 3626: 	rtp->expectedrxseqno = -1;
If we're told that the source has changed (ast_rtp_change_source) we should probably reset this to -1 as well since they likely won't follow the same seqnos.


https://gerrit.asterisk.org/#/c/9224/2/res/res_rtp_asterisk.c@6749
PS2, Line 6749: 				/* Define our starting point, or recover from a large gap of lost packets */
If that happened for the large gap case is it possible that there would be payloads still in the data buffer?


https://gerrit.asterisk.org/#/c/9224/2/res/res_rtp_asterisk.c@6777
PS2, Line 6777: 					ast_data_buffer_put(rtp->recv_buffer, seqno, f);
What happens if we can't dupe the frame here? Will things pile up until we hit the max data buffer size and then it'll all get flushed? (Since this is a packet we would never get again)


https://gerrit.asterisk.org/#/c/9224/2/res/res_rtp_asterisk.c@6785
PS2, Line 6785: 						/* We don't want missing sequence number duplicates */
Can you extend the comment to explain the scenario where this would happen?


https://gerrit.asterisk.org/#/c/9224/2/res/res_rtp_asterisk.c@6844
PS2, Line 6844: 					/* Things are a little out of hand. Salvage what we can and start fresh */
If this is carrying video (as it always will right now) it may be wise to preemptively call rtp_write_rtcp_fir - this will tell the remote side to send a full frame, allowing video to be restored faster in the case of this messy situation.



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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: Idab644b08a1593659c92cda64132ccc203fe991d
Gerrit-Change-Number: 9224
Gerrit-PatchSet: 2
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 10:09:31 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180621/2fc40aaf/attachment.html>