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? 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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel