After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 729524 - rtpjitterbuffer: if retransmissions enabled, a gap larger than the latency can result in a stuck jitterbuffer
rtpjitterbuffer: if retransmissions enabled, a gap larger than the latency ca...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal normal
: 1.3.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-04 17:22 UTC by Jason Litzinger
Modified: 2014-05-09 16:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test case to demonstrate issue. (5.84 KB, patch)
2014-05-04 17:22 UTC, Jason Litzinger
committed Details | Review
An example fix, only tested via the unit test. (932 bytes, patch)
2014-05-04 17:23 UTC, Jason Litzinger
none Details | Review

Description Jason Litzinger 2014-05-04 17:22:30 UTC
Created attachment 275833 [details] [review]
Test case to demonstrate issue.

Observed in git master and the 1.2 branch.

Test case and sample fix patches attached.  A description of the steps that trigger the stuck buffer are:

1.  seqnum 0 arrives, seqnum 1 expected
2.  seqnum 1 arrives, seqnum 2 expected
3.  Data stops arriving, RTX requests sent for seqnum 2 until it is finally declared lost.
4.  lost event for seqnum 2 is pushed, incrementing next_seqnumi to 3.
5.  Seqnum 16 arrives, packets 2->6 are part of a single "lost" event.
6.  RTX packets begin arriving with seqnum 8
7.  Lost event from 5 is popped to be pushed, but seqnum 2 < next_seqnum (3), so it is dropped.
8.  jbuf is now stalled forever. 

The problem is that for large gaps, calculate_expected creates the "bulk" (for lack of a better word) lost event starting at a seqnum that has already been declared lost.  This doesn't happen if RTX is disabled, but will stall the buffer if enabled.
Comment 1 Jason Litzinger 2014-05-04 17:23:08 UTC
Created attachment 275834 [details] [review]
An example fix, only tested via the unit test.
Comment 2 Jason Litzinger 2014-05-04 17:53:36 UTC
For the attached proposed fix, the reason that the do_retransmissions check is performed is to preserve backward compatibility for non RTX enabled cases.  If the old behavior is wrong, or this preservation is not necessary, then the following test cases will need updates:

*  test_only_one_lost_event_on_large_gaps 
*  test_late_packets_still_makes_lost_events
Comment 3 Wim Taymans 2014-05-09 16:12:03 UTC
I fixed it slightly differently, the real error was that we still were expecting a packet for which we produced Lost events.

commit b2e1598e4abba7beba37e4c43e32598408e82663
Author: Wim Taymans <wtaymans@redhat.com>
Date:   Fri May 9 18:01:28 2014 +0200

    rtpjitterbuffer: increment accepted packets after loss
    
    When we detect a lost packet, expect packets with higher
    seqnum on the input.
    
    Also update the unit test.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=729524