From patchwork Fri Mar 13 10:28:39 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anton Khirnov X-Patchwork-Id: 18158 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 A0F8244B540 for ; Fri, 13 Mar 2020 12:30:18 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 8FC9B68B0B1; Fri, 13 Mar 2020 12:30:18 +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 ESMTP id 1A2C868B08D for ; Fri, 13 Mar 2020 12:30:11 +0200 (EET) Received: from localhost (localhost [IPv6:::1]) by mail.red.khirnov.net (Postfix) with ESMTP id DC2F4295747 for ; Fri, 13 Mar 2020 11:30:10 +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 a9FeQe53SsqP for ; Fri, 13 Mar 2020 11:30:10 +0100 (CET) Received: from quelana.khirnov.net (unknown [IPv6:2a00:c500:61:23b::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) client-signature RSA-PSS (2048 bits)) (Client CN "quelana.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail.red.khirnov.net (Postfix) with ESMTPS id 4F551295796 for ; Fri, 13 Mar 2020 11:30:10 +0100 (CET) Received: from localhost (quelana.khirnov.net [IPv6:::1]) by quelana.khirnov.net (Postfix) with ESMTP id D57342534A for ; Fri, 13 Mar 2020 11:30:09 +0100 (CET) Received: from quelana.khirnov.net ([IPv6:::1]) by localhost (quelana.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id h4xDw2zp4Lox for ; Fri, 13 Mar 2020 11:30:07 +0100 (CET) Received: from libav.daenerys.khirnov.net (libav.daenerys.khirnov.net [IPv6:2a00:c500:561:201::7]) by quelana.khirnov.net (Postfix) with ESMTP id 24B7D2534E for ; Fri, 13 Mar 2020 11:30:00 +0100 (CET) Received: by libav.daenerys.khirnov.net (Postfix, from userid 1000) id 854E820E035B; Fri, 13 Mar 2020 11:29:56 +0100 (CET) From: Anton Khirnov To: ffmpeg-devel@ffmpeg.org Date: Fri, 13 Mar 2020 11:28:39 +0100 Message-Id: <20200313102850.23913-7-anton@khirnov.net> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200313102850.23913-1-anton@khirnov.net> References: <20200313102850.23913-1-anton@khirnov.net> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 07/18] lavc: do not implicitly share the frame pool between threads 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" Currently the frame pool used by the default get_buffer2() implementation is a single struct, allocated when opening the decoder. A pointer to it is simply copied to each frame thread and we assume that no thread attempts to modify it at an unexpected time. This is rather fragile and potentially dangerous. With this commit, the frame pool is made refcounted, with the reference being propagated across threads along with other context variables. The frame pool is now also immutable - when the stream parameters change we drop the old reference and create a new one. --- libavcodec/decode.c | 102 ++++++++++++++++++++++++++++--------- libavcodec/internal.h | 21 +------- libavcodec/pthread_frame.c | 12 +++++ libavcodec/utils.c | 13 +---- 4 files changed, 94 insertions(+), 54 deletions(-) diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 07b64b387b..1b8c76352e 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -45,6 +45,25 @@ #include "internal.h" #include "thread.h" +typedef struct FramePool { + /** + * Pools for each data plane. For audio all the planes have the same size, + * so only pools[0] is used. + */ + AVBufferPool *pools[4]; + + /* + * Pool parameters + */ + int format; + int width, height; + int stride_align[AV_NUM_DATA_POINTERS]; + int linesize[4]; + int planes; + int channels; + int samples; +} FramePool; + static int apply_param_change(AVCodecContext *avctx, const AVPacket *avpkt) { int size = 0, ret; @@ -1504,10 +1523,61 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt) return ret; } +static void frame_pool_free(void *opaque, uint8_t *data) +{ + FramePool *pool = (FramePool*)data; + int i; + + for (i = 0; i < FF_ARRAY_ELEMS(pool->pools); i++) + av_buffer_pool_uninit(&pool->pools[i]); + + av_freep(&data); +} + +static AVBufferRef *frame_pool_alloc(void) +{ + FramePool *pool = av_mallocz(sizeof(*pool)); + AVBufferRef *buf; + + if (!pool) + return NULL; + + buf = av_buffer_create((uint8_t*)pool, sizeof(*pool), + frame_pool_free, NULL, 0); + if (!buf) { + av_freep(&pool); + return NULL; + } + + return buf; +} + static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) { - FramePool *pool = avctx->internal->pool; - int i, ret; + FramePool *pool = avctx->internal->pool ? + (FramePool*)avctx->internal->pool->data : NULL; + AVBufferRef *pool_buf; + int i, ret, ch, planes; + + if (avctx->codec_type == AVMEDIA_TYPE_AUDIO) { + int planar = av_sample_fmt_is_planar(frame->format); + ch = frame->channels; + planes = planar ? ch : 1; + } + + if (pool && pool->format == frame->format) { + if (avctx->codec_type == AVMEDIA_TYPE_VIDEO && + pool->width == frame->width && pool->height == frame->height) + return 0; + if (avctx->codec_type == AVMEDIA_TYPE_AUDIO && pool->planes == planes && + pool->channels == ch && frame->nb_samples == pool->samples) + return 0; + } + + pool_buf = frame_pool_alloc(); + if (!pool_buf) + return AVERROR(ENOMEM); + pool = (FramePool*)pool_buf->data; switch (avctx->codec_type) { case AVMEDIA_TYPE_VIDEO: { @@ -1518,10 +1588,6 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) int h = frame->height; int tmpsize, unaligned; - if (pool->format == frame->format && - pool->width == frame->width && pool->height == frame->height) - return 0; - avcodec_align_dimensions2(avctx, &w, &h, pool->stride_align); do { @@ -1548,7 +1614,6 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) size[i] = tmpsize - (data[i] - data[0]); for (i = 0; i < 4; i++) { - av_buffer_pool_uninit(&pool->pools[i]); pool->linesize[i] = linesize[i]; if (size[i]) { pool->pools[i] = av_buffer_pool_init(size[i] + 16 + STRIDE_ALIGN - 1, @@ -1568,15 +1633,6 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) break; } case AVMEDIA_TYPE_AUDIO: { - int ch = frame->channels; //av_get_channel_layout_nb_channels(frame->channel_layout); - int planar = av_sample_fmt_is_planar(frame->format); - int planes = planar ? ch : 1; - - if (pool->format == frame->format && pool->planes == planes && - pool->channels == ch && frame->nb_samples == pool->samples) - return 0; - - av_buffer_pool_uninit(&pool->pools[0]); ret = av_samples_get_buffer_size(&pool->linesize[0], ch, frame->nb_samples, frame->format, 0); if (ret < 0) @@ -1596,19 +1652,19 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame) } default: av_assert0(0); } + + av_buffer_unref(&avctx->internal->pool); + avctx->internal->pool = pool_buf; + return 0; fail: - for (i = 0; i < 4; i++) - av_buffer_pool_uninit(&pool->pools[i]); - pool->format = -1; - pool->planes = pool->channels = pool->samples = 0; - pool->width = pool->height = 0; + av_buffer_unref(&pool_buf); return ret; } static int audio_get_buffer(AVCodecContext *avctx, AVFrame *frame) { - FramePool *pool = avctx->internal->pool; + FramePool *pool = (FramePool*)avctx->internal->pool->data; int planes = pool->planes; int i; @@ -1653,7 +1709,7 @@ fail: static int video_get_buffer(AVCodecContext *s, AVFrame *pic) { - FramePool *pool = s->internal->pool; + FramePool *pool = (FramePool*)s->internal->pool->data; const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pic->format); int i; diff --git a/libavcodec/internal.h b/libavcodec/internal.h index 700807cd75..721fd017d4 100644 --- a/libavcodec/internal.h +++ b/libavcodec/internal.h @@ -108,25 +108,6 @@ # define STRIDE_ALIGN 8 #endif -typedef struct FramePool { - /** - * Pools for each data plane. For audio all the planes have the same size, - * so only pools[0] is used. - */ - AVBufferPool *pools[4]; - - /* - * Pool parameters - */ - int format; - int width, height; - int stride_align[AV_NUM_DATA_POINTERS]; - int linesize[4]; - int planes; - int channels; - int samples; -} FramePool; - typedef struct DecodeSimpleContext { AVPacket *in_pkt; AVFrame *out_frame; @@ -154,7 +135,7 @@ typedef struct AVCodecInternal { AVFrame *to_free; - FramePool *pool; + AVBufferRef *pool; void *thread_ctx; diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 63a0d4b39b..8a9905dee2 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -296,6 +296,17 @@ static int update_context_from_thread(AVCodecContext *dst, AVCodecContext *src, } dst->hwaccel_flags = src->hwaccel_flags; + + if (!!dst->internal->pool != !!src->internal->pool || + (dst->internal->pool && dst->internal->pool->data != src->internal->pool->data)) { + av_buffer_unref(&dst->internal->pool); + + if (src->internal->pool) { + dst->internal->pool = av_buffer_ref(src->internal->pool); + if (!dst->internal->pool) + return AVERROR(ENOMEM); + } + } } if (for_user) { @@ -707,6 +718,7 @@ void ff_frame_thread_free(AVCodecContext *avctx, int thread_count) } if (p->avctx) { + av_buffer_unref(&p->avctx->internal->pool); av_freep(&p->avctx->internal); av_buffer_unref(&p->avctx->hw_frames_ctx); } diff --git a/libavcodec/utils.c b/libavcodec/utils.c index c4dc136d3c..50ab320b43 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -583,12 +583,6 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code } avctx->internal = avci; - avci->pool = av_mallocz(sizeof(*avci->pool)); - if (!avci->pool) { - ret = AVERROR(ENOMEM); - goto free_and_end; - } - avci->to_free = av_frame_alloc(); if (!avci->to_free) { ret = AVERROR(ENOMEM); @@ -1076,7 +1070,7 @@ FF_ENABLE_DEPRECATION_WARNINGS av_packet_free(&avci->ds.in_pkt); ff_decode_bsfs_uninit(avctx); - av_freep(&avci->pool); + av_buffer_unref(&avci->pool); } av_freep(&avci); avctx->internal = NULL; @@ -1111,7 +1105,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) return 0; if (avcodec_is_open(avctx)) { - FramePool *pool = avctx->internal->pool; if (CONFIG_FRAME_THREAD_ENCODER && avctx->internal->frame_thread_encoder && avctx->thread_count > 1) { ff_frame_thread_encoder_free(avctx); @@ -1130,9 +1123,7 @@ av_cold int avcodec_close(AVCodecContext *avctx) av_packet_free(&avctx->internal->ds.in_pkt); - for (i = 0; i < FF_ARRAY_ELEMS(pool->pools); i++) - av_buffer_pool_uninit(&pool->pools[i]); - av_freep(&avctx->internal->pool); + av_buffer_unref(&avctx->internal->pool); if (avctx->hwaccel && avctx->hwaccel->uninit) avctx->hwaccel->uninit(avctx);