[vlc-devel] [RFC PATCH 3/9] aout: fix data races with aout_RequestRetiming()
Rémi Denis-Courmont
remi at remlab.net
Thu Dec 10 19:48:46 CET 2020
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?
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?
> 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.
> {
> 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
More information about the vlc-devel
mailing list