On 25.03.2018 15:48, Tanu Kaskinen wrote: > 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. > Well, you are again right. I tested it here and it does not seem to make a difference in the initial sync. I will send a new patch and ask the user who had massive initial sync problems to re-test.
