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.
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.
>
> 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. (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.
>
> 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.
>
> 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.
Oops, I must have been sleeping when defining the constants. I
think the value of PA_SUBSCRIPTION_MASK_ALL confused me.
As said above, I see no need to add a special signal API.
>
>> +/** 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.
>
>> - /* 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.
