[pulseaudio-discuss] [PATCH 4/8] tunnel*: put sink/source after authentication

[Tanu Kaskinen]

On Mon, 2018-07-16 at 20:25 -0400, Yclept Nemo wrote:
> Call 'pa_sink_put' or 'pa_source_put' after the connection is
> authorized. For the new tunnel modules this involves sending a
> module-specific message from the IO thread to the main thread. On
> receipt the sink/source message handler calls 'pa_*_put' if connected
> (this doesn't need to by guarded by a conditional; the message should
> only be sent once). For the old tunnel module this involves moving
> 'pa_*_put' down the call chain of the main thread: 'pa__init' ->
> 'on_connection' -> 'setup_complete_callback'.
> ---
>  src/modules/module-tunnel-sink-new.c   | 14 +++++++++++++-
>  src/modules/module-tunnel-source-new.c | 14 +++++++++++++-
>  src/modules/module-tunnel.c            | 12 ++++++------
>  3 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/src/modules/module-tunnel-sink-new.c b/src/modules/module-tunnel-sink-new.c
> index 802e6a59..b301f999 100644
> --- a/src/modules/module-tunnel-sink-new.c
> +++ b/src/modules/module-tunnel-sink-new.c
> @@ -60,6 +60,10 @@ PA_MODULE_USAGE(
>  #define MAX_LATENCY_USEC (200 * PA_USEC_PER_MSEC)
>  #define TUNNEL_THREAD_FAILED_MAINLOOP 1
>  
> +enum {
> +    SINK_MESSAGE_PUT = PA_SINK_MESSAGE_MAX,
> +};
> +
>  static void stream_state_cb(pa_stream *stream, void *userdata);
>  static void stream_changed_buffer_attr_cb(pa_stream *stream, void *userdata);
>  static void stream_set_buffer_attr_cb(pa_stream *stream, int success, void *userdata);
> @@ -345,6 +349,7 @@ static void context_state_cb(pa_context *c, void *userdata) {
>                  u->thread_mainloop_api->quit(u->thread_mainloop_api, TUNNEL_THREAD_FAILED_MAINLOOP);
>              }
>              u->connected = true;
> +            pa_asyncmsgq_send(u->thread_mq->outq, PA_MSGOBJECT(u->sink), SINK_MESSAGE_PUT, NULL, 0, NULL);

pa_asyncmsgq_send() is synchronous, and it shouldn't be used from the
IO thread, because that can cause deadlocks. I think an asynchronous
message would work fine in this case (i.e. pa_asyncmsgq_post()).

>              break;
>          }
>          case PA_CONTEXT_FAILED:
> @@ -402,6 +407,7 @@ static int sink_process_msg_cb(pa_msgobject *o, int code, void *data, int64_t of
>      struct userdata *u = PA_SINK(o)->userdata;
>  
>      switch (code) {
> +        /* Delivered from the main thread, handled in the IO thread. */
>          case PA_SINK_MESSAGE_GET_LATENCY: {
>              int negative;
>              pa_usec_t remote_latency;
> @@ -429,6 +435,13 @@ static int sink_process_msg_cb(pa_msgobject *o, int code, void *data, int64_t of
>              *((int64_t*) data) = remote_latency;
>              return 0;
>          }
> +
> +        /* Delivered from the IO thread, handled in the main thread. */
> +        case SINK_MESSAGE_PUT: {
> +            if (u->connected)
> +                pa_sink_put(u->sink);

u->connected is supposed to be used only in the IO thread. In what
situation would you want to not put the sink? I suppose at least during
module unloading it could happen that the SINK_MESSAGE_PUT message is
processed, but we don't want to call pa_sink_put().

A separate flag could be set in the beginning of pa__done() to indicate
that the module is being unloaded, but I think it would make sense to
have that flag in the core pa_module struct instead, and set it in
pa_module_free(). There's already pa_module.unload_requested, which is
kind of what we want, but it's currently only set if
pa_module_unload_request() is called. I think
pa_module.unload_requested could be set also from pa_module_free(), and
then it would be a reliable flag for checking whether the module is
being unloaded.

-- 
Tanu

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