From patchwork Tue Feb 16 20:24:15 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anton Khirnov X-Patchwork-Id: 25660 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 1D1AB449BFC for ; Tue, 16 Feb 2021 22:25:57 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 07A62689D23; Tue, 16 Feb 2021 22:25:57 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 52BF4689C64 for ; Tue, 16 Feb 2021 22:25:49 +0200 (EET) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id C9CA2240696 for ; Tue, 16 Feb 2021 21:25:48 +0100 (CET) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id 3fo0JX0FtydV for ; Tue, 16 Feb 2021 21:25:46 +0100 (CET) Received: from libav.khirnov.net (libav.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.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail0.khirnov.net (Postfix) with ESMTPS id DEA14240684 for ; Tue, 16 Feb 2021 21:25:46 +0100 (CET) Received: by libav.khirnov.net (Postfix, from userid 1000) id C2F503A02B6; Tue, 16 Feb 2021 21:25:46 +0100 (CET) From: Anton Khirnov To: ffmpeg-devel@ffmpeg.org Date: Tue, 16 Feb 2021 21:24:15 +0100 Message-Id: <20210216202416.12116-5-anton@khirnov.net> X-Mailer: git-send-email 2.28.0 In-Reply-To: <20210216202416.12116-1-anton@khirnov.net> References: <20210216202416.12116-1-anton@khirnov.net> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 5/6] pngdec: fix and simplify apng reference handling 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" Current code is very confused and confusing. It uses two different reference frames - "previous" and "last" - when only one is really necessary. It also confuses the two, leading to incorrect output with APNG_DISPOSE_OP_PREVIOUS mode. Fixes #9017. --- libavcodec/pngdec.c | 93 ++++++++++++++++++++------------------------- 1 file changed, 42 insertions(+), 51 deletions(-) diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index d02c45f1a7..cece08ebca 100644 --- a/libavcodec/pngdec.c +++ b/libavcodec/pngdec.c @@ -54,7 +54,6 @@ typedef struct PNGDecContext { AVCodecContext *avctx; GetByteContext gb; - ThreadFrame previous_picture; ThreadFrame last_picture; ThreadFrame picture; @@ -711,13 +710,10 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, s->bpp += byte_depth; } + ff_thread_release_buffer(avctx, &s->picture); if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0) return ret; - if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { - ff_thread_release_buffer(avctx, &s->previous_picture); - if ((ret = ff_thread_get_buffer(avctx, &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0) - return ret; - } + p->pict_type = AV_PICTURE_TYPE_I; p->key_frame = 1; p->interlaced_frame = !!s->interlace_type; @@ -1020,7 +1016,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s, return AVERROR_INVALIDDATA; } - if ((sequence_number == 0 || !s->previous_picture.f->data[0]) && + if ((sequence_number == 0 || !s->last_picture.f->data[0]) && dispose_op == APNG_DISPOSE_OP_PREVIOUS) { // No previous frame to revert to for the first frame // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s, if (!buffer) return AVERROR(ENOMEM); + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); - // Do the disposal operation specified by the last frame on the frame - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); - - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; ++y) - memset(buffer + s->image_linesize * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); + // need to reset a rectangle to background: + // create a new writable copy + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { + int ret = av_frame_make_writable(s->last_picture.f); + if (ret < 0) + return ret; - memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize * s->height); - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); - } else { - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); - memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize * s->height); + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) { + memset(s->last_picture.f->data[0] + s->image_linesize * y + + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); + } } + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); + // Perform blending if (s->blend_op == APNG_BLEND_OP_SOURCE) { for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) { @@ -1448,22 +1444,17 @@ exit_loop: if (CONFIG_PNG_DECODER && avctx->codec_id != AV_CODEC_ID_APNG) handle_p_frame_png(s, p); else if (CONFIG_APNG_DECODER && - s->previous_picture.f->width == p->width && - s->previous_picture.f->height== p->height && - s->previous_picture.f->format== p->format && avctx->codec_id == AV_CODEC_ID_APNG && (ret = handle_p_frame_apng(avctx, s, p)) < 0) goto fail; } } ff_thread_report_progress(&s->picture, INT_MAX, 0); - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); return 0; fail: ff_thread_report_progress(&s->picture, INT_MAX, 0); - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); return ret; } @@ -1475,14 +1466,10 @@ static int decode_frame_png(AVCodecContext *avctx, PNGDecContext *const s = avctx->priv_data; const uint8_t *buf = avpkt->data; int buf_size = avpkt->size; - AVFrame *p; + AVFrame *p = s->picture.f; int64_t sig; int ret; - ff_thread_release_buffer(avctx, &s->last_picture); - FFSWAP(ThreadFrame, s->picture, s->last_picture); - p = s->picture.f; - bytestream2_init(&s->gb, buf, buf_size); /* check signature */ @@ -1519,6 +1506,11 @@ static int decode_frame_png(AVCodecContext *avctx, if ((ret = av_frame_ref(data, s->picture.f)) < 0) goto the_end; + if (!(avctx->active_thread_type & FF_THREAD_FRAME)) { + ff_thread_release_buffer(avctx, &s->last_picture); + FFSWAP(ThreadFrame, s->picture, s->last_picture); + } + *got_frame = 1; ret = bytestream2_tell(&s->gb); @@ -1536,11 +1528,7 @@ static int decode_frame_apng(AVCodecContext *avctx, { PNGDecContext *const s = avctx->priv_data; int ret; - AVFrame *p; - - ff_thread_release_buffer(avctx, &s->last_picture); - FFSWAP(ThreadFrame, s->picture, s->last_picture); - p = s->picture.f; + AVFrame *p = s->picture.f; if (!(s->hdr_state & PNG_IHDR)) { if (!avctx->extradata_size) @@ -1576,6 +1564,15 @@ static int decode_frame_apng(AVCodecContext *avctx, if ((ret = av_frame_ref(data, s->picture.f)) < 0) goto end; + if (!(avctx->active_thread_type & FF_THREAD_FRAME)) { + if (s->dispose_op == APNG_DISPOSE_OP_PREVIOUS) { + ff_thread_release_buffer(avctx, &s->picture); + } else if (s->dispose_op == APNG_DISPOSE_OP_NONE) { + ff_thread_release_buffer(avctx, &s->last_picture); + FFSWAP(ThreadFrame, s->picture, s->last_picture); + } + } + *got_frame = 1; ret = bytestream2_tell(&s->gb); @@ -1590,16 +1587,14 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) { PNGDecContext *psrc = src->priv_data; PNGDecContext *pdst = dst->priv_data; + ThreadFrame *src_frame = NULL; int ret; if (dst == src) return 0; - ff_thread_release_buffer(dst, &pdst->picture); - if (psrc->picture.f->data[0] && - (ret = ff_thread_ref_frame(&pdst->picture, &psrc->picture)) < 0) - return ret; if (CONFIG_APNG_DECODER && dst->codec_id == AV_CODEC_ID_APNG) { + pdst->width = psrc->width; pdst->height = psrc->height; pdst->bit_depth = psrc->bit_depth; @@ -1619,15 +1614,15 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) memcpy(pdst->palette, psrc->palette, sizeof(pdst->palette)); pdst->hdr_state |= psrc->hdr_state; + } - ff_thread_release_buffer(dst, &pdst->last_picture); - if (psrc->last_picture.f->data[0] && - (ret = ff_thread_ref_frame(&pdst->last_picture, &psrc->last_picture)) < 0) - return ret; + src_frame = psrc->dispose_op == APNG_DISPOSE_OP_NONE ? + &psrc->picture : &psrc->last_picture; - ff_thread_release_buffer(dst, &pdst->previous_picture); - if (psrc->previous_picture.f->data[0] && - (ret = ff_thread_ref_frame(&pdst->previous_picture, &psrc->previous_picture)) < 0) + ff_thread_release_buffer(dst, &pdst->last_picture); + if (src_frame && src_frame->f->data[0]) { + ret = ff_thread_ref_frame(&pdst->last_picture, src_frame); + if (ret < 0) return ret; } @@ -1642,11 +1637,9 @@ static av_cold int png_dec_init(AVCodecContext *avctx) avctx->color_range = AVCOL_RANGE_JPEG; s->avctx = avctx; - s->previous_picture.f = av_frame_alloc(); s->last_picture.f = av_frame_alloc(); s->picture.f = av_frame_alloc(); - if (!s->previous_picture.f || !s->last_picture.f || !s->picture.f) { - av_frame_free(&s->previous_picture.f); + if (!s->last_picture.f || !s->picture.f) { av_frame_free(&s->last_picture.f); av_frame_free(&s->picture.f); return AVERROR(ENOMEM); @@ -1661,8 +1654,6 @@ static av_cold int png_dec_end(AVCodecContext *avctx) { PNGDecContext *s = avctx->priv_data; - ff_thread_release_buffer(avctx, &s->previous_picture); - av_frame_free(&s->previous_picture.f); ff_thread_release_buffer(avctx, &s->last_picture); av_frame_free(&s->last_picture.f); ff_thread_release_buffer(avctx, &s->picture);