[pulseaudio-discuss] [PATCH v2 4/5] pass pa_suspend_cause_t to set_state_in_io_thread() callbacks

[Georg Chini]

On 13.03.2018 18:40, Tanu Kaskinen wrote:
> The suspend cause isn't yet used by any of the callbacks. The alsa sink
> and source will use it to sync the mixer when the SESSION suspend cause
> is removed. Currently the syncing is done in pa_sink/source_suspend(),
> and I want to change that, because pa_sink/source_suspend() shouldn't
> have any alsa specific code.
> ---
>   src/modules/alsa/alsa-sink.c                 |  7 +++-
>   src/modules/alsa/alsa-source.c               |  7 +++-
>   src/modules/bluetooth/module-bluez4-device.c |  4 +-
>   src/modules/bluetooth/module-bluez5-device.c |  4 +-
>   src/modules/echo-cancel/module-echo-cancel.c |  2 +-
>   src/modules/module-combine-sink.c            |  7 +++-
>   src/modules/module-equalizer-sink.c          |  2 +-
>   src/modules/module-esound-sink.c             |  7 +++-
>   src/modules/module-ladspa-sink.c             |  2 +-
>   src/modules/module-null-sink.c               |  2 +-
>   src/modules/module-null-source.c             |  2 +-
>   src/modules/module-pipe-sink.c               |  2 +-
>   src/modules/module-remap-sink.c              |  2 +-
>   src/modules/module-sine-source.c             |  2 +-
>   src/modules/module-solaris.c                 | 14 ++++++-
>   src/modules/module-tunnel-sink-new.c         |  7 +++-
>   src/modules/module-tunnel-source-new.c       |  7 +++-
>   src/modules/module-virtual-sink.c            |  2 +-
>   src/modules/module-virtual-surround-sink.c   |  2 +-
>   src/modules/oss/module-oss.c                 | 14 ++++++-
>   src/modules/raop/raop-sink.c                 |  7 +++-
>   src/pulsecore/sink.c                         | 57 +++++++++++++++++++++-------
>   src/pulsecore/sink.h                         |  2 +-
>   src/pulsecore/source.c                       | 57 +++++++++++++++++++++-------
>   src/pulsecore/source.h                       |  2 +-
>   25 files changed, 168 insertions(+), 55 deletions(-)
>

...

>   
>   static void pa_sink_volume_change_push(pa_sink *s);
> @@ -429,19 +434,47 @@ static int sink_set_state(pa_sink *s, pa_sink_state_t state, pa_suspend_cause_t
>        * current approach of not setting any suspend cause works well enough. */
>   
>       if (s->set_state_in_main_thread) {
> -        ret = s->set_state_in_main_thread(s, state, suspend_cause);
> -        /* set_state_in_main_thread() is allowed to fail only when resuming. */
> -        pa_assert(ret >= 0 || resuming);
> +        if ((ret = s->set_state_in_main_thread(s, state, suspend_cause)) < 0) {
> +            /* set_state_in_main_thread() is allowed to fail only when resuming. */
> +            pa_assert(resuming);
> +
> +            /* If resuming fails, we set the state to SUSPENDED and
> +             * suspend_cause to 0. */
> +            state = PA_SINK_SUSPENDED;
> +            suspend_cause = 0;
> +            state_changed = state != s->state;
> +            suspend_cause_changed = suspend_cause != s->suspend_cause;
> +            suspending = PA_SINK_IS_OPENED(s->state);
> +            resuming = false;
> +
> +            if (!state_changed && !suspend_cause_changed)
> +                return ret;
> +        }
>       }

I think the above is not correct. When set_state_in_main_thread() fails,
the state of the sink will be SUSPENDED before and after the call. We
know it was suspended before and if resume fails it will still be suspended
after the call. So if the (failing) set_state() forgot to set the state back
to SUSPENDED, we should just do so. Because we don't have a state
change, it does not make sense to send notifications if the handler
falsely set the state to IDLE or RUNNING. This leaves only suspend
changes for further processing. Same comment applies for the source
side.

>   
> -    if (ret >= 0 && s->asyncmsgq && state_changed)
> -        if ((ret = pa_asyncmsgq_send(s->asyncmsgq, PA_MSGOBJECT(s), PA_SINK_MESSAGE_SET_STATE, PA_UINT_TO_PTR(state), 0, NULL)) < 0) {
> +    if (s->asyncmsgq) {
> +        struct set_state_data data = { .state = state, .suspend_cause = suspend_cause };
> +
> +        if ((ret = pa_asyncmsgq_send(s->asyncmsgq, PA_MSGOBJECT(s), PA_SINK_MESSAGE_SET_STATE, &data, 0, NULL)) < 0) {
>               /* SET_STATE is allowed to fail only when resuming. */
>               pa_assert(resuming);
>   
>               if (s->set_state_in_main_thread)
>                   s->set_state_in_main_thread(s, PA_SINK_SUSPENDED, 0);
> +
> +            /* If resuming fails, we set the state to SUSPENDED and
> +             * suspend_cause to 0. */
> +            state = PA_SINK_SUSPENDED;
> +            suspend_cause = 0;
> +            state_changed = state != s->state;
> +            suspend_cause_changed = suspend_cause != s->suspend_cause;
> +            suspending = PA_SINK_IS_OPENED(s->state);
> +            resuming = false;
> +
> +            if (!state_changed && !suspend_cause_changed)
> +                return ret;
>           }
> +    }

The same applies here, only we should call set_state_in_main_thread() again
with state=SUSPENDED and suspend_cause=0. Again, the same is valid for
the source side.

Otherwise looks good.