GNOME Bugzilla – Bug 752791
basesink: drops first few buffers with dts/pts b-frame shift
Last modified: 2016-01-15 14:09:41 UTC
gst-launch-1.0 videotestsrc ! x264enc ! identity dump=true ! fakesink dump=true | grep ^0000000 | head -n 5 Should show each line twice, but the first two lines are only shown once because basesink drops the first two buffers as out of segment.
I can fix it next week. Checking and dropping a buffer as "out of segment" base on the DTS is obviously wrong (unless PTS is unset).
Ok, I have given first take on this one. Basically, basesink is trying to clip buffers regardless of what they are. The code is from the root quite wrong, as clipping encoded data like an H264 stream can only render the stream unusable. The solution I foresee will require changes in multiple elements. The side effect of not making these changes is that raw frame could endup not being clipped (quick "playback" of few frames). It's not a major issue, at least it's does not break anything that exist. It consist of better defining the use of DTS. Basically, we would never clip if DTS is set. That means, we need not to set DTS on raw streams if we want them to be clipped (also for streams like JPEG streams, where you can clip safely).
Created attachment 308417 [details] [review] basesink: Don't drop buffers with DTS Specially for H264, it's common that the DTS falls outside (before) the segment. More generally, it's not a good idea to drop buffers that are encoded. In order to fix this problem, this patch propose that basesink rely on the presence of a DTS to avoid dropping. This means element that rely on out of segment buffer to be removed, need not to set a DTS. This applies to raw stream, but also to streams without state, like MJPEG.
Created attachment 308696 [details] [review] basesink: Don't drop buffers Basesink is suppose to be media agnostic, but only raw medias can possibly be dropped safely. So simply don't drop any buffers, even if out of segment, and leave this task to sub-classes.
This second patch is being proposed after an IRC discussion with Sebastien. The idea being that it make not sense for basesink to systematically drop buffers, as this depends on the media (basesink is media agnostic). This is not the only place basesink may drop, it could drop if a buffer if late. Though, this requires subclass approval. This approval is given if the sub-class implements get_times() and that it sets the start to something different from -1. I think a big issue here is that this dropping code does not obey to the permission to drop given by the subclass.
Created attachment 308705 [details] [review] basesink: Restrict dropping out of segment buffer Basesink shall not drop buffers unless it is doing synchronization. Subclasses that do require synchronization implements get_times() and indicate that they want a buffer to be synchronized by returning a valid start time. This aligns the code dropping out of segment with the code dealing with late buffers.
Might just confuse thing, but in this 3rd iteration, I simply don't clip unless the subclass implements get_times() and start != -1. I think this last patch is what makes most sense.
Comment on attachment 308705 [details] [review] basesink: Restrict dropping out of segment buffer Ok, after a long discussion with Olivier, we ruled out this one (so no short cut). The thing I missed is that get_time() is always implemented, so by default start won't be -1. It simply does not solve the initial problem.
Comment on attachment 308696 [details] [review] basesink: Don't drop buffers So as brought up by Tim, a completly valid scenario would be existing code the do: Take a snapshot: uridecodebin ! video/x-raw ! appsink If it picks a file containing raw video directly, and a demuxer that don't try to clip on accurate seek (take TS), the resulting snapshot would be wrong if we don't do any clipping.
Created attachment 308752 [details] [review] videotestsrc: Don't set DTS on buffer DTS is for encoded data. Setting it prevent basesink from being able to clip out of segment buffers.
Comment on attachment 308752 [details] [review] videotestsrc: Don't set DTS on buffer Seems right to me, we shouldn't set DTS on raw video buffers IMHO
So with the remaining approach, similar simple patches would be added to make PTS/DTS coherent to their media type. I can't see any ambiguity with this to be fair.
Attachment 308752 [details] pushed as 7db376d - videotestsrc: Don't set DTS on buffer
Created attachment 308813 [details] [review] basesink: Only drop buffer if their PTS is out of segment As of now, even for stream completly inside segment, there is no guarantied that the DTS will be inside the segment. Specifically for H.264 with B-Frames, the first few frames often have DTS that are before the segment. Instead of using the sync timestamp to clip out of segment buffer, take the duration from the start/stop provided by the sub-class, and check if the pts and pts_end is out of segment.
So I think there is two bugs, the patch in comment 14 fixes the regression. Basically an out of segment buffer is a buffer that has a PTS and PTS + duration that falls out of segment. The second bug, which was present before the integration of proper DTS support for H.264 is that basesink will clip and break certain stream (if segment is in TIME format). This problem already existed, and should be handled differently. A simple option would be to add a clip-to-segment property to basesink, this way application (or sub-class) can choose. But let's file a seperate bug for that.
Attachment 308813 [details] pushed as e24e902 - basesink: Only drop buffer if their PTS is out of segment With suggested rename.
Oops, I have pushed this accidentally, leaving open for comment, will revert if any comment, sorry about this.
Looks like there is no or at least little objections.