[FFmpeg-devel] [PATCH] avcodec: deprecate thread_safe_callbacks
James Almer
jamrial at gmail.com
Tue Aug 4 22:18:33 EEST 2020
On 8/4/2020 8:59 AM, Anton Khirnov wrote:
> They add considerable complexity to frame-threading implementation,
> which includes an unavoidably leaking error path, while the advantages
> of this option to the users are highly dubious.
>
> It should be always possible and desirable for the callers to make their
> get_buffer2() implementation thread-safe, so deprecate this option.
> ---
> doc/APIchanges | 4 +++
> doc/multithreading.txt | 3 +-
> fftools/ffmpeg.c | 2 ++
> libavcodec/avcodec.h | 7 ++++
> libavcodec/pthread_frame.c | 69 +++++++++++++++++++++++++++++++++++---
> libavcodec/thread.h | 4 +++
> libavcodec/utils.c | 13 +++++++
> libavcodec/version.h | 3 ++
> 8 files changed, 99 insertions(+), 6 deletions(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 208258be05..44167a602b 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil: 2017-10-21
>
> API changes, most recent first:
>
> +2020-xx-xx - xxxxxxxxxx - lavc 58.87.100 - avcodec.h
> + Deprecate AVCodecContext.thread_safe_callbacks. After the next major bump,
Maybe "Once removed" or similar instead, like you added to the field's doxy.
> + user callbacks must always be thread-safe when frame threading is used.
> +
> 2020-xx-xx - xxxxxxxxxx - lavc 58.87.100 - avcodec.h codec_par.h
> Move AVBitstreamFilter-related public API to new header bsf.h.
> Move AVCodecParameters-related public API to new header codec_par.h.
> diff --git a/doc/multithreading.txt b/doc/multithreading.txt
> index 4f645dc147..470194ff85 100644
> --- a/doc/multithreading.txt
> +++ b/doc/multithreading.txt
> @@ -20,8 +20,7 @@ Slice threading -
>
> Frame threading -
> * Restrictions with slice threading also apply.
> -* For best performance, the client should set thread_safe_callbacks if it
> - provides a thread-safe get_buffer() callback.
> +* Custom get_buffer2() and get_format() callbacks must be thread-safe.
> * There is one frame of delay added for every thread beyond the first one.
> Clients must be able to handle this; the pkt_dts and pkt_pts fields in
> AVFrame will work as usual.
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index ad95a0e417..eed72b67f1 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2883,7 +2883,9 @@ static int init_input_stream(int ist_index, char *error, int error_len)
> ist->dec_ctx->opaque = ist;
> ist->dec_ctx->get_format = get_format;
> ist->dec_ctx->get_buffer2 = get_buffer;
> +#if LIBAVCODEC_VERSION_MAJOR < 59
> ist->dec_ctx->thread_safe_callbacks = 1;
> +#endif
>
> av_opt_set_int(ist->dec_ctx, "refcounted_frames", 1, 0);
> if (ist->dec_ctx->codec_id == AV_CODEC_ID_DVB_SUBTITLE &&
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index f10b7a06ec..2dec0d8ca0 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1934,6 +1934,7 @@ typedef struct AVCodecContext {
> */
> int active_thread_type;
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> /**
> * Set by the client if its custom get_buffer() callback can be called
> * synchronously from another thread, which allows faster multithreaded decoding.
> @@ -1941,8 +1942,14 @@ typedef struct AVCodecContext {
> * Ignored if the default get_buffer() is used.
> * - encoding: Set by user.
> * - decoding: Set by user.
> + *
> + * @deprecated the custom get_buffer2() callback should always be
> + * thread-safe. Thread-unsafe get_buffer2() implementations will be
> + * invalid once this field is removed.
> */
> + attribute_deprecated
> int thread_safe_callbacks;
> +#endif
>
> /**
> * The codec may call this to execute several independent things.
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 64121f5a9a..75722e359e 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -89,6 +89,7 @@ typedef struct PerThreadContext {
>
> atomic_int state;
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> /**
> * Array of frames passed to ff_thread_release_buffer().
> * Frames are released after all threads referencing them are finished.
> @@ -102,6 +103,7 @@ typedef struct PerThreadContext {
>
> const enum AVPixelFormat *available_formats; ///< Format array for get_format()
> enum AVPixelFormat result_format; ///< get_format() result
> +#endif
>
> int die; ///< Set when the thread should exit.
>
> @@ -137,8 +139,10 @@ typedef struct FrameThreadContext {
> */
> } FrameThreadContext;
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> #define THREAD_SAFE_CALLBACKS(avctx) \
> ((avctx)->thread_safe_callbacks || (avctx)->get_buffer2 == avcodec_default_get_buffer2)
> +#endif
>
> static void async_lock(FrameThreadContext *fctx)
> {
> @@ -178,8 +182,14 @@ static attribute_align_arg void *frame_worker_thread(void *arg)
>
> if (p->die) break;
>
> - if (!codec->update_thread_context && THREAD_SAFE_CALLBACKS(avctx))
> +FF_DISABLE_DEPRECATION_WARNINGS
> + if (!codec->update_thread_context
> +#if FF_API_THREAD_SAFE_CALLBACKS
> + && THREAD_SAFE_CALLBACKS(avctx)
> +#endif
> + )
> ff_thread_finish_setup(avctx);
> +FF_ENABLE_DEPRECATION_WARNINGS
>
> /* If a decoder supports hwaccel, then it must call ff_get_format().
> * Since that call must happen before ff_thread_finish_setup(), the
> @@ -352,7 +362,11 @@ static int update_context_from_user(AVCodecContext *dst, AVCodecContext *src)
>
> dst->frame_number = src->frame_number;
> dst->reordered_opaque = src->reordered_opaque;
> +#if FF_API_THREAD_SAFE_CALLBACKS
> +FF_DISABLE_DEPRECATION_WARNINGS
> dst->thread_safe_callbacks = src->thread_safe_callbacks;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>
> if (src->slice_count && src->slice_offset) {
> if (dst->slice_count < src->slice_count) {
> @@ -368,6 +382,7 @@ static int update_context_from_user(AVCodecContext *dst, AVCodecContext *src)
> return 0;
> }
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> /// Releases the buffers that this decoding thread was the last user of.
> static void release_delayed_buffers(PerThreadContext *p)
> {
> @@ -388,6 +403,7 @@ static void release_delayed_buffers(PerThreadContext *p)
> pthread_mutex_unlock(&fctx->buffer_mutex);
> }
> }
> +#endif
>
> static int submit_packet(PerThreadContext *p, AVCodecContext *user_avctx,
> AVPacket *avpkt)
> @@ -411,7 +427,9 @@ static int submit_packet(PerThreadContext *p, AVCodecContext *user_avctx,
> (p->avctx->debug & FF_DEBUG_THREADS) != 0,
> memory_order_relaxed);
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> release_delayed_buffers(p);
> +#endif
>
> if (prev_thread) {
> int err;
> @@ -441,6 +459,8 @@ static int submit_packet(PerThreadContext *p, AVCodecContext *user_avctx,
> pthread_cond_signal(&p->input_cond);
> pthread_mutex_unlock(&p->mutex);
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> +FF_DISABLE_DEPRECATION_WARNINGS
> /*
> * If the client doesn't have a thread-safe get_buffer(),
> * then decoding threads call back to the main thread,
> @@ -474,6 +494,8 @@ static int submit_packet(PerThreadContext *p, AVCodecContext *user_avctx,
> pthread_mutex_unlock(&p->progress_mutex);
> }
> }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>
> fctx->prev_thread = p;
> fctx->next_decoding++;
> @@ -665,7 +687,10 @@ void ff_frame_thread_free(AVCodecContext *avctx, int thread_count)
> {
> FrameThreadContext *fctx = avctx->internal->thread_ctx;
> const AVCodec *codec = avctx->codec;
> - int i, j;
> + int i;
> +#if FF_API_THREAD_SAFE_CALLBACKS
> + int j;
> +#endif
Just remove this and make the loop below for (int j ...)
>
> park_frame_worker_threads(fctx, thread_count);
>
> @@ -698,7 +723,9 @@ void ff_frame_thread_free(AVCodecContext *avctx, int thread_count)
> if (codec->close && p->avctx)
> codec->close(p->avctx);
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> release_delayed_buffers(p);
> +#endif
> av_frame_free(&p->frame);
> }
>
> @@ -712,9 +739,11 @@ 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 (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)
> @@ -892,7 +921,9 @@ void ff_thread_flush(AVCodecContext *avctx)
> av_frame_unref(p->frame);
> p->result = 0;
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> release_delayed_buffers(p);
> +#endif
>
> if (avctx->codec->flush)
> avctx->codec->flush(p->avctx);
> @@ -902,10 +933,16 @@ void ff_thread_flush(AVCodecContext *avctx)
> int ff_thread_can_start_frame(AVCodecContext *avctx)
> {
> PerThreadContext *p = avctx->internal->thread_ctx;
> +FF_DISABLE_DEPRECATION_WARNINGS
> if ((avctx->active_thread_type&FF_THREAD_FRAME) && atomic_load(&p->state) != STATE_SETTING_UP &&
> - (avctx->codec->update_thread_context || !THREAD_SAFE_CALLBACKS(avctx))) {
> + (avctx->codec->update_thread_context
> +#if FF_API_THREAD_SAFE_CALLBACKS
> + || !THREAD_SAFE_CALLBACKS(avctx)
> +#endif
> + )) {
> return 0;
> }
> +FF_ENABLE_DEPRECATION_WARNINGS
> return 1;
> }
>
> @@ -919,8 +956,14 @@ static int thread_get_buffer_internal(AVCodecContext *avctx, ThreadFrame *f, int
> if (!(avctx->active_thread_type & FF_THREAD_FRAME))
> return ff_get_buffer(avctx, f->f, flags);
>
> +FF_DISABLE_DEPRECATION_WARNINGS
> if (atomic_load(&p->state) != STATE_SETTING_UP &&
> - (avctx->codec->update_thread_context || !THREAD_SAFE_CALLBACKS(avctx))) {
> + (avctx->codec->update_thread_context
> +#if FF_API_THREAD_SAFE_CALLBACKS
> + || !THREAD_SAFE_CALLBACKS(avctx)
> +#endif
> + )) {
> +FF_ENABLE_DEPRECATION_WARNINGS
> av_log(avctx, AV_LOG_ERROR, "get_buffer() cannot be called after ff_thread_finish_setup()\n");
> return -1;
> }
> @@ -938,6 +981,10 @@ static int thread_get_buffer_internal(AVCodecContext *avctx, ThreadFrame *f, int
> }
>
> pthread_mutex_lock(&p->parent->buffer_mutex);
> +#if !FF_API_THREAD_SAFE_CALLBACKS
> + err = ff_get_buffer(avctx, f->f, flags);
> +#else
> +FF_DISABLE_DEPRECATION_WARNINGS
> if (THREAD_SAFE_CALLBACKS(avctx)) {
> err = ff_get_buffer(avctx, f->f, flags);
> } else {
> @@ -957,6 +1004,8 @@ static int thread_get_buffer_internal(AVCodecContext *avctx, ThreadFrame *f, int
> }
> if (!THREAD_SAFE_CALLBACKS(avctx) && !avctx->codec->update_thread_context)
> ff_thread_finish_setup(avctx);
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> if (err)
> av_buffer_unref(&f->progress);
>
> @@ -965,6 +1014,8 @@ static int thread_get_buffer_internal(AVCodecContext *avctx, ThreadFrame *f, int
> return err;
> }
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> +FF_DISABLE_DEPRECATION_WARNINGS
> enum AVPixelFormat ff_thread_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
> {
> enum AVPixelFormat res;
> @@ -990,6 +1041,8 @@ enum AVPixelFormat ff_thread_get_format(AVCodecContext *avctx, const enum AVPixe
>
> return res;
> }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>
> int ff_thread_get_buffer(AVCodecContext *avctx, ThreadFrame *f, int flags)
> {
> @@ -1001,12 +1054,16 @@ int ff_thread_get_buffer(AVCodecContext *avctx, ThreadFrame *f, int flags)
>
> void ff_thread_release_buffer(AVCodecContext *avctx, ThreadFrame *f)
> {
> +#if FF_API_THREAD_SAFE_CALLBACKS
> +FF_DISABLE_DEPRECATION_WARNINGS
> PerThreadContext *p = avctx->internal->thread_ctx;
> FrameThreadContext *fctx;
> AVFrame *dst;
> int ret = 0;
> int can_direct_free = !(avctx->active_thread_type & FF_THREAD_FRAME) ||
> THREAD_SAFE_CALLBACKS(avctx);
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>
> if (!f->f)
> return;
> @@ -1017,6 +1074,9 @@ void ff_thread_release_buffer(AVCodecContext *avctx, ThreadFrame *f)
> av_buffer_unref(&f->progress);
> f->owner[0] = f->owner[1] = NULL;
>
> +#if !FF_API_THREAD_SAFE_CALLBACKS
> + av_frame_unref(f->f);
> +#else
> // when the frame buffers are not allocated, just reset it to clean state
> if (can_direct_free || !f->f->buf[0]) {
> av_frame_unref(f->f);
> @@ -1058,4 +1118,5 @@ fail:
> memset(f->f->extended_buf, 0, f->f->nb_extended_buf * sizeof(*f->f->extended_buf));
> av_frame_unref(f->f);
> }
> +#endif
> }
> diff --git a/libavcodec/thread.h b/libavcodec/thread.h
> index 540135fbc9..9361d481f6 100644
> --- a/libavcodec/thread.h
> +++ b/libavcodec/thread.h
> @@ -96,6 +96,7 @@ void ff_thread_report_progress(ThreadFrame *f, int progress, int field);
> */
> void ff_thread_await_progress(ThreadFrame *f, int progress, int field);
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> /**
> * Wrapper around get_format() for frame-multithreaded codecs.
> * Call this function instead of avctx->get_format().
> @@ -105,6 +106,9 @@ void ff_thread_await_progress(ThreadFrame *f, int progress, int field);
> * @param fmt The list of available formats.
> */
> enum AVPixelFormat ff_thread_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt);
> +#else
> +#define ff_thread_get_format ff_get_format
> +#endif
>
> /**
> * Wrapper around get_buffer() for frame-multithreaded codecs.
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 3255679550..10d5e552c2 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -673,6 +673,19 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
> goto free_and_end;
> }
>
> +#if FF_API_THREAD_SAFE_CALLBACKS
> +FF_DISABLE_DEPRECATION_WARNINGS
> + if ((avctx->thread_type & FF_THREAD_FRAME) &&
> + avctx->get_buffer2 != avcodec_default_get_buffer2 &&
> + !avctx->thread_safe_callbacks) {
> + av_log(avctx, AV_LOG_WARNING, "Requested frame threading with a "
> + "custom get_buffer2() implementation which is not marked as "
> + "thread safe. This is not supported anymore, make your "
> + "callback thread-safe.\n");
> + }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> +
> avctx->codec = codec;
> if ((avctx->codec_type == AVMEDIA_TYPE_UNKNOWN || avctx->codec_type == codec->type) &&
> avctx->codec_id == AV_CODEC_ID_NONE) {
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 9fc637313d..5b4dfea579 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -144,6 +144,9 @@
> #ifndef FF_API_UNUSED_CODEC_CAPS
> #define FF_API_UNUSED_CODEC_CAPS (LIBAVCODEC_VERSION_MAJOR < 59)
> #endif
> +#ifndef FF_API_THREAD_SAFE_CALLBACKS
> +#define FF_API_THREAD_SAFE_CALLBACKS (LIBAVCODEC_VERSION_MAJOR < 59)
> +#endif
>
>
> #endif /* AVCODEC_VERSION_H */
>
More information about the ffmpeg-devel
mailing list