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.
--
Tanu
https://liberapay.com/tanuk
https://www.patreon.com/tanuk
