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

[Tanu Kaskinen]

On Sun, 2018-03-25 at 14:36 +0200, Georg Chini wrote:
> On 25.03.2018 09:44, Georg Chini wrote:
> > On 25.03.2018 08:34, Tanu Kaskinen wrote:
> > > On Sat, 2018-03-24 at 19:27 +0100, Georg Chini wrote:
> > > > 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.
> > > > > > +    }
> > > > > >           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.
> > > 
> > > Dropped? I don't see how that happens. In the first iteration
> > > time_passed is some small value, likely less than one block. audio_sent
> > > is zero. Skipping is done only if (time_passed - audio_sent) is more
> > > than two blocks.
> > 
> > It definitely is more than two blocks.
> > 
> > > 
> > > If the delay is large for some reason, then skipping might happen, but
> > > that's not good. A long startup delay shouldn't cause audio to be
> > > dropped.
> > 
> > Why shouldn't it? If audio accumulated, it needs to be dropped
> > to achieve initial sync.
> > 
> > > AV sync only requires accurate latency reporting, and the GET_LATENCY
> > > handler uses u->started_at in its calculations. The delay between
> > > setup_stream() where started_at is first set and the first
> > > write_block() call doesn't affect the latency at all (and if it did,
> > > GET_LATENCY would not take that latency into account). Consider this
> > > thought experiment: if the startup delay is 10 seconds for some reason,
> > > surely the end latency is still much less 10 seconds (even without
> > > dropping any audio).
> > 
> > Don't understand. If the startup delay is 10 seconds and audio is 
> > accumulating
> > during that time, the delay will always be 10 seconds. How should it 
> > ever change?
> > So without dropping (and without resetting start time) A/V Sync does 
> > not work. 
> 
> Maybe some additional explanation helps:
> 
> Consider you are watching a video and switch from your internal sound card
> to your BT headset. At some point, the thread function will be set up 
> and audio
> will start to accumulate. On the first iteration of the thread function 
> nothing
> happens (and nothing is dropped), it will simply fall through and then 
> wait for
> POLLOUT. This can take some time and the video is continuing. Now, when
> POLLOUT is set, the audio that was sent before needs to be dropped, 
> otherwise
> A/V sync will be lost.

Where do you think the audio accumulates? If waiting for POLLOUT takes
time, then playback will just stall. The video player won't keep
writing more audio if old audio isn't being consumed.

Dropping audio when switching devices during video playback perhaps
isn't a big problem, but dropping audio when starting to play something
is a significant problem. Event sounds may end up not being heard at
all, and when starting to play music, the beginning of a song is lost.

-- 
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk