[pulseaudio-discuss] [PATCH v2] bluez5-device: Rewrite of thread function, reduce send buffer size for a2dp sink

[Georg Chini]

On 25.03.2018 15:48, Tanu Kaskinen wrote:
> On Sun, 2018-03-25 at 14:36 +0200, Georg Chini wrote:
>> On 25.03.2018 09:44, Georg Chini wrote:
>>> On 25.03.2018 08:34, Tanu Kaskinen wrote:
>>>> On Sat, 2018-03-24 at 19:27 +0100, Georg Chini wrote:
>>>>> On 24.03.2018 18:13, Tanu Kaskinen wrote:
>>>>>> On Mon, 2018-03-05 at 08:49 +0100, Georg Chini wrote:
>>>>>>> The rewrite of the thread function does not change functionality
>>>>>>> much,
>>>>>>> most of it is only cleanup, minor bug fixing  and documentation work.
>>>>>>>
>>>>>>> This patch also changes the send buffer size for a2dp sink to
>>>>>>> avoid lags
>>>>>>> after temporary connection drops, following the proof-of-concept
>>>>>>> patch
>>>>>>> posted by Dmitry Kalyanov.
>>>>>>>
>>>>>>> Bug-Link: https://bugs.freedesktop.org/show_bug.cgi?id=58746
>>>>>>> ---
>>>>>>>     src/modules/bluetooth/module-bluez5-device.c | 275
>>>>>>> ++++++++++++++++++---------
>>>>>>>     1 file changed, 182 insertions(+), 93 deletions(-)
>>>>>> Thanks! Reading the new code caused less trouble than I recall the
>>>>>> bluetooth thread_func() previously having caused, so the changes are
>>>>>> good.
>>>>>>
>>>>>> There are some issues pointed out below.
>>>>>>> +    }
>>>>>>>            u->rtpoll_item = pa_rtpoll_item_new(u->rtpoll,
>>>>>>> PA_RTPOLL_NEVER, 1);
>>>>>>>         pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>>>>>>> @@ -861,7 +903,7 @@ static void setup_stream(struct userdata *u) {
>>>>>>>         pollfd->events = pollfd->revents = 0;
>>>>>>>            u->read_index = u->write_index = 0;
>>>>>>> -    u->started_at = 0;
>>>>>>> +    u->started_at = pa_rtclock_now();
>>>>>> This seems unnecessary. write_block() resets started_at anyway when
>>>>>> the
>>>>>> first write is done.
>>>>>>
>>>>>> Hmm... Now I noticed that u->started_at is used before the first
>>>>>> write.
>>>>>> But it seems wrong to set u->started_at twice using (slightly)
>>>>>> different values. u->started_at is used in this code before the first
>>>>>> write:
>>>>>>
>>>>>>        time_passed = pa_rtclock_now() - u->started_at;
>>>>>>
>>>>>> I think before the first write time_passed should be set to exactly 0.
>>>>> We have to discard audio that accumulated during the startup
>>>>> time of the device to make sure audio is in sync. There will always
>>>>> be some bytes dropped at the start. So it is correct to set the
>>>>> initial time twice.
>>>> Dropped? I don't see how that happens. In the first iteration
>>>> time_passed is some small value, likely less than one block. audio_sent
>>>> is zero. Skipping is done only if (time_passed - audio_sent) is more
>>>> than two blocks.
>>> It definitely is more than two blocks.
>>>
>>>> If the delay is large for some reason, then skipping might happen, but
>>>> that's not good. A long startup delay shouldn't cause audio to be
>>>> dropped.
>>> Why shouldn't it? If audio accumulated, it needs to be dropped
>>> to achieve initial sync.
>>>
>>>> AV sync only requires accurate latency reporting, and the GET_LATENCY
>>>> handler uses u->started_at in its calculations. The delay between
>>>> setup_stream() where started_at is first set and the first
>>>> write_block() call doesn't affect the latency at all (and if it did,
>>>> GET_LATENCY would not take that latency into account). Consider this
>>>> thought experiment: if the startup delay is 10 seconds for some reason,
>>>> surely the end latency is still much less 10 seconds (even without
>>>> dropping any audio).
>>> Don't understand. If the startup delay is 10 seconds and audio is
>>> accumulating
>>> during that time, the delay will always be 10 seconds. How should it
>>> ever change?
>>> So without dropping (and without resetting start time) A/V Sync does
>>> not work.
>> Maybe some additional explanation helps:
>>
>> Consider you are watching a video and switch from your internal sound card
>> to your BT headset. At some point, the thread function will be set up
>> and audio
>> will start to accumulate. On the first iteration of the thread function
>> nothing
>> happens (and nothing is dropped), it will simply fall through and then
>> wait for
>> POLLOUT. This can take some time and the video is continuing. Now, when
>> POLLOUT is set, the audio that was sent before needs to be dropped,
>> otherwise
>> A/V sync will be lost.
> Where do you think the audio accumulates? If waiting for POLLOUT takes
> time, then playback will just stall. The video player won't keep
> writing more audio if old audio isn't being consumed.
>
> Dropping audio when switching devices during video playback perhaps
> isn't a big problem, but dropping audio when starting to play something
> is a significant problem. Event sounds may end up not being heard at
> all, and when starting to play music, the beginning of a song is lost.
>
Well, you are again right. I tested it here and it does not seem to
make a difference in the initial sync. I will send a new patch and
ask the user who had massive initial sync problems to re-test.