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. 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.
- 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. Otherwise looks good. _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
