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

[Wan-Teh Chang]

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.

Wan-Teh Chang