Hi Ronald, On Fri, Feb 26, 2016 at 12:16 PM, Ronald S. Bultje <rsbul...@gmail.com> wrote: > > This will be slower, right? Is this an actual issue? Like, can you point to > cases like [1] (i.e. a real-world race condition with real-world > consequences) that were fixed with your patch? > > Ronald > > https://blogs.gnome.org/rbultje/2014/01/12/brute-force-thread-debugging/
With the new relaxed and acquire/release atomic int get and set functions I proposed in http://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/190516.html, we have the means to make the current code portable while preserving the exact performance for x86/x86_64. But unlike the progress[field] values, I am strongly against using this approach for |state| for two reasons. 1) |state| is guarded by one of two mutexes: |mutex| and |progress_mutex|. Which mutex should guard |state| when is not documented, and I haven't reverse-engineered that policy. 2) progress[field] is only accessed in a pair of short functions (ff_thread_report_progress and ff_thread_await_progress). For such a simple problem, my first attempt at using atomics still needed to be corrected by an expert (Dmitry Vyukov, who works on ThreadSanitizer). In contrast, |state| is accessed in many places in libavcodec/pthread_frame.c. Using atomics on |state| will be more difficult, and the resulting code will be difficult to maintain. So, to fix the |state| data races I propose using the approach of this patch. Note: this proposal is my personal judgement and may not reflect Dmitry's opinion. Dmitry and I seem to disagree on how to best fix the |state| data races. To my surprise, he is fine with using atomics to make the double-checked locking style access to |state| correct and portable. Thanks, Wan-Teh Chang _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
