Re: [FFmpeg-devel] [PATCH] frame_thread_encoder: extend critical code covered by finished_task_mutex.

[Clément Bœsch]

On Wed, May 24, 2017 at 12:15:05PM -0400, Ronald S. Bultje wrote:
> Should fix tsan errors in utvideoenc_rgb_left and related tests.
> ---
>  libavcodec/frame_thread_encoder.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/frame_thread_encoder.c 
> b/libavcodec/frame_thread_encoder.c
> index 27ae356..2746964 100644
> --- a/libavcodec/frame_thread_encoder.c
> +++ b/libavcodec/frame_thread_encoder.c
> @@ -273,14 +273,21 @@ int ff_thread_video_encode_frame(AVCodecContext *avctx, 
> AVPacket *pkt, const AVF
>  
>          c->task_index = (c->task_index+1) % BUFFER_SIZE;
>  
> -        if(!c->finished_tasks[c->finished_task_index].outdata && 
> (c->task_index - c->finished_task_index) % BUFFER_SIZE <= avctx->thread_count)
> +        pthread_mutex_lock(&c->finished_task_mutex);
> +        if(!c->finished_tasks[c->finished_task_index].outdata &&
> +           (c->task_index - c->finished_task_index) % BUFFER_SIZE <= 
> avctx->thread_count) {
> +            pthread_mutex_unlock(&c->finished_task_mutex);
>              return 0;
> +        }
> +    } else {
> +        pthread_mutex_lock(&c->finished_task_mutex);
>      }
>  
> -    if(c->task_index == c->finished_task_index)
> +    if(c->task_index == c->finished_task_index) {
> +        pthread_mutex_unlock(&c->finished_task_mutex);
>          return 0;
> +    }
>  
> -    pthread_mutex_lock(&c->finished_task_mutex);
>      while (!c->finished_tasks[c->finished_task_index].outdata) {
>          pthread_cond_wait(&c->finished_task_cond, &c->finished_task_mutex);
>      }


LGTM

alternatively you could do sth like the following (totally untested) to
reduce the pthread spaghettis:

diff --git a/libavcodec/frame_thread_encoder.c 
b/libavcodec/frame_thread_encoder.c
index 27ae356af3..c1899dbe2e 100644
--- a/libavcodec/frame_thread_encoder.c
+++ b/libavcodec/frame_thread_encoder.c
@@ -272,15 +272,16 @@ int ff_thread_video_encode_frame(AVCodecContext *avctx, 
AVPacket *pkt, const AVF
         pthread_mutex_unlock(&c->task_fifo_mutex);
 
         c->task_index = (c->task_index+1) % BUFFER_SIZE;
-
-        if(!c->finished_tasks[c->finished_task_index].outdata && 
(c->task_index - c->finished_task_index) % BUFFER_SIZE <= avctx->thread_count)
-            return 0;
     }
 
-    if(c->task_index == c->finished_task_index)
+    pthread_mutex_lock(&c->finished_task_mutex);
+    if(c->task_index == c->finished_task_index ||
+       (frame && !c->finished_tasks[c->finished_task_index].outdata &&
+        (c->task_index - c->finished_task_index) % BUFFER_SIZE <= 
avctx->thread_count)) {
+        pthread_mutex_unlock(&c->finished_task_mutex);
         return 0;
+    }
 
-    pthread_mutex_lock(&c->finished_task_mutex);
     while (!c->finished_tasks[c->finished_task_index].outdata) {
         pthread_cond_wait(&c->finished_task_cond, &c->finished_task_mutex);
     }

-- 
Clément B.

signature.asc
Description: PGP signature

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel