diff mbox

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

Message ID 20170524165126.GE5698@golem.pkh.me
State New
Headers show

Commit Message

Clément Bœsch May 24, 2017, 4:51 p.m. UTC 
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:

Comments

Ronald S. Bultje May 24, 2017, 4:59 p.m. UTC | #1 
Hi,

On May 24, 2017 12:51 PM, "Clément Bœsch" <u@pkh.me> wrote:

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:


Yes, I initially wrote it like that but considered the spaghetti too much
for the edge case benefit it brings to flushing only.

But if people prefer that, I can do that too. What do others think?

Ronald
Michael Niedermayer May 25, 2017, 11:21 a.m. UTC | #2 
On Wed, May 24, 2017 at 12:59:58PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On May 24, 2017 12:51 PM, "Clément Bœsch" <u@pkh.me> wrote:
> 
> 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:
> 
> 
> Yes, I initially wrote it like that but considered the spaghetti too much
> for the edge case benefit it brings to flushing only.
> 
> But if people prefer that, I can do that too. What do others think?

I like ubitux version

thanks

[...]
diff mbox

Patch

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);
     }