GNOME Bugzilla – Bug 796648
Add support for instantaneous rate changes
Last modified: 2018-11-03 12:46:48 UTC
Currently doing smooth playback rate changes requires doing a non-flushing seek to the current position (start/stop_type=NONE). This generally takes a while as the new rate will only take place once the segment event from the demuxer arrives in the sink, so after all currently buffered data was consumed by the sink. The code here implements an approach that allows the rate to change more or less instantaneously: https://cgit.freedesktop.org/~slomo/gstreamer/log/?h=instant-rate-change https://cgit.freedesktop.org/~slomo/gst-plugins-base/log/?h=instant-rate-change https://cgit.freedesktop.org/~slomo/gst-plugins-good/log/?h=instant-rate-change https://cgit.freedesktop.org/~slomo/gst-plugins-bad/log/?h=instant-rate-change Supported demuxers are matroskademux, qtdemux and tsdemux. Most testing was done with qtdemux but it has all kinds of unrelated seeking problems, matroskademux seems to work best at this point. The design is described here: https://cgit.freedesktop.org/~slomo/gst-docs/tree/markdown/design/gapless-rate-change.md?h=instant-rate-change In short the idea is that demuxer notify all sinks immediately about the new rate that is to be applied, the sinks apply it immediately from that point onwards until the corresponding segment event arrives and then compensate for the difference in running time that actually has passed and that upstream believes has passed.
Why not just have the demuxer do a downstream query, that would fail if any of the sinks don't support instant-seek, then put the running time in the event? Wouldn't that be more simple? The current API/code seems overly complex to achieve the goal. Are the segnums really always meant to be increasing? They corresponds to when the seek event was created, so I'm not sure we can compare the numbers in a valid way without changing the API. Also, the latex comments in the markdown are really ugly, can we not do that please?
(In reply to Olivier Crête from comment #1) > Why not just have the demuxer do a downstream query, that would fail if any > of the sinks don't support instant-seek, then put the running time in the > event? Wouldn't that be more simple? > > The current API/code seems overly complex to achieve the goal. > Apart from the fact that a query is blocking (which would hinder the goal of switching as-soon-as-possible), there is more to it. You want to switch as fast-as-possible... and you could potentially have multiple seek handlers in the pipelines ... and you could have some seek handlers that don't *reliably* handle SEEK_TYPE_NONE seeking (which is essential to having proper rate switching working). opt-in API: * Only seek handlers which we know can reliably handle SEEK_TYPE_NONE seeking send the out-of-band event easy API: * The only thing seek handlers need to do is send a downstream event with the same seqnum as the seek event. So 3 lines ? complex API: * Actually the only thing which is "tricky" is doing the runtime-shifting (i.e. applying a different rate in all synchronizing elements which is different from the view that upstream has). But at least it is essentially concentrated in one location (GstBaseSink) and not that big (code is actually in the design doc if other runtime-based elements want to handle it themselves) * The rest of the API is: * Creating and sending a simple EVENT downstream in seek handlers. * Sending a message upstream in GstBaseSink when they receive that event * GstPipeline sending an event upstream when all sinks have sent that message * GstBaseSink handling it Deciding the *optimal* runtime at which to switch: * The initial version we tried did what you proposed (i.e. use a runtime in seek handlers). The problem is that you can only do a best-effort in the seek handlers at guessing when to switch. If you query it downstream you'd be blocking (slowwww). If you have multiple seek handlers you'd get different values (goodbye A/V sync). * So that's why the most reasonable solution was to "accumulate" at the pipeline level all the requests to switch to a different rate. And as soon as we have requests from all sinks, we send the current switching runtime. => Not too early (where sinks wouldn't be ready) => Not too late (where it wouldn't be as-fast-as-possible) > Are the segnums really always meant to be increasing? They corresponds to > when the seek event was created, so I'm not sure we can compare the numbers > in a valid way without changing the API. gst_util_seqnum_next() is what's used whenever a new event/message is created by default. Yes, it's always increasing. No, there's no reason whatsoever to override that seqnum as an application. > > > Also, the latex comments in the markdown are really ugly, can we not do that > please? Read the document in a markdown viewer and it'll be fine. Markdown sadly doesn't have a decent table support.
The infrastructure seems to be quite complex but I think I understand the reasons that lead to those decisions. My question is, why does the user need to specify GST_SEEK_FLAG_INSTANT_RATE_CHANGE? can't we simply detect it ourself from the seek arguments and make the decision to bring the new mechanism in instead? (I can't think of a case where the user sending a rate changing only seek means "do it but later in the future I do not know when" :-).
(In reply to Thibault Saunier from comment #3) > (I can't think of a case ... for now :) I'm not saying your point isn't valid, on the contrary. But I generally think making such changes explicit help stick with the "opt-in and don't break existing behaviour" model. The overhead (i.e. having to explicitely add that flag for "main" user apps/pipelines) is rather small in the end.
(In reply to Edward Hervey from comment #2) > (In reply to Olivier Crête from comment #1) > > Why not just have the demuxer do a downstream query, that would fail if any > > of the sinks don't support instant-seek, then put the running time in the > > event? Wouldn't that be more simple? > > > > The current API/code seems overly complex to achieve the goal. > > Apart from the fact that a query is blocking (which would hinder the goal > of switching as-soon-as-possible), there is more to it. I don't get that part? The query isn't blocking on any lock, so it should get the answer in an instant from all the sinks? > You want to switch as fast-as-possible... and you could potentially have > multiple seek handlers in the pipelines ... and you could have some seek > handlers that don't *reliably* handle SEEK_TYPE_NONE seeking (which is > essential to having proper rate switching working). Then they'll answer FALSE and they won't seek, no? Just like any other seek in GStreamer. > complex API: > * Actually the only thing which is "tricky" is doing the runtime-shifting > (i.e. applying a different rate in all synchronizing elements which is > different from the view that upstream has). But at least it is essentially > concentrated in one location (GstBaseSink) and not that big (code is > actually in the design doc if other runtime-based elements want to handle it > themselves) > * The rest of the API is: > * Creating and sending a simple EVENT downstream in seek handlers. > * Sending a message upstream in GstBaseSink when they receive that event > * GstPipeline sending an event upstream when all sinks have sent that > message > * GstBaseSink handling it But it's not only basesink, it's also any element using the clocks such as identity, aggregtor, rtpjitterbuffer? Also, so many steps means they can easily get out of sync. > Deciding the *optimal* runtime at which to switch: > * The initial version we tried did what you proposed (i.e. use a runtime in > seek handlers). The problem is that you can only do a best-effort in the > seek handlers at guessing when to switch. If you query it downstream you'd > be blocking (slowwww). If you have multiple seek handlers you'd get > different values (goodbye A/V sync). > * So that's why the most reasonable solution was to "accumulate" at the > pipeline level all the requests to switch to a different rate. And as soon > as we have requests from all sinks, we send the current switching runtime. > => Not too early (where sinks wouldn't be ready) > => Not too late (where it wouldn't be as-fast-as-possible) The demuxer also has the clock, the base time and the pipeline latency, so it can just decide on the optimal time (which is "now" after the query returned TRUE from all sinks), so need to get the time from the sinks? > > Are the segnums really always meant to be increasing? They corresponds to > > when the seek event was created, so I'm not sure we can compare the numbers > > in a valid way without changing the API. > > gst_util_seqnum_next() is what's used whenever a new event/message is > created by default. Yes, it's always increasing. No, there's no reason > whatsoever to override that seqnum as an application. Apps could very well create a seek event and send it later, hence, the ordering being meaningless. > > Also, the latex comments in the markdown are really ugly, can we not do that > > please? > > Read the document in a markdown viewer and it'll be fine. Markdown sadly > doesn't have a decent table support. Can you use it only for tables then instead of everywhere? And yes markdown sucks, but this is life.. This latex stuff is only to produce PDFs which is really not waht we do anyway, so it's not very useful.
-- 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/297.