From patchwork Thu Oct 29 14:01:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anton Khirnov X-Patchwork-Id: 23274 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 7A11F449B77 for ; Thu, 29 Oct 2020 16:01:38 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 5F82D68B171; Thu, 29 Oct 2020 16:01:38 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail.red.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 1726168A427 for ; Thu, 29 Oct 2020 16:01:32 +0200 (EET) Received: from localhost (localhost [IPv6:::1]) by mail.red.khirnov.net (Postfix) with ESMTP id A318E2965E4 for ; Thu, 29 Oct 2020 15:01:31 +0100 (CET) Received: from mail.red.khirnov.net ([IPv6:::1]) by localhost (mail.red.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id tFbqDvR9EEmA for ; Thu, 29 Oct 2020 15:01:29 +0100 (CET) Received: from libav.daenerys.khirnov.net (libav.daenerys.khirnov.net [IPv6:2a00:c500:561:201::7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "libav.daenerys.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail.red.khirnov.net (Postfix) with ESMTPS id 876032965DF for ; Thu, 29 Oct 2020 15:01:29 +0100 (CET) Received: by libav.daenerys.khirnov.net (Postfix, from userid 1000) id DAD8D20E01A2; Thu, 29 Oct 2020 15:01:24 +0100 (CET) From: Anton Khirnov To: ffmpeg-devel@ffmpeg.org Date: Thu, 29 Oct 2020 15:01:22 +0100 Message-Id: <20201029140122.10411-1-anton@khirnov.net> X-Mailer: git-send-email 2.28.0 In-Reply-To: <4d6658a1-6b43-864a-771c-97594ed1c48c@gmail.com> References: <4d6658a1-6b43-864a-771c-97594ed1c48c@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH] avcodec: deprecate thread_safe_callbacks X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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. --- Postponed the removal until major 60, also documented this explicitly in APIchanges, so users can test for it. --- doc/APIchanges | 5 +++ doc/multithreading.txt | 3 +- fftools/ffmpeg.c | 2 ++ libavcodec/avcodec.h | 13 ++++++-- libavcodec/pthread_frame.c | 68 +++++++++++++++++++++++++++++++++++--- libavcodec/thread.h | 4 +++ libavcodec/utils.c | 13 ++++++++ libavcodec/version.h | 5 ++- 8 files changed, 102 insertions(+), 11 deletions(-) diff --git a/doc/APIchanges b/doc/APIchanges index c00f103bab..df3d6d1fb0 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,11 @@ libavutil: 2017-10-21 API changes, most recent first: +2020-xx-xx - xxxxxxxxxx - lavc 58.113.100 - avcodec.h + Deprecate AVCodecContext.thread_safe_callbacks. Starting with + LIBAVCODEC_VERSION_MAJOR=60, user callbacks must always be + thread-safe when frame threading is used. + 2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h Add AVSTREAM_EVENT_FLAG_NEW_PACKETS. 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 cb7644de6a..c3b9ec01c9 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -2890,7 +2890,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 < 60 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 20af3ef00d..3528d1e637 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -1314,9 +1314,9 @@ typedef struct AVCodecContext { * * Some decoders do not support linesizes changing between frames. * - * If frame multithreading is used and thread_safe_callbacks is set, - * this callback may be called from a different thread, but not from more - * than one at once. Does not need to be reentrant. + * If frame multithreading is used, this callback may be called from a + * different thread, but not from more than one at once. Does not need to be + * reentrant. * * @see avcodec_align_dimensions2() * @@ -1803,6 +1803,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. @@ -1810,8 +1811,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 f8a01ad8cd..c9d2e00ce3 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 @@ -344,7 +354,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) { @@ -360,6 +374,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) { @@ -380,6 +395,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) @@ -403,7 +419,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; @@ -433,6 +451,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, @@ -466,6 +486,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++; @@ -657,7 +679,7 @@ 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; park_frame_worker_threads(fctx, thread_count); @@ -690,7 +712,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); } @@ -704,9 +728,11 @@ void ff_frame_thread_free(AVCodecContext *avctx, int thread_count) pthread_cond_destroy(&p->output_cond); av_packet_unref(&p->avpkt); - for (j = 0; j < p->released_buffers_allocated; j++) +#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) @@ -889,7 +915,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); @@ -899,10 +927,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; } @@ -916,8 +950,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; } @@ -935,6 +975,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 { @@ -954,6 +998,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); @@ -962,6 +1008,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; @@ -987,6 +1035,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) { @@ -998,12 +1048,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; @@ -1014,6 +1068,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); @@ -1055,4 +1112,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 87bf154da9..413196269f 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 fb40962c47..9ac13fadfe 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -678,6 +678,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 78c4dd64ee..187a59bee7 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -28,7 +28,7 @@ #include "libavutil/version.h" #define LIBAVCODEC_VERSION_MAJOR 58 -#define LIBAVCODEC_VERSION_MINOR 112 +#define LIBAVCODEC_VERSION_MINOR 113 #define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ @@ -147,5 +147,8 @@ #ifndef FF_API_AVPRIV_PUT_BITS #define FF_API_AVPRIV_PUT_BITS (LIBAVCODEC_VERSION_MAJOR < 59) #endif +#ifndef FF_API_THREAD_SAFE_CALLBACKS +#define FF_API_THREAD_SAFE_CALLBACKS (LIBAVCODEC_VERSION_MAJOR < 60) +#endif #endif /* AVCODEC_VERSION_H */