GNOME Bugzilla – Bug 705870
audiobasesink: add callback for custom logic based on the sink's average skew value
Last modified: 2015-01-03 12:37:59 UTC
Created attachment 251421 [details] [review] Average skew callback patch Currently, if a drift between the pipeline and the audio clock happens, the audiobasesink can make use of three methods: ignore it, or skew the playout pointer (that is, dropping samples/inserting null samples), or resampling. The resample method does not work very well, leaving the skew one as the method of choice. These hard cuts are very bad for audio quality. At high volumes, resulting "clicks" may also damage speaker equipment. Without additional hardware support, however, the sink cannot do anything more. This patch introduces a new user-defined callback function. This callback is invoked inside the sink's playback loop, each time the average skew value is updated. The callback is passed the current avg_skew value. This way, custom adjustments may be made. For example, if the audio hardware's clock is controlled by a PLL, this PLL may be adjusted to let the audio output run slightly faster or slower, depending on the skew. With this patch, drifts <1ms can be reached, without having to reset the playout pointer all the time.
Review of attachment 251421 [details] [review]: ::: gst-libs/gst/audio/gstaudiobasesink.h @@ +104,2 @@ /** + * GstAudioRingBufferCallback: Shouldn't this instead be a vfunc inside the class? Isn't it class specific? Also shouldn't it somehow circumvent the existing skew logic? @@ +106,3 @@ + * @rbuf: a #GstAudioRingBuffer + * @data: (array length=len): target to fill + * @len: amount to fill Copy&paste mistake in the docs @@ +218,3 @@ + GstClockTime threshold, + GstAudioBaseSinkAvgSkewCallback callback, + gpointer user_data); Needs a GDestroyNotify for the user_data
Please also add 'Since: 1.2' to the gtk-doc chunks for the new API.
(In reply to comment #1) > Review of attachment 251421 [details] [review]: > > ::: gst-libs/gst/audio/gstaudiobasesink.h > @@ +104,2 @@ > /** > + * GstAudioRingBufferCallback: > > Shouldn't this instead be a vfunc inside the class? Isn't it class specific? > Also shouldn't it somehow circumvent the existing skew logic? I do not see this as class specific. This kind of logic is most likely written by application developers, _not_ plugin developers. Plus, putting this in a class would imply that the interface for adjusting the audio hardware is tightly coupled to the interface used for audio output, which is rarely the case. Consider what can be inside the callback. That would typically be either some kind of PLL adjustment (which implies customized audio hardware and a nonstandard extra interface for the adjusting), or an asynchronous resampler (in software or in hardware). If this were a vfunc, you'd have to extend all sinks you want to support: alsa, pulse, waveform, jack, ... In my opinion, this is related to the case of OOP composition over inheritance. I expect this feature to be used mostly by embedded developers who have customized hardware and want to take care that the drift is precisely controlled. A common way to do that is to use a standard interface like ALSA for the common audio I/O and some additional interface for the drift control. A completely new audio interface is much less common (it is cheaper to get some audio DAC with existing drivers and customize these a bit). This callback complements the skew logic, it does not replace or circumvent it. Its effects can indirectly have an impact on the skew logic, of course; if I speed up the hardware based on the average skew, then the subsequent skew values will be affected. But the intent is to be able to do more than just skew the playout pointer; skewing should be an operation for large drifts, while something else should be used for fine-grained adjustments. With the callback, I can combine both: set drift-tolerance to something large, and the custom, platform/hardware-specific code inside the callback can do the fine-grained work. I will fix the other points tomorrow, thanks for highlighting the problems.
Sounds sensible, thanks for the explanations :)
Created attachment 251626 [details] [review] Average skew callback patch, v2 Revised version of the patch
Review of attachment 251626 [details] [review]: Looks almost good, could you add a link to the bug URL in the commit message? Also would be good to get Wim's ok on this patch :) ::: gst-libs/gst/audio/gstaudiobasesink.c @@ +772,3 @@ + g_return_if_fail (GST_IS_AUDIO_BASE_SINK (sink)); + + GST_OBJECT_LOCK (sink); If there already was a callback set, it should call the destroy notify first before setting the new one
Created attachment 251718 [details] [review] Average skew callback patch, v3 Added missing notify call and bugzilla link. Interestingly, on the other PC (using Ubuntu Raring, with GStreamer 1.0.6 installed), I was working on before, the code style checker did apparently not pick up some style errors. On this PC (using Arch Linux with GStreamer 1.0.9), it did. Perhaps the style checker got more strict?
Comment on attachment 251718 [details] [review] Average skew callback patch, v3 Looks good now :) Just waiting for Wim to confirm. The style check issue can also happen if different versions of indent are used. indent likes to change behaviour every now and then.
This is not the right thing to do. It implements a callback for an implementation detail for one specific slave method. I can't suggest a perfect solution but just implementing a function that returns the various values calculated in the current slave method as a GstStructure sounds already much more interesting.
What should happen here?
This is obsolete. There is a new approach at https://bugzilla.gnome.org/show_bug.cgi?id=708362 .