Zhao Zhili: > > >> On Feb 7, 2025, at 19:22, Andreas Rheinhardt >> <andreas.rheinha...@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.) > > As I understand it, there is no real race condition, that’s why the patch > says “silence tsan warning”.
There is a race (in fact, both a data race and a race condition (the latter doesn't happen on x86 with its strong memory model though); see my other mail for an explanation. > > I have considered another idea to keep tsan clean and keep the benefit > of set progress earlier: use another non-atomic progress together with > mutex/cond, so atomic and mutex/cond are used separately. Not sure > whether it’s worth the complexity. > So we would have one atomic and one non-atomic progress variable and a mutex? And the atomic progress is set as now and only read for the fast path and the nonatomic variable is set and read inside the mutex? I don't really think this is better than any of the alternatives. - Andreas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
