[FFmpeg-devel] [PATCH 3/3] pthread : Protect progress & progress_used access with progress_mutex.

[Aaron Colwell]

Here I am just protecting progress & progress_used with progress_mutex. I
expect this to be one of the most controversial changes because I believe
this may cause a noticable performance drop.

Questions:
Why is the existing code considered safe?
How are thread protected from accidentally observing a MAX_INT progress
right before progress gets set to -1? It seems like this could cause
ff_thread_await_progress() calls to return early.

---
 libavcodec/pthread.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/libavcodec/pthread.c b/libavcodec/pthread.c
index 9e4686b..e874016 100644
--- a/libavcodec/pthread.c
+++ b/libavcodec/pthread.c
@@ -525,7 +525,9 @@ static void free_progress(AVFrame *f)
     PerThreadContext *p = f->owner->thread_opaque;
     int *progress = f->thread_opaque;

+    pthread_mutex_lock(&p->progress_mutex);
     p->progress_used[(progress - p->progress[0]) / 2] = 0;
+    pthread_mutex_unlock(&p->progress_mutex);
 }

 /// Releases the buffers that this decoding thread was the last user of.
@@ -693,7 +695,7 @@ void ff_thread_report_progress(AVFrame *f, int n, int
field)
     PerThreadContext *p;
     int *progress = f->thread_opaque;

-    if (!progress || progress[field] >= n) return;
+    if (!progress) return;

     p = f->owner->thread_opaque;

@@ -701,6 +703,10 @@ void ff_thread_report_progress(AVFrame *f, int n, int
field)
         av_log(f->owner, AV_LOG_DEBUG, "%p finished %d field %d\n",
progress, n, field);

     pthread_mutex_lock(&p->progress_mutex);
+    if (progress[field] >= n) {
+      pthread_mutex_unlock(&p->progress_mutex);
+      return;
+    }
     progress[field] = n;
     pthread_cond_broadcast(&p->progress_cond);
     pthread_mutex_unlock(&p->progress_mutex);
@@ -711,7 +717,7 @@ void ff_thread_await_progress(AVFrame *f, int n, int
field)
     PerThreadContext *p;
     int *progress = f->thread_opaque;

-    if (!progress || progress[field] >= n) return;
+    if (!progress) return;

     p = f->owner->thread_opaque;

@@ -719,6 +725,10 @@ void ff_thread_await_progress(AVFrame *f, int n, int
field)
         av_log(f->owner, AV_LOG_DEBUG, "thread awaiting %d field %d from
%p\n", n, field, progress);

     pthread_mutex_lock(&p->progress_mutex);
+    if (progress[field] >= n) {
+      pthread_mutex_unlock(&p->progress_mutex);
+      return;
+    }
     while (progress[field] < n)
         pthread_cond_wait(&p->progress_cond, &p->progress_mutex);
     pthread_mutex_unlock(&p->progress_mutex);
@@ -929,15 +939,18 @@ static int *allocate_progress(PerThreadContext *p)
 {
     int i;

+    pthread_mutex_lock(&p->progress_mutex);
     for (i = 0; i < MAX_BUFFERS; i++)
         if (!p->progress_used[i]) break;

     if (i == MAX_BUFFERS) {
         av_log(p->avctx, AV_LOG_ERROR, "allocate_progress() overflow\n");
+        pthread_mutex_unlock(&p->progress_mutex);
         return NULL;
     }

     p->progress_used[i] = 1;
+    pthread_mutex_unlock(&p->progress_mutex);

     return p->progress[i];
 }

-- 
1.7.7.3