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

[Ronald S. Bultje]

Hi,

On Tue, Mar 1, 2016 at 1:47 PM, Wan-Teh Chang <wtc-at-google.com at ffmpeg.org>
wrote:

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


I can help there.

state is an enum with the following values:

        STATE_INPUT_READY
        STATE_SETTING_UP
        STATE_GET_BUFFER
        STATE_GET_FORMAT
        STATE_SETUP_FINISHED

Transitions are like this:

- INPUT_READY to SETTING_UP in submit_packet() (main thread), guarded by
mutex. frame_worker_thread() (frame decoder thread[s]) awaits this state
change holding mutex. The accompanying cond is input_cond.

- any transition between SETTING_UP, GET_BUFFER and GET_FORMAT in
submit_packet (main thread) and
thread_get_buffer_internal/ff_thread_get_format (decoder thread[s]),
guarded by progress_mutex. The accompanying cond is progress_cond.

- SETTING_UP/GET_BUFFER/GET_FORMAT to SETUP_FINISHED in
ff_thread_finish_setup() (decoder thread[s]), guarded by progress_mutex.
The accompanying cond is progress_cond. This condition is awaited before
submit_packet() (main thread) returns to ensure that get_formats and
get_buffer was called in the context of the call to submit_packet(),
assuming the application did not explicitly say that calls to these
callbacks are threadsafe. I agree that the code at the bottom of
submit_packet() is ... Strange :) But I'm not convinced it's buggy. I'm
sure tsan hates it. I wouldn't mind patches that clean it up assuming they
don't make the code harder to read or slower.

- SETUP_FINISHED to INPUT_READY is in frame_worker_thread() (decoder
thread[s]) and is guarded by both mutex as well as progress_mutex. This
signals output_cond and progress_cond (both under control of
progress_mutex).  ff_thread_decode_frame() (main thread) uses this
transition controlled by output_cond to return decoded frames to the
application after calling submit_packet().

Hope this helps,
Ronald