[pulseaudio-discuss] [PATCH 0/3] Consider sample format in case of avoid-resampling

[Sangchul Lee]

These patches make it possible to use a stream's original sample format
without resampling when the avoid-resampling option is set to a sink or
a source if the sample format is supported by alsa driver.

In the first patch, it is added to check supported sample formats of a
sink or source in a similar way of getting supported rates. The last
patch has some logics to consider sample format including reinitialization
of size information when it is needed.

Sangchul Lee (3):
alsa-util/sink/source: Add infrastructure for supported sample formats
alsa-sink/source: Rename a variable for supported sample rates in
userdata
alsa-sink/source, sink, source: Consider sample format for
avoid-resampling

src/modules/alsa/alsa-sink.c | 109 +++++++++++++++++++++++++++++++++++------
src/modules/alsa/alsa-source.c | 101 +++++++++++++++++++++++++++++++++-----
src/modules/alsa/alsa-util.c | 78 +++++++++++++++++++++++++++++
src/modules/alsa/alsa-util.h | 1 +
src/pulsecore/sink.c | 7 ++-
src/pulsecore/source.c | 7 ++-
6 files changed, 271 insertions(+), 32 deletions(-)

[Sangchul Lee]

There has been a function to get supported sample rates from alsa and
an array for it in userdata of each module-alsa-sink/source. Similarly,
this patch adds a function to get supported sample formats(bit depth)
from alsa and an array for it to each userdata of the modules.

Signed-off-by: Sangchul Lee <***@samsung.com>
---
src/modules/alsa/alsa-sink.c | 10 ++++++
src/modules/alsa/alsa-source.c | 10 ++++++
src/modules/alsa/alsa-util.c | 78 ++++++++++++++++++++++++++++++++++++++++++
src/modules/alsa/alsa-util.h | 1 +
4 files changed, 99 insertions(+)

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index 871c829..cf07393 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -111,6 +111,7 @@ struct userdata {

pa_cvolume hardware_volume;

+ pa_sample_format_t *supported_formats;
unsigned int *rates;

size_t
@@ -2349,6 +2350,12 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
if ((is_iec958(u) || is_hdmi(u)) && ss.channels == 2)
set_formats = true;

+ u->supported_formats = pa_alsa_get_supported_formats(u->pcm_handle, ss.format);
+ if (!u->supported_formats) {
+ pa_log_error("Failed to find any supported sample formats.");
+ goto fail;
+ }
+
u->rates = pa_alsa_get_supported_rates(u->pcm_handle, ss.rate);
if (!u->rates) {
pa_log_error("Failed to find any supported sample rates.");
@@ -2629,6 +2636,9 @@ static void userdata_free(struct userdata *u) {
if (u->formats)
pa_idxset_free(u->formats, (pa_free_cb_t) pa_format_info_free);

+ if (u->supported_formats)
+ pa_xfree(u->supported_formats);
+
if (u->rates)
pa_xfree(u->rates);

diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
index c32e7e9..cf8644b 100644
--- a/src/modules/alsa/alsa-source.c
+++ b/src/modules/alsa/alsa-source.c
@@ -99,6 +99,7 @@ struct userdata {

pa_cvolume hardware_volume;

+ pa_sample_format_t *supported_formats;
unsigned int *rates;

size_t
@@ -2025,6 +2026,12 @@ pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
pa_log_info("Disabling latency range changes on overrun");
}

+ u->supported_formats = pa_alsa_get_supported_formats(u->pcm_handle, ss.format);
+ if (!u->supported_formats) {
+ pa_log_error("Failed to find any supported sample formats.");
+ goto fail;
+ }
+
u->rates = pa_alsa_get_supported_rates(u->pcm_handle, ss.rate);
if (!u->rates) {
pa_log_error("Failed to find any supported sample rates.");
@@ -2259,6 +2266,9 @@ static void userdata_free(struct userdata *u) {
if (u->smoother)
pa_smoother_free(u->smoother);

+ if (u->supported_formats)
+ pa_xfree(u->supported_formats);
+
if (u->rates)
pa_xfree(u->rates);

diff --git a/src/modules/alsa/alsa-util.c b/src/modules/alsa/alsa-util.c
index 41134ea..3f50aac 100644
--- a/src/modules/alsa/alsa-util.c
+++ b/src/modules/alsa/alsa-util.c
@@ -1430,6 +1430,84 @@ unsigned int *pa_alsa_get_supported_rates(snd_pcm_t *pcm, unsigned int fallback_
return rates;
}

+pa_sample_format_t *pa_alsa_get_supported_formats(snd_pcm_t *pcm, pa_sample_format_t fallback_format) {
+ static const snd_pcm_format_t format_trans_to_pa[] = {
+ [SND_PCM_FORMAT_U8] = PA_SAMPLE_U8,
+ [SND_PCM_FORMAT_A_LAW] = PA_SAMPLE_ALAW,
+ [SND_PCM_FORMAT_MU_LAW] = PA_SAMPLE_ULAW,
+ [SND_PCM_FORMAT_S16_LE] = PA_SAMPLE_S16LE,
+ [SND_PCM_FORMAT_S16_BE] = PA_SAMPLE_S16BE,
+ [SND_PCM_FORMAT_FLOAT_LE] = PA_SAMPLE_FLOAT32LE,
+ [SND_PCM_FORMAT_FLOAT_BE] = PA_SAMPLE_FLOAT32BE,
+ [SND_PCM_FORMAT_S32_LE] = PA_SAMPLE_S32LE,
+ [SND_PCM_FORMAT_S32_BE] = PA_SAMPLE_S32BE,
+ [SND_PCM_FORMAT_S24_3LE] = PA_SAMPLE_S24LE,
+ [SND_PCM_FORMAT_S24_3BE] = PA_SAMPLE_S24BE,
+ [SND_PCM_FORMAT_S24_LE] = PA_SAMPLE_S24_32LE,
+ [SND_PCM_FORMAT_S24_BE] = PA_SAMPLE_S24_32BE,
+ };
+ static snd_pcm_format_t all_formats[] = {
+ SND_PCM_FORMAT_U8,
+ SND_PCM_FORMAT_A_LAW,
+ SND_PCM_FORMAT_MU_LAW,
+ SND_PCM_FORMAT_S16_LE,
+ SND_PCM_FORMAT_S16_BE,
+ SND_PCM_FORMAT_FLOAT_LE,
+ SND_PCM_FORMAT_FLOAT_BE,
+ SND_PCM_FORMAT_S32_LE,
+ SND_PCM_FORMAT_S32_BE,
+ SND_PCM_FORMAT_S24_3LE,
+ SND_PCM_FORMAT_S24_3BE,
+ SND_PCM_FORMAT_S24_LE,
+ SND_PCM_FORMAT_S24_BE,
+ };
+ bool supported[PA_ELEMENTSOF(all_formats)] = {
+ false,
+ };
+ snd_pcm_hw_params_t *hwparams;
+ unsigned int i, j, n;
+ pa_sample_format_t *formats = NULL;
+ int ret;
+
+ snd_pcm_hw_params_alloca(&hwparams);
+
+ if ((ret = snd_pcm_hw_params_any(pcm, hwparams)) < 0) {
+ pa_log_debug("snd_pcm_hw_params_any() failed: %s", pa_alsa_strerror(ret));
+ return NULL;
+ }
+
+ for (i = 0, n = 0; i < PA_ELEMENTSOF(all_formats); i++) {
+ if (snd_pcm_hw_params_test_format(pcm, hwparams, all_formats[i]) == 0) {
+ supported[i] = true;
+ n++;
+ }
+ }
+
+ if (n > 0) {
+ formats = pa_xnew(pa_sample_format_t, n + 1);
+
+ for (i = 0, j = 0; i < PA_ELEMENTSOF(all_formats); i++) {
+ if (supported[i])
+ formats[j++] = format_trans_to_pa[all_formats[i]];
+ }
+
+ formats[j] = PA_SAMPLE_MAX;
+ } else {
+ formats = pa_xnew(pa_sample_format_t, 2);
+
+ formats[0] = fallback_format;
+ if ((ret = snd_pcm_hw_params_set_format(pcm, hwparams, format_trans_to_pa[formats[0]])) < 0) {
+ pa_log_debug("snd_pcm_hw_params_set_format() failed: %s", pa_alsa_strerror(ret));
+ pa_xfree(formats);
+ return NULL;
+ }
+
+ formats[1] = PA_SAMPLE_MAX;
+ }
+
+ return formats;
+}
+
bool pa_alsa_pcm_is_hw(snd_pcm_t *pcm) {
snd_pcm_info_t* info;
snd_pcm_info_alloca(&info);
diff --git a/src/modules/alsa/alsa-util.h b/src/modules/alsa/alsa-util.h
index 8345a0b..6b27339 100644
--- a/src/modules/alsa/alsa-util.h
+++ b/src/modules/alsa/alsa-util.h
@@ -132,6 +132,7 @@ char *pa_alsa_get_driver_name_by_pcm(snd_pcm_t *pcm);
char *pa_alsa_get_reserve_name(const char *device);

unsigned int *pa_alsa_get_supported_rates(snd_pcm_t *pcm, unsigned int fallback_rate);
+pa_sample_format_t *pa_alsa_get_supported_formats(snd_pcm_t *pcm, pa_sample_format_t fallback_format);

bool pa_alsa_pcm_is_hw(snd_pcm_t *pcm);
bool pa_alsa_pcm_is_modem(snd_pcm_t *pcm);

[Tanu Kaskinen]

Post by Sangchul Lee
+ static snd_pcm_format_t all_formats[] = {

I changed the values to be const. Otherwise this patch looks good,
applied.

[Sangchul Lee]

It is changed from 'rates' to 'supported_rates'.

Signed-off-by: Sangchul Lee <***@samsung.com>
---
src/modules/alsa/alsa-sink.c | 18 +++++++++---------
src/modules/alsa/alsa-source.c | 14 +++++++-------
2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index cf07393..1fa2689 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -112,7 +112,7 @@ struct userdata {
pa_cvolume hardware_volume;

pa_sample_format_t *supported_formats;
- unsigned int *rates;
+ unsigned int *supported_rates;

size_t
frame_size,
@@ -1647,14 +1647,14 @@ static bool sink_set_formats(pa_sink *s, pa_idxset *formats) {
* framework, but this must be changed if we do. */

/* Count how many sample rates we support */
- for (idx = 0, n = 0; u->rates[idx]; idx++)
+ for (idx = 0, n = 0; u->supported_rates[idx]; idx++)
n++;

/* First insert non-PCM formats since we prefer those. */
PA_IDXSET_FOREACH(f, formats, idx) {
if (!pa_format_info_is_pcm(f)) {
g = pa_format_info_copy(f);
- pa_format_info_set_prop_int_array(g, PA_PROP_FORMAT_RATE, (int *) u->rates, n);
+ pa_format_info_set_prop_int_array(g, PA_PROP_FORMAT_RATE, (int *) u->supported_rates, n);
pa_idxset_put(u->formats, g, NULL);
}
}
@@ -1680,8 +1680,8 @@ static int sink_reconfigure_cb(pa_sink *s, pa_sample_spec *spec, bool passthroug

pa_assert(u);

- for (i = 0; u->rates[i]; i++) {
- if (u->rates[i] == spec->rate) {
+ for (i = 0; u->supported_rates[i]; i++) {
+ if (u->supported_rates[i] == spec->rate) {
supported = true;
break;
}
@@ -2356,8 +2356,8 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
goto fail;
}

- u->rates = pa_alsa_get_supported_rates(u->pcm_handle, ss.rate);
- if (!u->rates) {
+ u->supported_rates = pa_alsa_get_supported_rates(u->pcm_handle, ss.rate);
+ if (!u->supported_rates) {
pa_log_error("Failed to find any supported sample rates.");
goto fail;
}
@@ -2639,8 +2639,8 @@ static void userdata_free(struct userdata *u) {
if (u->supported_formats)
pa_xfree(u->supported_formats);

- if (u->rates)
- pa_xfree(u->rates);
+ if (u->supported_rates)
+ pa_xfree(u->supported_rates);

reserve_done(u);
monitor_done(u);
diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
index cf8644b..fab592f 100644
--- a/src/modules/alsa/alsa-source.c
+++ b/src/modules/alsa/alsa-source.c
@@ -100,7 +100,7 @@ struct userdata {
pa_cvolume hardware_volume;

pa_sample_format_t *supported_formats;
- unsigned int *rates;
+ unsigned int *supported_rates;

size_t
frame_size,
@@ -1476,8 +1476,8 @@ static int source_reconfigure_cb(pa_source *s, pa_sample_spec *spec, bool passth

pa_assert(u);

- for (i = 0; u->rates[i]; i++) {
- if (u->rates[i] == spec->rate) {
+ for (i = 0; u->supported_rates[i]; i++) {
+ if (u->supported_rates[i] == spec->rate) {
supported = true;
break;
}
@@ -2032,8 +2032,8 @@ pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
goto fail;
}

- u->rates = pa_alsa_get_supported_rates(u->pcm_handle, ss.rate);
- if (!u->rates) {
+ u->supported_rates = pa_alsa_get_supported_rates(u->pcm_handle, ss.rate);
+ if (!u->supported_rates) {
pa_log_error("Failed to find any supported sample rates.");
goto fail;
}
@@ -2269,8 +2269,8 @@ static void userdata_free(struct userdata *u) {
if (u->supported_formats)
pa_xfree(u->supported_formats);

- if (u->rates)
- pa_xfree(u->rates);
+ if (u->supported_rates)
+ pa_xfree(u->supported_rates);

reserve_done(u);
monitor_done(u);

[Tanu Kaskinen]

Post by Sangchul Lee
It is changed from 'rates' to 'supported_rates'.
---
src/modules/alsa/alsa-sink.c | 18 +++++++++---------
src/modules/alsa/alsa-source.c | 14 +++++++-------
2 files changed, 16 insertions(+), 16 deletions(-)

Thanks! Applied.

[Sangchul Lee]

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 <***@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;

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));
+ }
+
+ 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);
+}
+
/* 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;
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;
+ }
+
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) {
+ 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);
+
+ } 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;
+ }
+
+ 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;

} else if (default_rate == spec->rate || alternate_rate == spec->rate) {
@@ -1514,7 +1516,8 @@ 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) {
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index dac085c..efbb5ce 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -1064,8 +1064,10 @@ int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
/* We have to try to use the source output 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 source output's sample rate if it's not too low */
+ desired_spec.format = spec->format;
desired_spec.rate = spec->rate;

} else if (default_rate == spec->rate || alternate_rate == spec->rate) {
@@ -1095,7 +1097,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)

[Tanu Kaskinen]

Post by Sangchul Lee
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.
---
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?

Post by Sangchul Lee
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.

Post by Sangchul Lee
+
+ 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.

Post by Sangchul Lee
+}
+
/* 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).

Post by Sangchul Lee
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)

Post by Sangchul Lee
+
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)

Post by Sangchul Lee
+ 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.

Post by Sangchul Lee
+
+ } 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.

Post by Sangchul Lee
+
+ 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.

[Sangchul Lee]

Thanks for the comments.
Before sharing new patchset, I have two questions about your comment. :)

Post by Tanu Kaskinen

Post by Sangchul Lee
@@ -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?

Post by Sangchul Lee
+ 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

I though these parameters are related to user preference(used for
tuning) with a specific format and rate from module argument.
So I intended to keep these values only for the initial format and
rate from module argument. Therefore default variables are used
when the format or rate is changed to another.
As per your comment, does it need to be changed to keep using original
variables(frag size, tsched size, and so on..) of module argument
in case of the change of format or rate?

Post by Tanu Kaskinen

Post by Sangchul Lee
- } 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.

If having the condition with default rate and alternate_rate, it can
not be set with new sample format of a stream
even if it is different with previous sample format of sink/source. I
though it is no little limitation. For example, if the default
sample rate is 48k and alternative sample rate is 48k or 96k, with a
rate of 44.1k stream will be resampled anyway even though
avoid-resampling feature is enabled and this condition also affects
the case of different sample format similarly.
Is there any reason for this..?

Regards,
Sangchul

[Tanu Kaskinen]

Post by Sangchul Lee
Thanks for the comments.
Before sharing new patchset, I have two questions about your comment. :)

Sorry for the delay in replying, I'm quite a bit behind on the mailing
list mails...

Post by Sangchul Lee

Post by Tanu Kaskinen

Post by Sangchul Lee
@@ -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?

Post by Sangchul Lee
+ 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

I though these parameters are related to user preference(used for
tuning) with a specific format and rate from module argument.
So I intended to keep these values only for the initial format and
rate from module argument. Therefore default variables are used
when the format or rate is changed to another.
As per your comment, does it need to be changed to keep using original
variables(frag size, tsched size, and so on..) of module argument
in case of the change of format or rate?

Yes, I think it's best to use the same logic when setting up the
initial parameters and when setting up the changed format parameters.
Your code can drastically change the latency, for example, which is not
good.

If the user has explicitly requested a particular sample format, then
it would make sense to not do any automatic format changes. If buffer
size related module arguments are used, those don't need to be kept
exactly the same, but we should try to keep the buffer size parameters
as close to the requested values as possible (as we already do during
the initialization).

Post by Sangchul Lee

Post by Tanu Kaskinen

Post by Sangchul Lee
- } 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.

If having the condition with default rate and alternate_rate, it can
not be set with new sample format of a stream
even if it is different with previous sample format of sink/source. I
though it is no little limitation. For example, if the default
sample rate is 48k and alternative sample rate is 48k or 96k, with a
rate of 44.1k stream will be resampled anyway even though
avoid-resampling feature is enabled and this condition also affects
the case of different sample format similarly.
Is there any reason for this..?

I'm not sure I understand what you're trying to say... Are you
wondering why we do resampling even if avoid_resampling is set, if the
stream rate is less than both default_rate and alternate_rate? The
reason is that if there's a stream with very low sample rate, for
example 8000 Hz, that can force other streams to be resampled to the
same low rate as well. To prevent that, we set a lower limit for when
avoid_resampling applies.

Regarding how to deal with the sample format: the desired format and
desired rate should be handled separately. You should add a separate if
block for setting the format:

if (avoid_resampling)
desired_spec.format = spec->format;