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

[Tanu Kaskinen]

On Wed, 2017-07-26 at 08:33 +0200, Georg Chini wrote:
> On 26.07.2017 02:21, Tanu Kaskinen wrote:
> > 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.
> 
> You are right, we should do some filtering here, but I don't see the need
> to be able to subscribe to individual signals. I would rather go for a
> "category mask" like the subscription mask and let the client do at least
> some of the filtering.
> The mask could be passed as argument to pa_context_set_signal_callback(),
> no need to implement additional functions.

The filtering should be done at the server end, so
pa_context_set_signal_callback() can't be used, because it doesn't talk
to the server.

Also, can you elaborate on how you would define the masks? They can't
be bitfields like the existing subscription masks, because modules
can't extend a bitfield (at least out-of-tree modules, which I would
like to be able to use the signal system).

> For me, the simplicity of my approach is one of the big advantages.
> You subscribe to just another event and set up a handler with one
> additional function. There is no need for a new "signal API", the only
> difference to the current subscription is that it uses another handler.

Describing that as "the only difference" seems, if not incorrect, a
little bit of stretching the words. One could also say that it's almost
completely different, the only thing shared (from an application
perspective) is that both systems pa_context_subscribe()...

> > 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?
> 
> Well, subscriptions are also handled asynchronously and a signal can
> carry a lot more information. So for me signals are better suited for
> inter-module communication than the current subscriptions.

Yes, the signal system is a better inter-module communication mechanism
than the subscription system, but that's hardly a fair comparison,
because the subscription system is not an inter-module communication
mechanism at all. Modules can't define their own subscription events.

Modules can define hooks, if they want to send events to other modules.
If your grand plan is to replace hooks with something else, can we
leave that discussion for a later time?

> (The
> message interface opens another - synchronous - way of inter-module
> communication, but that's a different patch set.) We could add a
> signal category "internal" that will not be passed on to clients.
> 
> Additionally the native protocol is using pa_subscription_new() to set up
> the client subscription, so both are coupled and you cannot easily change
> one without the other. I tried to avoid code duplication wherever possible.

You could define a new hook that the native protocol can use to get the
signals: PA_CORE_HOOK_SEND_SIGNAL.

> > 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.
> 
> Let's first agree on the API, then I'll do the documentation.
> The message stuff will also need some documentation.

Regarding documentation, the PROTOCOL file needs to be updated too. And
PA_PROTOCOL_VERSION needs to be incremented.

> > > +/** 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".
> 
> I intentionally named the parameter "info" to distinguish from the similar
> argument "message_parameters" in the message functions. For me there
> is a difference, because the parameters in a message are meant to be
> processed and replied to, while for a signal the additional argument has
> the role of just supplying more information to the recipient. Maybe the
> recipient does not even need this information.
> I'm not too attached to the name, so if you want to have it changed, no
> problem.

I still prefer "parameters".

> > 
> > > -    /* 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.
> 
> Do you think it makes sense to have an individual patch for a 
> one-character change?
> I can however send a separate patch if you prefer.

Yes, I think that makes sense. Typos are commonly fixed by patches that
only change the spelling of one word.

You don't need to send comment typo fix patches to the list, you can
just push them.

-- 
Tanu

https://www.patreon.com/tanuk