Hi, On Fri, Feb 26, 2016 at 3:26 PM, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> Hi, > > On Thu, Feb 25, 2016 at 9:32 PM, Wan-Teh Chang < > wtc-at-google....@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel