Re: [pulseaudio-discuss] [RAOP] [PATCH] Fix audio synchronisation

[Hajime Fujita]

Hi Colin,

Thank you for working on this!

I haven’t had a chance to actually test the patch, but just skimmed it. The 
idea seems reasonable. Will try to see if I can test it this weekend.

> -c->rtptime += length / 4;
> +c->rtpdiff = length / 4;
> +c->rtptime += c->rtpdiff;

Could you remind me what was “/4”? Was that because 16-bit audio data x 2 
channels?
Sorry, it’s been a few years since I started working on RAOP code.


Thanks,
Hajime

> On Jul 22, 2017, at 9:21 AM, Colin Leroy  wrote:
> 
> On 22 July 2017 at 14h53, Colin Leroy wrote:
> 
> Hi, 
> 
>> I think I've figured out how to cleanly synchronise audio and video
>> with RAOP with relying on my empirical "shifting audio 2300ms in
>> mplayer works".
> 
> *without*relying on, obviously :)
> 
> -- 
> Colin
> ___
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [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
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss