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

[Ronald S. Bultje]

Hi,

On May 24, 2017 12:51 PM, "Clément Bœsch" <u at 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