[pulseaudio-discuss] [PATCH 1/2] subscription: Add signal receiving capability

[Tanu Kaskinen]

On Mon, 2017-07-24 at 14:19 +0200, Georg Chini wrote:
> This patch extends the subscription API, so that signals sent from PulseAudio
> can be processed. A signal can be emitted using pa_signal_post().
> 
> A client can subscribe to signals by specifying PA_SUBSCRIPTION_MASK_SIGNAL as
> one of the subscription mask flags in pa_context_subscribe(). It then needs to
> install a signal handler in addtion to (or instead of) the subsription
> callback. A new function pa_context_set_signal_callback() has been implemented
> to set the signal handler.
> 
> The signal handler will receive three arguments in addition to the usual context
> and userdata:
> sender - string that specifies the origin of the signal
> signal - string that specifies the type of the signal
> signal_info - optional string for additional information
> 
> Implementing signal handling as an extension of the subscription API avoids
> unnecessary code duplication.

With this interface you either subscribe to all signals or none. I
think there needs to be a way to set a filter. Something like this:

pa_signal_set *signals = pa_signal_set_new();
pa_signal_set_add(signals, "some_signal");
pa_signal_set_add(signals, "...");
...
operation = pa_context_subscribe_to_signals(context, signals,
success_cb, userdata);

Alternatively, pa_signal_set_new() could have a variable length
argument list for listing all the signal names in one go.

I'm not convinced that these signals are a good mechanism for inter-
module communication, especially if it's implemented in an asynchronous
manner, so can we drop the change to the pa_subscription_new()
interface?

When we've agreed on the API, high-level documentation needs to be
added to pulse/subscribe.h. I don't think the signal API should be a
part of the subscription API, but subscribe.h also contains the source
for subscribe.html, which is a separate page containing the high-level
description, and that HTML page should describe both event types.

Some more comments below.

> @@ -549,8 +549,14 @@ typedef enum pa_subscription_mask {
>      PA_SUBSCRIPTION_MASK_CARD = 0x0200U,
>      /**< Card events. \since 0.9.15 */
>  
> -    PA_SUBSCRIPTION_MASK_ALL = 0x02ffU
> +    PA_SUBSCRIPTION_MASK_ALL = 0x02ffU,
>      /**< Catch all events */
> +
> +    PA_SUBSCRIPTION_MASK_SIGNAL = 0x0300U,
> +    /**< Signal events */
> +
> +    PA_SUBSCRIPTION_MASK_ALL_PLUS_SIGNALS = 0x03ffU
> +    /**< Catch all events including signals*/

This won't be needed at all if we add
pa_context_subscribe_to_signals(), but I'll note a bug anyway: if the
mask is 0x0300, it will overlap with PA_SUBSCRIPTION_MASK_AUTOLOAD and
PA_SUBSCRIPTION_MASK_CARD. If a separate signal mask is to be defined,
it should be 0x0400.

> +/** Signal event callback prototype */
> +typedef void (*pa_context_signal_cb_t)(pa_context *c, const char *sender, const char *signal, const char *signal_info, void *userdata);

I think "parameters" would be a better name than "signal_info".

> -    /* No need for queuing subscriptions of no one is listening */
> +    /* No need for queuing subscriptions if no one is listening */

You could push this change as a separate patch.

-- 
Tanu

https://www.patreon.com/tanuk