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

[Tanu Kaskinen]

On Fri, 2018-03-16 at 11:15 +0100, Georg Chini wrote:
> 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.

The set_state_in_io_thread() callback is never responsible for setting
the state, and as you explained, the state isn't changing, so there
isn't anything to set anyway.

> 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.

Since the state isn't changing, I should change this

+            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;

to this:

+            suspend_cause = 0;
+            state_changed = false;
+            suspend_cause_changed = suspend_cause != s->suspend_cause;
+            resuming = false;

> >   
> > -    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.

Yep. Same change needed as above.

Thanks for the reviews!

-- 
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk