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

[Benjamin Keith Ford]

Benjamin Keith Ford 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:

(3 comments)

Open to suggestions on all of these, wanted to discuss here in case anyone felt strongly one way or another.

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@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 
My idea to work around this is to pull everything from the buffer, queue it like we would if we had hit the buffer max, and then add the frame we received to the end and pick up from there. That way, if it was a cycle, we get those frames we had stored and it should be like a seamless transition, and if it was a gap, it will play the catch up game and then resume video in the present state.


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 
For this one I have a couple different ideas. The first idea is queuing a &ast_null_frame (if that can be inserted into the list). The second idea is doing similar to what I proposed for the above case, going through buffer and queuing frames, then adding this frame in the appropriate spot, but using rtp->f to retain the data rather than duping it, since we will be using it now rather than later. Personally I like the second idea more, since the first could maybe result in many null frames being passed through rather than video, and I think we would all rather have video than a null frame.


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?
I've got a decent little scenario I added to this comment that should better explain how this could happen, dealing with very, very out of order packets :)



-- 
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 19:57:26 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180621/bb0f6b7b/attachment.html>