GNOME Bugzilla – Bug 744587
queue2: incorrect current-level-time reported after seek (with packetized data)
Last modified: 2018-11-03 12:25:49 UTC
Created attachment 296913 [details] [review] The testcase Steps to reproduce the bug: 1) Have a queue2 element, 2) Send SEGMENT (GST_FORMAT_TIME) event with non-zero start/position/time. 3) Start pushing buffers with PTSes starting at 0 to the queue. Effects: 1) As soon the queue obtains buffers with valid in-segment PTS, it reports buffered time level correctly, 2) As soon the queue pushes first buffer with PTS < segment.start to the srcpad, the queue stops reporting buffered time level correctly, it reports 0 buffered. This is caused by rewriting src_segment.position with buffer's PTS, and then gst_segment_to_running_time() returns GST_CLOCK_TIME_NONE. At this point, if we have a very fast source element, and a very slow decoder, the decoder may block on the first frame (with PTS below segment start) for quite a long time (especially if it's a software decoder decoding 1080p HEVC or VP9 frame). In this time, the queue reports current-level-time == 0, and what's worse, if the queue has buffer & byte limits 0, and only the time limit set, fast source may push megabytes of data, tens of seconds of data, even though the limit may be 0.5 seconds. The queue will not block, as it thinks it's 0% filled. Additional bug (fixed by the same fix): some streams have non-monotonic PTSes. Push buffer with PTS=1s, duration=1s, and then push buffer with PTS=0s, duration=1s. The queue has now 2 seconds of data buffered, but it will report only one second. I am attaching the extended unit tests for queue2 element that demonstrate both these problems.
Created attachment 296914 [details] [review] The fix The fix I made works in positive playback rates. As for playing backwards, I'm not sure if that's not a special case that would have to be handled differently, my segment-fu is too low.
Will this be taken into consideration for 1.6.0 release?
This bug still reproduces on GStreamer 1.6.0 (checked by running unit tests that I've attached in the first comment).
The attached patch may need reworking, as on 1.6.x using gst_segment_to_running_time_full() may be a better option.
Created attachment 314225 [details] [review] The corrected fix and testcase The corrected fix makes use of gst_segment_to_running_time_full(). It still fixes out-of-segment PTSes, and also non-monotonic PTSes. It still breaks negative playback rates.
Would you be willing to write a test that shows the problem with negative playback? That would make it easier to understand why it causes problems in that case. Also, please attach patches generated with git format-patch so that we retain your authorship and message in commits.
Is this still an issue with this commit (in core since yesterday) ? commit a9c923d585affc85262e41b4790d378294706d7a Author: Edward Hervey <bilboed@bilboed.com> Date: Tue Oct 27 08:48:07 2015 +0100 queue/queue2: Use GST_BUFFER_DTS_OR_PTS The input of queue/queue2 might have DTS set, in which cas we want to take that into account (instead of the PTS) to calculate position and queue levels. https://bugzilla.gnome.org/show_bug.cgi?id=756507
Created attachment 314284 [details] [review] The testcase and the fix Attaching the git format-patch patch.
Created attachment 314286 [details] [review] TC and fix for the negative playback rates Attaching the TC and fix for negative playback rates. It's a separate issue: when we're playing segment with rate=-1.0, start=0, stop=GST_SECOND, and we feed buffers with timestamps from 2*GST_SECOND to 0 (reverse order), then the queue will report 0.9 seconds buffered, because it calculates the sinktime basing on buffer's PTS+DURATION. In case of negative rates, just the PTS should be used.
(In reply to Edward Hervey from comment #7) > Is this still an issue with this commit (in core since yesterday) ? > > commit a9c923d585affc85262e41b4790d378294706d7a > Author: Edward Hervey <bilboed@bilboed.com> > Date: Tue Oct 27 08:48:07 2015 +0100 > > queue/queue2: Use GST_BUFFER_DTS_OR_PTS > > The input of queue/queue2 might have DTS set, in which cas we want > to take that into account (instead of the PTS) to calculate position > and queue levels. > > https://bugzilla.gnome.org/show_bug.cgi?id=756507 I think we've sorted this out on IRC, just leaving a comment for other readers, for consistency. The root cause is that the timestamp is before the segment start, it would reproducealso when DTSes are used. I've tested it today with a master branch containing your fix. It still fails, although probably the issue with non-monotonic timestamps would not reproduce if monotonic DTSes are present.
I was discussing this yesterday, and I'm thinking that with negative rate, the queue won't report the proper duration. Specially with video data, the reason is that in reverse playback, we push GOP in reverse order, with buffer inside in the normal order (when encoded). So for GOP of 3 seconds and 9 seconds queue, we'd have in positive rate: [0 ..... 3] [3 ..... 6] [6 ..... 9] Which in reverse would be: [6 ..... 9] [3 ..... 6] [0 ..... 3] As we only look at the first and last buffer, we will report a shorter duration (3s) which will lead to over buffering. At least this is what I think happens, I hope this is useful. For the case of out of segment DTS, this need fixing, there is bugs for multiqueue already bug 757193. What I'm unsure for this second case is if we should let the queue grow for out of segment, so we queue in duration a certain amount of in-segment data ?
(In reply to Nicolas Dufresne (stormer) from comment #11) > What I'm > unsure for this second case is if we should let the queue grow for out of > segment, so we queue in duration a certain amount of in-segment data ? I think that if you report out-of-segment data as buffered, then a player that uses queue2 for buffering when playing from network may misbehave. Eg. the key frame is in T=5 and T2=15, the time limit is 3 seconds, you seek to t=10. The queue obtains buffers [5..8], reports 100% buffered, and the player unpauses playback. The buffers are out of segment, so will be dropped immedately when they reach the sink (or even before that?), and the player will pause for buffering, while the purpose of the 3s limit was to be able to know when to unpause after the seek to have fluent playback. In players which shuold limit memory usage because of other constraints (eg. embedded with low RAM), the additional limit max-size-bytes should be used, that will prevent overbuffering. Also, if we can merge the first patch, leaving the negative playback rates for further investigation, that would be great. My original issue was that first one. Maybe there should be a separate bug created for negative rates in queue/queue2/multiqueue.
Considering the history of miss-behaviour, my advise here would be to write unit tests for that, marking as skip the one that are not yet supported. (a side note, a DTS before segment does not mean this buffer is to be skipped. The decoded buffer will be skipped only if the PTS is out of segment.)
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/96.