[gstreamer-bugs] [Bug 640859] basesink default drift-tolerance causes glitches on some clips

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #29 from Felipe Contreras <felipe.contreras at gmail.com> 2011-02-07 14:34:45 UTC ---
(In reply to comment #28)
> Looks like that breath was not deep enough or the air not quite fresh ...
> Anyway, here goes nothing ...
> 
> There may be some merit in the "not being hasty" approach/patch, if only as its
> approach compares to the current one similarly to how
> http://gentrans.sourceforge.net/docs/head/gst-entrans-plugins/html/gst-entrans-plugins-stamp.html
> compares to stock videorate ;)
> 
> However, some other thoughts:
> * after quite some comments given above on the "arbitrariness" of
> drift-tolerance, another arbitrary parameter has now been sneaked in; a
> non-configurable 1s (whereas e.g. stamp's sync-margin and sync-interval are
> configurable, as is basesink's drift-tolerance)

The problem with drift-tolerance is that we cannot really choose any value, if
the value is below certain threshold, crap is ensured. Therefore it's
understandable that it was made a parameter.

However, would we really need to change this "drift-wait" (1s value)? Perhaps
not if it depends on an already configurable value, like "buffer-time".

IMO ideally both "drift-tolerance" and "drift-wait" should disappear from the
user's view, and the stream would Just Work in any situations.

> * implementation problems (?): e.g. note the asymmetry in 
> time - sink->priv->discont_time >= GST_SECOND
> so what if time is that busted that it is not even advancing enough?  A better
> measure might be the amount of samples that have "passed", and maybe the
> current playback rate has to be kept in mind somewhere as well.

That's true. I would prefer a value that matches some user-visible elapsed
time.

> In summary, I would not mind such an approach, provided safe implementation,
> and any new (few?) parameters are configurable, and preferably have defaults
> that maintain current behaviour [*]

I disagree. Perhaps the first patch would make sense to keep with the current
behavior "drift-wait" = 0, but there's no point in keeping it at a value that
we already know is broken.

> Also, btw, it is not quite hard to come up with broken timestamps
> configurations that are not handled by current or patched approach, other than
> maybe setting some parameters to "infinity" which pretty comes down to
> operating in sync=false (so it's all at least partially academic anyway).

Well, what would those be? Patches welcome for my dummyfilter :)

> [*] even though some might consider the current one needs "fixing".
> However, the above has precisely illustrated that render behaviour can and will
> very well change, and while one might imagine it improves or has no ill effect
> in some cases (e.g. a/v playback sync), it may or may not have a practical
> impact in other cases (e.g. live voip).  That practical impact also has a
> subjective aspect similar to sensitivity threshold for a/v sync etc, and as
> such feel any messing with it should err on the side of caution.  Any
> application is free to configure other values and/or to run with patched up
> alternative defaults.

I disagree.

Suppose a worst case scenario of 40ms drift per second, that is so bad, that
you would hear a glitch of at least 40ms each second. Very bad, but it works
(there could be worst scenarios, but I'm not considering them for this
demonstration purpose, although I could if needed, the conclusion is the same).

My patch would affect negatively by delaying the resync one second, and thus
the glitch would be 80ms each 2 seconds. This could be reduced significantly if
we use the buffer-time (200ms) instead, so the resync glitch would be 48ms each
1.2s.

Now, if your objective is to never exceed the current 40ms drift-tolerance, we
can set the drift-tolerance to 20ms and drift-wait to 500ms, and we would never
exceed that.

max_resync_time = drift-tolerance + drift-rate * drift-wait
max_resync_interval = drift-tolerance / drift-rate + drift-wait

Or even better, we can calculate this dynamically, so that max_resync_interval
never exceeds the drift-tolerance, but in that case, we would need to
dynamically calculate the average drift-rate.

-- 
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.