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 749536 - rtspsrc: handle gap in tcp mode
rtspsrc: handle gap in tcp mode
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-18 08:39 UTC by Nicola
Modified: 2015-08-16 13:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
sample pcap that show gap in sequence number (1.34 MB, application/octet-stream)
2015-05-18 08:39 UTC, Nicola
  Details
sample file produced by gstreamer 1.4 (377.77 KB, video/x-matroska)
2015-05-25 12:26 UTC, Nicola
  Details
traffic dump while receving rtsp over tcp in low bandwidth condition (641.87 KB, application/octet-stream)
2015-05-25 12:29 UTC, Nicola
  Details
sample file produced using rtsp over udp in the same network conditions (637.36 KB, video/x-matroska)
2015-05-25 12:52 UTC, Nicola
  Details
rtpjitterbuffer: Calculate DTS from the clock if we had none for the first packet after a reset (1.62 KB, patch)
2015-07-08 16:59 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Nicola 2015-05-18 08:39:06 UTC
Created attachment 303507 [details]
sample pcap that show gap in sequence number

In tcp mode several cameras can generate gap in sequence number for example if there is no enough bandwidth between client and server (in this scenario udp is useless, almost all packets are late, with tcp you can at least receive some seconds, than a gap and then other few seconds and so on).

GStreamer should properly handle the gap in tcp mode (based on rtptime???)
Comment 1 Sebastian Dröge (slomo) 2015-05-18 08:45:02 UTC
See what I wrote on the mailing list about this. RTP timestamps are not sufficient here, what is needed is the capture times together with the RTP timestamps. And we don't have the capture times for TCP except for the very first packet.
Comment 2 Nicola 2015-05-25 12:26:32 UTC
Created attachment 303917 [details]
sample file produced by gstreamer 1.4

I limited the bandwidth on my pc using wondershaper to 256kbps

this is a sample file captured with gstreamer 1.4 and this pipeline:

rtspsrc location="rtspt:...." ! rtph264depay ! h264parse ! matroskamux streamable=true ! filesink location=/tmp/test.mkv

please take a look at the date/time overlay and the timestamps inside the file. gstreamer 1.4 seems to handle gap in tcp mode correctly.

With git master the pipeline fails after few seconds with the error

Received packet without DTS after a gap

this seems cleary a regression
Comment 3 Nicola 2015-05-25 12:29:07 UTC
Created attachment 303918 [details]
traffic dump while receving rtsp over tcp in low bandwidth condition
Comment 4 Nicola 2015-05-25 12:52:52 UTC
Created attachment 303920 [details]
sample file produced using rtsp over udp in the same network conditions

this file show that udp basically does not work in low bandwith conditions (same results with 1.4 and git master).
Comment 5 Nicola 2015-05-25 13:25:58 UTC
reverting this patch

http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=87c8c163a876f45024a817e17af6dfdc52e71257

seems to restore the 1.4 behaviour
Comment 6 Sebastian Dröge (slomo) 2015-05-25 14:47:14 UTC
(In reply to Nicola from comment #5)
> reverting this patch
> 
> http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/
> ?id=87c8c163a876f45024a817e17af6dfdc52e71257
> 
> seems to restore the 1.4 behaviour

Obviously :P That's the change that introduces this error.


The problem is that without this change, the calculations later will run into overflows/underflows and calculate things with GST_CLOCK_TIME_NONE. Just reverting this is not going to help much, other than hiding the problem for the cases where the wrong calculations don't hurt... and causing hard to debug problems otherwise (like completely wrong timestamps without an error anywhere).


The real fix for this would be to somehow get the capture time of the RTP packet after the gap, which we currently can't for TCP (we only set a timestamp on the very first packet).
Comment 7 Nicola 2015-07-02 22:10:46 UTC
just in case someone want to reproduce the error here is a public rtsp url with bandwidth limited to 512kbps, you can try this pipeline:


gst-launch-1.0 -v rtspsrc location="rtspt://service:password@93.63.189.11/rtsp_tunnel?h26x=4&line=1&inst=1" ! fakesink silent=false

it fails after few seconds with "Received packet without DTS after a gap" error, 

additionally there is also another new critical error


(gst-launch-1.0:4248): GStreamer-CRITICAL **: 
Trying to dispose element pipeline0, but it is in PLAYING instead of the NULL state.
You need to explicitly set elements to the NULL state before
dropping the final reference, to allow them to clean up.
This problem may also be caused by a refcounting bug in the
application or some element.
Comment 8 Sebastian Dröge (slomo) 2015-07-08 08:57:08 UTC
I can confirm that this stream reproduces the problem. We could maybe use the previous packet spacing to estimate a timestamp
Comment 9 Sebastian Dröge (slomo) 2015-07-08 12:15:40 UTC
commit 243730ced44e61997f5338007ccc3c3dae6f337a
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Jul 8 15:13:17 2015 +0300

    rtpjitterbuffer: Handle seqnum gaps in TCP streams without erroring out or overflowing calculations
    
    That is, handle DTS==GST_CLOCK_TIME_NONE correctly.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749536
Comment 10 Nicola 2015-07-08 12:39:13 UTC
after this patch seems that dts and pts both none after a gap, so basically you cannot save the stream in a container ecc..
Comment 11 Sebastian Dröge (slomo) 2015-07-08 14:04:11 UTC
commit 4e23481d9fd09863be91cb88d1d3d33760ed8fad
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Jul 8 17:02:05 2015 +0300

    rtpjitterbuffer: Calculate receive time if we don't have any
    
    This is required to properly schedule packet loss timers and make
    sure all our calculations work properly.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749536
Comment 12 Nicola 2015-07-08 14:36:08 UTC
it is better now, thanks!

However still happen "sometime" to have buffer without timestamp

/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1923 bytes, dts: 0:00:21.328776087, pts: 0:00:21.328776087, duration: none, offset: -1, offset_end: -1, flags: 00002000 delta-unit ) 0x7f79d8064bd0
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (2033 bytes, dts: 0:00:21.362153864, pts: 0:00:21.362153864, duration: none, offset: -1, offset_end: -1, flags: 00002000 delta-unit ) 0x7f79c40065b0
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (3236 bytes, dts: 0:00:21.395531642, pts: 0:00:21.395531642, duration: none, offset: -1, offset_end: -1, flags: 00002000 delta-unit ) 0x7f79d8057cd0
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (919 bytes, dts: none, pts: none, duration: none, offset: -1, offset_end: -1, flags: 00002040 discont delta-unit ) 0x7f79d8077410
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1047 bytes, dts: none, pts: none, duration: none, offset: -1, offset_end: -1, flags: 00002000 delta-unit ) 0x7f79d8057cd0
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1462 bytes, dts: none, pts: none, duration: none, offset: -1, offset_end: -1, flags: 00002000 delta-unit ) 0x7f79d80771f0
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (2477 bytes, dts: none, pts: none, duration: none, offset: -1, offset_end: -1, flags: 00002000 delta-unit ) 0x7f79d809b120
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (2629 bytes, dts: none, pts: none, duration: none, offset: -1, offset_end: -1, flags: 00002000 delta-unit ) 0x7f79d807d240

and after that all buffers will have no timestamps
Comment 13 Sebastian Dröge (slomo) 2015-07-08 14:43:51 UTC
Can you get me a debug log with GST_DEBUG=2,rtpjitterbufer:6 ?
Comment 14 Nicola 2015-07-08 15:27:16 UTC
here is the log

0:00:12.484344491  8052      0x1cc2590 WARN               rtpsource rtpsource.c:1120:update_receiver_stats: unacceptable seqnum received

this happen with the provided test pipeline if you keep it running for a minute or so
Comment 15 Nicola 2015-07-08 15:34:25 UTC
Here is a proper log file (there is a typo in rtpjitterbuffer log directive above)

http://195.250.34.59/temp/749536.log.tar.bz2
Comment 16 Sebastian Dröge (slomo) 2015-07-08 15:45:06 UTC
Ok, that means that we got a discont of more than 3000 seqnums, which then causes everything to reset... not sure what to do about that one
Comment 17 Nicola 2015-07-08 15:51:57 UTC
(In reply to Sebastian Dröge (slomo) from comment #16)
> Ok, that means that we got a discont of more than 3000 seqnums, which then
> causes everything to reset... not sure what to do about that one

maybe we can reset the timestamp to something like:

last_timestamp + elapsed time from last received timestamp,

what do you think about?

Sending buffer without ts should be avoided, for example matroskamux will silently discard all buffers whitout timestamp, I don't think that existing apps handle this case (at least not mine)
Comment 18 Sebastian Dröge (slomo) 2015-07-08 16:09:08 UTC
Well, that would be one option, yes. Can you check how well that works? You could get the current clock time minus base time in gstrtpjitterbuffer.c in the "First buffer" case in the chain function if dts == NONE.
Comment 19 Sebastian Dröge (slomo) 2015-07-08 16:11:42 UTC
And with base time I mean gst_element_get_base_time().
Comment 20 Nicola 2015-07-08 16:12:53 UTC
Ok, I'll read the code and try to write a patch tomorrow, thanks!
Comment 21 Sebastian Dröge (slomo) 2015-07-08 16:59:53 UTC
Created attachment 307098 [details] [review]
rtpjitterbuffer: Calculate DTS from the clock if we had none for the first packet after a reset
Comment 22 Sebastian Dröge (slomo) 2015-07-08 17:03:06 UTC
Seems to work here, let me know if it also works for you. This stream is borderline broken though :)
Comment 23 Nicola 2015-07-08 20:18:10 UTC
Thanks!

Here is a file produced with rtspsrc ! rtph264depay ! h264parse ! matroskamux ! filesink

http://195.250.34.59/temp/749536.mkv

duration is more than one hour, if you play it with totem and compare the running time with the video overlay date/time is quite good!
Comment 24 Sebastian Dröge (slomo) 2015-07-08 20:26:29 UTC
Thanks for confirming!

commit 6e7c724afadd6a4d04d6e68938d314e1be570f9e
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Jul 8 19:59:13 2015 +0300

    rtpjitterbuffer: Calculate DTS from the clock if we had none for the first packet after a reset
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749536
Comment 25 Sebastian Dröge (slomo) 2015-07-09 15:53:21 UTC
That's actually not complete yet, going to fix that now.
Comment 26 Nicola 2015-07-09 20:26:19 UTC
ok, the test stream is available again if you need it
Comment 27 Sebastian Dröge (slomo) 2015-07-09 21:14:07 UTC
commit ae8acc0973a3da6546c2158ef8f61e3450155e4c
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Jul 9 23:59:10 2015 +0300

    rtpjitterbuffer: Always estimate DTS from the current clock time
    
    Estimating it from the RTP time will give us the PTS, so in cases of PTS!=DTS
    we would produce wrong DTS. As now the estimated DTS is based on the clock,
    don't store it in the jitterbuffer items as it would otherwise be used in the
    skew calculations and would influence the results. We only really need the DTS
    for timer calculations.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749536
Comment 28 Nicola 2015-07-15 15:33:29 UTC
the latest patch seems to cause buffers without timestamp, you can reproduce with the public stream (please keep running for some minutes)

gst-launch-1.0 -v rtspsrc location="rtspt://service:password@93.63.189.11/rtsp_tunnel?h26x=4&line=1&inst=1" ! fakesink silent=false
Comment 29 Tim-Philipp Müller 2015-08-10 13:46:51 UTC
I think this particular bug is solved.

The fact that buffers don't always have timestamps is not necessarily incorrect, we might not be able to make up timestamps for all buffers. And I don't think this is a regression either, but could have happened before too?

My guess is that your issue from comment #28 is basically bug #659489. In any case, it should probably be handled in a different bug.
Comment 30 Nicola 2015-08-10 15:52:58 UTC
(In reply to Tim-Philipp Müller from comment #29)
> I think this particular bug is solved.
> 
> The fact that buffers don't always have timestamps is not necessarily
> incorrect, we might not be able to make up timestamps for all buffers. And I
> don't think this is a regression either, but could have happened before too?
> 

in 1.4 seems to not happen

> My guess is that your issue from comment #28 is basically bug #659489. In
> any case, it should probably be handled in a different bug.

seems something different since it happen with the pipeline in comment 28: no h264parse

in my app I use the patches from this bug but not the latest one and I have no buffers without timestamp
Comment 31 Nicola 2015-08-11 07:08:14 UTC
(In reply to Tim-Philipp Müller from comment #29)
> I think this particular bug is solved.
> 
> The fact that buffers don't always have timestamps is not necessarily
> incorrect, we might not be able to make up timestamps for all buffers. And I
> don't think this is a regression either, but could have happened before too?
> 
> My guess is that your issue from comment #28 is basically bug #659489. In
> any case, it should probably be handled in a different bug.

To be more clear:
1) the problem does not happen using the patch in comment 21 but not the one in comment 27. 
2) the problem happen applying the patch in comment 27 too
3) With the patch in comment 27 all buffers will have no timestamp after some time and not only some buffer 
4) no h264parse here only rtspsrc ! fakesink
Comment 32 Sebastian Dröge (slomo) 2015-08-11 10:06:37 UTC
Ok, so we'll have to find yet another solution for this.
Comment 33 Nicola 2015-08-12 07:47:56 UTC
(In reply to Sebastian Dröge (slomo) from comment #32)
> Ok, so we'll have to find yet another solution for this.

what's wrong in reverting patch in comment 27 and keep only the one in comment 21? Is what I'm using and it seems to work fine
Comment 34 Sebastian Dröge (slomo) 2015-08-12 08:10:35 UTC
It's explained in the commit message.
Comment 35 Sebastian Dröge (slomo) 2015-08-13 11:25:42 UTC
I can confirm that your stream ends up without PTS after a few minutes
Comment 36 Sebastian Dröge (slomo) 2015-08-13 14:45:59 UTC
commit 68a9209408c51d822d2a28239a86d6311a4bbd22
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Aug 13 16:32:55 2015 +0200

    rtpjitterbuffer: Keep the DTS estimate if we got no DTS after a jitterbuffer reset
    
    Otherwise we will just output buffers without timestamps after a reset if no
    timestamps are provided by upstream, e.g. when using RTSP over TCP.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749536