GNOME Bugzilla – Bug 760677
basesink: always drops buffers before segment
Last modified: 2018-11-03 12:32:33 UTC
+++ This bug was initially created as a clone of Bug #752791 +++ Basesink drops buffers before segment, this can be harmful for streams that have keyframes. Appsink, network sinks and any other sinks that can handle any format would drop relevant data before the segment and rendering might not work as expected.
Created attachment 319108 [details] [review] tests: basesink: shows that basesink drops keyframes before segment Test that pushes a keyframe and a deltaunit before segment.start and checks if they are preserved, otherwise the first buffers of the stream would be useless.
A few ideas that were discussed on IRC some time ago: 1) Add a new flag to stream-start event to identify if the stream can have its buffers dropped or not by elements that don't fully understand the format. 2) Add flags to the caps event 3) Let basesink have a property to control that. Subclasses can automatically set what they need. === Between 1 and 2 I think 2 is better because dropping or not dropping is associated with the format and not with the stream identifier. Besides that, the stream-start event might be created before the caps is known for some elements or for elements that just forward the stream-start from upstream. OTOH the stream start already has a flags field with GST_STREAM_FLAG_* stuff, having similar flags in caps can cause some confusion. As someone said on IRC, 3 can be implemented independently in the future to allow users to control this by themselves but we can postpone to when needed. There is also an idea to have an automatic mapping of caps formats to whether a stream can drop buffers or not but this is harder to extend and maintain as people can have their custom formats.
Created attachment 320466 [details] [review] event: add GST_STREAM_FLAG_DROPPABLE Signals that the stream buffers can be dropped independently. For example, in raw video streams this flag can be set to make generic elements aware that they can drop buffers safely. On encoded video streams that have I-frames this is not true as elements can't drop buffers if they don't fully understand the format. This is useful for generic elements to act on synchronization of encoded streams such as appsink when receiving encoded video to be handled at the application layer.
Created attachment 320467 [details] [review] basesink: use stream-droppable flag Uses the GST_FLAG_STREAM_DROPPABLE to prevent dropping frames from streams that can't have their frames dropped without properly understanding the format. This prevents basesink from dropping keyframes for relevant streams as it is an agnostic element.
Created attachment 320468 [details] [review] videodecoder: set output stream to droppable Showcases how videodecoders should mark the stream to allow downstream to drop it. Notice that this changes the standard behavior to *not-drop* by default, assuming this is a safer default. If we want to mantain the previous behavior the DROPPABLE flag should be NOT_DROPPABLE by default and we set when we want to not-drop it.
A downside is that render will be called for buffers that are late as well and the subclass wouldn't know about it. Perhaps we can add some new api like a drop_frame() method or some function to check if a buffer is late or not from the rendering function.
Also GST_BUFFER_FLAG_DROPPABLE can be used here if we agree on setting that to every raw media buffer out there.
Guess we can discuss this now that 1.8 is out.
1-month ping
(In reply to Thiago Sousa Santos from comment #6) > A downside is that render will be called for buffers that are late as well > and the subclass wouldn't know about it. Perhaps we can add some new api > like a drop_frame() method or some function to check if a buffer is late or > not from the rendering function. Some kind of is_late() function I'd say :) Adding flags to caps is an interesting idea, we could also make GstVideoInfo/GstAudioInfo add them automatically to cover most cases. The question is just how the flags interact with all the caps operations. Do you already have some ideas? stream-start flags seem less complicated but I agree that it's more a format specific thing than stream specific. Buffer flags seem like the worst option and seem to have no advantage over any of the others other than making things potentially more complicated :) A property on basesink would also work for me, it seems like the safe and least intrusive option. However having this information generally available in the stream somehow seems useful on its own, not just for basesink.
(In reply to Sebastian Dröge (slomo) from comment #10) > (In reply to Thiago Sousa Santos from comment #6) > > A downside is that render will be called for buffers that are late as well > > and the subclass wouldn't know about it. Perhaps we can add some new api > > like a drop_frame() method or some function to check if a buffer is late or > > not from the rendering function. > > Some kind of is_late() function I'd say :) > > > Adding flags to caps is an interesting idea, we could also make > GstVideoInfo/GstAudioInfo add them automatically to cover most cases. The > question is just how the flags interact with all the caps operations. Do you > already have some ideas? > Haven't thought deeply about this, my idea is that everything that generates a new caps loses its flags, pretty much like buffer meta that has to be 'manually' added whenever needed. It could work as simple flags or as GstMeta like objects depending on the amount of possibilities we want to cover. From my POV this would be derived information from a format, for example: subtitles -> sparse raw video/audio -> droppable It should not interfere with negotiation but work as extra hints to elements to be able to handle data generically. > > > A property on basesink would also work for me, it seems like the safe and > least intrusive option. However having this information generally available > in the stream somehow seems useful on its own, not just for basesink. Yes, a property would require going in every sink and updating it depending on caps. And then you have generic sinks that don't care about formats at all and wouldn't be able to decide.
(In reply to Thiago Sousa Santos from comment #11) > (In reply to Sebastian Dröge (slomo) from comment #10) > > (In reply to Thiago Sousa Santos from comment #6) > > > A downside is that render will be called for buffers that are late as well > > > and the subclass wouldn't know about it. Perhaps we can add some new api > > > like a drop_frame() method or some function to check if a buffer is late or > > > not from the rendering function. > > > > Some kind of is_late() function I'd say :) > > > > > > Adding flags to caps is an interesting idea, we could also make > > GstVideoInfo/GstAudioInfo add them automatically to cover most cases. The > > question is just how the flags interact with all the caps operations. Do you > > already have some ideas? > > > > Haven't thought deeply about this, my idea is that everything that generates > a new caps loses its flags, pretty much like buffer meta that has to be > 'manually' added whenever needed. It could work as simple flags or as > GstMeta like objects depending on the amount of possibilities we want to > cover. So every caps operation would have the flags disappear in the result? It would seem difficult to get the flags downstream then without changes to every element, like with GstMeta. > It should not interfere with negotiation but work as extra hints to elements > to be able to handle data generically. That seems quite ugly in the caps then as it serves a different purpose :)
(In reply to Sebastian Dröge (slomo) from comment #12) > > > > Haven't thought deeply about this, my idea is that everything that generates > > a new caps loses its flags, pretty much like buffer meta that has to be > > 'manually' added whenever needed. It could work as simple flags or as > > GstMeta like objects depending on the amount of possibilities we want to > > cover. > > So every caps operation would have the flags disappear in the result? It > would seem difficult to get the flags downstream then without changes to > every element, like with GstMeta. I don't know how this would work with non-fixed caps. Considering only fixed caps, elements either just pass the same format or create a new one. For the later case, I can only think of parsers that would keep the same format, others (like decoders) are actually transforming the data to a new format and those flags would need to be updated anyway. I don't think we can avoid manipulating the flags in some elements anyway. > > > It should not interfere with negotiation but work as extra hints to elements > > to be able to handle data generically. > > That seems quite ugly in the caps then as it serves a different purpose :) I still see this as something similar to buffers' meta. It's like additional information about a format. Anyway I see that handling those flags across all caps operations is going to be tricky/confusing. Perhaps it makes more sense to put that directly in the caps event?
I wonder if this can't be done using some logic which is a mix of GST_BUFFER_FLAG_DROPPABLE + GST_BUFFER_FLAG_DELTA_UNIT + GST_BUFFER_FLAG_HEADER ? Maybe somethign like if (has (HEADER) no drop; else if (has DROPPABLE) drop else if (PTS != DTS) no drop; else if (has seen DELTA UNIT) nodrop; .. else drop That may still drop the first keyframe in stream that have no B-frames... Another option is to do "if (DTS!=NONE) nodrop;", then make sure streams that have no concept of keyframe or of DTS always set it to NONE? Adding a new concept of caps flags for this seems overkill..
I don't like all these implicit rules, it is easy to forget and makes it harder to maintain and read. Sounds more like a workaround. Using only the droppable flag we could have utility functions like "gst_pad_always_set_buffer_flags(pad, buffer_flags)/always_unset_buffer_flags(pad, buffer_flags) and the pad would automatically set/unset those requested flags. This required 2 extra statements for every buffer push and iterating on buffer lists before pushing.
-- 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/152.