[pulseaudio-discuss] [PATCH 6/8] pass pa_suspend_cause_t to SINK/SOURCE_SET_STATE handlers

[Georg Chini]

On 22.02.2018 08:47, Tanu Kaskinen wrote:
> On Thu, 2018-02-22 at 08:25 +0100, Georg Chini wrote:
>> On 19.02.2018 15:48, Tanu Kaskinen wrote:
>>> The suspend cause isn't yet used by any of the handlers. 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                 | 11 +++++++++--
>>>    src/modules/alsa/alsa-source.c               | 11 +++++++++--
>>>    src/modules/bluetooth/module-bluez4-device.c | 22 ++++++++++++++++++----
>>>    src/modules/bluetooth/module-bluez5-device.c | 22 ++++++++++++++++++----
>>>    src/modules/echo-cancel/module-echo-cancel.c |  2 +-
>>>    src/modules/module-combine-sink.c            | 10 +++++++++-
>>>    src/modules/module-equalizer-sink.c          |  2 +-
>>>    src/modules/module-esound-sink.c             | 11 +++++++++--
>>>    src/modules/module-ladspa-sink.c             |  2 +-
>>>    src/modules/module-null-sink.c               |  6 ++++--
>>>    src/modules/module-null-source.c             |  2 +-
>>>    src/modules/module-pipe-sink.c               |  9 ++++++---
>>>    src/modules/module-remap-sink.c              |  2 +-
>>>    src/modules/module-sine-source.c             |  2 +-
>>>    src/modules/module-solaris.c                 | 23 ++++++++++++++++++-----
>>>    src/modules/module-tunnel-sink-new.c         | 12 ++++++++++--
>>>    src/modules/module-tunnel-source-new.c       | 12 ++++++++++--
>>>    src/modules/module-virtual-sink.c            |  2 +-
>>>    src/modules/module-virtual-surround-sink.c   |  2 +-
>>>    src/modules/oss/module-oss.c                 | 24 ++++++++++++++++++------
>>>    src/modules/raop/raop-sink.c                 |  9 ++++++++-
>>>    src/pulsecore/sink.c                         | 15 +++++++++------
>>>    src/pulsecore/sink.h                         | 23 +++++++++++++++++++++++
>>>    src/pulsecore/source.c                       | 15 +++++++++------
>>>    src/pulsecore/source.h                       | 23 +++++++++++++++++++++++
>>>    25 files changed, 218 insertions(+), 56 deletions(-)
>>>
>> I would rename pa_sink_message_set_state to pa_sink_set_state_message
>> because the first name sounds like a function name.
> Ok, pa_sink_set_state_message sounds better to me too.
>
>> Also there may be
>> some changes required due to the new patch 2 of the series. Otherwise
>> looks good.
> Raman suggested adding suspend_cause to thread_info. That's not as
> simple as it seems, because the suspend cause has to be updated also
> when resuming fails, but pa_sink_process_msg() isn't called when
> resuming fails, and thread_info.suspend_cause would be set in
> pa_sink_process_msg().
>
> The way I'd solve the suspend_cause updating problem is to add a new
> callback set_state_in_io_thread() that is called from the core
> SET_STATE handler, and all modules would use the callback instead of
> handling the SET_STATE message. Having the callback allows the core
> SET_STATE handler to manage the state updating so that
> thread_info.suspend_cause is always updated no matter what happens in
> the set_state_in_io_thread() callback.
>
> This would also get rid of the awkward rule that the SET_STATE handlers
> must call pa_sink_process_msg() on success but not on failure.
>
> Adding suspend_cause to thread_info is not necessary, so one option is
> to just do nothing, but the refactoring seems beneficial, so I'd prefer
> to do that.
>
Well, I don't mind either way. For Raman's patches it is not necessary
to update the suspend cause in the I/O-thread since the old and new
cause are accessible within the SET_STATE handler (because the main
thread is waiting). On the other hand I like your idea because it makes
the sink/source code more generic and removes possible error causes.