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 725159 - rtpjitterbuffer: RTP sequence number rollover problems
rtpjitterbuffer: RTP sequence number rollover problems
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.2.3
Other Windows
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-02-25 15:23 UTC by Jake Foytik
Modified: 2014-02-26 20:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
changes made to rtpjitterbuffer (1.26 KB, patch)
2014-02-25 15:23 UTC, Jake Foytik
needs-work Details | Review
Updated patch (2.06 KB, patch)
2014-02-26 12:44 UTC, Jake Foytik
committed Details | Review

Description Jake Foytik 2014-02-25 15:23:56 UTC
Created attachment 270285 [details] [review]
changes made to rtpjitterbuffer

Test Environment : 
 - gstreamer 1.2.3 
 - Windows
 - Pipeline : rtspsrc ! fakesink
 - Pulling an Uncompressed MJPEG stream from an Axis Camera at 30 fps which results to approximately 2700 RTP packets per second.

Symptom :
The rtpjitterbuffer will occasionally enter a state where it is unable to push new buffers to downstream elements. The rtpjitterbuffer::handle_next_buffer() function will repeatedly post the debug message "Sequence number GAP detected : expected .... instead of ...." This eventually results in "Packet too late" errors in the rtpjitterbuffer:gst_rtp_jitter_buffer_chain() function.
The problem appears to occur when a packet is received ahead of what is expected and at the RTP sequence number rollover point. (eg. expected seqnum = 65533, received seqnum = 2). In this case, the rtpjitterbuffer:calculate_expected() function is called and should create timers for the expected arrival of packets 65533, 65534, 65535, 0, and 1. However, in the rtpjitterbuffer:calculate_expected() function, the while loop that adds these timers uses the condition : 
 while(expected < seqnum)
which does not account for the rollover of the RTP sequence number.

Suggested Changes:
In gst-plugins-good/gst/rtpmanager/gstrtpjitterbuffer.c
Change all raw comparisons of RTP sequence numbers to use the gst_rtp_buffer_compare_seqnum() function. I found 3 lines where this was happening. See attached patch.
Comment 1 Sebastian Dröge (slomo) 2014-02-26 08:39:10 UTC
Review of attachment 270285 [details] [review]:

Can you attach this as a "git format-patch" style patch?

::: gstrtpjitterbuffer.c.orig
@@ +2868,3 @@
         /* earlier timer */
         save_best = TRUE;
+	  } else if (test_timeout == timer_timeout && (gst_rtp_buffer_compare_seqnum (test->seqnum < timer->seqnum) > 0)) {

Here is a typo, the < should be a comma.
Comment 2 Jake Foytik 2014-02-26 12:44:53 UTC
Created attachment 270379 [details] [review]
Updated patch

Fixed the typo in the original patch, and used "git format-patch" this time.
Comment 3 Sebastian Dröge (slomo) 2014-02-26 20:12:38 UTC
Thanks for the patch :)

commit 6dd9142592475e6bbe56a5c07e81c14412ae18f1
Author: Jake Foytik <jake.foytik@ipconfigure.com>
Date:   Wed Feb 26 07:32:32 2014 -0500

    rtpjitterbuffer: Remove raw comparisons of RTP sequence numbers
    
    Several conditional statements perform comparison on RTP sequence
    numbers without taking the sequence number rollover into account.
    Instead, use the gst_rtp_buffer_compare_seqnum function to perform the
    comparison.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=725159