diff mbox series

[FFmpeg-devel] avcodec/pthread_frame: Fix checks and cleanup during init

Message ID 20210211155759.309391-1-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/pthread_frame: Fix checks and cleanup during init
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Feb. 11, 2021, 3:57 p.m. UTC
Up until now, ff_frame_thread_init had several bugs:
1. It did not check whether the condition and mutexes
   could be successfully created.
2. In case an error happened when setting up the child threads,
   ff_frame_thread_free is called to clean up all threads set up so far,
   including the current one. But a half-allocated context needs
   special handling which ff_frame_thread_frame_free doesn't provide.
   Notably, if allocating the AVCodecInternal, the codec's private data
   or setting the options fails, the codec's close function will be
   called (if there is one); it will also be called if the codec's init
   function fails, regardless of whether the FF_CODEC_CAP_INIT_CLEANUP
   is set. This is not supported by all codecs; in ticket #9099 (which
   this commit fixes) it led to a crash.

This commit fixes both of these issues. Given that it is not documented
to be save to destroy mutexes/conditions that have only been zeroed, but
not initialized (or whose initialization has failed), one either needs to
duplicate the code to destroy them in ff_frame_thread_init or record
which mutexes/condition variables need to be destroyed and check for this
in ff_frame_thread_free. This patch uses the former way for the
mutexes/condition variables, but lets ff_frame_thread_free take
over for all threads whose AVCodecContext could be allocated.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
After several unsuccessful attempts to make pthread_cond/mutex_init
fail, I looked at the source [1] and it seems that the glibc versions of
these functions can't fail at all (unless one sets nondefault attributes).
Therefore this part of the code is untested, unfortunately.

(Removing all pthread_mutex/cond_destroy calls does not result in any
complaints from Valgrind/ASAN either; seems the glibc implementation
doesn't need allocations.)

[1]: https://github.com/bminor/glibc/blob/master/nptl/pthread_cond_init.c
https://github.com/bminor/glibc/blob/master/nptl/pthread_mutex_init.c

 libavcodec/pthread_frame.c | 175 ++++++++++++++++++++++---------------
 1 file changed, 106 insertions(+), 69 deletions(-)

Comments

Nuo Mi Feb. 12, 2021, 6:26 a.m. UTC | #1
On Fri, Feb 12, 2021 at 12:06 AM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Up until now, ff_frame_thread_init had several bugs:
> 1. It did not check whether the condition and mutexes
>    could be successfully created.
> 2. In case an error happened when setting up the child threads,
>    ff_frame_thread_free is called to clean up all threads set up so far,
>    including the current one. But a half-allocated context needs
>    special handling which ff_frame_thread_frame_free doesn't provide.
>    Notably, if allocating the AVCodecInternal, the codec's private data
>    or setting the options fails, the codec's close function will be
>    called (if there is one); it will also be called if the codec's init
>    function fails, regardless of whether the FF_CODEC_CAP_INIT_CLEANUP
>    is set. This is not supported by all codecs; in ticket #9099 (which
>    this commit fixes) it led to a crash.
>
> This commit fixes both of these issues. Given that it is not documented
> to be save to destroy mutexes/conditions that have only been zeroed, but
> not initialized (or whose initialization has failed), one either needs to
> duplicate the code to destroy them in ff_frame_thread_init or record
> which mutexes/condition variables need to be destroyed and check for this
> in ff_frame_thread_free. This patch uses the former way for the
> mutexes/condition variables, but lets ff_frame_thread_free take
> over for all threads whose AVCodecContext could be allocated.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> After several unsuccessful attempts to make pthread_cond/mutex_init
> fail, I looked at the source [1] and it seems that the glibc versions of
> these functions can't fail at all (unless one sets nondefault attributes).
> Therefore this part of the code is untested, unfortunately.
>
> (Removing all pthread_mutex/cond_destroy calls does not result in any
> complaints from Valgrind/ASAN either; seems the glibc implementation
> doesn't need allocations.)
>
> [1]: https://github.com/bminor/glibc/blob/master/nptl/pthread_cond_init.c
> https://github.com/bminor/glibc/blob/master/nptl/pthread_mutex_init.c
>
>  libavcodec/pthread_frame.c | 175 ++++++++++++++++++++++---------------
>  1 file changed, 106 insertions(+), 69 deletions(-)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 4429a4d59c..a10d8c1266 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -64,6 +64,12 @@ enum {
>      STATE_SETUP_FINISHED,
>  };
>
> +enum {
> +    UNINITIALIZED,  ///< Thread has not been created, AVCodec->close
> mustn't be called
> +    NEEDS_CLOSE,    ///< AVCodec->close needs to be called
> +    INITIALIZED,    ///< Thread has been properly set up
> +};
> +
>  /**
>   * Context used by codec threads and stored in their AVCodecInternal
> thread_ctx.
>   */
> @@ -698,27 +704,40 @@ void ff_frame_thread_free(AVCodecContext *avctx, int
> thread_count)
>
>      for (i = 0; i < thread_count; i++) {
>          PerThreadContext *p = &fctx->threads[i];
> +        AVCodecContext *ctx = p->avctx;
>
> -        pthread_mutex_lock(&p->mutex);
> -        p->die = 1;
> -        pthread_cond_signal(&p->input_cond);
> -        pthread_mutex_unlock(&p->mutex);
> -
> -        if (p->thread_init)
> -            pthread_join(p->thread, NULL);
> -        p->thread_init=0;
> +        if (ctx->internal) {
> +            if (p->thread_init == INITIALIZED) {
> +                pthread_mutex_lock(&p->mutex);
> +                p->die = 1;
> +                pthread_cond_signal(&p->input_cond);
> +                pthread_mutex_unlock(&p->mutex);
>
> -        if (codec->close && p->avctx)
> -            codec->close(p->avctx);
> +                pthread_join(p->thread, NULL);
>
This will signal exit for one thread, then wait for it completely. All
operation are serilized.
How about we split it into two loops. one loop signal all threads for exit.
One loop to join all thread.
When we join a thread, other threads can work on exiting.

> +            }
> +            if (codec->close && p->thread_init != UNINITIALIZED)
> +                codec->close(ctx);
>
>  #if FF_API_THREAD_SAFE_CALLBACKS
> -        release_delayed_buffers(p);
> +            release_delayed_buffers(p);
> +            for (int j = 0; j < p->released_buffers_allocated; j++)
> +                av_frame_free(&p->released_buffers[j]);
> +            av_freep(&p->released_buffers);
>  #endif
> -        av_frame_free(&p->frame);
> -    }
> +            if (ctx->priv_data) {
> +                if (codec->priv_class)
> +                    av_opt_free(ctx->priv_data);
> +                av_freep(&ctx->priv_data);
> +            }
>
> -    for (i = 0; i < thread_count; i++) {
> -        PerThreadContext *p = &fctx->threads[i];
> +            av_freep(&ctx->slice_offset);
> +
> +            av_buffer_unref(&ctx->internal->pool);
> +            av_freep(&ctx->internal);
> +            av_buffer_unref(&ctx->hw_frames_ctx);
> +        }
> +
> +        av_frame_free(&p->frame);
>
>          pthread_mutex_destroy(&p->mutex);
>          pthread_mutex_destroy(&p->progress_mutex);
> @@ -727,26 +746,6 @@ void ff_frame_thread_free(AVCodecContext *avctx, int
> thread_count)
>          pthread_cond_destroy(&p->output_cond);
>          av_packet_unref(&p->avpkt);
>
> -#if FF_API_THREAD_SAFE_CALLBACKS
> -        for (int j = 0; j < p->released_buffers_allocated; j++)
> -            av_frame_free(&p->released_buffers[j]);
> -        av_freep(&p->released_buffers);
> -#endif
> -
> -        if (p->avctx) {
> -            if (codec->priv_class)
> -                av_opt_free(p->avctx->priv_data);
> -            av_freep(&p->avctx->priv_data);
> -
> -            av_freep(&p->avctx->slice_offset);
> -        }
> -
> -        if (p->avctx) {
> -            av_buffer_unref(&p->avctx->internal->pool);
> -            av_freep(&p->avctx->internal);
> -            av_buffer_unref(&p->avctx->hw_frames_ctx);
> -        }
> -
>          av_freep(&p->avctx);
>      }
>
> @@ -791,14 +790,19 @@ int ff_frame_thread_init(AVCodecContext *avctx)
>
>      fctx->threads = av_mallocz_array(thread_count,
> sizeof(PerThreadContext));
>      if (!fctx->threads) {
> -        av_freep(&avctx->internal->thread_ctx);
> -        return AVERROR(ENOMEM);
> +        err = AVERROR(ENOMEM);
> +        goto threads_alloc_fail;
>      }
>
> -    pthread_mutex_init(&fctx->buffer_mutex, NULL);
> -    pthread_mutex_init(&fctx->hwaccel_mutex, NULL);
> -    pthread_mutex_init(&fctx->async_mutex, NULL);
> -    pthread_cond_init(&fctx->async_cond, NULL);
> +#define PTHREAD_INIT(type, ctx, field)                         \
> +    if (err = pthread_ ## type ## _init(&ctx->field, NULL)) {  \
> +        err = AVERROR(err);                                    \
> +        goto field ## _fail;                                   \
> +    }
> +    PTHREAD_INIT(mutex, fctx, buffer_mutex)
> +    PTHREAD_INIT(mutex, fctx, hwaccel_mutex)
> +    PTHREAD_INIT(mutex, fctx, async_mutex)
> +    PTHREAD_INIT(cond, fctx, async_cond)
>
Since we used a macro here, could we just record type to an on-stack array,
then loop the array to free them.
It will help us get rid of most of the labels. example:

pthread_mutex_t mutexes[3];  mutex_count = 0;
pthread_cond_t   conds[1], cond_count = 0;
#define PTHREAD_INIT(type, ctx, field)                         \
   if (err = pthread_ ## type ## _init(&ctx->field, NULL)) {  \
        err = AVERROR(err);                                    \
        goto fail;                                   \
   }\
  type##es[type##_count++] = &ctx->field;

failed:
  free_mutexs( mutexes,  mutex_count);
  free_conditions(conds,  cond_count);



>
>      fctx->async_lock = 1;
>      fctx->delaying = 1;
> @@ -806,40 +810,37 @@ int ff_frame_thread_init(AVCodecContext *avctx)
>      if (codec->type == AVMEDIA_TYPE_VIDEO)
>          avctx->delay = src->thread_count - 1;
>
> -    for (i = 0; i < thread_count; i++) {
> -        AVCodecContext *copy = av_malloc(sizeof(AVCodecContext));
> +    for (i = 0; i < thread_count; ) {
>
Is it possible to move this loop body to a function?  It will make this
function more readable.
and we can use a stack array to get rid of labels too.

         PerThreadContext *p  = &fctx->threads[i];
> +        AVCodecContext *copy;
> +        int first = !i;
>
> -        pthread_mutex_init(&p->mutex, NULL);
> -        pthread_mutex_init(&p->progress_mutex, NULL);
> -        pthread_cond_init(&p->input_cond, NULL);
> -        pthread_cond_init(&p->progress_cond, NULL);
> -        pthread_cond_init(&p->output_cond, NULL);
> -
> -        p->frame = av_frame_alloc();
> -        if (!p->frame) {
> -            av_freep(&copy);
> -            err = AVERROR(ENOMEM);
> -            goto error;
> -        }
> +        PTHREAD_INIT(mutex, p, mutex)
> +        PTHREAD_INIT(mutex, p, progress_mutex)
> +        PTHREAD_INIT(cond,  p, input_cond)
> +        PTHREAD_INIT(cond,  p, progress_cond)
> +        PTHREAD_INIT(cond,  p, output_cond)
>
> -        p->parent = fctx;
> -        p->avctx  = copy;
> +        atomic_init(&p->state, STATE_INPUT_READY);
>
> +        copy = av_memdup(src, sizeof(*src));
>          if (!copy) {
>              err = AVERROR(ENOMEM);
> -            goto error;
> +            goto copy_fail;
>          }
> +        /* From now on, this PerThreadContext will be cleaned up by
> +         * ff_frame_thread_free in case of errors. */
> +        i++;
>
> -        *copy = *src;
> +        p->parent = fctx;
> +        p->avctx  = copy;
>
> -        copy->internal = av_malloc(sizeof(AVCodecInternal));
> +        copy->internal = av_memdup(src->internal, sizeof(*src->internal));
>          if (!copy->internal) {
>              copy->priv_data = NULL;
>              err = AVERROR(ENOMEM);
>              goto error;
>          }
> -        *copy->internal = *src->internal;
>          copy->internal->thread_ctx = p;
>          copy->internal->last_pkt_props = &p->avpkt;
>
> @@ -860,30 +861,66 @@ int ff_frame_thread_init(AVCodecContext *avctx)
>              }
>          }
>
> -        if (i)
> +        p->frame = av_frame_alloc();
> +        if (!p->frame) {
> +            err = AVERROR(ENOMEM);
> +            goto error;
> +        }
> +
> +        if (!first)
>              copy->internal->is_copy = 1;
>
> -        if (codec->init)
> +        if (codec->init) {
>              err = codec->init(copy);
> +            if (err) {
> +                if (codec->caps_internal & FF_CODEC_CAP_INIT_CLEANUP)
> +                    p->thread_init = NEEDS_CLOSE;
> +                goto error;
> +            }
> +        }
> +        p->thread_init = NEEDS_CLOSE;
>
> -        if (err) goto error;
> -
> -        if (!i)
> +        if (first)
>              update_context_from_thread(avctx, copy, 1);
>
>          atomic_init(&p->debug_threads, (copy->debug & FF_DEBUG_THREADS)
> != 0);
>
>          err = AVERROR(pthread_create(&p->thread, NULL,
> frame_worker_thread, p));
> -        p->thread_init= !err;
> -        if(!p->thread_init)
> +        if (err)
>              goto error;
> +        p->thread_init = INITIALIZED;
> +        continue;
> +
> +    copy_fail:
> +        pthread_cond_destroy(&p->output_cond);
> +    output_cond_fail:
> +        pthread_cond_destroy(&p->progress_cond);
> +    progress_cond_fail:
> +        pthread_cond_destroy(&p->input_cond);
> +    input_cond_fail:
> +        pthread_mutex_destroy(&p->progress_mutex);
> +    progress_mutex_fail:
> +        pthread_mutex_destroy(&p->mutex);
> +    mutex_fail:
> +        goto error;
>      }
>
>      return 0;
>
>  error:
> -    ff_frame_thread_free(avctx, i+1);
> +    ff_frame_thread_free(avctx, i);
> +    return err;
>
> +async_cond_fail:
> +    pthread_mutex_destroy(&fctx->async_mutex);
> +async_mutex_fail:
> +    pthread_mutex_destroy(&fctx->hwaccel_mutex);
> +hwaccel_mutex_fail:
> +    pthread_mutex_destroy(&fctx->buffer_mutex);
> +buffer_mutex_fail:
> +    av_freep(&fctx->threads);
> +threads_alloc_fail:
> +    av_freep(&avctx->internal->thread_ctx);
>      return err;
>  }
>
> --
> 2.27.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index 4429a4d59c..a10d8c1266 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -64,6 +64,12 @@  enum {
     STATE_SETUP_FINISHED,
 };
 
+enum {
+    UNINITIALIZED,  ///< Thread has not been created, AVCodec->close mustn't be called
+    NEEDS_CLOSE,    ///< AVCodec->close needs to be called
+    INITIALIZED,    ///< Thread has been properly set up
+};
+
 /**
  * Context used by codec threads and stored in their AVCodecInternal thread_ctx.
  */
@@ -698,27 +704,40 @@  void ff_frame_thread_free(AVCodecContext *avctx, int thread_count)
 
     for (i = 0; i < thread_count; i++) {
         PerThreadContext *p = &fctx->threads[i];
+        AVCodecContext *ctx = p->avctx;
 
-        pthread_mutex_lock(&p->mutex);
-        p->die = 1;
-        pthread_cond_signal(&p->input_cond);
-        pthread_mutex_unlock(&p->mutex);
-
-        if (p->thread_init)
-            pthread_join(p->thread, NULL);
-        p->thread_init=0;
+        if (ctx->internal) {
+            if (p->thread_init == INITIALIZED) {
+                pthread_mutex_lock(&p->mutex);
+                p->die = 1;
+                pthread_cond_signal(&p->input_cond);
+                pthread_mutex_unlock(&p->mutex);
 
-        if (codec->close && p->avctx)
-            codec->close(p->avctx);
+                pthread_join(p->thread, NULL);
+            }
+            if (codec->close && p->thread_init != UNINITIALIZED)
+                codec->close(ctx);
 
 #if FF_API_THREAD_SAFE_CALLBACKS
-        release_delayed_buffers(p);
+            release_delayed_buffers(p);
+            for (int j = 0; j < p->released_buffers_allocated; j++)
+                av_frame_free(&p->released_buffers[j]);
+            av_freep(&p->released_buffers);
 #endif
-        av_frame_free(&p->frame);
-    }
+            if (ctx->priv_data) {
+                if (codec->priv_class)
+                    av_opt_free(ctx->priv_data);
+                av_freep(&ctx->priv_data);
+            }
 
-    for (i = 0; i < thread_count; i++) {
-        PerThreadContext *p = &fctx->threads[i];
+            av_freep(&ctx->slice_offset);
+
+            av_buffer_unref(&ctx->internal->pool);
+            av_freep(&ctx->internal);
+            av_buffer_unref(&ctx->hw_frames_ctx);
+        }
+
+        av_frame_free(&p->frame);
 
         pthread_mutex_destroy(&p->mutex);
         pthread_mutex_destroy(&p->progress_mutex);
@@ -727,26 +746,6 @@  void ff_frame_thread_free(AVCodecContext *avctx, int thread_count)
         pthread_cond_destroy(&p->output_cond);
         av_packet_unref(&p->avpkt);
 
-#if FF_API_THREAD_SAFE_CALLBACKS
-        for (int j = 0; j < p->released_buffers_allocated; j++)
-            av_frame_free(&p->released_buffers[j]);
-        av_freep(&p->released_buffers);
-#endif
-
-        if (p->avctx) {
-            if (codec->priv_class)
-                av_opt_free(p->avctx->priv_data);
-            av_freep(&p->avctx->priv_data);
-
-            av_freep(&p->avctx->slice_offset);
-        }
-
-        if (p->avctx) {
-            av_buffer_unref(&p->avctx->internal->pool);
-            av_freep(&p->avctx->internal);
-            av_buffer_unref(&p->avctx->hw_frames_ctx);
-        }
-
         av_freep(&p->avctx);
     }
 
@@ -791,14 +790,19 @@  int ff_frame_thread_init(AVCodecContext *avctx)
 
     fctx->threads = av_mallocz_array(thread_count, sizeof(PerThreadContext));
     if (!fctx->threads) {
-        av_freep(&avctx->internal->thread_ctx);
-        return AVERROR(ENOMEM);
+        err = AVERROR(ENOMEM);
+        goto threads_alloc_fail;
     }
 
-    pthread_mutex_init(&fctx->buffer_mutex, NULL);
-    pthread_mutex_init(&fctx->hwaccel_mutex, NULL);
-    pthread_mutex_init(&fctx->async_mutex, NULL);
-    pthread_cond_init(&fctx->async_cond, NULL);
+#define PTHREAD_INIT(type, ctx, field)                         \
+    if (err = pthread_ ## type ## _init(&ctx->field, NULL)) {  \
+        err = AVERROR(err);                                    \
+        goto field ## _fail;                                   \
+    }
+    PTHREAD_INIT(mutex, fctx, buffer_mutex)
+    PTHREAD_INIT(mutex, fctx, hwaccel_mutex)
+    PTHREAD_INIT(mutex, fctx, async_mutex)
+    PTHREAD_INIT(cond, fctx, async_cond)
 
     fctx->async_lock = 1;
     fctx->delaying = 1;
@@ -806,40 +810,37 @@  int ff_frame_thread_init(AVCodecContext *avctx)
     if (codec->type == AVMEDIA_TYPE_VIDEO)
         avctx->delay = src->thread_count - 1;
 
-    for (i = 0; i < thread_count; i++) {
-        AVCodecContext *copy = av_malloc(sizeof(AVCodecContext));
+    for (i = 0; i < thread_count; ) {
         PerThreadContext *p  = &fctx->threads[i];
+        AVCodecContext *copy;
+        int first = !i;
 
-        pthread_mutex_init(&p->mutex, NULL);
-        pthread_mutex_init(&p->progress_mutex, NULL);
-        pthread_cond_init(&p->input_cond, NULL);
-        pthread_cond_init(&p->progress_cond, NULL);
-        pthread_cond_init(&p->output_cond, NULL);
-
-        p->frame = av_frame_alloc();
-        if (!p->frame) {
-            av_freep(&copy);
-            err = AVERROR(ENOMEM);
-            goto error;
-        }
+        PTHREAD_INIT(mutex, p, mutex)
+        PTHREAD_INIT(mutex, p, progress_mutex)
+        PTHREAD_INIT(cond,  p, input_cond)
+        PTHREAD_INIT(cond,  p, progress_cond)
+        PTHREAD_INIT(cond,  p, output_cond)
 
-        p->parent = fctx;
-        p->avctx  = copy;
+        atomic_init(&p->state, STATE_INPUT_READY);
 
+        copy = av_memdup(src, sizeof(*src));
         if (!copy) {
             err = AVERROR(ENOMEM);
-            goto error;
+            goto copy_fail;
         }
+        /* From now on, this PerThreadContext will be cleaned up by
+         * ff_frame_thread_free in case of errors. */
+        i++;
 
-        *copy = *src;
+        p->parent = fctx;
+        p->avctx  = copy;
 
-        copy->internal = av_malloc(sizeof(AVCodecInternal));
+        copy->internal = av_memdup(src->internal, sizeof(*src->internal));
         if (!copy->internal) {
             copy->priv_data = NULL;
             err = AVERROR(ENOMEM);
             goto error;
         }
-        *copy->internal = *src->internal;
         copy->internal->thread_ctx = p;
         copy->internal->last_pkt_props = &p->avpkt;
 
@@ -860,30 +861,66 @@  int ff_frame_thread_init(AVCodecContext *avctx)
             }
         }
 
-        if (i)
+        p->frame = av_frame_alloc();
+        if (!p->frame) {
+            err = AVERROR(ENOMEM);
+            goto error;
+        }
+
+        if (!first)
             copy->internal->is_copy = 1;
 
-        if (codec->init)
+        if (codec->init) {
             err = codec->init(copy);
+            if (err) {
+                if (codec->caps_internal & FF_CODEC_CAP_INIT_CLEANUP)
+                    p->thread_init = NEEDS_CLOSE;
+                goto error;
+            }
+        }
+        p->thread_init = NEEDS_CLOSE;
 
-        if (err) goto error;
-
-        if (!i)
+        if (first)
             update_context_from_thread(avctx, copy, 1);
 
         atomic_init(&p->debug_threads, (copy->debug & FF_DEBUG_THREADS) != 0);
 
         err = AVERROR(pthread_create(&p->thread, NULL, frame_worker_thread, p));
-        p->thread_init= !err;
-        if(!p->thread_init)
+        if (err)
             goto error;
+        p->thread_init = INITIALIZED;
+        continue;
+
+    copy_fail:
+        pthread_cond_destroy(&p->output_cond);
+    output_cond_fail:
+        pthread_cond_destroy(&p->progress_cond);
+    progress_cond_fail:
+        pthread_cond_destroy(&p->input_cond);
+    input_cond_fail:
+        pthread_mutex_destroy(&p->progress_mutex);
+    progress_mutex_fail:
+        pthread_mutex_destroy(&p->mutex);
+    mutex_fail:
+        goto error;
     }
 
     return 0;
 
 error:
-    ff_frame_thread_free(avctx, i+1);
+    ff_frame_thread_free(avctx, i);
+    return err;
 
+async_cond_fail:
+    pthread_mutex_destroy(&fctx->async_mutex);
+async_mutex_fail:
+    pthread_mutex_destroy(&fctx->hwaccel_mutex);
+hwaccel_mutex_fail:
+    pthread_mutex_destroy(&fctx->buffer_mutex);
+buffer_mutex_fail:
+    av_freep(&fctx->threads);
+threads_alloc_fail:
+    av_freep(&avctx->internal->thread_ctx);
     return err;
 }