[FFmpeg-devel] [PATCH] Read |state| under the protection of |mutex| or |progress_mutex|.

[Wan-Teh Chang]

Hi Ronald,

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 [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