GNOME Bugzilla – Bug 730039
rtpjitterbuffer: jitter buffer stalls if first seqnum is different than seqnum-base
Last modified: 2014-05-14 17:41:14 UTC
After, this commit: commit 18b69419fd0a262f4ad2aa80a412a868e67870d6 Author: Wim Taymans <wtaymans@redhat.com> Date: Tue Apr 29 15:29:31 2014 +0200 rtpjitterbuffer: rework packet insert Rework the packet queue so that the most common action (insert a packet at the tail of the queue) goes very fast. Report if a packet was inserted at the head instead of the tail so that we can know when to retry _pop or _peek. If the first received packet is different than the one in seqnum-base, items will not be popped from the jitter buffer. seqnum-base is obtained from RTP-Info (in PLAY response). It could be that RTP-Info contains a different sequence number than the first packet really received.
Created attachment 276416 [details] [review] always push first packet This is just a workaround. It solves the problem, but it's probably not the way to solve this.
Not sure if the following is a realistic case, but I think it could happen. With the new approach, what would happen in this case? 10345 10346 -1 h t 10344 is to be inserted. But because of this: for (; list; list = g_list_previous (list)) { ... if (qitem->seqnum == -1) break; It seems it will be inserted in the tail: 10345 10346 -1 10344 h t instead of: 10344 10345 10346 -1 h t
(In reply to comment #2) > Not sure if the following is a realistic case, but I think it could happen. > With the new approach, what would happen in this case? > > 10345 10346 -1 > h t > > 10344 is to be inserted. But because of this: > > for (; list; list = g_list_previous (list)) { > ... > > if (qitem->seqnum == -1) > break; > > It seems it will be inserted in the tail: > > 10345 10346 -1 10344 > h t > > instead of: > > 10344 10345 10346 -1 > h t It looks like that break; should be a continue;
(In reply to comment #3) > > It looks like that break; should be a continue; the problem with a continue and the new approach is that buffers would be inserted before events. continue was fine before.
(In reply to comment #4) > (In reply to comment #3) > > > > It looks like that break; should be a continue; > > the problem with a continue and the new approach is that buffers would be > inserted before events. continue was fine before. True, it should only move the buffer before the event if there is another buffer before the event that has a larger seqnum.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > > > > It looks like that break; should be a continue; > > > > the problem with a continue and the new approach is that buffers would be > > inserted before events. continue was fine before. > > True, it should only move the buffer before the event if there is another > buffer before the event that has a larger seqnum. Right. I can try to fix this today, unless you are already fixing it. Let me know.
(In reply to comment #6) > Right. I can try to fix this today, unless you are already fixing it. Let me > know. Go ahead and propose a fix.
(In reply to comment #7) > (In reply to comment #6) > > > Right. I can try to fix this today, unless you are already fixing it. Let me > > know. > > Go ahead and propose a fix. Just filed bug 730078 with a patch. Interestingly, my problem mentioned in this bug was fixed too. I was receiving the packet with seqnum-base late, but because this new bug it was never inserted properly. In any case, what happens if packet with seqnum-base is lost?
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > > > Right. I can try to fix this today, unless you are already fixing it. Let me > > > know. > > > > Go ahead and propose a fix. > > Just filed bug 730078 with a patch. > > Interestingly, my problem mentioned in this bug was fixed too. I was receiving > the packet with seqnum-base late, but because this new bug it was never > inserted properly. > > In any case, what happens if packet with seqnum-base is lost? It should probably estimate when it should be received and wait until the last moment before generating a lost-packet event. We don't do anything like this, it would involve estimating a time in the past using seqnum-base+1 timestamp and seqnum-base+2 packet spacing. not sure it is worth it. What happens now is that seqnum-base+1 is delayed as much as possible (waiting for seqnum-base) but there is never a lost-packet event generated for seqnum-base.
(In reply to comment #9) > > > > In any case, what happens if packet with seqnum-base is lost? > > It should probably estimate when it should be received and wait until the last > moment before > generating a lost-packet event. > > We don't do anything like this, it would involve estimating a time in the past > using seqnum-base+1 > timestamp and seqnum-base+2 packet spacing. not sure it is worth it. What > happens now is that > seqnum-base+1 is delayed as much as possible (waiting for seqnum-base) but > there is never a lost-packet > event generated for seqnum-base. Right. Thanks for the comment. I just verified that this bug is not an issue anymore and that it was a consequence of bug 730078. I verfied it by sending a sequence number a bit lower than the first packet in RTP-Info (gst-rtsp-server/rtsp-stream-transport.c::gst_rtsp_stream_transport_get_rtpinfo).