GNOME Bugzilla – Bug 696952
Seeking as early as possible (before elements output data)
Last modified: 2018-11-03 12:17:43 UTC
We could implement seeking in READY/NULL the following way: - Elements that can seek will store any seek event they get before PAUSED and will automatically handle that seek event before the pre-roll - GstBin will cache the last seek event and will send it to all elements (including elements that are added later) until the state change to PAUSED is finished (and will then remove the cached seek event) The first part is already implemented in some demuxer, e.g. qtdemux.
I have given it a quick look. Adding this in GstBin is fairly easy but requires some work on seeking elements. Basically we can store the most recent seek event in the bin, and redistribute it to every children (and newly added children) instead of sink children only. I still feel there might be some limitation. On of the aspect I have notice, is that it might not be easy to do type conversion, limiting seeks to what is supported by the element that gets to do the seek. But that need more investivation obviously. On src and sink side, this seems to already be supported, but on demuxer there will be quite some work to allow performing an initial seek before starting the task.
Could you upload your changes to a GIT repository somewhere and link here? Maybe someone else has time to continue working on that :) For implementing this in demuxers, I guess we should do a proof-of-concept with a single demuxer and then better focus on a demuxer base class that handles this properly.
So the idea is that when sending the seek to the pipeline, it is not distributed the regular way (to sinks and hen travel upstream), but instead send directly to bins and sources?
My first try can be found here http://cgit.collabora.com/git/user/nicolas/gstreamer.git/log/?h=seek-on-ready Yes, this is the basic, though I feel it need to be more controlled then that, and most demuxer don't cache those seek event on ready, or don't even have support for early seeks.
(In reply to comment #3) > So the idea is that when sending the seek to the pipeline, it is not > distributed the regular way (to sinks and hen travel upstream), but instead > send directly to bins and sources? Send directly to *any* element that is inside the bin, and any element that is added to the bin until it goes to PAUSED. After that it is normal event distribution as always. (In reply to comment #4) > Yes, this is the basic, though I feel it need to be more controlled then that, > and most demuxer don't cache those seek event on ready, or don't even have > support for early seeks. Maybe we could add a GstElementFlag to mark elements that do handle seeks, or can handle seeks in READY. Not sure... What problem do you see from sending this event just to everything? That when going to READY there could be millions of seeks happening? I don't think that will happen as this would only do something useful for those elements that drive the pipeline later (i.e. the demuxers or parsers in pull mode).
(In reply to comment #5) > (In reply to comment #4) > What problem do you see from sending this event just to everything? That when > going to READY there could be millions of seeks happening? I don't think that > will happen as this would only do something useful for those elements that > drive the pipeline later (i.e. the demuxers or parsers in pull mode). You have said "until it goes to PAUSED", this is the part that my patch does not do explicitly. I think it breaks things because it ends up sending seek to elements added in playing, but I'm unsure of the real cause. Should find it soon.
Well, without sending seeks to events that are added inside playbin/decodebin this will not be usable with both elements. Or what do you mean?
(In reply to comment #5) > (In reply to comment #3) > > So the idea is that when sending the seek to the pipeline, it is not > > distributed the regular way (to sinks and hen travel upstream), but instead > > send directly to bins and sources? > > Send directly to *any* element that is inside the bin, and any element that is > added to the bin until it goes to PAUSED. After that it is normal event > distribution as always. Right, that sounds good. Having a flag would be even better, at least better that make it so that the elements would explicitly need to pick the stored seek and send it.
*** Bug 732572 has been marked as a duplicate of this bug. ***
Created attachment 286876 [details] [review] bin: Add seek on ready support to GstBin This patch is a first step toward SEEK on ready management in GstBin. In this patch the we make sure that whenever a seek event is received in the send_event implementation of the GstElement vmethod, while in the READY state, we foward it to all our children. In a second step we should also handle the case where elements are added to the bin after the seek event was received.
Review of attachment 286876 [details] [review]: Would like to clarify. ::: gst/gstbin.c @@ +2859,2 @@ gst_event_ref (event); + tmpres = gst_element_send_event (child, event); Just checking, is the lock dropped at this point ? @@ +2861,3 @@ + + if (one_sucess_is_enough && tmpres) + res = TRUE; I have doubt this is doing the right thing. Should it be two if, so we never do res &= when one_success_is_enough ? if (once_success_is_enough) { if (tempret) ret = TRUE; }
Not related to the patch (sorry), but I would like us to find a better name than 'seek in ready' for this stuff, perhaps something like 'set initial position' or 'initial seek' or somesuch.
Review of attachment 286876 [details] [review]: ::: gst/gstbin.c @@ +2859,2 @@ gst_event_ref (event); + tmpres = gst_element_send_event (child, event); Our STATE_LOCK? No it is not, I think it should not. @@ +2861,3 @@ + + if (one_sucess_is_enough && tmpres) + res = TRUE; Indeed, the idea is that if 1 child says it handles the seek event, gst_element_send_event will return TRUE.Updating the patch.
Created attachment 286884 [details] [review] bin: Add seek on ready support to GstBin This patch is a first step toward SEEK on ready management in GstBin. In this patch the we make sure that whenever a seek event is received in the send_event implementation of the GstElement vmethod, while in the READY state, we foward it to all our children. In a second step we should also handle the case where elements are added to the bin after the seek event was received.
(In reply to comment #12) > Not related to the patch (sorry), but I would like us to find a better name > than 'seek in ready' for this stuff, perhaps something like 'set initial > position' or 'initial seek' or somesuch. "Initial seek" sounds neat.
More curiosity, I initially thought that to set the initial position, we'd have to set the pipeline to READY as a prerequisite (hence the name). Does this still holds ? The reflection behind that was the some stuff get stored on the elements/pads, hence being on READY let's us clean the state by going to NULL. It brings me other question, will there be a difference if one do READY/Seek/PLAYING/READY/PLAYING, and READY/Seek/PLAYING/NULL/PLAYING regarding initial playback position or the last move to PLAYING ? p.s. I also like "Initial seek" and that seems different from an "initial position".
(In reply to comment #16) > More curiosity, I initially thought that to set the initial position, we'd have > to set the pipeline to READY as a prerequisite (hence the name). Does this > still holds ? I would say so yes. > The reflection behind that was the some stuff get stored on the > elements/pads, hence being on READY let's us clean the state by going to NULL. > It brings me other question, will there be a difference if one do > READY/Seek/PLAYING/READY/PLAYING, and READY/Seek/PLAYING/NULL/PLAYING regarding > initial playback position or the last move to PLAYING ? When going from PLAYING to READY the initial Seek should be removed anyway so the result should be the same in that case fmpov.
What do you think about getting that commit in?
I think it would make more sense to finish the implementation first and then get it in. This is not complete yet :) Also it might make more sense to store the seek event, and then send it to all children when going from READY to PAUSED (and all children that are added in that time).
Also some tests for this would be good to have, and making sure that it works in some interesting scenarios (playbin for example).
(In reply to comment #19) > I think it would make more sense to finish the implementation first and then > get it in. This is not complete yet :) > > Also it might make more sense to store the seek event, and then send it to all > children when going from READY to PAUSED (and all children that are added in > that time). The thing is that we already have partial support in basesrc for example, I guess implementing that by step would not be bad and this is why I proposed that patch.
For basesrc or souphttpsrc or also demuxers that's a local change, but here you're changing GstBin behaviour for every possible bin out there. As such I think it should be relatively proven already (and have tests!) that this approach works for all the interesting scenarios.
These days, I think we might do this better using a GstContext. I also think it should be explicit that elements which might drive the pipeline are modified to check for the seek. Certainly sending seek events to *all* children of a bin sounds like a way to break something somewhere.
What purpose would the GstContext serve ? Do you mean to use it as a container of the seek information under the hood, or by replacing normal seek function? The original idea is about having the ability to tell the pipeline where to start without the usual racyness of the seeks and without the need for an extra seeking thread. With the addition that bin like decodebin can remember this information until the pipeline is constructed. If using GstContext helps I'm fine, otherwise changing core elements makes sense also (seeking is a core feature). A small difference here could be that support for that in bin becomes explicit (implemented by dynamic bin with good helper, or a basclass on top of bin). We could introduce a gstobject flag to indicate that it is supported by the element / bins. This way, seek can return false if it cannot be handled in ready state.
Summary of discussions that happened during the hackfest. The generic issue to be solved here is to be able to specify an initial playback region for a given stream provider (or more generically seek handler). And the problem is: * Knowing the earliest moment when a seek handler can respond to seeks, so that it has enough info to respond and configure itself, but hasn't started outputting yet * Knowing which element that seek should be sent to. That moment in time does exist: It's when stream providers post a GST_MESSAGE_COLLECTION on the bus (i.e. they know all available streams and are about to start pushing data). The solution is therefore to listen synchronously on the bus for a GST_MESSAGE_COLLECTION and then: * Optionally send a GST_EVENT_SELECT_STREAMS (if you only want the element to start with a specific stream set) * Send a seek event on that element (you know which element it is, it hasn't started outputting, and will be able to respond to seek events). When the element posting that collection gets back control, it is fully configured for: * Which streams it should really use (in case there are multiple) * The playback interval you want Bonus points: * No messing around with GstContext * You can select streams in addition to configuring a playback range
*** Bug 778763 has been marked as a duplicate of this bug. ***
-- 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/35.