[FFmpeg-devel] [PATCH] submit_thread should also lock the mutex that guards the source avctx (prev_thread->avctx) when calling update_context_from_thread.

Ronald S. Bultje rsbultje at gmail.com
Fri Feb 26 21:26:21 CET 2016


Hi,

On Thu, Feb 25, 2016 at 9:32 PM, Wan-Teh Chang <wtc-at-google.com at ffmpeg.org
> wrote:

> This is because the codec->decode() call in frame_worker_thread may be
> modifying that avctx at the same time. This data race is reported by
> ThreadSanitizer.
>
> Although submit_thread holds two locks simultaneously, it always
> acquires them in the same order because |prev_thread| is always the
> array element before |p| (with a wraparound at the end of array), and
> submit_thread is the only function that acquires those two locks, so
> this won't result in a deadlock.
> ---
>  libavcodec/pthread_frame.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index b77dd1e..e7879e3 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -330,7 +330,9 @@ static int submit_packet(PerThreadContext *p, AVPacket
> *avpkt)
>              pthread_mutex_unlock(&prev_thread->progress_mutex);
>          }
>
> +        pthread_mutex_lock(&prev_thread->mutex);
>          err = update_context_from_thread(p->avctx, prev_thread->avctx, 0);
> +        pthread_mutex_unlock(&prev_thread->mutex);
>          if (err) {
>              pthread_mutex_unlock(&p->mutex);
>              return err;
> --
> 2.7.0.rc3.207.g0ac5344


Uhm, isn't this the lock that is being held while we call decode()?

static attribute_align_arg void *frame_worker_thread(void *arg)
{
[..]
    pthread_mutex_lock(&p->mutex);
    while (1) {
            while (p->state == STATE_INPUT_READY && !fctx->die)
                pthread_cond_wait(&p->input_cond, &p->mutex);
[..]
        p->result = codec->decode(avctx, p->frame, &p->got_frame,
&p->avpkt);
[..]
    }
    pthread_mutex_unlock(&p->mutex);
[..]
}

So doesn't this patch remove all concurrency by making the decode() of
thread N-1 finish before decode() of thread N can start? More practically,
what is the output of time ffmpeg -threads N -i large-h264-file -f null -
with N being 1 and 2 before and after your patch?

The contract between threads is like this: there is two types of fields,
these required for decoding the next frame (A), and those irrelevant for
decoding the next frame (B). When finish_setup() is called, we require all
values of (A) to have reached their final value. We don't care about values
of (B). Then, in the main thread, (A) are copied from prev_thread to this
thread and we continue decoding. We should have continuous race conditions
and real-world decoding failures if this contract were violated. But that's
not the case, frame-MT has been fairly stable over the past years.

So, let's make this real-world: what kind of variables is tsan complaining
about? Are they really written after setup_finished is called and used in
the next decode call? Or are they copied but never used? Or something else?

Ronald


More information about the ffmpeg-devel mailing list