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

[Georg Chini]

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.

Thanks for the review.

>
>>   /* Run from I/O thread */
>> @@ -852,8 +892,10 @@ static void setup_stream(struct userdata *u) {
>>   
>>       pa_log_debug("Stream properly set up, we're ready to roll!");
>>   
>> -    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK)
>> +    if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
>>           a2dp_set_bitpool(u, u->sbc_info.max_bitpool);
>> +        update_buffer_size(u);
> This is redundant, a2dp_set_bitpool() calls update_buffer_size() by
> itself.

It is not redundant. If the bit pools are equal, a2dp_set_bitpool() returns
immediately without calling update_buffer_size(). Therefore the call is
needed here.

>
>> +    }
>>   
>>       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.

>
>>   
>> +                    /* A new block needs to be sent. */
>>                       if (audio_sent <= time_passed) {
>> -                        pa_usec_t audio_to_send = time_passed - audio_sent;
>> +                        size_t bytes_to_send = pa_usec_to_bytes(time_passed - audio_sent, &u->sample_spec);
>>   
>> -                        /* Never try to catch up for more than 100ms */
>> -                        if (u->write_index > 0 && audio_to_send > MAX_PLAYBACK_CATCH_UP_USEC) {
>> -                            pa_usec_t skip_usec;
>> +                        /* There are more than two blocks that need to be written.
>> +                         * We cannot catch up, therefore discard everything older
>> +                         * than two block sizes. */
> It seems that "cannot catch up" is not always true. It's probably true
> that usually the buffer has room for only two blocks, but it's possible
> that the buffer size is large enough for more blocks. Suggested
> wording:
>
> "There are more than two blocks that need to be written. Usually the
> socket buffer has room for only two blocks, but even if there's more
> room, let's not bother trying to catch up more than two blocks. We'll
> discard everything older than two block sizes."

"Catch up" means catch up with the accumulated audio. We cannot play
faster, so everything we do not discard here will cause a permanent delay.
This has nothing to do with buffer size.

> + if ((result = write_block(u)) < 0)
>>                               goto fail;
>> -                    }
>> -
>> -                    if (n_written == 0)
>> -                        pa_log("Broken kernel: we got EAGAIN on write() after POLLOUT!");
>>   
>> -                    do_write -= n_written;
>> -                    writable = false;
>> -                }
>> +                        blocks_to_write -= result;
>> +                        writable = false;
>> +                        have_written = true;
> Shouldn't have_written be set to true only if result > 0? If we got
> EAGAIN, then no write actually happened.
>
It needs to be set anyway, because we will try to send the same block
again on the next iteration if we got EAGAIN. If have_written is false, the
thread will not be woken up by POLLOUT.