[pulseaudio-discuss] GSOC: bluetooth latency - status report [2/?]

[Tanu Kaskinen]

On Fri, 2012-06-08 at 15:12 +0200, poljarinho at gmail.com wrote:
> Hello mailing list, time for the second status report.
> 
> The cleaned up port implementation for the bluetooth device is already
> on the mailing list as you already saw.
> 
> Now I would like some feedback for the next stage of my GSOC project.
> I have added a latency variable to the pa_device_port struct. This
> latency from the port should then be added to the sink if the port is
> currently active.
> 
> I have some problems following where I should add the latency so it
> will become active. As I have discovered there are two types of sinks.
> Some are with dynamic and some are with fixed latency. 

Yes. In theory there shouldn't be big difference for you: when someone
asks what the latency is right now, regardless of the sink type you
should add the latency offset to what would otherwise be returned.

> If I add the extra latency just to sink->min_latency would that do the
> job? Or is that a bad idea?

I don't see how that could work. The minimum latency is the smallest
latency that the sink can provide, which is very different from the
concept of the latency offset. Your question raises another question,
though: should the latency offset affect pa_sink.thread_info.min_latency
and pa_sink.thread_info.max_latency, i.e. the latency range? I don't
know myself. My guess is that you can get away with ignoring the latency
range, but for 100% correctness it might still be a good idea to change
the latency range in some way... My advice is to ignore it for now.

I think the latency offset should have a separate variable in pa_sink
like what you have added to pa_device_port (with the additional twist
that you'll probably need to copy it to pa_sink.thread_info too for
accessing it from the IO thread). The way to make the offset "active" is
to add it to the value that is returned from... umm... this is more
complicated than I thought. It looks to me like there's no single
central function that would be called always when the sink latency is
needed.

Maybe it makes sense to focus on what matters for A/V sync with
bluetooth. The applications use the timing info from pulseaudio to do
the A/V sync, and the server sees this as
PA_COMMAND_GET_PLAYBACK_LATENCY commands. That command is handled by
command_get_playback_latency() in src/pulsecore/protocol-native.c.

command_get_playback_latency() gets the timing information with the
SINK_INPUT_MESSAGE_UPDATE_LATENCY message, which is sent to the IO
thread. (Btw, is it clear to you what "IO thread" means in pulseaudio?)
That message is processed by sink_input_process_msg() in
protocol-native.c.

The SINK_INPUT_MESSAGE_UPDATE_LATENCY handler calls
pa_sink_get_latency_within_thread(). That sounds like the function that
you'll need to modify. pa_sink_get_latency_within_thread() "sends"
PA_SINK_MESSAGE_GET_LATENCY to the sink, and it's supposed to be handled
by the sink implementation. (I wrote "sends" in quotation marks, because
the message handler is actually called directly instead of actually
sending a message through the message queue. The call is made directly,
because the message queue is meant for inter-thread communication, but
pa_sink_get_latency_within_thread() is executed in the same thread where
PA_SINK_MESSAGE_GET_LATENCY is handled, i.e. the sink IO thread.)

You can have a look at the PA_SINK_MESSAGE_GET_LATENCY handler in
module-bluetooth-device.c if you want, but since we would like to not
require changes in each sink implementation separately, the goal is to
only modify pa_sink_get_latency_within_thread(). It looks like the only
change that is needed is to add the sink's latency offset to the value
that was returned by the PA_SINK_MESSAGE_GET_LATENCY handler.

There are also other situations where the sink's latency is needed. For
example, "pactl list sinks" prints the sink latency, and that number is
acquired through pa_sink_get_latency() instead of
pa_sink_get_latency_within_thread(). So, it would probably make sense to
add the same change to pa_sink_get_latency() too. I'm not sure if it's
enough to modify those two functions to make all cases work, but at
least it's a good start, and enough for the most important case, i.e.
A/V sync.

-- 
Tanu