[Bug 640859] basesink incorrectly categorizes timestamp jitter as drift

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=640859
  GStreamer | gst-plugins-base | git

--- Comment #38 from Felipe Contreras <felipe.contreras at gmail.com> 2011-05-02 10:39:50 UTC ---
(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > We (Tandberg/Cisco) recently changed this value from 125ms to 225ms based on
> > > simulations of real-life data from users experiencing glitches in their audio.
> > 
> > Did you try my patch in attachment #179691 [details] [details]?
> 
> I am very happy to try out your patch, but could you make it apply against
> current git?

Done.

> I also think there is a bit of a syntax-mixup here. As I pointed out, the
> drift-tolerance variable is actually (IMO) used for two different things. One
> is the clock-slaving, where I mean it belongs, and the other is for a
> "aligment-threshold" when trying to figure out whether to align the current
> buffer with the previous one.

True.

> Now, for the sake of argument, lets say that we make another configurable
> variable and name it "alignment-threshold", and it will be the maximum amount
> of time you will displace a buffer from its rightful timestamp-location in the
> ringbuffer in order to align it with the previous buffer. 
> 
> Now you could see how this variable is very useful for us. We don´t want to
> drift out of lipsync too much, and if we allow the "alignment-threshold" to be
> very big, we could potentially be accepting to play out buffers that are either
> too early or too late compared to video.

Yeap.

> However, since we deal with jittery networks, the timestamps on the buffers are
> sometimes very differently inter-spaced, and if we always placed the buffers
> according to their timestamps, we would have glitches-galore.

Not necessarily, it depends how bit the jitter is. If it's small, it might not
be noticeable.

> Now I believe the playback-case is similar. You don´t want to allow yourself to
> drift too far from your timestamp (sync with video), but you certainly don´t
> want glitches in the sound.
> 
> If I read your patch correctly, you are allowing for the timestamp to do a sort
> of a "back-and-forth" jump, without resynching on both accounts. This I think
> is a good idea. We often see one positive and one negative resync shortly after
> eachother in the logs. Starvation or bursting are typical causes for this.

Exactly.

> I have written a baseaudiosink testsuite where I feed it with actual real-life
> "problematic" data. (people who have had problems with audio, and logged it for
> me) If you clean up your patch, I could try to run it against the scenarios we
> have, and see if there is any improvements.

That would be great :)

> Also, could you consider making the "jitter-window" configurable with getters
> and setters, as well as maybe creating this second "alignment-threshold"
> variable so that the confusion drift/jitter stops... :)

I have made a property called drift-wait so that this amount of time can be
configured, but I think renaming to "alignment-threshold" should be on a
separate bug report, and I'm not sure people would be in favor of braking ABI
in that way.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.