GNOME Bugzilla – Bug 641544
baseaudiosink: Add profile support for selecting the use case and automatically selecting a good default latency setting
Last modified: 2018-11-03 11:18:22 UTC
Created attachment 180113 [details] [review] alsasink bug fix Audio sinks derived from baseaudiosink rely on default buffering and latency settings, respectively 200 and 10ms. This is not good for media playback (too many interrupts) and not good for speech (too much latency). The buffer-time and latency-time can be used to modify the default, but this is typically not enabled in applications. Instead of using defaults that are not optimized for any application, the default should be to use the maximum latency in the audio backend. In case an application has strict latency requirements (speech calls or gaming) it needs to set buffer-time and latency-time to relevant values (as done by farsight) This topic has been addressed a number of times on the mailing lists. See [1] for example. You will find attached a set of patches to demonstrate the concept. I added a new MAX_LATENCY property in the baseaudiosink that is true by default. And I included alsasink and pulsesink changes to demonstrate how this can be used. Note that the first patch is really an alsasink bug and should be applied no matter what the result of this discussion is -Pierre [1] https://tango.0pointer.de/pipermail/pulseaudio-discuss/2010-December/008470.html
Created attachment 180115 [details] [review] add max latency property
Created attachment 180117 [details] [review] use max latency in alsasink
Created attachment 180118 [details] [review] use max latency in pulsesink
Review of attachment 180113 [details] [review]: ::: ext/alsa/gstalsasink.c @@ +455,1 @@ wouldn't that overwrite what we set in alsasink_parse_spec() that is called before set_hwparams()?
Review of attachment 180115 [details] [review]: Could we call that "use-max-latency" (and also rename the var/defines accordingly). IMHO that reads easier for a boolean property.
Review of attachment 180118 [details] [review]: ::: ext/pulse/pulsesink.c @@ +814,3 @@ + wanted.tlength = -1; + wanted.minreq = -1; + } else { Would be nice if we could report the actual values, so that when the app reads the buffer-time, latency-time properties one can see the chosen value.
Review of attachment 180117 [details] [review]: ::: ext/alsa/gstalsasink.c @@ +408,3 @@ + snd_pcm_hw_params_get_buffer_time_max (params, &buffer_time, NULL); + } + would it make sense to update alsa->buffer_time to and ev. g_object_notify the property so that the app can read the chose value?
(In reply to comment #4) > Review of attachment 180113 [details] [review]: > > ::: ext/alsa/gstalsasink.c > @@ +455,1 @@ > > > wouldn't that overwrite what we set in alsasink_parse_spec() that is called > before set_hwparams()? That's correct. In the existing code, alsa->buffer_time/size are overwritten. My patch overwrites period-time. It's necessary to overwrite initial values since the ALSA driver will adjust them to account for hardware limitations (alignment, DMA sizes, etc). There could be a problem in that the GstRingBuffer fields are not inline with what is actually used by ALSA, but this was a pre-existing condition, not something I made worse...
(In reply to comment #7) > Review of attachment 180117 [details] [review]: > > ::: ext/alsa/gstalsasink.c > @@ +408,3 @@ > + snd_pcm_hw_params_get_buffer_time_max (params, &buffer_time, NULL); > + } > + > > would it make sense to update alsa->buffer_time to and ev. g_object_notify the > property so that the app can read the chose value? This would need to be done even without my patches. Currently, if the user defines a buffer-time property that cannot be set by ALSA, we use defaults. The app has no idea changes happened in the sink. Likewise, even when the property value is reasonable, since snd_pcm_hw_params_set_buffer_time_near() is used you also have no guarantee that what the user sets is actually going to be used.
Created attachment 183253 [details] corrected set of patches fixes after Stefan's comments, the property name was changed and the values updated to actual settings
Created attachment 183254 [details] corrected set of patches for gst-plugins-good fixes as per Stefan's feedback
Having this true by default will break Rhythmbox' crossfading backend - you'll end up with latency-length waits for response to pause/play/next. I suggest we keep this as false by default and enable it per-application so that existing applications don't unexpectedly change behaviour. In the long run, though, is there a reason to not move the profiles we define in gconfaudiosink to baseaudiosink so that all apps can just set that on whatever sink is use?
Is this still needed/desired? What's the status?
Also, having buffer-time == latency time is probably a bad idea. Though changing the default might not be ABI friendly.
I'm thinking it might make sense to expand the scope of this just a little bit. What I propose we have: 1. A 'profile' property on baseaudiosink, which will be an enum of the profile types we support: at the moment, I can the the value of having - music, event, voice recording, voice call. 2. A GstAudioBaseSink->set_profile() class function that derived classes can optionally provide, which will be used to communicate desired profiles So a derived class would then be able to do all the things it needs to do to set itself up for the given profile -- set stream properties, buffering parameters, load effects, etc. as required. The upside of doing things this way is that we would then provide a unified interface for clients to set these parameters, without worrying about how it would need to be done in the underlying sink. The downside of this is that we're providing a higher level abstraction and we lose some amount of control (buffering parameters getting automatically set up, for example), and that we might have some cases that don't map into concepts we provide (for example, the "system" stream type on Android doesn't match any of the profiles I listed above).
(The usage profile should really be on GstBin imho)
On second thought, I think an enum would be a bad idea. From a user perspective, exposing a static set of profiles would make it seem that sinks always support all profiles, which is confusing. I think it makes more sense to go with a set of well-documented strings. These would be documented in gtk-doc documentation with an explicit caveat about support in subclasses being optional.
Created attachment 299688 [details] [review] audio: Add a profile property to audio base sink and source Subclasses can use the profile to configure themselves for the appopriate buffering, latency, and filtering, if they wish to. We use a string instead of a fixed enum since each subclass might end up implementing a set of profiles that make sense on the given platform. However, we would like some consistency so that clients are able to set common types of profiles in a subclass-agnostic way. For this, we define a few common profile names in documentation that subclasses are expected to use for those common use cases.
Created attachment 299689 [details] [review] audio: Add a profile property to audio base sink and source Subclasses can use the profile to configure themselves for the appopriate buffering, latency, and filtering, if they wish to. We use a string instead of a fixed enum since each subclass might end up implementing a set of profiles that make sense on the given platform. However, we would like some consistency so that clients are able to set common types of profiles in a subclass-agnostic way. For this, we define a few common profile names in documentation that subclasses are expected to use for those common use cases.
Arun, this seems useful to get in. What's the status from your side?
-- 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/44.