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
