[vlc-devel] [PATCH v3 2/6] clock: add vlc_clock_main_SetInputDrift

[Rémi Denis-Courmont]

Le maanantaina 15. maaliskuuta 2021, 11.05.17 EET Thomas Guillem a écrit :
> On Sat, Mar 13, 2021, at 08:46, Rémi Denis-Courmont wrote:
> > Le vendredi 12 mars 2021, 18:08:17 EET Thomas Guillem a écrit :
> > > ---
> > > 
> > >  src/clock/clock.c | 15 +++++++++++++--
> > >  src/clock/clock.h |  8 ++++++++
> > >  2 files changed, 21 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/clock/clock.c b/src/clock/clock.c
> > > index cfbb5eb6f6d..0bb2a9fe375 100644
> > > --- a/src/clock/clock.c
> > > +++ b/src/clock/clock.c
> > > @@ -45,6 +45,7 @@ struct vlc_clock_main_t
> > > 
> > >      average_t coeff_avg; /* Moving average to smooth out the instant
> > >      coeff
> > > 
> > > */ double rate;
> > > 
> > >      double coeff;
> > > 
> > > +    vlc_tick_t input_drift;
> > > 
> > >      vlc_tick_t offset;
> > >      vlc_tick_t delay;
> > > 
> > > @@ -88,6 +89,7 @@ static vlc_tick_t
> > > main_stream_to_system(vlc_clock_main_t
> > > *main_clock, static void vlc_clock_main_reset(vlc_clock_main_t
> > > *main_clock)
> > > 
> > >  {
> > >  
> > >      AvgReset(&main_clock->coeff_avg);
> > > 
> > > +    main_clock->input_drift = 0;
> > > 
> > >      main_clock->coeff = 1.0f;
> > >      main_clock->rate = 1.0f;
> > >      main_clock->offset = VLC_TICK_INVALID;
> > > 
> > > @@ -266,7 +268,8 @@ static vlc_tick_t
> > > vlc_clock_slave_to_system_locked(vlc_clock_t *clock, system =
> > > vlc_clock_monotonic_to_system_locked(clock, now, ts, rate); }
> > > 
> > > -    return system + (clock->delay - main_clock->delay) * rate;
> > > +    return system + main_clock->input_drift +
> > > +        (clock->delay - main_clock->delay) * rate;
> > > 
> > >  }
> > 
> > That does not look right. AFAIK, we already account for the input in the
> > current design,
> 
> How do we account for the input in the current design?

I don't know or don't remember.

> We are accounting for the input just during the buffering phase to get the
> pts_delay/dejitter, then input timing is totally ignored.

It may well be so. In fact, there is no API for access-demux to report its 
timings anyway. es_out_SetPCR() fails miserably in that role. All it really 
says is that the demuxer has queued all data necessary to present up to the 
given timestamp, the so-called PCR.

If we really want to report input timings, we need a separate function to 
provide a pair of an input timestamp and a local monotonic clock timestamp, 
from which we can actually make a reasonable correlation. The es_out_SetPCR() 
approach adds a lot of noise on a potentially already imprecise clock source.

And then some paced input pretend not to be paced.

> > and it seems very suspicious that there'd be both jitter and
> > drift values from the same input in this computation.
> > 
> > IMO, it's on you to explain what is wrong with the current design and how
> > this fixes it, if it does. You can't just add a factor with
> > self-referential documentation that does not really explain much.
> 
> I will try to improve the documentation.
> 
> Meanwhile, here is an explanation of this commit:
> 
> Users reported us that VLC 4.0 would block (no video, no audio, but not a
> deadlock) after a long time with some live streams.

Geez, why does it take a dozen messages before you explain the problem?

> VLC 3.0 doesn't have the issue.

I'm pretty sure I have seen similar issues with the "old" clock.

Obviously, if there is no input clock on a paced input, then depending if the 
input is slower or faster than the output, we will eventually permanently get 
too far ahead or behind. And the only way to avoid this is to have an input 
clock as the reference clock.

Though again, I don't think we have ever had a proper solution for that. It 
kinda worked 20 years ago with SetPCR, when VLC was much more lightweight. 
Though even then, it was highly prone to excessive pitch shifting. Nowadays, I 
think that approach is just too noisy to work in any reasonable way.

-- 
レミ・デニ-クールモン
http://www.remlab.net/