GNOME Bugzilla – Bug 630441
baseaudiosink: Newly added sinks don't get notified about the bin's latency
Last modified: 2018-11-03 11:17:29 UTC
Created attachment 170939 [details] [review] patch 1. make sure we have us_latency. 2. remember to subtract render_delay, as this is added on to the latency
I don't understand how the us_latency could not have been configured. gst_base_sink_do_preroll() only returns after the state of the sink is PLAYING and before the state is set to PLAYING the latency is queried and configured. Maybe you are changing the state of the elements yourself?
commit c89082b2ddd1c96c899c6c76ebd2cf8c41339605 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Fri Sep 24 12:54:47 2010 +0200 baseaudiosink: subtract the render_delay from our latency The latency reported by the base class includes the render_delay, which we don't want to include when we start slaving our clocks. See #630441
(In reply to comment #1) > I don't understand how the us_latency could not have been configured. > gst_base_sink_do_preroll() only returns after the state of the sink is PLAYING > and before the state is set to PLAYING the latency is queried and configured. > Maybe you are changing the state of the elements yourself? Yup, we have a running pipeline, (PLAYING) which we add elements to. I saw this in an internal test I wrote though, so I could check it out and get back to you.
Is this issue OBSOLETE then?
Well, Wim committed the part about subtracting render-delay (most important one). However, the bit that asks for us_latency is not in yet. We need this one because we don´t know if the sink has done the latency query at this point, since we don´t use elements in the more traditional "put them all in a pipeline, and press play"-kind of way.
Ping?
(In reply to comment #6) > Ping? Not for me I hope? Comment 5 explains what is left of this patch. I could maybe update it?
Created attachment 188002 [details] [review] patch Update the patch with what is left of it
Wim, Havard, is that latest patch still needed in master ?
I would say so yes.
(In reply to comment #7) > (In reply to comment #6) > > Ping? > > Not for me I hope? Comment 5 explains what is left of this patch. I could maybe > update it? You too, yes :) Would be good to have Wim and you agree on something about the remaining changes
Wim?
How should we move forward with this?
(In reply to Sebastian Dröge (slomo) from comment #13) > How should we move forward with this? Take Wim's silence as a don't-care. That said, it seems like there's something missing in core if latency isn't reconfigured when the new sink is added - even if it is done later.
I would say GStreamer is becoming much more mature now as a dynamic real-time framework, and not just the traditional media-player use-cases. In that case, if you add a sink to your running pipeline, that sink will not know its upstream latency without this patch, which I would argue is quite crucial. (Disclaimer: I have not tested this, since this patch was written at a time when I thought it was OK to submit patches without tests) ;)
I was more thinking that maybe the bin should instantiate a remeasuring of the latency as it does when things are present from the start.
My main "concern" with your patch, btw, is that it'll now do all the latency measuring twice in the "normal" case, right?
Should we maybe just add something to GstPipeline that whenever a sink is added, it will get a LATENCY event right away? And that new sink might post a LATENCY message if reconfiguration is needed. Jan, Håvard?
It might be sufficient to store us_latency = GST_CLOCK_TIME_NONE (resetting as needed), and do the query in gst_base_audio_sink_sync_latency if the query hasn't been done yet.
I think this needs a test even if the patch is old. Somehow, elements exposing latency should be posting latency messages to their parent. The application is responsible for redistributing the latency (gst_bin_recalculate_latency()). The important thing here, is that the latency that get applied (through latency event) might not match the latency that an element would query. This is because the latency distributed will be the highest latency obtained by querying each sinks, and this is important if you care about a/v sync. Right now, we clearly recalculate latency on state change to PLAYING, I'm not sure if we have a mechanism for dynamically added element. But clearly, the solution should be around having th message being posted. The Pipeline should probably drop this message if sent in PAUSED (or less) state, as it will recalculate once anyway.
-- 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/gst-plugins-base/issues/38.