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

[Georg Chini]

On 26.04.2018 17:38, Georg Chini wrote:
> On 26.04.2018 15:13, Tanu Kaskinen wrote:
>> On Thu, 2018-03-29 at 15:33 +0200, 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
>>> ---
>>> Changes in v2:
>>>   - fix issues pointed out by Tanu
>>>   - set writable to false for HSP only if a block really needs to be 
>>> written
>>>
>>>   src/modules/bluetooth/module-bluez5-device.c | 289 
>>> ++++++++++++++++++---------
>>>   1 file changed, 191 insertions(+), 98 deletions(-)
>>>
>>>   +                        blocks_to_write -= result;
>>> +                        if (blocks_to_write > 0)
>>> +                            writable = false;
>> This "blocks_to_write > 0" check is new. In the previous version
>> writable was set to false unconditionally, and that's how I believe it
>> should be done. I guess this is an optimization: we won't write
>> anything before we get POLLIN, so to you it seemed unnecessary to wake
>> up on POLLOUT. Getting the POLLOUT notification is indeed unnecessary
>> in itself, but we still need to set POLLOUT when polling, because
>> otherwise when we wake up due to POLLIN, pollfd->revents will not have
>> POLLOUT set even if the socket is writable. As a result we won't write
>> when we're supposed to.
>>
>> Or actually... if we wake up on POLLIN, and POLLOUT isn't in revents
>> even if the socket is writable, we'll set POLLOUT on the subsequent
>> poll, and that will return immediately, so there's not much delay with
>> the write. So maybe the "blocks_to_write > 0" is fine after all. Some
>> kind of a comment would probably be good in any case. For example:
>>
>> "writable controls whether we set POLLOUT when polling - we set it to
>> false to enable POLLOUT. If there are more blocks to write, we want to
>> be woken up immediately when the socket becomes writable. If there
>> aren't currently any more blocks to write, then we'll have to wait
>> until we've received more data, so in that case we only want to set
>> POLLIN. Note that when we are woken up the next time, POLLOUT won't be
>> set in revents even if the socket has meanwhile become writable, which
>> may seem bad, but in that case we'll set POLLOUT in the subsequent
>> poll, and the poll will return immediately, so our writes won't be
>> delayed."
>>
> Yes it is an optimization and it works fine and significantly reduces
> CPU load on slow systems (which was the reason to implement it).
> I will add your comment to clarify.
Thinking again, you will end up in the situation you describe anyway:
If writable is set to false and POLLOUT is set, the rtpoll run will
immediately return. Then writable will be true, but since we have no
block to write, it will not be set to false. So again you disable POLLOUT,
just one iteration later (unless POLLIN was also set).