[FFmpeg-devel] [PATCH] threadprogress: reorder instructions to silence tsan warning.

[Ronald S. Bultje]

Hi,

On Fri, Feb 7, 2025 at 6:22 AM Andreas Rheinhardt <
andreas.rheinhardt at outlook.com> wrote:

> Ronald S. Bultje:
> > Fixes #11456.
> > ---
> >  libavcodec/threadprogress.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/threadprogress.c b/libavcodec/threadprogress.c
> > index 62c4fd898b..aa72ff80e7 100644
> > --- a/libavcodec/threadprogress.c
> > +++ b/libavcodec/threadprogress.c
> > @@ -55,9 +55,8 @@ void ff_thread_progress_report(ThreadProgress *pro,
> int n)
> >      if (atomic_load_explicit(&pro->progress, memory_order_relaxed) >= n)
> >          return;
> >
> > -    atomic_store_explicit(&pro->progress, n, memory_order_release);
> > -
> >      ff_mutex_lock(&pro->progress_mutex);
> > +    atomic_store_explicit(&pro->progress, n, memory_order_release);
> >      ff_cond_broadcast(&pro->progress_cond);
> >      ff_mutex_unlock(&pro->progress_mutex);
> >  }
>
> I don't really understand why this is supposed to fix a race; after all,
> the synchronisation of ff_thread_progress_(report|await) is not supposed
> to be provided by the mutex (which is avoided altogether in the fast
> path in ff_thread_report_await()), but by storing and loading the
> progress variable.
> That's also the reason why I moved this outside of the mutex (compared
> to ff_thread_report_progress(). (This way it is possible for a consumer
> thread to see the new progress value earlier and possibly avoid the
> mutex altogether.)


The consumer thread already checks the value without the lock. so the
significance of that last point seems minor to me. This would be different
if the wait() counterpart had no lockless path. Or am I missing something?

Ronald