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

[Tanu Kaskinen]

On Mon, 2018-03-19 at 22:24 +0200, Tanu Kaskinen wrote:
> On Fri, 2018-03-16 at 20:04 +0100, Georg Chini wrote:
> > On 16.03.2018 19:26, Tanu Kaskinen wrote:
> > > 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;
> > 
> > Yes, that should be OK.
> 
> Actually, the "state = PA_SINK_SUSPENDED" assignment still makes sense,
> because originally the variable was set to IDLE or RUNNING, and
> state_changed should be set to false.

Uh, ignore the last bit: "state_changed should be set to false". Yes,
it should be set to false, but me saying that here doesn't make sense,
since it was going to be set to false anyway...

-- 
Tanu

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