[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:44:47 CET 2016


Hi,

On Fri, Feb 26, 2016 at 3:26 PM, Ronald S. Bultje <rsbultje at gmail.com>
wrote:

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

I actually tested this, just for fun:

before:
$ for t in 1 2; do time ffmpeg -threads $t -i tos3k-vp9-b5000.webm -f null
-v 0 -nostats -vframes 500 -; done

real 0m6.732s
user 0m6.643s
sys 0m0.064s

real 0m4.124s <<< note how this is significantly smaller than 6.732
user 0m6.566s
sys 0m0.114s

after:
$ for t in 1 2; do time ffmpeg -threads $t -i tos3k-vp9-b5000.webm -f null
-v 0 -nostats -vframes 500 -; done

real 0m6.808s
user 0m6.541s
sys 0m0.071s

real 0m7.001s <<< note how this is no longer significantly smaller than
6.808
user 0m6.945s
sys 0m0.108s

That seems like a pretty major malfunction of MT performance to me...

Ronald


More information about the ffmpeg-devel mailing list