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

Submitted by Sangchul Lee on Oct. 31, 2018, 2:48 p.m.

Details

Message ID 1540997331-12440-1-git-send-email-sangchul1011@gmail.com
State New
Series "alsa-sink/source, sink, source: Consider sample format for avoid-resampling/passthrough"
Headers show

Commit Message

Sangchul Lee Oct. 31, 2018, 2:48 p.m.
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.

functions are added as below.
 pa_sink_set_sample_format(), pa_sink_set_sample_rate(),
 pa_source_set_sample_format(), pa_source_set_sample_rate()

Signed-off-by: Sangchul Lee <sc11.lee@samsung.com>
---
Thanks for the kind comments.

 src/modules/alsa/alsa-sink.c   | 103 +++++++++++++++++++++++++++++++---------
 src/modules/alsa/alsa-source.c | 100 ++++++++++++++++++++++++++++++---------
 src/pulsecore/sink-input.c     |  22 ++++-----
 src/pulsecore/sink-input.h     |   2 +-
 src/pulsecore/sink.c           |  90 ++++++++++++++++++++++++-----------
 src/pulsecore/sink.h           |   5 +-
 src/pulsecore/source-output.c  |  22 ++++-----
 src/pulsecore/source-output.h  |   2 +-
 src/pulsecore/source.c         | 104 +++++++++++++++++++++++++----------------
 src/pulsecore/source.h         |   5 +-
 10 files changed, 314 insertions(+), 141 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index 200d12b..3c2110b 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,13 +1089,36 @@  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;
+    bool frame_size_changed = false;
 
     pa_assert(u);
     pa_assert(!u->pcm_handle);
@@ -1111,13 +1142,19 @@  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;
+        frame_size_changed = true;
+    }
+
     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 +1169,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 (frame_size_changed) {
+        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 +1717,41 @@  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);
 
-    for (i = 0; u->supported_rates[i]; i++) {
-        if (u->supported_rates[i] == spec->rate) {
-            supported = true;
+    for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
+        if (u->supported_formats[i] == spec->format) {
+            pa_sink_set_sample_format(u->sink, spec->format);
+            format_supported = true;
             break;
         }
     }
 
-    if (!supported) {
-        pa_log_info("Sink does not support sample rate of %d Hz", spec->rate);
-        return -1;
+    if (!format_supported) {
+        pa_log_info("Sink does not support sample format of %s, set it to default value",
+                    pa_sample_format_to_string(spec->format));
+        pa_sink_set_sample_format(u->sink, u->core->default_sample_spec.format);
     }
 
-    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;
+    for (i = 0; u->supported_rates[i]; i++) {
+        if (u->supported_rates[i] == spec->rate) {
+            pa_sink_set_sample_rate(u->sink, spec->rate);
+            rate_supported = true;
+            break;
+        }
+    }
+
+    if (!rate_supported) {
+        pa_log_info("Sink does not support sample rate of %u, set it to default value", spec->rate);
+        pa_sink_set_sample_rate(u->sink, u->core->default_sample_spec.rate);
     }
 
     /* 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..6f31a6e 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,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->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;
+    bool frame_size_changed = false;
 
     pa_assert(u);
     pa_assert(!u->pcm_handle);
@@ -968,13 +997,19 @@  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;
+        frame_size_changed = true;
+    }
+
     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 +1024,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 (frame_size_changed) {
+        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 +1511,39 @@  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);
 
-    for (i = 0; u->supported_rates[i]; i++) {
-        if (u->supported_rates[i] == spec->rate) {
-            supported = true;
+    for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
+        if (u->supported_formats[i] == spec->format) {
+            pa_source_set_sample_format(u->source, spec->format);
+            format_supported = true;
             break;
         }
     }
 
-    if (!supported) {
-        pa_log_info("Source does not support sample rate of %d Hz", spec->rate);
-        return -1;
+    if (!format_supported) {
+        pa_log_info("Source does not support sample format of %s, set it to default value",
+                    pa_sample_format_to_string(spec->format));
+        pa_source_set_sample_format(u->source, u->core->default_sample_spec.format);
     }
 
-    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;
+    for (i = 0; u->supported_rates[i]; i++) {
+        if (u->supported_rates[i] == spec->rate) {
+            pa_source_set_sample_rate(u->source, spec->rate);
+            rate_supported = true;
+            break;
+        }
     }
 
-    return -1;
+    if (!rate_supported) {
+        pa_log_info("Source does not support sample rate of %u, set it to default value", spec->rate);
+        pa_source_set_sample_rate(u->source, u->core->default_sample_spec.rate);
+    }
+
+    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..45e005e 100644
--- a/src/pulsecore/sink-input.c
+++ b/src/pulsecore/sink-input.c
@@ -417,12 +417,11 @@  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");
-        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);
+        pa_log_info("Trying to change sample spec");
+        pa_sink_reconfigure(data->sink, &data->sample_spec, pa_sink_input_new_data_is_passthrough(data));
     }
 
     if (pa_sink_input_new_data_is_passthrough(data) &&
@@ -616,7 +615,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 +1900,12 @@  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");
-        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("Trying to change sample spec");
+        pa_sink_reconfigure(dest, &i->sample_spec, pa_sink_input_is_passthrough(i));
     }
 
     if (i->moving)
@@ -1925,7 +1923,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 +2262,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..fff7fd6 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -1437,8 +1437,7 @@  void pa_sink_render_full(pa_sink *s, size_t length, pa_memchunk *result) {
 }
 
 /* Called from main thread */
-int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
-    int ret = -1;
+void pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
     pa_sample_spec desired_spec;
     uint32_t default_rate = s->default_sample_rate;
     uint32_t alternate_rate = s->alternate_sample_rate;
@@ -1448,50 +1447,53 @@  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;
+        return;
 
     if (!s->reconfigure)
-        return -1;
+        return;
 
     if (PA_UNLIKELY(default_rate == alternate_rate && !passthrough && !avoid_resampling)) {
         pa_log_debug("Default and alternate sample rates are the same, so there is no point in switching.");
-        return -1;
+        return;
     }
 
     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;
+        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;
     }
 
     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;
+            pa_log_info("Cannot update sample spec, monitor source is RUNNING");
+            return;
         }
     }
 
     if (PA_UNLIKELY(!pa_sample_spec_valid(spec)))
-        return -1;
+        return;
 
     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;
+        desired_spec.format = spec->format;
 
     } else if (default_rate == spec->rate || alternate_rate == spec->rate) {
         /* We can directly try to use this rate */
         desired_spec.rate = spec->rate;
 
-    } else {
+    }
+
+    if (desired_spec.rate != spec->rate) {
         /* See if we can pick a rate that results in less resampling effort */
         if (default_rate % 11025 == 0 && spec->rate % 11025 == 0)
             default_rate_is_usable = true;
@@ -1509,31 +1511,28 @@  int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
     }
 
     if (pa_sample_spec_equal(&desired_spec, &s->sample_spec) && passthrough == pa_sink_is_passthrough(s))
-        return -1;
+        return;
 
     if (!passthrough && pa_sink_used_by(s) > 0)
-        return -1;
+        return;
 
-    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;
     }
 
     pa_sink_suspend(s, false, PA_SUSPEND_INTERNAL);
-
-    return ret;
 }
 
 /* Called from main thread */
@@ -3862,6 +3861,43 @@  done:
     return out_formats;
 }
 
+/* Called from the main thread */
+void pa_sink_set_sample_format(pa_sink *s, pa_sample_format_t format) {
+    pa_sample_format_t old_format;
+
+    pa_assert(s);
+    pa_assert(pa_sample_format_valid(format));
+
+    old_format = s->sample_spec.format;
+    if (old_format == format)
+        return;
+
+    pa_log_info("%s: format: %s -> %s",
+                s->name, pa_sample_format_to_string(old_format), pa_sample_format_to_string(format));
+
+    s->sample_spec.format = format;
+
+    pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SINK | PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
+}
+
+/* Called from the main thread */
+void pa_sink_set_sample_rate(pa_sink *s, uint32_t rate) {
+    uint32_t old_rate;
+
+    pa_assert(s);
+    pa_assert(pa_sample_rate_valid(rate));
+
+    old_rate = s->sample_spec.rate;
+    if (old_rate == rate)
+        return;
+
+    pa_log_info("%s: rate: %u -> %u", s->name, old_rate, rate);
+
+    s->sample_spec.rate = rate;
+
+    pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SINK | PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
+}
+
 /* Called from the main thread. */
 void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume) {
     pa_cvolume old_volume;
diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index 8f25440..a0d0599 100644
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -443,7 +443,7 @@  unsigned pa_device_init_priority(pa_proplist *p);
 
 /**** May be called by everyone, from main context */
 
-int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough);
+void pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough);
 void pa_sink_set_port_latency_offset(pa_sink *s, int64_t offset);
 
 /* The returned value is supposed to be in the time domain of the sound card! */
@@ -512,6 +512,9 @@  bool pa_sink_set_formats(pa_sink *s, pa_idxset *formats);
 bool pa_sink_check_format(pa_sink *s, pa_format_info *f);
 pa_idxset* pa_sink_check_formats(pa_sink *s, pa_idxset *in_formats);
 
+void pa_sink_set_sample_format(pa_sink *s, pa_sample_format_t format);
+void pa_sink_set_sample_rate(pa_sink *s, uint32_t rate);
+
 /*** To be called exclusively by the sink driver, from IO context */
 
 void pa_sink_render(pa_sink*s, size_t length, pa_memchunk *result);
diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
index 955a2ac..1ab90ab 100644
--- a/src/pulsecore/source-output.c
+++ b/src/pulsecore/source-output.c
@@ -365,12 +365,11 @@  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");
-        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("Trying to change sample spec");
+        pa_source_reconfigure(data->source, &data->sample_spec, pa_source_output_new_data_is_passthrough(data));
     }
 
     if (pa_source_output_new_data_is_passthrough(data) &&
@@ -542,7 +541,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 +1532,12 @@  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");
-        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("Trying to change sample spec");
+        pa_source_reconfigure(dest, &o->sample_spec, pa_source_output_is_passthrough(o));
     }
 
     if (o->moving)
@@ -1554,7 +1552,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 +1727,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..859a677 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -1020,7 +1020,7 @@  void pa_source_post_direct(pa_source*s, pa_source_output *o, const pa_memchunk *
 }
 
 /* Called from main thread */
-int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough) {
+void pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough) {
     int ret;
     pa_sample_spec desired_spec;
     uint32_t default_rate = s->default_sample_rate;
@@ -1029,50 +1029,53 @@  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;
+        return;
 
     if (!s->reconfigure && !s->monitor_of)
-        return -1;
+        return;
 
     if (PA_UNLIKELY(default_rate == alternate_rate && !passthrough && !avoid_resampling)) {
         pa_log_debug("Default and alternate sample rates are the same, so there is no point in switching.");
-        return -1;
+        return;
     }
 
     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);
-        return -1;
+        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;
     }
 
     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.");
-            return -1;
+            pa_log_info("Cannot update sample spec, this is a monitor source and the sink is running.");
+            return;
         }
     }
 
     if (PA_UNLIKELY(!pa_sample_spec_valid(spec)))
-        return -1;
+        return;
 
     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 */
         desired_spec.rate = spec->rate;
 
-    } else {
+    }
+
+    if (desired_spec.rate != spec->rate) {
         /* See if we can pick a rate that results in less resampling effort */
         if (default_rate % 11025 == 0 && spec->rate % 11025 == 0)
             default_rate_is_usable = true;
@@ -1090,12 +1093,13 @@  int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
     }
 
     if (pa_sample_spec_equal(&desired_spec, &s->sample_spec) && passthrough == pa_source_is_passthrough(s))
-        return -1;
+        return;
 
     if (!passthrough && pa_source_used_by(s) > 0)
-        return -1;
+        return;
 
-    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)
@@ -1107,24 +1111,9 @@  int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
          * have no idea whether the behaviour with passthrough streams is
          * sensible. */
         if (!passthrough) {
-            pa_sample_spec old_spec = s->sample_spec;
-
             s->sample_spec = desired_spec;
-            ret = pa_sink_reconfigure(s->monitor_of, &desired_spec, false);
-
-            if (ret < 0) {
-                /* Changing the sink rate failed, roll back the old rate for
-                 * the monitor source. Why did we set the source rate before
-                 * calling pa_sink_reconfigure(), you may ask. The reason is
-                 * that pa_sink_reconfigure() tries to update the monitor
-                 * source rate, but we are already in the process of updating
-                 * the monitor source rate, so there's a risk of entering an
-                 * infinite loop. Setting the source rate before calling
-                 * pa_sink_reconfigure() makes the rate == s->sample_spec.rate
-                 * check in the beginning of this function return early, so we
-                 * avoid looping. */
-                s->sample_spec = old_spec;
-            }
+            pa_sink_reconfigure(s->monitor_of, &desired_spec, false);
+            s->sample_spec = s->monitor_of->sample_spec;
         } else
             ret = -1;
     }
@@ -1135,15 +1124,13 @@  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);
-
-    return ret;
 }
 
 /* Called from main thread */
@@ -2935,6 +2922,43 @@  done:
     return out_formats;
 }
 
+/* Called from the main thread */
+void pa_source_set_sample_format(pa_source *s, pa_sample_format_t format) {
+    pa_sample_format_t old_format;
+
+    pa_assert(s);
+    pa_assert(pa_sample_format_valid(format));
+
+    old_format = s->sample_spec.format;
+    if (old_format == format)
+        return;
+
+    pa_log_info("%s: format: %s -> %s",
+                s->name, pa_sample_format_to_string(old_format), pa_sample_format_to_string(format));
+
+    s->sample_spec.format = format;
+
+    pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SOURCE | PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
+}
+
+/* Called from the main thread */
+void pa_source_set_sample_rate(pa_source *s, uint32_t rate) {
+    uint32_t old_rate;
+
+    pa_assert(s);
+    pa_assert(pa_sample_rate_valid(rate));
+
+    old_rate = s->sample_spec.rate;
+    if (old_rate == rate)
+        return;
+
+    pa_log_info("%s: rate: %u -> %u", s->name, old_rate, rate);
+
+    s->sample_spec.rate = rate;
+
+    pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SOURCE | PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
+}
+
 /* Called from the main thread. */
 void pa_source_set_reference_volume_direct(pa_source *s, const pa_cvolume *volume) {
     pa_cvolume old_volume;
diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
index f4b69fe..c1588e8 100644
--- a/src/pulsecore/source.h
+++ b/src/pulsecore/source.h
@@ -416,7 +416,7 @@  bool pa_source_update_proplist(pa_source *s, pa_update_mode_t mode, pa_proplist
 
 int pa_source_set_port(pa_source *s, const char *name, bool save);
 
-int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough);
+void pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough);
 
 unsigned pa_source_linked_by(pa_source *s); /* Number of connected streams */
 unsigned pa_source_used_by(pa_source *s); /* Number of connected streams that are not corked */
@@ -442,6 +442,9 @@  pa_idxset* pa_source_get_formats(pa_source *s);
 bool pa_source_check_format(pa_source *s, pa_format_info *f);
 pa_idxset* pa_source_check_formats(pa_source *s, pa_idxset *in_formats);
 
+void pa_source_set_sample_format(pa_source *s, pa_sample_format_t format);
+void pa_source_set_sample_rate(pa_source *s, uint32_t rate);
+
 /*** To be called exclusively by the source driver, from IO context */
 
 void pa_source_post(pa_source*s, const pa_memchunk *chunk);

Comments

Tanu Kaskinen Nov. 14, 2018, 9:37 a.m.
On Wed, 2018-10-31 at 23:48 +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.
> 
> functions are added as below.
>  pa_sink_set_sample_format(), pa_sink_set_sample_rate(),
>  pa_source_set_sample_format(), pa_source_set_sample_rate()
> 
> Signed-off-by: Sangchul Lee <sc11.lee@samsung.com>
> ---

> @@ -1674,33 +1717,41 @@ 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);
>  
> -    for (i = 0; u->supported_rates[i]; i++) {
> -        if (u->supported_rates[i] == spec->rate) {
> -            supported = true;
> +    for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
> +        if (u->supported_formats[i] == spec->format) {
> +            pa_sink_set_sample_format(u->sink, spec->format);
> +            format_supported = true;
>              break;
>          }
>      }
>  
> -    if (!supported) {
> -        pa_log_info("Sink does not support sample rate of %d Hz", spec->rate);
> -        return -1;
> +    if (!format_supported) {
> +        pa_log_info("Sink does not support sample format of %s, set it to default value",
> +                    pa_sample_format_to_string(spec->format));
> +        pa_sink_set_sample_format(u->sink, u->core->default_sample_spec.format);

This is still not right. We can't assume that u->core-
>default_sample_spec.format is supported by the hardware.

When the sink is created, it opens the device for the first time, using
the default format. If that format isn't supported, some other format
is selected instead (or otherwise the whole sink initialization fails).
I think this format should be used as the default here, because it's
known to work.

>      }
>  
> -    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;
> +    for (i = 0; u->supported_rates[i]; i++) {
> +        if (u->supported_rates[i] == spec->rate) {
> +            pa_sink_set_sample_rate(u->sink, spec->rate);
> +            rate_supported = true;
> +            break;
> +        }
> +    }
> +
> +    if (!rate_supported) {
> +        pa_log_info("Sink does not support sample rate of %u, set it to default value", spec->rate);
> +        pa_sink_set_sample_rate(u->sink, u->core->default_sample_spec.rate);

Same problem here with the rate as with the format earlier.

>      }
>  
>      /* Passthrough status change is handled during unsuspend */
>  
> -    return -1;
> +    return 0;

The function now reports success always. This made me have a look at
the places that check for errors from reconfigure(). There are just two
places that currently care about the reconfigure() return values:
pa_sink_reconfigure(), when it reconfigures a monitor sink, and
pa_source_reconfigure(), when it reconfigures a monitored sink. To me
it looks like those can be safely changed to just assume that the
reconfiguration always succeeds. We should change the reconfigure()
callback return types to void.
Sangchul Lee Nov. 15, 2018, 3:22 p.m.
2018년 11월 14일 (수) 오후 6:38, Tanu Kaskinen <tanuk@iki.fi>님이 작성:
>
> > -    if (!supported) {
> > -        pa_log_info("Sink does not support sample rate of %d Hz", spec->rate);
> > -        return -1;
> > +    if (!format_supported) {
> > +        pa_log_info("Sink does not support sample format of %s, set it to default value",
> > +                    pa_sample_format_to_string(spec->format));
> > +        pa_sink_set_sample_format(u->sink, u->core->default_sample_spec.format);
>
> This is still not right. We can't assume that u->core-
> >default_sample_spec.format is supported by the hardware.
>
> When the sink is created, it opens the device for the first time, using
> the default format. If that format isn't supported, some other format
> is selected instead (or otherwise the whole sink initialization fails).
> I think this format should be used as the default here, because it's
> known to work.

I got your point. I'll correct it with keeping the 'verified' sample
spec and setting it here.

> >
> >      /* Passthrough status change is handled during unsuspend */
> >
> > -    return -1;
> > +    return 0;
>
> The function now reports success always. This made me have a look at
> the places that check for errors from reconfigure(). There are just two
> places that currently care about the reconfigure() return values:
> pa_sink_reconfigure(), when it reconfigures a monitor sink, and
> pa_source_reconfigure(), when it reconfigures a monitored sink. To me
> it looks like those can be safely changed to just assume that the
> reconfiguration always succeeds. We should change the reconfigure()
> callback return types to void.

I agree with that, it'll be revised also. I appreciate your comment.

Regards,
Sangchul