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

[Rémi Denis-Courmont]

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

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


> +}
> +
>  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