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

[Thomas Guillem]

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? We are accounting for the input just during the buffering phase to get the pts_delay/dejitter, then input timing is totally ignored.


> 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. VLC 3.0 doesn't have the issue. Reproducing this issue is fastidious, and you need to wait a very long time. That is why I hacked the VLC 3.0 udp access_out to mimic the original issue (i.e. creating a drifting server), cf. https://code.videolan.org/tguillem/vlc/-/commit/4a7e799627bdacdea406a277e0a34d723ee32404

With this drifting udp server, VLC 3.0 can synchronize to the input (video are displayed faster/slower), audio is resampled). For VLC 4.0, you will get many frames too late (after 12 seconds with the current overkill drift of 100ms every 1secs) and then video and audio will stop.

> 
> > 
> >  static vlc_tick_t vlc_clock_master_to_system_locked(vlc_clock_t *clock,
> > @@ -282,7 +285,7 @@ static vlc_tick_t
> > vlc_clock_master_to_system_locked(vlc_clock_t *clock, system =
> > vlc_clock_monotonic_to_system_locked(clock, now, ts, rate); }
> > 
> > -    return system + clock->delay * rate;
> > +    return system + main_clock->input_drift + clock->delay * rate;
> >  }
> > 
> >  static vlc_tick_t vlc_clock_slave_update(vlc_clock_t *clock,
> > @@ -370,6 +373,7 @@ vlc_clock_main_t *vlc_clock_main_New(void)
> >      main_clock->master = NULL;
> >      main_clock->rc = 1;
> > 
> > +    main_clock->input_drift = 0;
> >      main_clock->coeff = 1.0f;
> >      main_clock->rate = 1.0f;
> >      main_clock->offset = VLC_TICK_INVALID;
> > @@ -439,6 +443,13 @@ void vlc_clock_main_SetDejitter(vlc_clock_main_t
> > *main_clock, vlc_mutex_unlock(&main_clock->lock);
> >  }
> > 
> > +void vlc_clock_main_SetInputDrift(vlc_clock_main_t *main_clock, double
> > drift) +{
> > +    vlc_mutex_lock(&main_clock->lock);
> > +    main_clock->input_drift = drift;
> > +    vlc_mutex_unlock(&main_clock->lock);
> 
> I doubt that this works as a whole, but if it does, an atomic would work 
> better here.

I miss a signal call here to notify a waiting clock client.
input_drift is set rarely compared to the clock users that we read it for each convert (and we already need this lock).

> 
> 
> > +}
> > +
> >  void vlc_clock_main_ChangePause(vlc_clock_main_t *main_clock, vlc_tick_t
> > now, bool paused)
> >  {
> > diff --git a/src/clock/clock.h b/src/clock/clock.h
> > index f780d448bcc..f628024e057 100644
> > --- a/src/clock/clock.h
> > +++ b/src/clock/clock.h
> > @@ -90,6 +90,14 @@ void vlc_clock_main_SetFirstPcr(vlc_clock_main_t
> > *main_clock, void vlc_clock_main_SetInputDejitter(vlc_clock_main_t
> > *main_clock, vlc_tick_t delay);
> > 
> > +/**
> > + * Set the input drift
> > + *
> > + * Set the average drift calculted by the input_clock, it is used to delay
> > all + * output clocks according to this drift.
> > + **/
> > +void vlc_clock_main_SetInputDrift(vlc_clock_main_t *main_clock, double
> > drift); +
> >  /**
> >   * This function sets the dejitter delay to absorb the clock jitter
> >   *
> 
> 
> -- 
> Rémi Denis-Courmont
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel