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.
