[FFmpeg-devel] [PATCH 1/3] pthread : Remove one unnecessary lock/unlock pair in worker loop.

Aaron Colwell acolwell at chromium.org
Thu Mar 22 17:51:49 CET 2012


Hi,
This is the first in a sequence of patches about the pthread code. These
patches are the result of me trying to resolve the race conditions that the
Chromium ThreadSanitizer tool(TSAN) is reporting. I've already tried
telling the tool that certain member variables like state, progress, &
progress_update should be ignored, but this doesn't appear to resolve other
issues in the code. I believe there are real races on these variables, but
the code is too inconsistent about its locking for me to truly grasp what
is going on. My hope is that by discussing these patches we can actually
identify the real problems and clarify what particular pieces of code are
truly thread-safe and why.

If possible, I'd like to avoid the "your tool isn't smart enough" comments
and actually make this discussion about why people believe the code is safe
and what mechanism are in place to maintain that confidence. Ultimately my
goal is to fix real bugs and have enough information to be able to tell our
tools what is safe to ignore and what isn't.

This patch is simply move the locking around a little bit so that it is
easier to see that another thread can only acquire the lock while this
thread is waiting for state to change from STATE_INPUT_READY or
while ff_thread_finish_setup() is being called.

Question:
Why does ff_thread_finish_setup(avctx) really need to be called outside of
the p->mutex?
It seems like this mutex only really blocks submit_packet() and it isn't
clear to me why submit_packet should be allowed update the state during
ff_thread_finish_setup().


 libavcodec/pthread.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/libavcodec/pthread.c b/libavcodec/pthread.c
index 7834922..e90f21f 100644
--- a/libavcodec/pthread.c
+++ b/libavcodec/pthread.c
@@ -365,17 +365,16 @@ static attribute_align_arg void
*frame_worker_thread(void *arg)
     AVCodecContext *avctx = p->avctx;
     AVCodec *codec = avctx->codec;

+    pthread_mutex_lock(&p->mutex);
     while (1) {
         int i;
-        if (p->state == STATE_INPUT_READY && !fctx->die) {
-            pthread_mutex_lock(&p->mutex);
             while (p->state == STATE_INPUT_READY && !fctx->die)
                 pthread_cond_wait(&p->input_cond, &p->mutex);
-            pthread_mutex_unlock(&p->mutex);
-        }

         if (fctx->die) break;

+        pthread_mutex_unlock(&p->mutex);
+
         if (!codec->update_thread_context && (avctx->thread_safe_callbacks
|| avctx->get_buffer == avcodec_default_get_buffer))
             ff_thread_finish_setup(avctx);

@@ -398,8 +397,8 @@ static attribute_align_arg void
*frame_worker_thread(void *arg)
         pthread_cond_signal(&p->output_cond);
         pthread_mutex_unlock(&p->progress_mutex);

-        pthread_mutex_unlock(&p->mutex);
     }
+    pthread_mutex_unlock(&p->mutex);

     return NULL;
 }
-- 
1.7.7.3


More information about the ffmpeg-devel mailing list