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

[Georg Chini]

On 05.03.2018 07:53, Luiz Augusto von Dentz wrote:
> Hi George,
>
> On Sun, Mar 4, 2018 at 11:56 PM, Georg Chini <georg at chini.tk> 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 | 271 ++++++++++++++++++---------
>>   1 file changed, 178 insertions(+), 93 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
>> index 7970dda7..7e5f8900 100644
>> --- a/src/modules/bluetooth/module-bluez5-device.c
>> +++ b/src/modules/bluetooth/module-bluez5-device.c
>> @@ -56,7 +56,6 @@ PA_MODULE_LOAD_ONCE(false);
>>   PA_MODULE_USAGE("path=<device object path>"
>>                   "autodetect_mtu=<boolean>");
>>
>> -#define MAX_PLAYBACK_CATCH_UP_USEC (100 * PA_USEC_PER_MSEC)
>>   #define FIXED_LATENCY_PLAYBACK_A2DP (25 * PA_USEC_PER_MSEC)
>>   #define FIXED_LATENCY_PLAYBACK_SCO (125 * PA_USEC_PER_MSEC)
>>   #define FIXED_LATENCY_RECORD_A2DP   (25 * PA_USEC_PER_MSEC)
>> @@ -660,6 +659,34 @@ static int a2dp_process_push(struct userdata *u) {
>>       return ret;
>>   }
>>
>> +static void update_buffer_size(struct userdata *u) {
>> +    int old_bufsize;
>> +    socklen_t len = sizeof(int);
>> +    int ret;
>> +
>> +    ret = getsockopt(u->stream_fd, SOL_SOCKET, SO_SNDBUF, &old_bufsize, &len);
>> +    if (ret == -1) {
>> +        pa_log_warn("Changing bluetooth buffer size: Failed to getsockopt(SO_SNDBUF): %s", pa_cstrerror(errno));
>> +    } else {
>> +        int new_bufsize;
>> +
>> +        /* Set send buffer size as small as possible. The minimum value is 1024 according to the
>> +         * socket man page. The data is written to the socket in chunks of write_buffer_size,
>> +         * so there should at least be room for two chunks in the buffer. Generally, write_buf_size
>> +         * is larger than 512. If not, use the next multiple of write_block_size which is larger
>> +         * than 1024. */
>> +        new_bufsize = 2 * u->write_block_size;
>> +        if (new_bufsize < 1024)
>> +            new_bufsize = (1024 / u->write_block_size + 1) * u->write_block_size;
>> +
>> +         ret = setsockopt(u->stream_fd, SOL_SOCKET, SO_SNDBUF, &new_bufsize, len);
>> +         if (ret == -1)
>> +             pa_log_warn("Changing bluetooth buffer size: Failed to change from %d to %d: %s", old_bufsize, new_bufsize, pa_cstrerror(errno));
>> +         else
>> +             pa_log_info("Changing bluetooth buffer size: Changed from %d to %d", old_bufsize, new_bufsize);
>> +    }
>> +}
>> +
> Not sure why you are doing getsockopt beforehand if you just use it to
> print, perhaps you should check if any change should be done.

Yes, you are right, I will use it to see if a change is needed.

>   btw the
> bluetoothd actually attempts to set a proper buffer size base on the
> MTU size:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/profiles/audio/avdtp.c#n838

That does not seem to happen on my system. The buffer size is equal
to the default value found in /proc/sys/net/core/wmem_default (around
212 kB) and the same obviously seems to be true for other people,
otherwise they would not see lags.

>
> The socket is packet based so we have to be careful if it end up
> writing more than the MTU size the socket will reject the message,
> perhaps we should set the buffer size in terms of ms, like buffer at
> most 200ms, though bluetoothd cannot calculate that as it has no idea
> what codec is in use, so it must be done in PA anyway.
>
We are currently writing to the buffer in chunks of write_block_size,
which is calculated from the MTU. Therefore I think using twice
the block size should be the correct choice.