[FFmpeg-devel] [PATCH] Always read frame progress values under progress_mutex.

wm4 nfxjfg at googlemail.com
Fri Feb 26 11:30:38 CET 2016


On Fri, 26 Feb 2016 05:14:47 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Thu, Feb 25, 2016 at 04:42:22PM -0800, Wan-Teh Chang wrote:
> > On Thu, Feb 25, 2016 at 4:15 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:  
> > >
> > > Similar to the other patch, please explain what this actually fixes,
> > > those sanitizer tools produce a lot of false positives and nonsensical
> > > warnings.  
> > 
> > Yes, certainly.
> > 
> > In libavcodec/pthread_frame.c, we have:
> > 
> >  46 /**
> >  47  * Context used by codec threads and stored in their
> > AVCodecInternal thread_    ctx.
> >  48  */
> >  49 typedef struct PerThreadContext {
> >  ...
> >  59     pthread_mutex_t progress_mutex; ///< Mutex used to protect
> > frame progress values and progress_cond.
> > 
> > The comment on line 59 documents that progress_mutex guards frame
> > progress values and progress_cond. (In fact, progress_mutex also
> > guards the |state| field in many places, but that's not the subject of
> > this patch.)
> > 
> > I assume "frame progess values" mean f->progress->data, where |f|
> > points to a ThreadFrame. I inspected libavcodec/pthread_frame.c and
> > concluded the code (ff_thread_report_progress and
> > ff_thread_await_progress) matches that comment, except that it also
> > does a double-checked locking style performance optimization. For
> > example, here is ff_thread_report_progress:
> > 
> > 472 void ff_thread_report_progress(ThreadFrame *f, int n, int field)
> > 473 {
> > 474     PerThreadContext *p;
> > 475     volatile int *progress = f->progress ? (int*)f->progress->data : NULL;
> > 476
> > 477     if (!progress || progress[field] >= n) return;
> > 478
> > 479     p = f->owner->internal->thread_ctx;
> > 480
> > 481     if (f->owner->debug&FF_DEBUG_THREADS)
> > 482         av_log(f->owner, AV_LOG_DEBUG, "%p finished %d field
> > %d\n", progress    , n, field);
> > 483
> > 484     pthread_mutex_lock(&p->progress_mutex);
> > 485     progress[field] = n;
> > 486     pthread_cond_broadcast(&p->progress_cond);
> > 487     pthread_mutex_unlock(&p->progress_mutex);
> > 488 }
> > 
> > progress[field] can only increase. So, both ff_thread_report_progress
> > and ff_thread_await_progress have a fast code path (line 477 and line
> > 495, respectively) that reads progress[field] without acquiring
> > progress_mutex. If the speculatively read value of progress[field] is  
> > >= |n|, then the functions return immediately. Otherwise, the  
> > functions take the slow code path, which acquires progress_mutex
> > properly and does what the functions need to do.
> > 
> > The fast code path is in the style of double-checked locking and has a
> > data race. ThreadSanitizer is right in this case.  
> 
> ThreadSanitizer is right about that this is a data race yes, but
> isnt all lock-free code technically containing a data race
> 
> But is there a bug?
> Its basically a simple 2 thread thing, one says "iam finished" the
> other checks if that is set.
> In what case does this benefit from a lock ?
> also note the write is always under a lock, which id assume does
> any needed memory barriers
> 
> also considering this is performance relevant code, please provide
> a benchmark

I don't know about this case, but the frame threading code uses
improper double-checked logging. Basically it doesn't use proper
barrier or atomics, assumes x86 semantics, and hopes the compiler won't
get into the way.


More information about the ffmpeg-devel mailing list