On Aug 25, 2011, at 8:01 PM, Aaron Colwell wrote:
> Hi,
> I started a thread with the following message on the FFmpeg-devel list and
> someone suggested that I start a discussion on this list as well. Any help
> you could provide would be greatly appreciated.
>
> Thanks,
> Aaron
>
> ---------- Forwarded message ----------
> From: Aaron Colwell <acolw...@chromium.org>
> Date: Thu, Aug 25, 2011 at 11:26 AM
> Subject: Race conditions in libavcodec/pthread.c
> To: ffmpeg-de...@ffmpeg.org
>
>
> Hi,
>
> Some recent changes to Chromium unit tests have caused FFmpeg decoding to get
> regularly scrutinized by our ThreadSanitizer tool. This has led to the
> detection of a variety of race conditions in libavcodec/pthread.c . Quick
> inspection of the code reveals that there is significant use of the
> "double-checked locking" anti-pattern and inconsistent use of
> PerThreadContext::mutex and PerThreadContext::progress_mutex which are likely
> causing of most of the race conditions. I wanted to ask a few questions
> before jumping in and trying to fix this.
>
> 1. Are you already aware of these issues?
The important synchronization mechanisms used (documented in
libavcodec/thread.h) are, uh, custom, and as such are either ignored by
automated checkers or confuse them badly. I didn't use any during development
and relied on regression tests instead.
> 2. Is someone already working on fixing these?
> 3. Who is the expert on libavcodec/pthread.c so I can direct questions to
> them?
Me.
> 4. Why is "double-checked locking" being used? Will there be significant
> protest if I remove it?
Can you cite line numbers?
If you mean this:
if (prev_thread->state == STATE_SETTING_UP) {
<wait for state to change>
}
it would be fine to remove it.
If you mean:
if (!progress || progress[field] >= n) return;
I would prefer to keep it.
I believe both of these are safe because pthread_mutex_lock() is an external
function call and therefore prevents optimizing away reads of the variable
being checked.
> 5. What is the relationship between PerThreadContext::mutex &
> PerThreadContext::progress_mutex and what member variables are they supposed
> to protect? I've seen cases where only mutex is held, mutex & progress_mutex
> are held, and only progress_mutex is held. At times these 3 cases appear to
> modify the same state variables. ThreadSanitizer is getting stuck on some
> test runs now which leads me to believe that there is a locking pattern that
> results in deadlock.
progress_mutex is locked around pthread_cond_* calls using progress_cond and
output_cond. It also protects PerThreadContext.state.
mutex is used for input_cond and to protect the rest of the context (in
submit_packet vs frame_worker_thread).
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
