From patchwork Fri Dec 25 15:47:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 24654 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 4CB7B44AF05 for ; Fri, 25 Dec 2020 18:01:23 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3267B68AE70; Fri, 25 Dec 2020 18:01:23 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f41.google.com (mail-wr1-f41.google.com [209.85.221.41]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id A52DA68AE66 for ; Fri, 25 Dec 2020 18:01:17 +0200 (EET) Received: by mail-wr1-f41.google.com with SMTP id r3so4664357wrt.2 for ; Fri, 25 Dec 2020 08:01:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=4uN4lwrJlubge8Vdjc4G2LJHYo9sZCU7tA9n3+PRkBM=; b=VmkXWSEMzDbikv2LSi2/2Rk9Ex1WJC+zgRgahPnW3TRaMJlWnbGFZhihl7FCxW0Zgq TywoEB82QcCmP4HNq5ZwvTIRWtAZitRHYw/4dm4IygRFGyjF4ZhOK/y73fHwe1AopeQb nQ8V+0eIzk00nPfVqTSlBLGDfDrGzYyfP4wddYwwHDWi046Ba4/akk3r6AsacpehXl/I NBVf2ryLw+KLN1BMkp4Yi7IBdslSL2BM8MGSMj6iA/Mgsf4Narc5oswY9uIovx8W88rK Tc65NAxhlMFitQGwEQBH413AmThjkT/dfXSuTlTeuTqWSUNNgcznM8OpT9m7mt+xLKyX R/XQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=4uN4lwrJlubge8Vdjc4G2LJHYo9sZCU7tA9n3+PRkBM=; b=pI9qUOPD3SlcQVYZKO/0fKXuak/r53HzmFRLEw8yS0n+PDGAF5LumWtBrOrhRDzwqu V8Humb5EFFXEzLR59VzZwfUBGNPmMWEC2aC0jSTFwNq9jHDCJ3RoMnUUEQvupHZgLfxL hjGAtEKnup0G/VTz2iX7HcxSv8WYDzNsW1NdPbF/3XYUjsEpPgJwIzVp3wvdsc2rPVfE aAERc1spUZE/umOzYmJf+VElrBMiAuRfE8DGieK/EB4j0BCoDaKDUuky+yV06SJrYbGz F6aIBnXz1aDLAqaWzllLo35IDxFwKfA3xdvxqd16PtWMM/C8HVU37buRdhzp6E/AnQYb kjdQ== X-Gm-Message-State: AOAM5327cTNr2b8lz82skVwq4YiNAuhA9055nW2cEx920HpU4SqKYJww ic+0fESTj0yZX/k/It8hCRXanMXBSb4= X-Google-Smtp-Source: ABdhPJzI+MtrDVlG11wonujP7QX4Nxd3vHDLX+tmWXCa5c7bJGWyMWX/79S7BtiPGOoJ0XdQnarimQ== X-Received: by 2002:a50:b282:: with SMTP id p2mr33721334edd.210.1608911580396; Fri, 25 Dec 2020 07:53:00 -0800 (PST) Received: from sblaptop.fritz.box (ipbcc1aa4b.dynamic.kabel-deutschland.de. [188.193.170.75]) by smtp.gmail.com with ESMTPSA id ho12sm13733010ejc.45.2020.12.25.07.52.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Dec 2020 07:52:59 -0800 (PST) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Fri, 25 Dec 2020 16:47:22 +0100 Message-Id: <20201225154724.287465-5-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20201225154724.287465-1-andreas.rheinhardt@gmail.com> References: <20201225154724.287465-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 5/7] Revert "avcodec: add FF_CODEC_CAP_INIT_CLEANUP for all codecs which use ff_mpv_common_init()" 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 Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" This mostly reverts commit 4b2863ff01b1fe93d9a518523c9098d17a9d8c6f. Said commit removed the freeing code from ff_mpv_common_init(), ff_mpv_common_frame_size_change() and ff_mpeg_framesize_alloc() and instead added the FF_CODEC_CAP_INIT_CLEANUP to several codecs that use ff_mpv_common_init(). This introduced several bugs: a) Several decoders using ff_mpv_common_init() in their init function were forgotten: This affected FLV, Intel H.263, RealVideo 3.0 and V4.0 as well as VC-1/WMV3. b) ff_mpv_common_init() is not only called from the init function of codecs, it is also called from AVCodec.decode functions. If an error happens after an allocation has succeeded, it can lead to memleaks; furthermore, it is now possible for the MpegEncContext to be marked as initialized even when ff_mpv_common_init() returns an error and this can lead to segfaults because decoders that call ff_mpv_common_init() when decoding a frame can mistakenly think that the MpegEncContext has been properly initialized. This can e.g. happen with H.261 or MPEG-4. c) Removing code for freeing from ff_mpeg_framesize_alloc() (which can't be called from any init function) can lead to segfaults because the check for whether it needs to called consists of checking whether the first of the buffers allocated there has been allocated. d) ff_mpv_common_frame_size_change() can also not be reached from any AVCodec.init function; yet the changes can e.g. lead to segfaults with decoders using ff_h263_decode_frame() upon allocation failure, because the MpegEncContext will upon return be flagged as both initialized and not in need of reinitialization (granted, the fact that ff_h263_decode_frame() clears context_reinit before the context has been reinited is a bug in itself). With the earlier version, the context would be cleaned upon failure and it would be attempted to initialize the context again in the next call to ff_h263_decode_frame(). While a) could be fixed by adding the missing FF_CODEC_CAP_INIT_CLEANUP, keeping the current approach would entail adding cleanup code to several other places because of b). Therefore ff_mpv_common_init() is again made to clean up after itself; the changes to the wmv2 decoder and the SVQ1 encoder have not been reverted: The former fixed a memleak, the latter allowed to remove cleanup code. Signed-off-by: Andreas Rheinhardt --- libavcodec/h261dec.c | 1 - libavcodec/h263dec.c | 4 ++-- libavcodec/mpeg12dec.c | 9 +++++---- libavcodec/mpeg4videodec.c | 3 +-- libavcodec/mpegpicture.c | 4 +++- libavcodec/mpegvideo.c | 31 ++++++++++++++++++++----------- libavcodec/msmpeg4dec.c | 8 ++++---- libavcodec/rv10.c | 2 -- 8 files changed, 35 insertions(+), 27 deletions(-) diff --git a/libavcodec/h261dec.c b/libavcodec/h261dec.c index 4f1f22b279..6c54ca79fe 100644 --- a/libavcodec/h261dec.c +++ b/libavcodec/h261dec.c @@ -683,6 +683,5 @@ AVCodec ff_h261_decoder = { .close = h261_decode_end, .decode = h261_decode_frame, .capabilities = AV_CODEC_CAP_DR1, - .caps_internal = FF_CODEC_CAP_INIT_CLEANUP, .max_lowres = 3, }; diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c index 32e26a57de..39881dc713 100644 --- a/libavcodec/h263dec.c +++ b/libavcodec/h263dec.c @@ -770,7 +770,7 @@ AVCodec ff_h263_decoder = { .decode = ff_h263_decode_frame, .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 | AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .flush = ff_mpeg_flush, .max_lowres = 3, .pix_fmts = ff_h263_hwaccel_pixfmt_list_420, @@ -788,7 +788,7 @@ AVCodec ff_h263p_decoder = { .decode = ff_h263_decode_frame, .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 | AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .flush = ff_mpeg_flush, .max_lowres = 3, .pix_fmts = ff_h263_hwaccel_pixfmt_list_420, diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index 66556c7a82..ebafac209f 100644 --- a/libavcodec/mpeg12dec.c +++ b/libavcodec/mpeg12dec.c @@ -2855,7 +2855,8 @@ static av_cold int mpeg_decode_end(AVCodecContext *avctx) { Mpeg1Context *s = avctx->priv_data; - ff_mpv_common_end(&s->mpeg_enc_ctx); + if (s->mpeg_enc_ctx_allocated) + ff_mpv_common_end(&s->mpeg_enc_ctx); av_buffer_unref(&s->a53_buf_ref); return 0; } @@ -2872,7 +2873,7 @@ AVCodec ff_mpeg1video_decoder = { .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 | AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_SLICE_THREADS, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .flush = flush, .max_lowres = 3, .hw_configs = (const AVCodecHWConfigInternal*[]) { @@ -2904,7 +2905,7 @@ AVCodec ff_mpeg2video_decoder = { .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 | AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_SLICE_THREADS, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .flush = flush, .max_lowres = 3, .profiles = NULL_IF_CONFIG_SMALL(ff_mpeg2_video_profiles), @@ -2948,7 +2949,7 @@ AVCodec ff_mpegvideo_decoder = { .close = mpeg_decode_end, .decode = mpeg_decode_frame, .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 | AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_SLICE_THREADS, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .flush = flush, .max_lowres = 3, }; diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c index d85109c857..5b1de03e1c 100644 --- a/libavcodec/mpeg4videodec.c +++ b/libavcodec/mpeg4videodec.c @@ -3585,8 +3585,7 @@ AVCodec ff_mpeg4_decoder = { AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_FRAME_THREADS, .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | - FF_CODEC_CAP_ALLOCATE_PROGRESS | - FF_CODEC_CAP_INIT_CLEANUP, + FF_CODEC_CAP_ALLOCATE_PROGRESS, .flush = ff_mpeg_flush, .max_lowres = 3, .pix_fmts = ff_h263_hwaccel_pixfmt_list_420, diff --git a/libavcodec/mpegpicture.c b/libavcodec/mpegpicture.c index 13c11ec492..2aeabf221a 100644 --- a/libavcodec/mpegpicture.c +++ b/libavcodec/mpegpicture.c @@ -79,8 +79,10 @@ int ff_mpeg_framesize_alloc(AVCodecContext *avctx, MotionEstContext *me, // linesize * interlaced * MBsize // we also use this buffer for encoding in encode_mb_internal() needig an additional 32 lines if (!FF_ALLOCZ_TYPED_ARRAY(sc->edge_emu_buffer, alloc_size * EMU_EDGE_HEIGHT) || - !FF_ALLOCZ_TYPED_ARRAY(me->scratchpad, alloc_size * 4 * 16 * 2)) + !FF_ALLOCZ_TYPED_ARRAY(me->scratchpad, alloc_size * 4 * 16 * 2)) { + av_freep(&sc->edge_emu_buffer); return AVERROR(ENOMEM); + } me->temp = me->scratchpad; sc->rd_scratchpad = me->scratchpad; sc->b_scratchpad = me->scratchpad; diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c index c28d1adef7..1bf25566b8 100644 --- a/libavcodec/mpegvideo.c +++ b/libavcodec/mpegvideo.c @@ -933,17 +933,17 @@ av_cold int ff_mpv_common_init(MpegEncContext *s) for (i = 0; i < MAX_PICTURE_COUNT; i++) { s->picture[i].f = av_frame_alloc(); if (!s->picture[i].f) - return AVERROR(ENOMEM); + goto fail_nomem; } if (!(s->next_picture.f = av_frame_alloc()) || !(s->last_picture.f = av_frame_alloc()) || !(s->current_picture.f = av_frame_alloc()) || !(s->new_picture.f = av_frame_alloc())) - return AVERROR(ENOMEM); + goto fail_nomem; if ((ret = init_context_frame(s))) - return AVERROR(ENOMEM); + goto fail; s->parse_context.state = -1; @@ -957,10 +957,10 @@ av_cold int ff_mpv_common_init(MpegEncContext *s) if (i) { s->thread_context[i] = av_memdup(s, sizeof(MpegEncContext)); if (!s->thread_context[i]) - return AVERROR(ENOMEM); + goto fail_nomem; } if ((ret = init_duplicate_context(s->thread_context[i])) < 0) - return ret; + goto fail; s->thread_context[i]->start_mb_y = (s->mb_height * (i) + nb_slices / 2) / nb_slices; s->thread_context[i]->end_mb_y = @@ -968,7 +968,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s) } } else { if ((ret = init_duplicate_context(s)) < 0) - return ret; + goto fail; s->start_mb_y = 0; s->end_mb_y = s->mb_height; } @@ -976,6 +976,11 @@ av_cold int ff_mpv_common_init(MpegEncContext *s) // } return 0; + fail_nomem: + ret = AVERROR(ENOMEM); + fail: + ff_mpv_common_end(s); + return ret; } /** @@ -1068,10 +1073,10 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s) if ((s->width || s->height) && (err = av_image_check_size(s->width, s->height, 0, s->avctx)) < 0) - return err; + goto fail; if ((err = init_context_frame(s))) - return err; + goto fail; memset(s->thread_context, 0, sizeof(s->thread_context)); s->thread_context[0] = s; @@ -1083,11 +1088,12 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s) if (i) { s->thread_context[i] = av_memdup(s, sizeof(MpegEncContext)); if (!s->thread_context[i]) { - return AVERROR(ENOMEM); + err = AVERROR(ENOMEM); + goto fail; } } if ((err = init_duplicate_context(s->thread_context[i])) < 0) - return err; + goto fail; s->thread_context[i]->start_mb_y = (s->mb_height * (i) + nb_slices / 2) / nb_slices; s->thread_context[i]->end_mb_y = @@ -1096,7 +1102,7 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s) } else { err = init_duplicate_context(s); if (err < 0) - return err; + goto fail; s->start_mb_y = 0; s->end_mb_y = s->mb_height; } @@ -1104,6 +1110,9 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s) } return 0; + fail: + ff_mpv_common_end(s); + return err; } /* init common structure for both encoder and decoder */ diff --git a/libavcodec/msmpeg4dec.c b/libavcodec/msmpeg4dec.c index 49df06a9d7..16b67192b5 100644 --- a/libavcodec/msmpeg4dec.c +++ b/libavcodec/msmpeg4dec.c @@ -888,7 +888,7 @@ AVCodec ff_msmpeg4v1_decoder = { .close = ff_h263_decode_end, .decode = ff_h263_decode_frame, .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .max_lowres = 3, .pix_fmts = (const enum AVPixelFormat[]) { AV_PIX_FMT_YUV420P, @@ -906,7 +906,7 @@ AVCodec ff_msmpeg4v2_decoder = { .close = ff_h263_decode_end, .decode = ff_h263_decode_frame, .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .max_lowres = 3, .pix_fmts = (const enum AVPixelFormat[]) { AV_PIX_FMT_YUV420P, @@ -924,7 +924,7 @@ AVCodec ff_msmpeg4v3_decoder = { .close = ff_h263_decode_end, .decode = ff_h263_decode_frame, .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .max_lowres = 3, .pix_fmts = (const enum AVPixelFormat[]) { AV_PIX_FMT_YUV420P, @@ -942,7 +942,7 @@ AVCodec ff_wmv1_decoder = { .close = ff_h263_decode_end, .decode = ff_h263_decode_frame, .capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1, - .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP, + .caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM, .max_lowres = 3, .pix_fmts = (const enum AVPixelFormat[]) { AV_PIX_FMT_YUV420P, diff --git a/libavcodec/rv10.c b/libavcodec/rv10.c index d118515c3b..dec7bbea58 100644 --- a/libavcodec/rv10.c +++ b/libavcodec/rv10.c @@ -688,7 +688,6 @@ AVCodec ff_rv10_decoder = { .close = rv10_decode_end, .decode = rv10_decode_frame, .capabilities = AV_CODEC_CAP_DR1, - .caps_internal = FF_CODEC_CAP_INIT_CLEANUP, .max_lowres = 3, .pix_fmts = (const enum AVPixelFormat[]) { AV_PIX_FMT_YUV420P, @@ -706,7 +705,6 @@ AVCodec ff_rv20_decoder = { .close = rv10_decode_end, .decode = rv10_decode_frame, .capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY, - .caps_internal = FF_CODEC_CAP_INIT_CLEANUP, .flush = ff_mpeg_flush, .max_lowres = 3, .pix_fmts = (const enum AVPixelFormat[]) {