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.
