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

[Georg Chini]

On 09.03.2018 10:20, Luiz Augusto von Dentz wrote:
>   Hi George,
>
> On Mon, Mar 5, 2018 at 9:49 AM, 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 | 275 ++++++++++++++++++---------
>>   1 file changed, 182 insertions(+), 93 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
>> index 7970dda7..149d1708 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,38 @@ 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_block_size, so
>> +         * there should at least be room for two chunks in the buffer. Generally, write_block_size
>> +         * is larger than 512. If not, use the next multiple of write_block_size which is larger
>> +         * than 1024. */
> Btw, the man page actually says it is 2048 bytes but perhaps you are
> considering the kernel doubles the valuse set for bookkeeping? Anyway
> getsockopt shall always return 2048 at least.

According to the man page, the value you write with setsockopt is multiplied
by two by the kernel and 1024 is the minimum you can set, leading to 2048
bytes of buffer. (In fact, in my tests the number returned by getsockopt
never went below 4906, don't know why it is such a weird number.)

>
>> +        new_bufsize = 2 * u->write_block_size;
>> +        if (new_bufsize < 1024)
>> +            new_bufsize = (1024 / u->write_block_size + 1) * u->write_block_size;
>> +
>> +        /* The kernel internally doubles the buffer size that was set by setsockopt and getsockopt
>> +         * returns the doubled value. */
> Btw, what happens if the buffer is reduced, for example when the
> bitpool is reduced, and there are packets still on the buffer? Are
> they discarded?
>
Yes, they are discarded. This only happens when the connection is 
temporary lost,
so the audio is old anyway by the time it is discarded.