[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