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. I don't think my original code
was wrong, it just did some redundant things when updating the
state_changed and suspending variables. I'll send v2 without the
redundant things (and I'll also simplify the "if (!state_changed &&
!suspend_cause_changed)" check - we know that state_changed is false,
so checking it is redundant).
--
Tanu
https://liberapay.com/tanuk
https://www.patreon.com/tanuk
