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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 2018-07-28 at 00:38 +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.
> 
> Signed-off-by: Sangchul Lee <sc11.lee at samsung.com>
> ---
>  src/modules/alsa/alsa-sink.c   | 101 ++++++++++++++++++++++++++++++++---------
>  src/modules/alsa/alsa-source.c |  98 ++++++++++++++++++++++++++++++---------
>  src/pulsecore/sink-input.c     |  19 ++++----
>  src/pulsecore/sink-input.h     |   2 +-
>  src/pulsecore/sink.c           |  26 ++++++-----
>  src/pulsecore/source-output.c  |  22 +++++----
>  src/pulsecore/source-output.h  |   2 +-
>  src/pulsecore/source.c         |  24 +++++-----
>  8 files changed, 206 insertions(+), 88 deletions(-)
> 
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index 200d12b..60a4ed2 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c
> @@ -113,11 +113,19 @@ struct userdata {
>  
>      pa_sample_format_t *supported_formats;
>      unsigned int *supported_rates;
> +    struct {
> +        size_t fragment_size;
> +        size_t nfrags;
> +        size_t tsched_size;
> +        size_t tsched_watermark;
> +        size_t rewind_safeguard;
> +    } initial_info;
>  
>      size_t
>          frame_size,
>          fragment_size,
>          hwbuf_size,
> +        tsched_size,
>          tsched_watermark,
>          tsched_watermark_ref,
>          hwbuf_unused,
> @@ -1081,12 +1089,34 @@ static void reset_watermark(struct userdata *u, size_t tsched_watermark, pa_samp
>                  (double) u->tsched_watermark_usec / PA_USEC_PER_MSEC);
>  }
>  
> +/* Called from IO Context on unsuspend */
> +static void update_size(struct userdata *u, pa_sample_spec *ss) {
> +    pa_assert(u);
> +    pa_assert(ss);
> +
> +    u->frame_size = pa_frame_size(ss);
> +    u->frames_per_block = pa_mempool_block_size_max(u->core->mempool) / u->frame_size;
> +
> +    /* use initial values including module arguments */
> +    u->fragment_size = u->initial_info.fragment_size;
> +    u->hwbuf_size = u->initial_info.nfrags * u->fragment_size;
> +    u->tsched_size = u->initial_info.tsched_size;
> +    u->tsched_watermark = u->initial_info.tsched_watermark;
> +    u->rewind_safeguard = u->initial_info.rewind_safeguard;
> +
> +    u->tsched_watermark_ref = u->tsched_watermark;
> +
> +    pa_log_info("Updated frame_size %zu, frames_per_block %lu, fragment_size %zu, hwbuf_size %zu, tsched(size %zu, watermark %zu), rewind_safeguard %zu",
> +                u->frame_size, (unsigned long) u->frames_per_block, u->fragment_size, u->hwbuf_size, u->tsched_size, u->tsched_watermark, u->rewind_safeguard);
> +}
> +
>  /* Called from IO context */
>  static int unsuspend(struct userdata *u) {
>      pa_sample_spec ss;
>      int err;
>      bool b, d;
> -    snd_pcm_uframes_t period_size, buffer_size;
> +    snd_pcm_uframes_t period_frames, buffer_frames;
> +    snd_pcm_uframes_t tsched_frames = 0;
>      char *device_name = NULL;
>  
>      pa_assert(u);
> @@ -1111,13 +1141,18 @@ static int unsuspend(struct userdata *u) {
>          goto fail;
>      }
>  
> +    if (pa_frame_size(&u->sink->sample_spec) != u->frame_size) {
> +        update_size(u, &u->sink->sample_spec);
> +        tsched_frames = u->tsched_size / u->frame_size;
> +    }
> +
>      ss = u->sink->sample_spec;
> -    period_size = u->fragment_size / u->frame_size;
> -    buffer_size = u->hwbuf_size / u->frame_size;
> +    period_frames = u->fragment_size / u->frame_size;
> +    buffer_frames = u->hwbuf_size / u->frame_size;
>      b = u->use_mmap;
>      d = u->use_tsched;
>  
> -    if ((err = pa_alsa_set_hw_params(u->pcm_handle, &ss, &period_size, &buffer_size, 0, &b, &d, true)) < 0) {
> +    if ((err = pa_alsa_set_hw_params(u->pcm_handle, &ss, &period_frames, &buffer_frames, tsched_frames, &b, &d, true)) < 0) {
>          pa_log("Failed to set hardware parameters: %s", pa_alsa_strerror(err));
>          goto fail;
>      }
> @@ -1132,11 +1167,17 @@ static int unsuspend(struct userdata *u) {
>          goto fail;
>      }
>  
> -    if (period_size*u->frame_size != u->fragment_size ||
> -        buffer_size*u->frame_size != u->hwbuf_size) {
> -        pa_log_warn("Resume failed, couldn't restore original fragment settings. (Old: %lu/%lu, New %lu/%lu)",
> -                    (unsigned long) u->hwbuf_size, (unsigned long) u->fragment_size,
> -                    (unsigned long) (buffer_size*u->frame_size), (unsigned long) (period_size*u->frame_size));
> +    if (tsched_frames) {

I think a separate "frame_size_changed" flag should be used here,
because now it looks like u->fragment_size and u->hwbuf_size are
updated only if tsched is enabled (I got confused by this initially).

(Now when I read my previous review, I see that using tsched_frames was
my idea. It was a bad idea nevertheless.)

> +        u->fragment_size = (size_t)(period_frames * u->frame_size);
> +        u->hwbuf_size = (size_t)(buffer_frames * u->frame_size);
> +        pa_proplist_setf(u->sink->proplist, PA_PROP_DEVICE_BUFFERING_BUFFER_SIZE, "%zu", u->hwbuf_size);
> +        pa_proplist_setf(u->sink->proplist, PA_PROP_DEVICE_BUFFERING_FRAGMENT_SIZE, "%zu", u->fragment_size);
> +
> +    } else if (period_frames * u->frame_size != u->fragment_size ||
> +                buffer_frames * u->frame_size != u->hwbuf_size) {
> +        pa_log_warn("Resume failed, couldn't restore original fragment settings. (Old: %zu/%zu, New %lu/%lu)",
> +                    u->hwbuf_size, u->fragment_size,
> +                    (unsigned long) buffer_frames * u->frame_size, (unsigned long) period_frames * u->frame_size);
>          goto fail;
>      }
>  
> @@ -1674,33 +1715,43 @@ static bool sink_set_formats(pa_sink *s, pa_idxset *formats) {
>  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);
>  
> +    if (PA_SINK_IS_OPENED(s->state)) {
> +        pa_log_warn("Sink is opened, skip it");
> +        return -1;
> +    }

The sink should never be opened when reconfiguring. You can remove this
or change it into an assertion.

> +
> +    for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
> +        if (u->supported_formats[i] == spec->format) {
> +            u->sink->sample_spec.format = spec->format;
> +            format_supported = true;
> +            pa_log_info("Sink supports sample format of %s", pa_sample_format_to_string(spec->format));

I think it's more interesting that the format is changed than that the
format is supported. I suggest you add pa_sink_set_format() function
that logs the following message:

pa_log_info("%s: format: %s -> %s", sink->name, old_format, new_format);

and pa_sink_set_format() should also send a change notification with
pa_subscription_post(). Both of these things should be done only if the
format actually changes.

If the requested format is not supported, I think we should switch to
the default format. Imagine a device that supports 8-bit and 16-bit
audio. First a 8-bit stream plays, and the device switches to that, and
then a 24-bit stream plays. If we don't switch to the default format,
the device will keep using the 8-bit format, which is not nice.

In my previous review I said that if the user has requested a specific
format with the "format" modarg, we should never switch the format, but
now I think that's not a good idea, since it could prevent passthrough
and avoid-resampling from working. If the "format" modarg is given, it
should just be used as the default format.

> +            break;
> +        }
> +    }
> +
>      for (i = 0; u->supported_rates[i]; i++) {
>          if (u->supported_rates[i] == spec->rate) {
> -            supported = true;
> +            u->sink->sample_spec.rate = spec->rate;
> +            rate_supported = true;
> +            pa_log_info("Sink supports sample rate of %d Hz", spec->rate);

Same comments as above.

>              break;
>          }
>      }
>  
> -    if (!supported) {
> -        pa_log_info("Sink does not support sample rate of %d Hz", spec->rate);
> +    if (!format_supported && !rate_supported) {
> +        pa_log_warn("Sink does not support both sample format of %s and rate of %d Hz",
> +                    pa_sample_format_to_string(spec->format), spec->rate);

The log level should not be warning, because it's perfectly normal that
the hardware doesn't support all rates and formats.

>          return -1;
>      }
>  
> -    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;
> -    }
> -
>      /* Passthrough status change is handled during unsuspend */
>  
> -    return -1;
> +    return 0;
>  }
>  
>  static int process_rewind(struct userdata *u) {
> @@ -2212,6 +2263,12 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
>      u->module = m;
>      u->use_mmap = use_mmap;
>      u->use_tsched = use_tsched;
> +    u->tsched_size = tsched_size;
> +    u->initial_info.nfrags = (size_t) nfrags;
> +    u->initial_info.fragment_size = (size_t) frag_size;
> +    u->initial_info.tsched_size = (size_t) tsched_size;
> +    u->initial_info.tsched_watermark = (size_t) tsched_watermark;
> +    u->initial_info.rewind_safeguard = (size_t) rewind_safeguard;
>      u->deferred_volume = deferred_volume;
>      u->fixed_latency_range = fixed_latency_range;
>      u->first = true;
> diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
> index cba86ca..92b2125 100644
> --- a/src/modules/alsa/alsa-source.c
> +++ b/src/modules/alsa/alsa-source.c
> @@ -101,11 +101,18 @@ struct userdata {
>  
>      pa_sample_format_t *supported_formats;
>      unsigned int *supported_rates;
> +    struct {
> +        size_t fragment_size;
> +        size_t nfrags;
> +        size_t tsched_size;
> +        size_t tsched_watermark;
> +    } initial_info;
>  
>      size_t
>          frame_size,
>          fragment_size,
>          hwbuf_size,
> +        tsched_size,
>          tsched_watermark,
>          tsched_watermark_ref,
>          hwbuf_unused,
> @@ -947,12 +954,33 @@ static void reset_watermark(struct userdata *u, size_t tsched_watermark, pa_samp
>                  (double) u->tsched_watermark_usec / PA_USEC_PER_MSEC);
>  }
>  
> +/* Called from IO Context on unsuspend */
> +static void update_size(struct userdata *u, pa_sample_spec *ss) {
> +    pa_assert(u);
> +    pa_assert(ss);
> +
> +    u->frame_size = pa_frame_size(ss);
> +    u->frames_per_block = pa_mempool_block_size_max(u->core->mempool) / u->frame_size;
> +
> +    /* use initial values including module arguments */
> +    u->fragment_size = u->initial_info.fragment_size;
> +    u->hwbuf_size = u->initial_info.nfrags * u->fragment_size;
> +    u->tsched_size = u->initial_info.tsched_size;
> +    u->tsched_watermark = u->initial_info.tsched_watermark;
> +
> +    u->tsched_watermark_ref = u->tsched_watermark;
> +
> +    pa_log_info("Updated frame_size %zu, frames_per_block %lu, fragment_size %zu, hwbuf_size %zu, tsched(size %zu, watermark %zu)",
> +                u->frame_size, (unsigned long) u->frames_per_block, u->fragment_size, u->hwbuf_size, u->tsched_size, u->tsched_watermark);
> +}
> +
>  /* Called from IO context */
>  static int unsuspend(struct userdata *u) {
>      pa_sample_spec ss;
>      int err;
>      bool b, d;
> -    snd_pcm_uframes_t period_size, buffer_size;
> +    snd_pcm_uframes_t period_frames, buffer_frames;
> +    snd_pcm_uframes_t tsched_frames = 0;
>  
>      pa_assert(u);
>      pa_assert(!u->pcm_handle);
> @@ -968,13 +996,18 @@ static int unsuspend(struct userdata *u) {
>          goto fail;
>      }
>  
> +    if (pa_frame_size(&u->source->sample_spec) != u->frame_size) {
> +        update_size(u, &u->source->sample_spec);
> +        tsched_frames = u->tsched_size / u->frame_size;
> +    }
> +
>      ss = u->source->sample_spec;
> -    period_size = u->fragment_size / u->frame_size;
> -    buffer_size = u->hwbuf_size / u->frame_size;
> +    period_frames = u->fragment_size / u->frame_size;
> +    buffer_frames = u->hwbuf_size / u->frame_size;
>      b = u->use_mmap;
>      d = u->use_tsched;
>  
> -    if ((err = pa_alsa_set_hw_params(u->pcm_handle, &ss, &period_size, &buffer_size, 0, &b, &d, true)) < 0) {
> +    if ((err = pa_alsa_set_hw_params(u->pcm_handle, &ss, &period_frames, &buffer_frames, tsched_frames, &b, &d, true)) < 0) {
>          pa_log("Failed to set hardware parameters: %s", pa_alsa_strerror(err));
>          goto fail;
>      }
> @@ -989,11 +1022,17 @@ static int unsuspend(struct userdata *u) {
>          goto fail;
>      }
>  
> -    if (period_size*u->frame_size != u->fragment_size ||
> -        buffer_size*u->frame_size != u->hwbuf_size) {
> -        pa_log_warn("Resume failed, couldn't restore original fragment settings. (Old: %lu/%lu, New %lu/%lu)",
> -                    (unsigned long) u->hwbuf_size, (unsigned long) u->fragment_size,
> -                    (unsigned long) (buffer_size*u->frame_size), (unsigned long) (period_size*u->frame_size));
> +    if (tsched_frames) {
> +        u->fragment_size = (size_t)(period_frames * u->frame_size);
> +        u->hwbuf_size = (size_t)(buffer_frames * u->frame_size);
> +        pa_proplist_setf(u->source->proplist, PA_PROP_DEVICE_BUFFERING_BUFFER_SIZE, "%zu", u->hwbuf_size);
> +        pa_proplist_setf(u->source->proplist, PA_PROP_DEVICE_BUFFERING_FRAGMENT_SIZE, "%zu", u->fragment_size);
> +
> +    } else if (period_frames * u->frame_size != u->fragment_size ||
> +                buffer_frames * u->frame_size != u->hwbuf_size) {
> +        pa_log_warn("Resume failed, couldn't restore original fragment settings. (Old: %zu/%zu, New %lu/%lu)",
> +                    u->hwbuf_size, u->fragment_size,
> +                    (unsigned long) buffer_frames * u->frame_size, (unsigned long) period_frames * u->frame_size);
>          goto fail;
>      }
>  
> @@ -1470,31 +1509,41 @@ static void source_update_requested_latency_cb(pa_source *s) {
>  static int source_reconfigure_cb(pa_source *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);
>  
> +    if (PA_SOURCE_IS_OPENED(s->state)) {
> +        pa_log_warn("Source is opened, skip it");
> +        return -1;
> +    }
> +
> +    for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
> +        if (u->supported_formats[i] == spec->format) {
> +            u->source->sample_spec.format = spec->format;
> +            format_supported = true;
> +            pa_log_info("Source supports sample format of %s", pa_sample_format_to_string(spec->format));
> +            break;
> +        }
> +    }
> +
>      for (i = 0; u->supported_rates[i]; i++) {
>          if (u->supported_rates[i] == spec->rate) {
> -            supported = true;
> +            u->source->sample_spec.rate = spec->rate;
> +            rate_supported = true;
> +            pa_log_info("Source supports sample rate of %d Hz", spec->rate);
>              break;
>          }
>      }
>  
> -    if (!supported) {
> -        pa_log_info("Source does not support sample rate of %d Hz", spec->rate);
> +    if (!format_supported && !rate_supported) {
> +        pa_log_warn("Source does not support both sample format of %s and rate of %d Hz",
> +                    pa_sample_format_to_string(spec->format), spec->rate);
>          return -1;
>      }
>  
> -    if (!PA_SOURCE_IS_OPENED(s->state)) {
> -        pa_log_info("Updating rate for device %s, new rate is %d", u->device_name, spec->rate);
> -        u->source->sample_spec.rate = spec->rate;
> -        return 0;
> -    }
> -
> -    return -1;
> +    return 0;
>  }
>  
>  static void thread_func(void *userdata) {
> @@ -1899,6 +1948,11 @@ pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
>      u->module = m;
>      u->use_mmap = use_mmap;
>      u->use_tsched = use_tsched;
> +    u->tsched_size = tsched_size;
> +    u->initial_info.nfrags = (size_t) nfrags;
> +    u->initial_info.fragment_size = (size_t) frag_size;
> +    u->initial_info.tsched_size = (size_t) tsched_size;
> +    u->initial_info.tsched_watermark = (size_t) tsched_watermark;
>      u->deferred_volume = deferred_volume;
>      u->fixed_latency_range = fixed_latency_range;
>      u->first = true;
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 312ec4a..81b4456 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -417,10 +417,10 @@ int pa_sink_input_new(
>  
>      if (!(data->flags & PA_SINK_INPUT_VARIABLE_RATE) &&
>          !pa_sample_spec_equal(&data->sample_spec, &data->sink->sample_spec)) {
> -        /* try to change sink rate. This is done before the FIXATE hook since
> +        /* try to change sink format and rate. This is done before the FIXATE hook since
>             module-suspend-on-idle can resume a sink */
>  
> -        pa_log_info("Trying to change sample rate");
> +        pa_log_info("Trying to change sample spec");
>          if (pa_sink_reconfigure(data->sink, &data->sample_spec, pa_sink_input_new_data_is_passthrough(data)) >= 0)
>              pa_log_info("Rate changed to %u Hz", data->sink->sample_spec.rate);

The log message needs updating, because it may be that only the format
was changed. Or actually the message can be removed if you add the
pa_sink_set_format() and pa_sink_set_rate() functions that do the
logging as I suggested above.

>      }
> @@ -616,7 +616,7 @@ static void sink_input_set_state(pa_sink_input *i, pa_sink_input_state_t state)
>          if (i->state == PA_SINK_INPUT_CORKED && state == PA_SINK_INPUT_RUNNING && pa_sink_used_by(i->sink) == 0 &&
>              !pa_sample_spec_equal(&i->sample_spec, &i->sink->sample_spec)) {
>              /* We were uncorked and the sink was not playing anything -- let's try
> -             * to update the sample rate to avoid resampling */
> +             * to update the sample format and rate to avoid resampling */
>              pa_sink_reconfigure(i->sink, &i->sample_spec, pa_sink_input_is_passthrough(i));
>          }
>  
> @@ -1901,13 +1901,14 @@ int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, bool save) {
>  
>      if (!(i->flags & PA_SINK_INPUT_VARIABLE_RATE) &&
>          !pa_sample_spec_equal(&i->sample_spec, &dest->sample_spec)) {
> -        /* try to change dest sink rate if possible without glitches.
> +        /* try to change dest sink format and rate if possible without glitches.
>             module-suspend-on-idle resumes destination sink with
>             SINK_INPUT_MOVE_FINISH hook */
>  
> -        pa_log_info("Trying to change sample rate");
> +        pa_log_info("Trying to change sample spec");
>          if (pa_sink_reconfigure(dest, &i->sample_spec, pa_sink_input_is_passthrough(i)) >= 0)
> -            pa_log_info("Rate changed to %u Hz", dest->sample_spec.rate);
> +            pa_log_info("format, rate are updated to %s, %u Hz",
> +                        pa_sample_format_to_string(dest->sample_spec.format), dest->sample_spec.rate);

This message can also be removed if you add the pa_sink_set_format()
and pa_sink_set_rate() functions.

>      }
>  
>      if (i->moving)
> @@ -1925,7 +1926,7 @@ int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, bool save) {
>      if (i->state == PA_SINK_INPUT_CORKED)
>          i->sink->n_corked++;
>  
> -    pa_sink_input_update_rate(i);
> +    pa_sink_input_update_resampler(i);
>  
>      pa_sink_update_status(dest);
>  
> @@ -2264,8 +2265,8 @@ finish:
>  
>  /* Called from main context */
>  /* Updates the sink input's resampler with whatever the current sink requires
> - * -- useful when the underlying sink's rate might have changed */
> -int pa_sink_input_update_rate(pa_sink_input *i) {
> + * -- useful when the underlying sink's sample spec might have changed */
> +int pa_sink_input_update_resampler(pa_sink_input *i) {
>      pa_resampler *new_resampler;
>      char *memblockq_name;
>  
> diff --git a/src/pulsecore/sink-input.h b/src/pulsecore/sink-input.h
> index 9e90291..6bf406c 100644
> --- a/src/pulsecore/sink-input.h
> +++ b/src/pulsecore/sink-input.h
> @@ -353,7 +353,7 @@ void pa_sink_input_request_rewind(pa_sink_input *i, size_t nbytes, bool rewrite,
>  void pa_sink_input_cork(pa_sink_input *i, bool b);
>  
>  int pa_sink_input_set_rate(pa_sink_input *i, uint32_t rate);
> -int pa_sink_input_update_rate(pa_sink_input *i);
> +int pa_sink_input_update_resampler(pa_sink_input *i);
>  
>  /* This returns the sink's fields converted into out sample type */
>  size_t pa_sink_input_get_max_rewind(pa_sink_input *i);
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index 566367f..b2ebc2f 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -1448,8 +1448,6 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>      bool alternate_rate_is_usable = false;
>      bool avoid_resampling = s->avoid_resampling;
>  
> -    /* We currently only try to reconfigure the sample rate */
> -
>      if (pa_sample_spec_equal(spec, &s->sample_spec))
>          return 0;
>  
> @@ -1462,14 +1460,14 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>      }
>  
>      if (PA_SINK_IS_RUNNING(s->state)) {
> -        pa_log_info("Cannot update rate, SINK_IS_RUNNING, will keep using %u Hz",
> -                    s->sample_spec.rate);
> +        pa_log_info("Cannot update sample spec, SINK_IS_RUNNING, will keep using %s and %u Hz",
> +                    pa_sample_format_to_string(s->sample_spec.format), s->sample_spec.rate);
>          return -1;
>      }
>  
>      if (s->monitor_source) {
>          if (PA_SOURCE_IS_RUNNING(s->monitor_source->state) == true) {
> -            pa_log_info("Cannot update rate, monitor source is RUNNING");
> +            pa_log_info("Cannot update sample spec, monitor source is RUNNING");
>              return -1;
>          }
>      }
> @@ -1480,12 +1478,15 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>      desired_spec = s->sample_spec;
>  
>      if (passthrough) {
> -        /* We have to try to use the sink input rate */
> +        /* We have to try to use the sink input format and rate */
> +        desired_spec.format = spec->format;
>          desired_spec.rate = spec->rate;
>  
> -    } else if (avoid_resampling && (spec->rate >= default_rate || spec->rate >= alternate_rate)) {
> +    } else if (avoid_resampling) {
>          /* We just try to set the sink input's sample rate if it's not too low */
> -        desired_spec.rate = spec->rate;
> +        if (spec->rate >= default_rate || spec->rate >= alternate_rate)
> +            desired_spec.rate = spec->rate;

If the rate is too low and desired_spec is not updated, then we should
apply the logic in the "else" section (where we pick either
default_rate or alternate_rate). With your current code this doesn't
happen.

To fix this, let's replace

    } else {

with

    }

    if (desired_spec.rate != spec->rate) {

> +        desired_spec.format = spec->format;
>  
>      } else if (default_rate == spec->rate || alternate_rate == spec->rate) {
>          /* We can directly try to use this rate */
> @@ -1514,18 +1515,19 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>      if (!passthrough && pa_sink_used_by(s) > 0)
>          return -1;
>  
> -    pa_log_debug("Suspending sink %s due to changing format, desired rate = %u", s->name, desired_spec.rate);
> +    pa_log_debug("Suspending sink %s due to changing format, desired format = %s rate = %u",
> +                 s->name, pa_sample_format_to_string(desired_spec.format), desired_spec.rate);
>      pa_sink_suspend(s, true, PA_SUSPEND_INTERNAL);
>  
>      if (s->reconfigure(s, &desired_spec, passthrough) >= 0) {
>          /* update monitor source as well */
>          if (s->monitor_source && !passthrough)
> -            pa_source_reconfigure(s->monitor_source, &desired_spec, false);
> -        pa_log_info("Changed format successfully");
> +            pa_source_reconfigure(s->monitor_source, &s->sample_spec, false);
> +        pa_log_info("Reconfigured successfully");
>  
>          PA_IDXSET_FOREACH(i, s->inputs, idx) {
>              if (i->state == PA_SINK_INPUT_CORKED)
> -                pa_sink_input_update_rate(i);
> +                pa_sink_input_update_resampler(i);
>          }
>  
>          ret = 0;
> diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
> index 955a2ac..6e56b92 100644
> --- a/src/pulsecore/source-output.c
> +++ b/src/pulsecore/source-output.c
> @@ -365,12 +365,13 @@ int pa_source_output_new(
>  
>      if (!(data->flags & PA_SOURCE_OUTPUT_VARIABLE_RATE) &&
>          !pa_sample_spec_equal(&data->sample_spec, &data->source->sample_spec)) {
> -        /* try to change source rate. This is done before the FIXATE hook since
> +        /* try to change source format and rate. This is done before the FIXATE hook since
>             module-suspend-on-idle can resume a source */
>  
> -        pa_log_info("Trying to change sample rate");
> +        pa_log_info("Trying to change sample spec");
>          if (pa_source_reconfigure(data->source, &data->sample_spec, pa_source_output_new_data_is_passthrough(data)) >= 0)
> -            pa_log_info("Rate changed to %u Hz", data->source->sample_spec.rate);
> +            pa_log_info("format, rate are updated to %s, %u Hz",
> +                        pa_sample_format_to_string(data->source->sample_spec.format), data->source->sample_spec.rate);
>      }
>  
>      if (pa_source_output_new_data_is_passthrough(data) &&
> @@ -542,7 +543,7 @@ static void source_output_set_state(pa_source_output *o, pa_source_output_state_
>          if (o->state == PA_SOURCE_OUTPUT_CORKED && state == PA_SOURCE_OUTPUT_RUNNING && pa_source_used_by(o->source) == 0 &&
>              !pa_sample_spec_equal(&o->sample_spec, &o->source->sample_spec)) {
>              /* We were uncorked and the source was not playing anything -- let's try
> -             * to update the sample rate to avoid resampling */
> +             * to update the sample format and rate to avoid resampling */
>              pa_source_reconfigure(o->source, &o->sample_spec, pa_source_output_is_passthrough(o));
>          }
>  
> @@ -1533,13 +1534,14 @@ int pa_source_output_finish_move(pa_source_output *o, pa_source *dest, bool save
>  
>      if (!(o->flags & PA_SOURCE_OUTPUT_VARIABLE_RATE) &&
>          !pa_sample_spec_equal(&o->sample_spec, &dest->sample_spec)) {
> -        /* try to change dest source rate if possible without glitches.
> +        /* try to change dest source format and rate if possible without glitches.
>             module-suspend-on-idle resumes destination source with
>             SOURCE_OUTPUT_MOVE_FINISH hook */
>  
> -        pa_log_info("Trying to change sample rate");
> +        pa_log_info("Trying to change sample spec");
>          if (pa_source_reconfigure(dest, &o->sample_spec, pa_source_output_is_passthrough(o)) >= 0)
> -            pa_log_info("Rate changed to %u Hz", dest->sample_spec.rate);
> +            pa_log_info("format, rate are updated to %s, %u Hz",
> +                        pa_sample_format_to_string(dest->sample_spec.format), dest->sample_spec.rate);
>      }
>  
>      if (o->moving)
> @@ -1554,7 +1556,7 @@ int pa_source_output_finish_move(pa_source_output *o, pa_source *dest, bool save
>      if (o->state == PA_SOURCE_OUTPUT_CORKED)
>          o->source->n_corked++;
>  
> -    pa_source_output_update_rate(o);
> +    pa_source_output_update_resampler(o);
>  
>      pa_source_update_status(dest);
>  
> @@ -1729,8 +1731,8 @@ finish:
>  
>  /* Called from main context */
>  /* Updates the source output's resampler with whatever the current source
> - * requires -- useful when the underlying source's rate might have changed */
> -int pa_source_output_update_rate(pa_source_output *o) {
> + * requires -- useful when the underlying source's sample spec might have changed */
> +int pa_source_output_update_resampler(pa_source_output *o) {
>      pa_resampler *new_resampler;
>      char *memblockq_name;
>  
> diff --git a/src/pulsecore/source-output.h b/src/pulsecore/source-output.h
> index 661b64b..9cbc286 100644
> --- a/src/pulsecore/source-output.h
> +++ b/src/pulsecore/source-output.h
> @@ -307,7 +307,7 @@ pa_usec_t pa_source_output_set_requested_latency(pa_source_output *o, pa_usec_t
>  void pa_source_output_cork(pa_source_output *o, bool b);
>  
>  int pa_source_output_set_rate(pa_source_output *o, uint32_t rate);
> -int pa_source_output_update_rate(pa_source_output *o);
> +int pa_source_output_update_resampler(pa_source_output *o);
>  
>  size_t pa_source_output_get_max_rewind(pa_source_output *o);
>  
> diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
> index b501733..b012516 100644
> --- a/src/pulsecore/source.c
> +++ b/src/pulsecore/source.c
> @@ -1029,8 +1029,6 @@ int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
>      bool alternate_rate_is_usable = false;
>      bool avoid_resampling = s->avoid_resampling;
>  
> -    /* We currently only try to reconfigure the sample rate */
> -
>      if (pa_sample_spec_equal(spec, &s->sample_spec))
>          return 0;
>  
> @@ -1043,14 +1041,14 @@ int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
>      }
>  
>      if (PA_SOURCE_IS_RUNNING(s->state)) {
> -        pa_log_info("Cannot update rate, SOURCE_IS_RUNNING, will keep using %u Hz",
> -                    s->sample_spec.rate);
> +        pa_log_info("Cannot update sample spec, SOURCE_IS_RUNNING, will keep using %s and %u Hz",
> +                    pa_sample_format_to_string(s->sample_spec.format), s->sample_spec.rate);
>          return -1;
>      }
>  
>      if (s->monitor_of) {
>          if (PA_SINK_IS_RUNNING(s->monitor_of->state)) {
> -            pa_log_info("Cannot update rate, this is a monitor source and the sink is running.");
> +            pa_log_info("Cannot update sample spec, this is a monitor source and the sink is running.");
>              return -1;
>          }
>      }
> @@ -1061,12 +1059,15 @@ int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
>      desired_spec = s->sample_spec;
>  
>      if (passthrough) {
> -        /* We have to try to use the source output rate */
> +        /* We have to try to use the source output format and rate */
> +        desired_spec.format = spec->format;
>          desired_spec.rate = spec->rate;
>  
> -    } else if (avoid_resampling && (spec->rate >= default_rate || spec->rate >= alternate_rate)) {
> +    } else if (avoid_resampling) {
>          /* We just try to set the source output's sample rate if it's not too low */
> -        desired_spec.rate = spec->rate;
> +        if (spec->rate >= default_rate || spec->rate >= alternate_rate)
> +            desired_spec.rate = spec->rate;
> +        desired_spec.format = spec->format;
>  
>      } else if (default_rate == spec->rate || alternate_rate == spec->rate) {
>          /* We can directly try to use this rate */
> @@ -1095,7 +1096,8 @@ int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
>      if (!passthrough && pa_source_used_by(s) > 0)
>          return -1;
>  
> -    pa_log_debug("Suspending source %s due to changing the sample rate to %u", s->name, desired_spec.rate);
> +    pa_log_debug("Suspending source %s due to changing format, desired format = %s rate = %u",
> +                 s->name, pa_sample_format_to_string(desired_spec.format), desired_spec.rate);
>      pa_source_suspend(s, true, PA_SUSPEND_INTERNAL);
>  
>      if (s->reconfigure)
> @@ -1135,10 +1137,10 @@ int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
>  
>          PA_IDXSET_FOREACH(o, s->outputs, idx) {
>              if (o->state == PA_SOURCE_OUTPUT_CORKED)
> -                pa_source_output_update_rate(o);
> +                pa_source_output_update_resampler(o);
>          }
>  
> -        pa_log_info("Changed sampling rate successfully");
> +        pa_log_info("Reconfigured successfully");
>      }
>  
>      pa_source_suspend(s, false, PA_SUSPEND_INTERNAL);
-- 
Tanu

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


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux