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

[Arun Raghavan]

On Sun, 15 Oct 2017, at 09:45 PM, Tanu Kaskinen wrote:
> 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.

I'm actually in favour of dropping the flag. Makes sense to not track
state when avoidable, so we don't have to deal with staleness problems.

> > @@ -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);
>      ^~~~~~~~~~~~~~~~~~~~~

Thanks, fixed this.

-- Arun