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
