[vlc-devel] [RFC PATCH 3/9] aout: fix data races with aout_RequestRetiming()
Thomas Guillem
thomas at gllm.fr
Thu Dec 10 20:02:08 CET 2020
On Thu, Dec 10, 2020, at 19:48, Rémi Denis-Courmont wrote:
> Le jeudi 10 décembre 2020, 19:10:09 EET Thomas Guillem a écrit :
> > aout_DecAdjustDrift() should never called called from this callback
> > since it causes an aout->play (for the silence).
>
> Grammar.
>
> > Store the last reported drift that will be processed before playing the
> > next audio block.
> >
> > sync.rate need also to be protected since it can now be read by any
> > threads (the thread calling aout_RequestRetiming()).
> > ---
> > src/audio_output/aout_internal.h | 3 +-
> > src/audio_output/dec.c | 55 ++++++++++++++++++++------------
> > src/audio_output/output.c | 3 ++
> > 3 files changed, 40 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/audio_output/aout_internal.h
> > b/src/audio_output/aout_internal.h index 86b42c6087..1d06ad5931 100644
> > --- a/src/audio_output/aout_internal.h
> > +++ b/src/audio_output/aout_internal.h
> > @@ -67,13 +67,14 @@ typedef struct
> > struct
> > {
> > struct vlc_clock_t *clock;
> > - float rate; /**< Play-out speed rate */
> > + _Atomic float rate; /**< Play-out speed rate */
> > vlc_tick_t resamp_start_drift; /**< Resampler drift absolute value
> > */ int resamp_type; /**< Resampler mode (FIXME: redundant / resampling) */
> > bool discontinuity;
> > vlc_tick_t request_delay;
> > vlc_tick_t delay;
> > vlc_tick_t first_pts;
> > + _Atomic vlc_tick_t reported_drift;
> > } sync;
> > vlc_tick_t original_pts;
> >
> > diff --git a/src/audio_output/dec.c b/src/audio_output/dec.c
> > index 3f88b6c6f9..e67d6423b1 100644
> > --- a/src/audio_output/dec.c
> > +++ b/src/audio_output/dec.c
> > @@ -125,11 +125,13 @@ error:
> > }
> > }
> >
> > - owner->sync.rate = 1.f;
> > + atomic_store_explicit(&owner->sync.rate, 1.f, memory_order_release);
> > owner->sync.resamp_type = AOUT_RESAMPLING_NONE;
> > owner->sync.discontinuity = true;
> > owner->original_pts = VLC_TICK_INVALID;
> > owner->sync.delay = owner->sync.request_delay = 0;
> > + atomic_store_explicit(&owner->sync.reported_drift, VLC_TICK_INVALID,
> > + memory_order_release);
>
> Two releases in a row seems rather odd. And what is being released anyway?
I will review it. By the way, this patch doesn't work with monotonic mode (audio ressampling), I need to test and fix it.
> With atomic variables involved, are we sure that clock updates are ordered
> correctly. e.g. two updates from different thread can't race with one using old
> rate?
That's a good question.
>
> > atomic_init (&owner->buffers_lost, 0);
> > atomic_init (&owner->buffers_played, 0);
> > @@ -242,7 +244,8 @@ static void aout_StopResampling (audio_output_t *aout)
> > aout_FiltersAdjustResampling (owner->filters, 0);
> > }
> >
> > -static void aout_DecSilence (audio_output_t *aout, vlc_tick_t length,
> > vlc_tick_t pts) +static void aout_DecSilence (audio_output_t *aout,
> > vlc_tick_t length, vlc_tick_t pts, + float
> > rate)
>
> Would be more legible to split the prototype change.
OK
>
> > {
> > aout_owner_t *owner = aout_owner (aout);
> > const audio_sample_format_t *fmt = &owner->mixer_format;
> > @@ -262,16 +265,14 @@ static void aout_DecSilence (audio_output_t *aout,
> > vlc_tick_t length, vlc_tick_t
> >
> > const vlc_tick_t system_now = vlc_tick_now();
> > const vlc_tick_t system_pts =
> > - vlc_clock_ConvertToSystem(owner->sync.clock, system_now, pts,
> > - owner->sync.rate);
> > + vlc_clock_ConvertToSystem(owner->sync.clock, system_now, pts, rate);
> > aout->play(aout, block, system_pts);
> > }
> >
> > static void aout_DecAdjustDrift(audio_output_t *aout, vlc_tick_t drift,
> > - vlc_tick_t audio_ts)
> > + vlc_tick_t audio_ts, float rate)
> > {
> > aout_owner_t *owner = aout_owner (aout);
> > - const float rate = owner->sync.rate;
> >
> > if (unlikely(drift == INT64_MAX) || owner->bitexact)
> > return; /* cf. INT64_MAX comment in aout_DecPlay() */
> > @@ -311,7 +312,7 @@ static void aout_DecAdjustDrift(audio_output_t *aout,
> > vlc_tick_t drift, if (!owner->sync.discontinuity)
> > msg_Warn (aout, "playback way too early (%"PRId64"): "
> > "playing silence", drift);
> > - aout_DecSilence (aout, -drift, audio_ts);
> > + aout_DecSilence (aout, -drift, audio_ts, rate);
> >
> > aout_StopResampling (aout);
> > owner->sync.discontinuity = true;
> > @@ -372,15 +373,15 @@ void aout_RequestRetiming(audio_output_t *aout,
> > vlc_tick_t system_ts, vlc_tick_t audio_ts)
> > {
> > aout_owner_t *owner = aout_owner (aout);
> > - const float rate = owner->sync.rate;
> > + const float rate = atomic_load_explicit(&owner->sync.rate,
> > memory_order_acquire); vlc_tick_t drift =
> > vlc_clock_Update(owner->sync.clock, system_ts, audio_ts, rate);
> >
> > - aout_DecAdjustDrift(aout, drift, audio_ts);
> > + atomic_store_explicit(&owner->sync.reported_drift, drift,
> > memory_order_release); }
> >
> > static void aout_DecSynchronize(audio_output_t *aout, vlc_tick_t
> > system_now, - vlc_tick_t dec_pts)
> > + vlc_tick_t dec_pts, float rate)
> > {
> > /**
> > * Depending on the drift between the actual and intended playback
> > times, @@ -401,7 +402,16 @@ static void aout_DecSynchronize(audio_output_t
> > *aout, vlc_tick_t system_now, aout_owner_t *owner = aout_owner (aout);
> > vlc_tick_t delay;
> >
> > - if (aout_TimeGet(aout, &delay) != 0)
> > + if (aout->time_get == NULL)
> > + {
> > + vlc_tick_t drift =
> > atomic_exchange_explicit(&owner->sync.reported_drift, +
> > VLC_TICK_INVALID, memory_order_acquire);
> > +
> > + if (drift != VLC_TICK_INVALID)
> > + aout_DecAdjustDrift(aout, drift, dec_pts, rate);
> > + return;
> > + }
> > + else if (aout_TimeGet(aout, &delay) != 0)
> > return; /* nothing can be done if timing is unknown */
> >
> > if (owner->sync.discontinuity)
> > @@ -417,21 +427,20 @@ static void aout_DecSynchronize(audio_output_t *aout,
> > vlc_tick_t system_now, */
> > vlc_tick_t play_date =
> > vlc_clock_ConvertToSystem(owner->sync.clock, system_now +
> > delay, - dec_pts, owner->sync.rate); +
> > dec_pts, rate);
> > vlc_tick_t jitter = play_date - system_now;
> > if (jitter > 0)
> > {
> > - aout_DecSilence (aout, jitter, dec_pts - delay);
> > + aout_DecSilence (aout, jitter, dec_pts - delay, rate);
> > if (aout_TimeGet(aout, &delay) != 0)
> > return;
> > }
> > }
> >
> > - const float rate = owner->sync.rate;
> > vlc_tick_t drift =
> > vlc_clock_Update(owner->sync.clock, system_now + delay, dec_pts,
> > rate);
> >
> > - aout_DecAdjustDrift(aout, drift, dec_pts);
> > + aout_DecAdjustDrift(aout, drift, dec_pts, rate);
> > }
> >
> > /**************************************************************************
> > *** @@ -465,6 +474,8 @@ int aout_DecPlay(audio_output_t *aout, block_t
> > *block) owner->original_pts = block->i_pts;
> > }
> >
> > + const float rate = atomic_load_explicit(&owner->sync.rate,
> > memory_order_relaxed); +
> > if (owner->filters)
> > {
> > if (atomic_load_explicit(&owner->vp.update, memory_order_relaxed))
> > @@ -475,7 +486,7 @@ int aout_DecPlay(audio_output_t *aout, block_t *block)
> > vlc_mutex_unlock (&owner->vp.lock);
> > }
> >
> > - block = aout_FiltersPlay(owner->filters, block, owner->sync.rate);
> > + block = aout_FiltersPlay(owner->filters, block, rate);
> > if (block == NULL)
> > return ret;
> > }
> > @@ -494,16 +505,16 @@ int aout_DecPlay(audio_output_t *aout, block_t *block)
> > if (owner->filters)
> > aout_FiltersSetClockDelay(owner->filters, owner->sync.delay);
> > if (delta > 0)
> > - aout_DecSilence (aout, delta, block->i_pts);
> > + aout_DecSilence (aout, delta, block->i_pts, rate);
> > }
> >
> > /* Drift correction */
> > vlc_tick_t system_now = vlc_tick_now();
> > - aout_DecSynchronize(aout, system_now, original_pts);
> > + aout_DecSynchronize(aout, system_now, original_pts, rate);
> >
> > vlc_tick_t play_date =
> > vlc_clock_ConvertToSystem(owner->sync.clock, system_now,
> > original_pts, - owner->sync.rate);
> > + rate);
> > if (unlikely(play_date == INT64_MAX))
> > {
> > /* The clock is paused but not the output, play the audio anyway
> > since @@ -556,7 +567,7 @@ void aout_DecChangeRate(audio_output_t *aout,
> > float rate) {
> > aout_owner_t *owner = aout_owner(aout);
> >
> > - owner->sync.rate = rate;
> > + atomic_store_explicit(&owner->sync.rate, rate, memory_order_release);
> > }
> >
> > void aout_DecChangeDelay(audio_output_t *aout, vlc_tick_t delay)
> > @@ -599,6 +610,8 @@ void aout_DecFlush(audio_output_t *aout)
> > }
> > owner->sync.discontinuity = true;
> > owner->original_pts = VLC_TICK_INVALID;
> > + atomic_store_explicit(&owner->sync.reported_drift, VLC_TICK_INVALID,
> > + memory_order_release);
> > }
> >
> > void aout_DecDrain(audio_output_t *aout)
> > @@ -623,4 +636,6 @@ void aout_DecDrain(audio_output_t *aout)
> >
> > owner->sync.discontinuity = true;
> > owner->original_pts = VLC_TICK_INVALID;
> > + atomic_store_explicit(&owner->sync.reported_drift, VLC_TICK_INVALID,
> > + memory_order_release);
> > }
> > diff --git a/src/audio_output/output.c b/src/audio_output/output.c
> > index b0ddfc64b5..098c00b317 100644
> > --- a/src/audio_output/output.c
> > +++ b/src/audio_output/output.c
> > @@ -228,6 +228,9 @@ audio_output_t *aout_New (vlc_object_t *parent)
> > vlc_atomic_rc_init(&owner->rc);
> > vlc_audio_meter_Init(&owner->meter, aout);
> >
> > + atomic_init(&owner->sync.rate, 1.0f);
> > + atomic_init(&owner->sync.reported_drift, VLC_TICK_INVALID);
> > +
> > /* Audio output module callbacks */
> > var_Create (aout, "volume", VLC_VAR_FLOAT);
> > var_AddCallback (aout, "volume", var_Copy, parent);
>
>
> --
> Rémi Denis-Courmont
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list