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
