[FFmpeg-devel] [PATCH] Read |state| under the protection of |mutex| or |progress_mutex|.
wtc at google.com
Tue Mar 1 19:47:51 CET 2016
On Fri, Feb 26, 2016 at 12:16 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> This will be slower, right? Is this an actual issue? Like, can you point to
> cases like  (i.e. a real-world race condition with real-world
> consequences) that were fixed with your patch?
With the new relaxed and acquire/release atomic int get and set
functions I proposed in
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
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
More information about the ffmpeg-devel