[1/2] sink: Allow reconfigure to change the complete sample spec

Submitted by Arun Raghavan on May 20, 2018, 6:28 a.m.

Details

Message ID 20180520062859.30207-2-arun@arunraghavan.net
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Arun Raghavan May 20, 2018, 6:28 a.m.
For the passthrough case, we allow the entire sink sample spec to be
changed in reconfigure. This will be needed for high bitrate formats.

Relatedly, we also restore the original spec on leaving passthrough
mode. We were getting away with not doing so in the past as, while
incorrect, not restoring the rate was not disastrous. With the ability
to change channel count, not restoring breaks the meaning of profiles
entirely. The saving and restoration logic is restricted to sink/source
reconfiguration code to allow it to be self-contained and easier to
reason about.

All this also applies to the sample spec. We don't actually explicitly
reconfigure the channel map at the moment, but since
pa_sink/source_reconfigure() can now change the channel count, it seems
to make sense to include the channel map along with that API change for
future use.
---
 src/pulsecore/sink-input.c    | 29 ++++++++++----
 src/pulsecore/sink.c          | 70 ++++++++++++++++++++++++++++------
 src/pulsecore/sink.h          |  8 ++--
 src/pulsecore/source-output.c | 21 ++++++++--
 src/pulsecore/source.c        | 72 +++++++++++++++++++++++++++++------
 src/pulsecore/source.h        |  8 ++--
 6 files changed, 167 insertions(+), 41 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
index ebe5cc957..c84ad2dda 100644
--- a/src/pulsecore/sink-input.c
+++ b/src/pulsecore/sink-input.c
@@ -416,9 +416,9 @@  int pa_sink_input_new(
         /* try to change sink 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");
+        if (pa_sink_reconfigure(data->sink, &data->sample_spec, NULL, pa_sink_input_new_data_is_passthrough(data), false) >= 0)
+            pa_log_info("Spec changed to %u Hz, %d channels", data->sink->sample_spec.rate, data->sink->sample_spec.channels);
     }
 
     if (pa_sink_input_new_data_is_passthrough(data) &&
@@ -617,7 +617,7 @@  static void sink_input_set_state(pa_sink_input *i, pa_sink_input_state_t state)
             !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 */
-            pa_sink_reconfigure(i->sink, &i->sample_spec, pa_sink_input_is_passthrough(i));
+            pa_sink_reconfigure(i->sink, &i->sample_spec, NULL, pa_sink_input_is_passthrough(i), false);
         }
 
         pa_assert_se(pa_asyncmsgq_send(i->sink->asyncmsgq, PA_MSGOBJECT(i), PA_SINK_INPUT_MESSAGE_SET_STATE, PA_UINT_TO_PTR(state), 0, NULL) == 0);
@@ -720,9 +720,16 @@  void pa_sink_input_unlink(pa_sink_input *i) {
     reset_callbacks(i);
 
     if (i->sink) {
-        if (PA_SINK_IS_LINKED(pa_sink_get_state(i->sink)))
+        if (PA_SINK_IS_LINKED(pa_sink_get_state(i->sink))) {
             pa_sink_update_status(i->sink);
 
+            if (pa_sink_input_is_passthrough(i)) {
+                pa_log_debug("Leaving passthrough, trying to restore previous configuration");
+                if (pa_sink_reconfigure(i->sink, NULL, NULL, false, true) >= 0)
+                    pa_log_info("Spec changed to %u Hz, %d channels", i->sink->sample_spec.rate, i->sink->sample_spec.channels);
+            }
+        }
+
         i->sink = NULL;
     }
 
@@ -1738,6 +1745,12 @@  int pa_sink_input_start_move(pa_sink_input *i) {
 
     pa_sink_update_status(i->sink);
 
+    if (pa_sink_input_is_passthrough(i)) {
+        pa_log_debug("Leaving passthrough, trying to restore previous configuration");
+        if (pa_sink_reconfigure(i->sink, NULL, NULL, false, true) >= 0)
+            pa_log_info("Spec changed to %u Hz, %d channels", i->sink->sample_spec.rate, i->sink->sample_spec.channels);
+    }
+
     PA_HASHMAP_FOREACH(v, i->volume_factor_sink_items, state)
         pa_cvolume_remap(&v->volume, &i->sink->channel_map, &i->channel_map);
 
@@ -1908,9 +1921,9 @@  int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, bool save) {
            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");
+        if (pa_sink_reconfigure(dest, &i->sample_spec, NULL, pa_sink_input_is_passthrough(i), false) >= 0)
+            pa_log_info("Spec changed to %u Hz, %d channels", dest->sample_spec.rate, dest->sample_spec.channels);
     }
 
     if (i->moving)
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index b801b6bcc..625dcae5a 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -265,6 +265,8 @@  pa_sink* pa_sink_new(
     s->sample_spec = data->sample_spec;
     s->channel_map = data->channel_map;
     s->default_sample_rate = s->sample_spec.rate;
+    pa_sample_spec_init(&s->saved_spec);
+    pa_channel_map_init(&s->saved_map);
 
     if (data->alternate_sample_rate_is_set)
         s->alternate_sample_rate = data->alternate_sample_rate;
@@ -1440,7 +1442,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 pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, pa_channel_map *map, bool passthrough, bool restore) {
     int ret = -1;
     pa_sample_spec desired_spec;
     uint32_t default_rate = s->default_sample_rate;
@@ -1450,16 +1452,20 @@  int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
     bool default_rate_is_usable = false;
     bool alternate_rate_is_usable = false;
     bool avoid_resampling = s->core->avoid_resampling;
+    pa_channel_map old_map, *new_map;
 
-    /* We currently only try to reconfigure the sample rate */
+    /* We currently only try to reconfigure the sample spec */
 
-    if (pa_sample_spec_equal(spec, &s->sample_spec))
+    pa_assert(restore || (spec != NULL));
+    pa_assert(!restore || (spec == NULL && map == NULL && pa_sample_spec_valid(&s->saved_spec)));
+
+    if (!restore && pa_sample_spec_equal(spec, &s->sample_spec))
         return 0;
 
     if (!s->reconfigure)
         return -1;
 
-    if (PA_UNLIKELY(default_rate == alternate_rate && !passthrough && !avoid_resampling)) {
+    if (PA_UNLIKELY(default_rate == alternate_rate && !passthrough && !restore && !avoid_resampling)) {
         pa_log_debug("Default and alternate sample rates are the same, so there is no point in switching.");
         return -1;
     }
@@ -1477,25 +1483,37 @@  int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
         }
     }
 
-    if (PA_UNLIKELY(!pa_sample_spec_valid(spec)))
+    if (PA_UNLIKELY(!restore && !pa_sample_spec_valid(spec)))
         return -1;
 
-    desired_spec = s->sample_spec;
-
     if (passthrough) {
-        /* We have to try to use the sink input rate */
-        desired_spec.rate = spec->rate;
+        /* Save the previous sample spec and channel map, we will try to restore it when leaving passthrough */
+        s->saved_spec = s->sample_spec;
+        s->saved_map = s->channel_map;
+    }
+
+    if (restore) {
+        /* We try to restore the saved spec */
+        desired_spec = s->saved_spec;
+
+    } else if (passthrough) {
+        /* We have to try to use the sink input spec */
+        desired_spec = *spec;
 
     } else if (avoid_resampling && (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 = s->sample_spec;
         desired_spec.rate = spec->rate;
 
     } else if (default_rate == spec->rate || alternate_rate == spec->rate) {
         /* We can directly try to use this rate */
+        desired_spec = s->sample_spec;
         desired_spec.rate = spec->rate;
 
     } else {
         /* See if we can pick a rate that results in less resampling effort */
+        desired_spec = s->sample_spec;
+
         if (default_rate % 11025 == 0 && spec->rate % 11025 == 0)
             default_rate_is_usable = true;
         if (default_rate % 4000 == 0 && spec->rate % 4000 == 0)
@@ -1520,10 +1538,27 @@  int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
     pa_log_debug("Suspending sink %s due to changing format.", s->name);
     pa_sink_suspend(s, true, PA_SUSPEND_INTERNAL);
 
-    if (s->reconfigure(s, &desired_spec, passthrough) >= 0) {
+    /* Keep the old channel map in case it changes */
+    old_map = s->channel_map;
+
+    if (restore) {
+        /* Restore the previous channel map as well */
+        new_map = &s->saved_map;
+    } else if (map) {
+        /* Set the requested channel map */
+        new_map = map;
+    } else if (desired_spec.channels == s->sample_spec.channels) {
+        /* No requested channel map, but channel count is unchanged so don't change */
+        new_map = &s->channel_map;
+    } else {
+        /* No requested channel map, let the device decide */
+        new_map = NULL;
+    }
+
+    if (s->reconfigure(s, &desired_spec, new_map, passthrough) >= 0) {
         /* update monitor source as well */
         if (s->monitor_source && !passthrough)
-            pa_source_reconfigure(s->monitor_source, &desired_spec, false);
+            pa_source_reconfigure(s->monitor_source, &desired_spec, new_map, false, false);
         pa_log_info("Changed format successfully");
 
         PA_IDXSET_FOREACH(i, s->inputs, idx) {
@@ -1534,8 +1569,21 @@  int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
         ret = 0;
     }
 
+    if (!pa_channel_map_equal(&old_map, &s->channel_map)) {
+        /* Remap stored volumes to the new channel map */
+        pa_cvolume_remap(&s->reference_volume, &old_map, &s->channel_map);
+        pa_cvolume_remap(&s->real_volume, &old_map, &s->channel_map);
+        pa_cvolume_remap(&s->soft_volume, &old_map, &s->channel_map);
+    }
+
     pa_sink_suspend(s, false, PA_SUSPEND_INTERNAL);
 
+    if (restore) {
+        /* Reset saved spec and channel map so we don't try to restore it again */
+        pa_sample_spec_init(&s->saved_spec);
+        pa_channel_map_init(&s->saved_map);
+    }
+
     return ret;
 }
 
diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index abf2d9244..661c04231 100644
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -105,7 +105,9 @@  struct pa_sink {
     bool save_volume:1;
     bool save_muted:1;
 
-    /* Saved volume state while we're in passthrough mode */
+    /* Saved state while we're in passthrough mode */
+    pa_sample_spec saved_spec;
+    pa_channel_map saved_map;
     pa_cvolume saved_volume;
     bool saved_save_volume:1;
 
@@ -266,7 +268,7 @@  struct pa_sink {
 
     /* Called whenever device parameters need to be changed. Called from
      * main thread. */
-    int (*reconfigure)(pa_sink *s, pa_sample_spec *spec, bool passthrough);
+    int (*reconfigure)(pa_sink *s, pa_sample_spec *spec, pa_channel_map *map, bool passthrough);
 
     /* Contains copies of the above data so that the real-time worker
      * thread can work without access locking */
@@ -441,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);
+int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, pa_channel_map *map, bool passthrough, bool restore);
 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! */
diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
index 17f5da5e1..e265ee505 100644
--- a/src/pulsecore/source-output.c
+++ b/src/pulsecore/source-output.c
@@ -365,7 +365,7 @@  int pa_source_output_new(
            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)
+        if (pa_source_reconfigure(data->source, &data->sample_spec, NULL, pa_source_output_new_data_is_passthrough(data), false) >= 0)
             pa_log_info("Rate changed to %u Hz", data->source->sample_spec.rate);
     }
 
@@ -539,7 +539,7 @@  static void source_output_set_state(pa_source_output *o, pa_source_output_state_
             !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 */
-            pa_source_reconfigure(o->source, &o->sample_spec, pa_source_output_is_passthrough(o));
+            pa_source_reconfigure(o->source, &o->sample_spec, NULL, pa_source_output_is_passthrough(o), false);
         }
 
         pa_assert_se(pa_asyncmsgq_send(o->source->asyncmsgq, PA_MSGOBJECT(o), PA_SOURCE_OUTPUT_MESSAGE_SET_STATE, PA_UINT_TO_PTR(state), 0, NULL) == 0);
@@ -608,9 +608,16 @@  void pa_source_output_unlink(pa_source_output*o) {
     reset_callbacks(o);
 
     if (o->source) {
-        if (PA_SOURCE_IS_LINKED(pa_source_get_state(o->source)))
+        if (PA_SOURCE_IS_LINKED(pa_source_get_state(o->source))) {
             pa_source_update_status(o->source);
 
+            if (pa_source_output_is_passthrough(o)) {
+                pa_log_debug("Leaving passthrough, trying to restore previous configuration");
+                if (pa_source_reconfigure(o->source, NULL, NULL, false, true) >= 0)
+                    pa_log_info("Spec changed to %u Hz, %d channels", o->source->sample_spec.rate, o->source->sample_spec.channels);
+            }
+        }
+
         o->source = NULL;
     }
 
@@ -1367,6 +1374,12 @@  int pa_source_output_start_move(pa_source_output *o) {
 
     pa_source_update_status(o->source);
 
+    if (pa_source_output_is_passthrough(o)) {
+        pa_log_debug("Leaving passthrough, trying to restore previous configuration");
+        if (pa_source_reconfigure(o->source, NULL, NULL, false, true) >= 0)
+            pa_log_info("Spec changed to %u Hz, %d channels", o->source->sample_spec.rate, o->source->sample_spec.channels);
+    }
+
     pa_cvolume_remap(&o->volume_factor_source, &o->source->channel_map, &o->channel_map);
 
     o->source = NULL;
@@ -1534,7 +1547,7 @@  int pa_source_output_finish_move(pa_source_output *o, pa_source *dest, bool save
            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)
+        if (pa_source_reconfigure(dest, &o->sample_spec, NULL, pa_source_output_is_passthrough(o), false) >= 0)
             pa_log_info("Rate changed to %u Hz", dest->sample_spec.rate);
     }
 
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index ffb8b6ce4..015e625d9 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -252,6 +252,8 @@  pa_source* pa_source_new(
     s->sample_spec = data->sample_spec;
     s->channel_map = data->channel_map;
     s->default_sample_rate = s->sample_spec.rate;
+    pa_sample_spec_init(&s->saved_spec);
+    pa_channel_map_init(&s->saved_map);
 
     if (data->alternate_sample_rate_is_set)
         s->alternate_sample_rate = data->alternate_sample_rate;
@@ -1023,7 +1025,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) {
+int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, pa_channel_map *map, bool passthrough, bool restore) {
     int ret;
     pa_sample_spec desired_spec;
     uint32_t default_rate = s->default_sample_rate;
@@ -1031,16 +1033,20 @@  int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
     bool default_rate_is_usable = false;
     bool alternate_rate_is_usable = false;
     bool avoid_resampling = s->core->avoid_resampling;
+    pa_channel_map old_map, *new_map;
 
-    /* We currently only try to reconfigure the sample rate */
+    /* We currently only try to reconfigure the sample spec */
 
-    if (pa_sample_spec_equal(spec, &s->sample_spec))
+    pa_assert(restore || (spec != NULL));
+    pa_assert(!restore || (spec == NULL && map == NULL && pa_sample_spec_valid(&s->saved_spec)));
+
+    if (!restore && pa_sample_spec_equal(spec, &s->sample_spec))
         return 0;
 
     if (!s->reconfigure && !s->monitor_of)
         return -1;
 
-    if (PA_UNLIKELY(default_rate == alternate_rate && !passthrough && !avoid_resampling)) {
+    if (PA_UNLIKELY(default_rate == alternate_rate && !passthrough && !restore && !avoid_resampling)) {
         pa_log_debug("Default and alternate sample rates are the same, so there is no point in switching.");
         return -1;
     }
@@ -1058,25 +1064,37 @@  int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
         }
     }
 
-    if (PA_UNLIKELY(!pa_sample_spec_valid(spec)))
+    if (PA_UNLIKELY(!restore && !pa_sample_spec_valid(spec)))
         return -1;
 
-    desired_spec = s->sample_spec;
-
     if (passthrough) {
-        /* We have to try to use the source output rate */
-        desired_spec.rate = spec->rate;
+        /* Save the previous sample spec and channel map, we will try to restore it when leaving passthrough */
+        s->saved_spec = s->sample_spec;
+        s->saved_map = s->channel_map;
+    }
+
+    if (restore) {
+        /* We try to restore the saved spec */
+        desired_spec = s->saved_spec;
+
+    } else if (passthrough) {
+        /* We have to try to use the source output spec */
+        desired_spec = *spec;
 
     } else if (avoid_resampling && (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 = s->sample_spec;
         desired_spec.rate = spec->rate;
 
     } else if (default_rate == spec->rate || alternate_rate == spec->rate) {
         /* We can directly try to use this rate */
+        desired_spec = s->sample_spec;
         desired_spec.rate = spec->rate;
 
     } else {
         /* See if we can pick a rate that results in less resampling effort */
+        desired_spec = s->sample_spec;
+
         if (default_rate % 11025 == 0 && spec->rate % 11025 == 0)
             default_rate_is_usable = true;
         if (default_rate % 4000 == 0 && spec->rate % 4000 == 0)
@@ -1098,11 +1116,28 @@  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.", s->name);
+    pa_log_debug("Suspending source %s due to changing the format.", s->name);
     pa_source_suspend(s, true, PA_SUSPEND_INTERNAL);
 
+    /* Keep the old channel map in case it changes */
+    old_map = s->channel_map;
+
+    if (restore) {
+        /* Restore the previous channel map as well */
+        new_map = &s->saved_map;
+    } else if (map) {
+        /* Set the requested channel map */
+        new_map = map;
+    } else if (desired_spec.channels == s->sample_spec.channels) {
+        /* No requested channel map, but channel count is unchanged so don't change */
+        new_map = &s->channel_map;
+    } else {
+        /* No requested channel map, let the device decide */
+        new_map = NULL;
+    }
+
     if (s->reconfigure)
-        ret = s->reconfigure(s, &desired_spec, passthrough);
+        ret = s->reconfigure(s, &desired_spec, new_map, passthrough);
     else {
         /* This is a monitor source. */
 
@@ -1113,7 +1148,7 @@  int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
             pa_sample_spec old_spec = s->sample_spec;
 
             s->sample_spec = desired_spec;
-            ret = pa_sink_reconfigure(s->monitor_of, &desired_spec, false);
+            ret = pa_sink_reconfigure(s->monitor_of, &desired_spec, NULL, false, false);
 
             if (ret < 0) {
                 /* Changing the sink rate failed, roll back the old rate for
@@ -1144,8 +1179,21 @@  int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, bool passthrough)
         pa_log_info("Changed sampling rate successfully");
     }
 
+    if (!pa_channel_map_equal(&old_map, &s->channel_map)) {
+        /* Remap stored volumes to the new channel map */
+        pa_cvolume_remap(&s->reference_volume, &old_map, &s->channel_map);
+        pa_cvolume_remap(&s->real_volume, &old_map, &s->channel_map);
+        pa_cvolume_remap(&s->soft_volume, &old_map, &s->channel_map);
+    }
+
     pa_source_suspend(s, false, PA_SUSPEND_INTERNAL);
 
+    if (restore) {
+        /* Reset saved spec and channel map so we don't try to restore it again */
+        pa_sample_spec_init(&s->saved_spec);
+        pa_channel_map_init(&s->saved_map);
+    }
+
     return ret;
 }
 
diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
index b0fa608de..acbc71733 100644
--- a/src/pulsecore/source.h
+++ b/src/pulsecore/source.h
@@ -106,7 +106,9 @@  struct pa_source {
     bool save_volume:1;
     bool save_muted:1;
 
-    /* Saved volume state while we're in passthrough mode */
+    /* Saved state while we're in passthrough mode */
+    pa_sample_spec saved_spec;
+    pa_channel_map saved_map;
     pa_cvolume saved_volume;
     bool saved_save_volume:1;
 
@@ -224,7 +226,7 @@  struct pa_source {
 
     /* Called whenever device parameters need to be changed. Called from
      * main thread. */
-    int (*reconfigure)(pa_source *s, pa_sample_spec *spec, bool passthrough);
+    int (*reconfigure)(pa_source *s, pa_sample_spec *spec, pa_channel_map *map, bool passthrough);
 
     /* Contains copies of the above data so that the real-time worker
      * thread can work without access locking */
@@ -414,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);
+int pa_source_reconfigure(pa_source *s, pa_sample_spec *spec, pa_channel_map *map, bool passthrough, bool restore);
 
 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 */

Comments

On Sun, 2018-05-20 at 11:58 +0530, Arun Raghavan wrote:
> Subject: sink: Allow reconfigure to change the complete sample spec

Sources are changed in the same way, so s/sink/sink, source/. Also, it
seems that the intention is to not allow changing the sample format, so
"complete sample spec" is not correct.

> For the passthrough case, we allow the entire sink sample spec to be
> changed in reconfigure. This will be needed for high bitrate formats.
> 
> Relatedly, we also restore the original spec on leaving passthrough
> mode. We were getting away with not doing so in the past as, while
> incorrect, not restoring the rate was not disastrous. With the ability
> to change channel count, not restoring breaks the meaning of profiles
> entirely. The saving and restoration logic is restricted to sink/source
> reconfiguration code to allow it to be self-contained and easier to
> reason about.
> 
> All this also applies to the sample spec. We don't actually explicitly
> reconfigure the channel map at the moment, but since
> pa_sink/source_reconfigure() can now change the channel count, it seems
> to make sense to include the channel map along with that API change for
> future use.

Do you expect there to be some future use? To me making the channel map
configurable seems to just make things more complicated for no reason.

> ---
>  src/pulsecore/sink-input.c    | 29 ++++++++++----
>  src/pulsecore/sink.c          | 70 ++++++++++++++++++++++++++++------
>  src/pulsecore/sink.h          |  8 ++--
>  src/pulsecore/source-output.c | 21 ++++++++--
>  src/pulsecore/source.c        | 72 +++++++++++++++++++++++++++++------
>  src/pulsecore/source.h        |  8 ++--
>  6 files changed, 167 insertions(+), 41 deletions(-)
> 
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index ebe5cc957..c84ad2dda 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -416,9 +416,9 @@ int pa_sink_input_new(
>          /* try to change sink 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");
> +        if (pa_sink_reconfigure(data->sink, &data->sample_spec, NULL, pa_sink_input_new_data_is_passthrough(data), false) >= 0)

This will try to change the channel count even with non-passthrough
streams.

> +            pa_log_info("Spec changed to %u Hz, %d channels", data->sink->sample_spec.rate, data->sink->sample_spec.channels);

In several places a message about the new spec is logged after calling
pa_sink_reconfigure(). I think that logging should be done in
pa_sink_reconfigure().

>      }
>  
>      if (pa_sink_input_new_data_is_passthrough(data) &&
> @@ -617,7 +617,7 @@ static void sink_input_set_state(pa_sink_input *i, pa_sink_input_state_t state)
>              !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 */
> -            pa_sink_reconfigure(i->sink, &i->sample_spec, pa_sink_input_is_passthrough(i));
> +            pa_sink_reconfigure(i->sink, &i->sample_spec, NULL, pa_sink_input_is_passthrough(i), false);

This will try to change the channel count even with non-passthrough
streams.

There was and still is error handling missing for passthrough streams.
Or if passthrough streams can't be corked, then the
pa_sink_input_is_passthrough() call can be replaced with simple
"false".

>          }
>  
>          pa_assert_se(pa_asyncmsgq_send(i->sink->asyncmsgq, PA_MSGOBJECT(i), PA_SINK_INPUT_MESSAGE_SET_STATE, PA_UINT_TO_PTR(state), 0, NULL) == 0);
> @@ -720,9 +720,16 @@ void pa_sink_input_unlink(pa_sink_input *i) {
>      reset_callbacks(i);
>  
>      if (i->sink) {
> -        if (PA_SINK_IS_LINKED(pa_sink_get_state(i->sink)))
> +        if (PA_SINK_IS_LINKED(pa_sink_get_state(i->sink))) {
>              pa_sink_update_status(i->sink);
>  
> +            if (pa_sink_input_is_passthrough(i)) {
> +                pa_log_debug("Leaving passthrough, trying to restore previous configuration");
> +                if (pa_sink_reconfigure(i->sink, NULL, NULL, false, true) >= 0)
> +                    pa_log_info("Spec changed to %u Hz, %d channels", i->sink->sample_spec.rate, i->sink->sample_spec.channels);
> +            }
> +        }
> +
>          i->sink = NULL;
>      }
>  
> @@ -1738,6 +1745,12 @@ int pa_sink_input_start_move(pa_sink_input *i) {
>  
>      pa_sink_update_status(i->sink);
>  
> +    if (pa_sink_input_is_passthrough(i)) {
> +        pa_log_debug("Leaving passthrough, trying to restore previous configuration");
> +        if (pa_sink_reconfigure(i->sink, NULL, NULL, false, true) >= 0)
> +            pa_log_info("Spec changed to %u Hz, %d channels", i->sink->sample_spec.rate, i->sink->sample_spec.channels);
> +    }
> +
>      PA_HASHMAP_FOREACH(v, i->volume_factor_sink_items, state)
>          pa_cvolume_remap(&v->volume, &i->sink->channel_map, &i->channel_map);
>  
> @@ -1908,9 +1921,9 @@ int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, bool save) {
>             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");
> +        if (pa_sink_reconfigure(dest, &i->sample_spec, NULL, pa_sink_input_is_passthrough(i), false) >= 0)
> +            pa_log_info("Spec changed to %u Hz, %d channels", dest->sample_spec.rate, dest->sample_spec.channels);

There was and still is error handling missing for passthrough streams.
Or if passthrough streams can't be moved, then the
pa_sink_input_is_passthrough() call can be replaced with simple
"false".

>      }
>  
>      if (i->moving)
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index b801b6bcc..625dcae5a 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -265,6 +265,8 @@ pa_sink* pa_sink_new(
>      s->sample_spec = data->sample_spec;
>      s->channel_map = data->channel_map;
>      s->default_sample_rate = s->sample_spec.rate;
> +    pa_sample_spec_init(&s->saved_spec);
> +    pa_channel_map_init(&s->saved_map);
>  
>      if (data->alternate_sample_rate_is_set)
>          s->alternate_sample_rate = data->alternate_sample_rate;
> @@ -1440,7 +1442,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 pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, pa_channel_map *map, bool passthrough, bool restore) {

The restore parameter is unnecessary. If spec is non-NULL, then we are
not restoring, and if it is NULL then we are restoring.

>      int ret = -1;
>      pa_sample_spec desired_spec;
>      uint32_t default_rate = s->default_sample_rate;
> @@ -1450,16 +1452,20 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>      bool default_rate_is_usable = false;
>      bool alternate_rate_is_usable = false;
>      bool avoid_resampling = s->core->avoid_resampling;
> +    pa_channel_map old_map, *new_map;
>  
> -    /* We currently only try to reconfigure the sample rate */
> +    /* We currently only try to reconfigure the sample spec */

Only rate and channel count can be changed, not the sample format.

It seems that a check is missing that would make pa_sink_reconfigure()
fail if format reconfiguration is attempted.

There should also be a check that prevents the channels to be changed
with non-passthrough streams.

>  
> -    if (pa_sample_spec_equal(spec, &s->sample_spec))
> +    pa_assert(restore || (spec != NULL));
> +    pa_assert(!restore || (spec == NULL && map == NULL && pa_sample_spec_valid(&s->saved_spec)));
> +
> +    if (!restore && pa_sample_spec_equal(spec, &s->sample_spec))
>          return 0;
>  
>      if (!s->reconfigure)
>          return -1;
>  
> -    if (PA_UNLIKELY(default_rate == alternate_rate && !passthrough && !avoid_resampling)) {
> +    if (PA_UNLIKELY(default_rate == alternate_rate && !passthrough && !restore && !avoid_resampling)) {
>          pa_log_debug("Default and alternate sample rates are the same, so there is no point in switching.");
>          return -1;
>      }

Not visible here, but the "Cannot update rate, SINK_IS_RUNNING" and "
Cannot update rate, monitor source is RUNNING" messages need to be
updated to reflect the fact that we may be reconfiguring the channel
count instead of the rate.

> @@ -1477,25 +1483,37 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>          }
>      }
>  
> -    if (PA_UNLIKELY(!pa_sample_spec_valid(spec)))
> +    if (PA_UNLIKELY(!restore && !pa_sample_spec_valid(spec)))
>          return -1;
>  
> -    desired_spec = s->sample_spec;
> -
>      if (passthrough) {
> -        /* We have to try to use the sink input rate */
> -        desired_spec.rate = spec->rate;
> +        /* Save the previous sample spec and channel map, we will try to restore it when leaving passthrough */
> +        s->saved_spec = s->sample_spec;
> +        s->saved_map = s->channel_map;
> +    }
> +
> +    if (restore) {
> +        /* We try to restore the saved spec */
> +        desired_spec = s->saved_spec;
> +
> +    } else if (passthrough) {
> +        /* We have to try to use the sink input spec */
> +        desired_spec = *spec;
>  
>      } else if (avoid_resampling && (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 = s->sample_spec;
>          desired_spec.rate = spec->rate;
>  
>      } else if (default_rate == spec->rate || alternate_rate == spec->rate) {
>          /* We can directly try to use this rate */
> +        desired_spec = s->sample_spec;
>          desired_spec.rate = spec->rate;
>  
>      } else {
>          /* See if we can pick a rate that results in less resampling effort */
> +        desired_spec = s->sample_spec;
> +
>          if (default_rate % 11025 == 0 && spec->rate % 11025 == 0)
>              default_rate_is_usable = true;
>          if (default_rate % 4000 == 0 && spec->rate % 4000 == 0)

There are a couple of checks that are not visible here that I don't
really understand. Can you tell me what these checks are for?

    if (pa_sample_spec_equal(&desired_spec, &s->sample_spec) && passthrough == pa_sink_is_passthrough(s))
        return -1;

    if (!passthrough && pa_sink_used_by(s) > 0)
        return -1;

The first one is a complete mystery to me. The pa_sink_used_by() check
makes some sense, but we already checked if the sink or its monitor are
running. Why is the pa_sink_used_by() check not done with passthrough
streams?

> @@ -1520,10 +1538,27 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>      pa_log_debug("Suspending sink %s due to changing format.", s->name);
>      pa_sink_suspend(s, true, PA_SUSPEND_INTERNAL);
>  
> -    if (s->reconfigure(s, &desired_spec, passthrough) >= 0) {
> +    /* Keep the old channel map in case it changes */
> +    old_map = s->channel_map;
> +
> +    if (restore) {
> +        /* Restore the previous channel map as well */
> +        new_map = &s->saved_map;
> +    } else if (map) {
> +        /* Set the requested channel map */
> +        new_map = map;
> +    } else if (desired_spec.channels == s->sample_spec.channels) {
> +        /* No requested channel map, but channel count is unchanged so don't change */
> +        new_map = &s->channel_map;
> +    } else {
> +        /* No requested channel map, let the device decide */
> +        new_map = NULL;

I don't see the need for the device to decide. Pushing the decision to
the sink backend requires extra code in the backend for no benefit.

> +    }
> +
> +    if (s->reconfigure(s, &desired_spec, new_map, passthrough) >= 0) {
>          /* update monitor source as well */
>          if (s->monitor_source && !passthrough)
> -            pa_source_reconfigure(s->monitor_source, &desired_spec, false);
> +            pa_source_reconfigure(s->monitor_source, &desired_spec, new_map, false, false);
>          pa_log_info("Changed format successfully");
>  
>          PA_IDXSET_FOREACH(i, s->inputs, idx) {
> @@ -1534,8 +1569,21 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
>          ret = 0;
>      }
>  
> +    if (!pa_channel_map_equal(&old_map, &s->channel_map)) {
> +        /* Remap stored volumes to the new channel map */
> +        pa_cvolume_remap(&s->reference_volume, &old_map, &s->channel_map);
> +        pa_cvolume_remap(&s->real_volume, &old_map, &s->channel_map);
> +        pa_cvolume_remap(&s->soft_volume, &old_map, &s->channel_map);
> +    }

Remapping isn't very nice, because it loses information. When we enter
passthrough, the original volume with the original channel map should
be saved and then the volume should be set to 100% with the new channel
map. Now we first remap the original volume and only then save it.

Could the volume change code be moved from
pa_sink_enter/leave_passthrough() to pa_sink_reconfigure() and
reordered so that remapping isn't needed?

I didn't review the source side changes.