[pulseaudio-discuss] [PATCH v3] alsa-sink/source, sink, source: Consider sample format for avoid-resampling/passthrough

[Tanu Kaskinen]

On Fri, 2018-09-28 at 01:37 +0900, Sangchul Lee wrote:
> Sample format(e.g. 16 bit, 24 bit) was not considered even if the
> avoid-resampling option is set or the passthrough mode is used.
> This patch checks both sample format and rate of a stream to
> determine whether to avoid resampling in case of the option is set.
> In other word, it is possble to use the stream's original sample
> format and rate without resampling as long as these are supported
> by the device.
> 
> pa_sink_input_update_rate() and pa_source_output_update_rate() are
> renamed to pa_sink_input_update_resampler() and pa_source_output
> _update_resampler() respectively.
> 
> functions are added as below.
>  pa_sink_set_sample_format(), pa_sink_set_sample_rate(),
>  pa_source_set_sample_format(), pa_source_set_sample_rate()
> 
> Signed-off-by: Sangchul Lee <sc11.lee at samsung.com>
> ---
>  src/modules/alsa/alsa-sink.c   | 105 ++++++++++++++++++++++++++++++++---------
>  src/modules/alsa/alsa-source.c | 102 ++++++++++++++++++++++++++++++---------
>  src/pulsecore/sink-input.c     |  22 ++++-----
>  src/pulsecore/sink-input.h     |   2 +-
>  src/pulsecore/sink.c           |  71 +++++++++++++++++++++++-----
>  src/pulsecore/sink.h           |   3 ++
>  src/pulsecore/source-output.c  |  22 ++++-----
>  src/pulsecore/source-output.h  |   2 +-
>  src/pulsecore/source.c         |  69 ++++++++++++++++++++++-----
>  src/pulsecore/source.h         |   3 ++
>  10 files changed, 304 insertions(+), 97 deletions(-)

Thanks for the updated patch, there are some things to improve still.

>  static int sink_reconfigure_cb(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>      struct userdata *u = s->userdata;
>      int i;
> -    bool supported = false;
> -
> -    /* FIXME: we only update rate for now */
> +    bool format_supported = false;
> +    bool rate_supported = false;
>  
>      pa_assert(u);
>  
> -    for (i = 0; u->supported_rates[i]; i++) {
> -        if (u->supported_rates[i] == spec->rate) {
> -            supported = true;
> +    for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
> +        if (u->supported_formats[i] == spec->format) {
> +            pa_sink_set_sample_format(u->sink, spec->format);
> +            format_supported = true;
>              break;
>          }
>      }
>  
> -    if (!supported) {
> -        pa_log_info("Sink does not support sample rate of %d Hz", spec->rate);
> -        return -1;
> +    if (!format_supported) {
> +        pa_log_info("Sink does not support sample format of %s, set it to default value",
> +                    pa_sample_format_to_string(spec->format));
> +        pa_sink_set_sample_format(u->sink, u->initial_info.sample_spec.format);

initial_info.sample_spec.format is not necessarily supported by the
hardware (the variable is initialized before opening the device or
querying the supported formats). The "default format" that is used here
should be one that we know is supported by the sink.

>      }
>  
> -    if (!PA_SINK_IS_OPENED(s->state)) {
> -        pa_log_info("Updating rate for device %s, new rate is %d", u->device_name, spec->rate);
> -        u->sink->sample_spec.rate = spec->rate;
> -        return 0;
> +    for (i = 0; u->supported_rates[i]; i++) {
> +        if (u->supported_rates[i] == spec->rate) {
> +            pa_sink_set_sample_rate(u->sink, spec->rate);
> +            rate_supported = true;
> +            break;
> +        }
> +    }
> +
> +    if (!rate_supported) {
> +        pa_log_info("Sink does not support sample rate of %u, set it to default value", spec->rate);
> +        pa_sink_set_sample_rate(u->sink, u->initial_info.sample_spec.rate);

Same issue as above.

>      }
>  
>      /* Passthrough status change is handled during unsuspend */
>  
> -    return -1;
> +    return 0;

sink_reconfigure_cb() now always succeeds. This made me look into the
cases where there's some logic that depends on the
pa_sink_reconfigure() return value. The only place where the return
value affects things is pa_source_reconfigure() when reconfiguring a
monitor source reconfigures also the monitored sink:

    s->sample_spec = desired_spec;
    ret = pa_sink_reconfigure(s->monitor_of, &desired_spec, false);

    if (ret < 0) {
        /* Changing the sink rate failed, roll back the old rate for
         * the monitor source. Why did we set the source rate before
         * calling pa_sink_reconfigure(), you may ask. The reason is
         * that pa_sink_reconfigure() tries to update the monitor
         * source rate, but we are already in the process of updating
         * the monitor source rate, so there's a risk of entering an
         * infinite loop. Setting the source rate before calling
         * pa_sink_reconfigure() makes the rate == s->sample_spec.rate
         * check in the beginning of this function return early, so we
         * avoid looping. */
        s->sample_spec = old_spec;
    }

That logic has to be changed. I think we can ignore the return value
and have this instead:

    s->sample_spec = desired_spec;
    pa_sink_reconfigure(s->monitor_of, &desired_spec, false);
    s->sample_spec = s->monitor_of->sample_spec;

That way the monitor source sample spec becomes whatever the monitored
sink sample spec was changed (or not changed) to.

There's apparently no reason to return any value from
pa_sink/source_reconfigure(), so their return type could be changed to
void.

> +/* Called from the main thread */
> +bool pa_sink_set_sample_format(pa_sink *s, pa_sample_format_t format) {

The return type should be void.

> +    pa_sample_format_t old_format;
> +
> +    pa_assert(s);
> +
> +    if (!pa_sample_format_valid(format))
> +        return false;

This should be an assertion.

> +
> +    old_format = s->sample_spec.format;

We should check whether the new format is the same as the old format,
and return early if it is.

> +
> +    pa_log_info("%s: format: %s -> %s",
> +                s->name, pa_sample_format_to_string(old_format), pa_sample_format_to_string(format));
> +
> +    s->sample_spec.format = format;
> +
> +    pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SINK | PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
> +
> +    return true;
> +}
> +
> +/* Called from the main thread */
> +bool pa_sink_set_sample_rate(pa_sink *s, uint32_t rate) {

The return type should be void.

> +    pa_sample_format_t old_rate;
> +
> +    pa_assert(s);
> +
> +    if (!pa_sample_rate_valid(rate))
> +        return false;

This should be an assertion.

> +
> +    old_rate = s->sample_spec.rate;

We should check whether the new rate is the same as the old rate, and
return early if it is.

Remember to do the same changes at the source side too (this is also a
reminder for myself to check this in the next review).

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk