[FFmpeg-devel] [PATCH] avcodec/pthread_frame: Fix checks and cleanup during init
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Thu Feb 11 17:57:59 EET 2021
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 at 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);
+ }
+ 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(©);
- 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
More information about the ffmpeg-devel
mailing list