[pulseaudio-discuss] [PATCH] sink, source: Rework reconfiguration logic to apply to more than rate

[Tanu Kaskinen]

On Sun, 2017-09-03 at 16:58 +0530, Arun Raghavan wrote:
> @@ -1447,52 +1449,57 @@ int pa_sink_update_rate(pa_sink *s, uint32_t rate, bool passthrough) {
>          }
>      }
>  
> -    if (PA_UNLIKELY(!pa_sample_rate_valid(rate)))
> +    if (PA_UNLIKELY(!pa_sample_spec_valid(spec)))
>          return -1;
>  
> +    desired_spec = s->sample_spec;
> +
>      if (passthrough) {
>          /* We have to try to use the sink input rate */
> -        desired_rate = rate;
> +        desired_spec.rate = spec->rate;
>  
> -    } else if (avoid_resampling && (rate >= default_rate || rate >= alternate_rate)) {
> +    } else if (avoid_resampling && (spec->rate >= default_rate || spec->rate >= alternate_rate)) {
>          /* We just try to set the sink input's sample rate if it's not too low */
> -        desired_rate = rate;
> +        desired_spec.rate = spec->rate;
>  
> -    } else if (default_rate == rate || alternate_rate == rate) {
> +    } else if (default_rate == spec->rate || alternate_rate == spec->rate) {
>          /* We can directly try to use this rate */
> -        desired_rate = rate;
> +        desired_spec.rate = spec->rate;
>  
>      } else {
>          /* See if we can pick a rate that results in less resampling effort */
> -        if (default_rate % 11025 == 0 && rate % 11025 == 0)
> +        if (default_rate % 11025 == 0 && spec->rate % 11025 == 0)
>              default_rate_is_usable = true;
> -        if (default_rate % 4000 == 0 && rate % 4000 == 0)
> +        if (default_rate % 4000 == 0 && spec->rate % 4000 == 0)
>              default_rate_is_usable = true;
> -        if (alternate_rate && alternate_rate % 11025 == 0 && rate % 11025 == 0)
> +        if (alternate_rate && alternate_rate % 11025 == 0 && spec->rate % 11025 == 0)
>              alternate_rate_is_usable = true;
> -        if (alternate_rate && alternate_rate % 4000 == 0 && rate % 4000 == 0)
> +        if (alternate_rate && alternate_rate % 4000 == 0 && spec->rate % 4000 == 0)
>              alternate_rate_is_usable = true;
>  
>          if (alternate_rate_is_usable && !default_rate_is_usable)
> -            desired_rate = alternate_rate;
> +            desired_spec.rate = alternate_rate;
>          else
> -            desired_rate = default_rate;
> +            desired_spec.rate = default_rate;
>      }
>  
> -    if (desired_rate == s->sample_spec.rate)
> +    if (pa_sample_spec_equal(&desired_spec, &s->sample_spec) && passthrough == pa_sink_is_passthrough(s))
>          return -1;
>  
>      if (!passthrough && pa_sink_used_by(s) > 0)
>          return -1;
>  
> -    pa_log_debug("Suspending sink %s due to changing the sample rate.", s->name);
> +    pa_log_debug("Suspending sink %s due to changing format.", s->name);
>      pa_sink_suspend(s, true, PA_SUSPEND_INTERNAL);
>  
> -    if (s->update_rate(s, desired_rate) >= 0) {
> +    /* Flag for the sink now that we want it configured for passthrough */
> +    s->is_passthrough_set = passthrough;
> +
> +    if (s->reconfigure(s, &desired_spec, passthrough) >= 0) {

The comment is not very good. "Now that we want it configured for
passthrough" seems to imply that at this point we want the sink
configured for passthrough, but that's not true if passthrough is
false.

Could we remove the passthrough argument from reconfigure()? The
information is available also via pa_sink.is_passthrough_set. Or could
we remove the is_passthrough_set flag? It seems to be used only in
pa_sink_is_passthrough(), and the previous implementation of that
function would presumably still work. The flag doesn't exist for
sources.

If reconfigure() fails and passthrough is true, is_passthrough_set
should be reverted to false, or alternatively is_passthrough_set could
be set only after a successful reconfigure() call. However, if
reconfigure() fails and passthrough is false, then is_passthrough_set
should still be set to false.

> @@ -1501,7 +1501,7 @@ int pa_source_output_finish_move(pa_source_output *o, pa_source *dest, bool save
>             SOURCE_OUTPUT_MOVE_FINISH hook */
>  
>          pa_log_info("Trying to change sample rate");
> -        if (pa_source_update_rate(dest, o->sample_spec.rate, pa_source_output_is_passthrough(o)) >= 0)
> +        if (pa_source_reconfigure(dest, o->sample_spec.rate, pa_source_output_is_passthrough(o)) >= 0)

  CC       pulsecore/libpulsecore_11.0_la-source-output.lo
pulsecore/source-output.c: In function ‘pa_source_output_finish_move’:
pulsecore/source-output.c:1526:41: warning: passing argument 2 of ‘pa_source_reconfigure’ makes pointer from integer without a cast [-Wint-conversion]
         if (pa_source_reconfigure(dest, o->sample_spec.rate, pa_source_output_is_passthrough(o)) >= 0)
                                         ^
In file included from ./pulsecore/sink.h:36:0,
                 from ./pulsecore/core.h:48,
                 from ./pulsecore/core-subscribe.h:26,
                 from pulsecore/source-output.c:36:
./pulsecore/source.h:399:5: note: expected ‘pa_sample_spec * {aka struct pa_sample_spec *}’ but argument is of type ‘uint32_t {aka unsigned int}’
 int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough);
     ^~~~~~~~~~~~~~~~~~~~~

-- 
Tanu

https://www.patreon.com/tanuk