On Fri, 2018-06-29 at 04:57 +0900, Sangchul Lee wrote:
> Sample format(e.g. 16 bit, 24 bit) was not considered even if the
> avoid-resampling option is set. 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.
>
> Signed-off-by: Sangchul Lee <sc11.lee at samsung.com>
> ---
> src/modules/alsa/alsa-sink.c | 83 ++++++++++++++++++++++++++++++++++++++----
> src/modules/alsa/alsa-source.c | 79 ++++++++++++++++++++++++++++++++++++----
> src/pulsecore/sink.c | 7 +++-
> src/pulsecore/source.c | 7 +++-
> 4 files changed, 158 insertions(+), 18 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index 1fa2689..355a4ef 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c
> @@ -113,11 +113,20 @@ struct userdata {
>
> pa_sample_format_t *supported_formats;
> unsigned int *supported_rates;
> + struct {
> + pa_sample_spec sample_spec;
> + size_t fragment_size;
> + size_t nfrags;
> + size_t tsched_size;
> + size_t tsched_watermark;
> + size_t rewind_safeguard;
> + } initial_info;
I'm not sure this is needed. What breaks if you don't save the initial
info?
>
> size_t
> frame_size,
> fragment_size,
> hwbuf_size,
> + tsched_size,
> tsched_watermark,
> tsched_watermark_ref,
> hwbuf_unused,
> @@ -1081,12 +1090,42 @@ 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;
> +
> + if (u->initial_info.sample_spec.rate == ss->rate && u->initial_info.sample_spec.format == ss->format) {
> + /* 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;
> + } else {
> + u->fragment_size = pa_usec_to_bytes(u->core->default_fragment_size_msec * PA_USEC_PER_MSEC, ss);
> + u->hwbuf_size = u->core->default_n_fragments * u->fragment_size;
> + u->tsched_size = pa_usec_to_bytes(DEFAULT_TSCHED_BUFFER_USEC, ss);
> + u->tsched_watermark = pa_usec_to_bytes(DEFAULT_TSCHED_WATERMARK_USEC, ss);
> + u->rewind_safeguard = PA_MAX(DEFAULT_REWIND_SAFEGUARD_BYTES, pa_usec_to_bytes(DEFAULT_REWIND_SAFEGUARD_USEC, ss));
> + }
The module arguments should be taken into account where applicable also
when changing the rate or format.
> +
> + u->tsched_watermark_ref = u->tsched_watermark;
> +
> + pa_log_info("Updated frame_size %lu, frames_per_block %lu, fragment_size %lu, hwbuf_size %lu, tsched(size %lu, watermark %lu), rewind_safeguard %lu",
> + u->frame_size, u->frames_per_block, u->fragment_size, u->hwbuf_size, u->tsched_size, u->tsched_watermark, u->rewind_safeguard);
Use %zu instead of %lu with size_t variables.
> +}
> +
> /* 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 tsched_size = 0;
The variable should be named tsched_frames for clarity (period_size and
buffer_size should be period_frames and buffer_frames too).
> char *device_name = NULL;
>
> pa_assert(u);
> @@ -1111,13 +1150,18 @@ static int unsuspend(struct userdata *u) {
> goto fail;
> }
>
> + if (u->sink->avoid_resampling) {
> + update_size(u, &u->sink->sample_spec);
> + tsched_size = u->tsched_size / u->frame_size;
> + }
The parameters need to be updated also when dealing with passthrough
streams. If the only reason for updating the buffer parameters is that
the old buffer size isn't any more divisible by the frame size, then
the if condition can be
if (pa_frame_size(&u->sink->sample_spec) != u->frame_size)
> +
> ss = u->sink->sample_spec;
> period_size = u->fragment_size / u->frame_size;
> buffer_size = 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_size, &buffer_size, tsched_size, &b, &d, true)) < 0) {
> pa_log("Failed to set hardware parameters: %s", pa_alsa_strerror(err));
> goto fail;
> }
> @@ -1132,8 +1176,14 @@ 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) {
> + if (u->sink->avoid_resampling) {
This again doesn't work with passthrough streams. This would be a
better if condition:
if (tsched_frames)
> + u->fragment_size = (size_t)(period_size * u->frame_size);
> + u->hwbuf_size = (size_t)(buffer_size * u->frame_size);
> + pa_proplist_setf(u->sink->proplist, PA_PROP_DEVICE_BUFFERING_BUFFER_SIZE, "%lu", (unsigned long)u->hwbuf_size);
> + pa_proplist_setf(u->sink->proplist, PA_PROP_DEVICE_BUFFERING_FRAGMENT_SIZE, "%lu", (unsigned long)u->fragment_size);
The (unsigned long) casts aren't needed if you use %zu instead of %lu.
> +
> + } else 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));
> @@ -1676,11 +1726,21 @@ static int sink_reconfigure_cb(pa_sink *s, pa_sample_spec *spec, bool passthroug
> int i;
> bool supported = false;
>
> - /* FIXME: we only update rate for now */
> -
> pa_assert(u);
>
> - for (i = 0; u->supported_rates[i]; i++) {
> + for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
> + if (u->supported_formats[i] == spec->format) {
> + supported = true;
> + break;
> + }
> + }
> +
> + if (!supported) {
> + pa_log_info("Sink does not support sample format of %s", pa_sample_format_to_string(spec->format));
> + return -1;
> + }
What if the sink doesn't support the sample format, but it supports the
sample rate? I think pa_sink_reconfigure() and the reconfigure()
callback should reconfigure whatever it can. A simple fail/success
error reporting doesn't really work when there can be partial success,
so we should either change the return type to void or come up with an
error reporting scheme that allows reporting partial success. Probably
just not returning anything is good enough.
> +
> + for (i = 0, supported = false; u->supported_rates[i]; i++) {
> if (u->supported_rates[i] == spec->rate) {
> supported = true;
> break;
> @@ -1693,7 +1753,9 @@ static int sink_reconfigure_cb(pa_sink *s, pa_sample_spec *spec, bool passthroug
> }
>
> if (!PA_SINK_IS_OPENED(s->state)) {
> - pa_log_info("Updating rate for device %s, new rate is %d", u->device_name, spec->rate);
> + pa_log_info("Updating format %s rate %d for device %s",
> + pa_sample_format_to_string(spec->format), spec->rate, u->device_name);
> + u->sink->sample_spec.format = spec->format;
> u->sink->sample_spec.rate = spec->rate;
> return 0;
> }
> @@ -2212,6 +2274,13 @@ 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.sample_spec = ss;
> + 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 fab592f..944c89b 100644
> --- a/src/modules/alsa/alsa-source.c
> +++ b/src/modules/alsa/alsa-source.c
> @@ -101,11 +101,19 @@ struct userdata {
>
> pa_sample_format_t *supported_formats;
> unsigned int *supported_rates;
> + struct {
> + pa_sample_spec sample_spec;
> + 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 +955,40 @@ 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;
> +
> + if (u->initial_info.sample_spec.rate == ss->rate && u->initial_info.sample_spec.format == ss->format) {
> + /* 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;
> + } else {
> + u->fragment_size = pa_usec_to_bytes(u->core->default_fragment_size_msec * PA_USEC_PER_MSEC, ss);
> + u->hwbuf_size = u->core->default_n_fragments * u->fragment_size;
> + u->tsched_size = pa_usec_to_bytes(DEFAULT_TSCHED_BUFFER_USEC, ss);
> + u->tsched_watermark = pa_usec_to_bytes(DEFAULT_TSCHED_WATERMARK_USEC, ss);
> + }
> +
> + u->tsched_watermark_ref = u->tsched_watermark;
> +
> + pa_log_info("Updated frame_size %lu, frames_per_block %lu, fragment_size %lu, hwbuf_size %lu, tsched(size %lu, watermark %lu)",
> + u->frame_size, 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 tsched_size = 0;
>
> pa_assert(u);
> pa_assert(!u->pcm_handle);
> @@ -968,13 +1004,18 @@ static int unsuspend(struct userdata *u) {
> goto fail;
> }
>
> + if (u->source->avoid_resampling) {
> + update_size(u, &u->source->sample_spec);
> + tsched_size = 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;
> 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_size, &buffer_size, tsched_size, &b, &d, true)) < 0) {
> pa_log("Failed to set hardware parameters: %s", pa_alsa_strerror(err));
> goto fail;
> }
> @@ -989,8 +1030,14 @@ 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) {
> + if (u->source->avoid_resampling) {
> + u->fragment_size = (size_t)(period_size * u->frame_size);
> + u->hwbuf_size = (size_t)(buffer_size * u->frame_size);
> + pa_proplist_setf(u->source->proplist, PA_PROP_DEVICE_BUFFERING_BUFFER_SIZE, "%lu", (unsigned long)u->hwbuf_size);
> + pa_proplist_setf(u->source->proplist, PA_PROP_DEVICE_BUFFERING_FRAGMENT_SIZE, "%lu", (unsigned long)u->fragment_size);
> +
> + } else 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));
> @@ -1472,11 +1519,21 @@ static int source_reconfigure_cb(pa_source *s, pa_sample_spec *spec, bool passth
> int i;
> bool supported = false;
>
> - /* FIXME: we only update rate for now */
> -
> pa_assert(u);
>
> - for (i = 0; u->supported_rates[i]; i++) {
> + for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
> + if (u->supported_formats[i] == spec->format) {
> + supported = true;
> + break;
> + }
> + }
> +
> + if (!supported) {
> + pa_log_info("Source does not support sample format of %s", pa_sample_format_to_string(spec->format));
> + return -1;
> + }
> +
> + for (i = 0, supported = false; u->supported_rates[i]; i++) {
> if (u->supported_rates[i] == spec->rate) {
> supported = true;
> break;
> @@ -1489,7 +1546,9 @@ static int source_reconfigure_cb(pa_source *s, pa_sample_spec *spec, bool passth
> }
>
> if (!PA_SOURCE_IS_OPENED(s->state)) {
> - pa_log_info("Updating rate for device %s, new rate is %d", u->device_name, spec->rate);
> + pa_log_info("Updating format %s rate %d for device %s",
> + pa_sample_format_to_string(spec->format), spec->rate, u->device_name);
> + u->source->sample_spec.format = spec->format;
> u->source->sample_spec.rate = spec->rate;
> return 0;
> }
> @@ -1899,6 +1958,12 @@ 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.sample_spec = ss;
> + 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.c b/src/pulsecore/sink.c
> index d617d27..6d1773f 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -1483,8 +1483,10 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
> /* We have to try to use the sink input rate */
> desired_spec.rate = spec->rate;
>
> - } else if (avoid_resampling && (spec->rate >= default_rate || spec->rate >= alternate_rate)) {
> + } else if (avoid_resampling && (spec->format != s->sample_spec.format ||
> + 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_spec.format = spec->format;
> desired_spec.rate = spec->rate;
desired_spec.rate shouldn't be set if spec->rate is less than
default_rate and alternate_rate. Now the rate will be set always if the
format needs changing.
This function needs many other changes too:
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);
return -1;
}
The message doesn't take into account that we may be changing the
format.
if (s->monitor_source) {
if (PA_SOURCE_IS_RUNNING(s->monitor_source->state) == true) {
pa_log_info("Cannot update rate, monitor source is RUNNING");
return -1;
}
}
The message doesn't take into account that we may be changing the
format.
if (passthrough) {
/* We have to try to use the sink input rate */
desired_spec.rate = spec->rate;
With passthrough streams we should set desired_spec.format too.
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);
Instead of desired_spec, we should pass s->sample_spec to
pa_source_reconfigure(), because the sink reconfiguration might have
succeeded only partially.
pa_log_info("Changed format 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_rate() should be renamed to
pa_sink_input_update_resampler(), because the function is not specific
to sample rate. The function is called not only from
pa_sink_reconfigure(), but also when moving the sink input from one
sink to another, in which case any of the sink parameters may have
changed. It looks like the function implementation doesn't need any
changes.
The places that call pa_sink_reconfigure() need to be updated, because
they currently assume that pa_sink_reconfigure() only updates the
sample rate.
--
Tanu
https://liberapay.com/tanuk
https://www.patreon.com/tanuk
