From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id F155F45EB4 for ; Wed, 14 Jun 2023 10:37:52 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 0856868C413; Wed, 14 Jun 2023 13:37:49 +0300 (EEST) Received: from shout01.mail.de (shout01.mail.de [62.201.172.24]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id F190D68C260 for ; Wed, 14 Jun 2023 13:37:42 +0300 (EEST) Received: from postfix03.mail.de (postfix03.bt.mail.de [10.0.121.127]) by shout01.mail.de (Postfix) with ESMTP id 978A9A0643 for ; Wed, 14 Jun 2023 12:37:42 +0200 (CEST) Received: from smtp03.mail.de (smtp03.bt.mail.de [10.0.121.213]) by postfix03.mail.de (Postfix) with ESMTP id 74DFF8014D for ; Wed, 14 Jun 2023 12:37:42 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=mail.de; s=mailde202009; t=1686739062; bh=zIl2PGkSVIaLEGlZydoZ/eYQkBP07DVzGzNF4kEdXWE=; h=Subject:To:From:Message-ID:Date:From:To:CC:Subject:Reply-To; b=xbMqe+5HWYZm4GVD42nJ7cnB0X7lEysKdu8Bk3I7AZ3kb6WDj2457DH67HLvlICWg TsNbZVhy9jW2y9Yj0lvLUK/hWPFP1HssM8MOnZtnKdHXTtd68z9WvNODD4D2dDJLPl ames0HyPmkLXqZmRk4mjFaCoM+3VQg+BBqWA+ZdRz+SFa6Vew3wbQFgAihFlvnFFOl QkZtRAXuPgNDDTRwY/BXa0c1zXWMgAUl59zz32ASekbaRC810hhvzggFlCMv5D+GqN ba4s55DvD0QSYMTfe4T78hLCEzeMh6Ww2pUIFqab9fDufBwz3Ef1A3TiWegG69BxoB 2aCEY0BlsMfXw== Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by smtp03.mail.de (Postfix) with ESMTPSA id 36F5AA0263 for ; Wed, 14 Jun 2023 12:37:42 +0200 (CEST) To: ffmpeg-devel@ffmpeg.org References: <20230608142029.16564-1-thilo.borgmann@mail.de> <20230608142029.16564-5-thilo.borgmann@mail.de> From: Thilo Borgmann Message-ID: <276b8cbb-52b1-2b86-819e-747814b0dc0e@mail.de> Date: Wed, 14 Jun 2023 12:38:55 +0200 MIME-Version: 1.0 In-Reply-To: Content-Language: de-DE X-purgate: clean X-purgate: This mail is considered clean (visit http://www.eleven.de for further information) X-purgate-type: clean X-purgate-Ad: Categorized by eleven eXpurgate (R) http://www.eleven.de X-purgate: This mail is considered clean (visit http://www.eleven.de for further information) X-purgate: clean X-purgate-size: 7712 X-purgate-ID: 154282::1686739062-4E7FA647-CF3BC8D1/0/0 Subject: Re: [FFmpeg-devel] [PATCH v1 4/4] avcodec/webp: make init_canvas_frame static X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 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 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Am 14.06.23 um 11:42 schrieb Andreas Rheinhardt: > Thilo Borgmann: >> --- >> libavcodec/webp.c | 143 +++++++++++++++++++++++----------------------- >> 1 file changed, 71 insertions(+), 72 deletions(-) >> >> diff --git a/libavcodec/webp.c b/libavcodec/webp.c >> index bee43fcf19..d3e3f85dd3 100644 >> --- a/libavcodec/webp.c >> +++ b/libavcodec/webp.c >> @@ -1337,7 +1337,77 @@ static int vp8_lossy_decode_frame(AVCodecContext *avctx, AVFrame *p, >> } >> return ret; >> } >> -int init_canvas_frame(WebPContext *s, int format, int key_frame); >> + >> +static int init_canvas_frame(WebPContext *s, int format, int key_frame) >> +{ >> + AVFrame *canvas = s->canvas_frame.f; >> + int height; >> + int ret; >> + >> + // canvas is needed only for animation >> + if (!(s->vp8x_flags & VP8X_FLAG_ANIMATION)) >> + return 0; >> + >> + // avoid init for non-key frames whose format and size did not change >> + if (!key_frame && >> + canvas->data[0] && >> + canvas->format == format && >> + canvas->width == s->canvas_width && >> + canvas->height == s->canvas_height) >> + return 0; >> + >> + // canvas changes within IPPP sequences will loose thread sync >> + // because of the ThreadFrame reallocation and will wait forever >> + // so if frame-threading is used, forbid canvas changes and unlock >> + // previous frames >> + if (!key_frame && canvas->data[0]) { >> + if (s->avctx->thread_count > 1) { >> + av_log(s->avctx, AV_LOG_WARNING, "Canvas change detected. The output will be damaged. Use -threads 1 to try decoding with best effort.\n"); >> + // unlock previous frames that have sent an _await() call >> + ff_thread_report_progress(&s->canvas_frame, INT_MAX, 0); >> + return AVERROR_PATCHWELCOME; >> + } else { >> + // warn for damaged frames >> + av_log(s->avctx, AV_LOG_WARNING, "Canvas change detected. The output will be damaged.\n"); >> + } >> + } >> + >> + s->avctx->pix_fmt = format; >> + canvas->format = format; >> + canvas->width = s->canvas_width; >> + canvas->height = s->canvas_height; >> + >> + // VP8 decoder changed the width and height in AVCodecContext. >> + // Change it back to the canvas size. >> + ret = ff_set_dimensions(s->avctx, s->canvas_width, s->canvas_height); >> + if (ret < 0) >> + return ret; >> + >> + ff_thread_release_ext_buffer(s->avctx, &s->canvas_frame); >> + ret = ff_thread_get_ext_buffer(s->avctx, &s->canvas_frame, AV_GET_BUFFER_FLAG_REF); >> + if (ret < 0) >> + return ret; >> + >> + if (canvas->format == AV_PIX_FMT_ARGB) { >> + height = canvas->height; >> + memset(canvas->data[0], 0, height * canvas->linesize[0]); >> + } else /* if (canvas->format == AV_PIX_FMT_YUVA420P) */ { >> + const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(canvas->format); >> + for (int comp = 0; comp < desc->nb_components; comp++) { >> + int plane = desc->comp[comp].plane; >> + >> + if (comp == 1 || comp == 2) >> + height = AV_CEIL_RSHIFT(canvas->height, desc->log2_chroma_h); >> + else >> + height = FFALIGN(canvas->height, 1 << desc->log2_chroma_h); >> + >> + memset(canvas->data[plane], s->transparent_yuva[plane], >> + height * canvas->linesize[plane]); >> + } >> + } >> + >> + return 0; >> +} >> >> static int webp_decode_frame_common(AVCodecContext *avctx, uint8_t *data, int size, >> int *got_frame, int key_frame) >> @@ -1577,77 +1647,6 @@ exif_end: >> return size; >> } >> >> -int init_canvas_frame(WebPContext *s, int format, int key_frame) >> -{ >> - AVFrame *canvas = s->canvas_frame.f; >> - int height; >> - int ret; >> - >> - // canvas is needed only for animation >> - if (!(s->vp8x_flags & VP8X_FLAG_ANIMATION)) >> - return 0; >> - >> - // avoid init for non-key frames whose format and size did not change >> - if (!key_frame && >> - canvas->data[0] && >> - canvas->format == format && >> - canvas->width == s->canvas_width && >> - canvas->height == s->canvas_height) >> - return 0; >> - >> - // canvas changes within IPPP sequences will loose thread sync >> - // because of the ThreadFrame reallocation and will wait forever >> - // so if frame-threading is used, forbid canvas changes and unlock >> - // previous frames >> - if (!key_frame && canvas->data[0]) { >> - if (s->avctx->thread_count > 1) { >> - av_log(s->avctx, AV_LOG_WARNING, "Canvas change detected. The output will be damaged. Use -threads 1 to try decoding with best effort.\n"); >> - // unlock previous frames that have sent an _await() call >> - ff_thread_report_progress(&s->canvas_frame, INT_MAX, 0); >> - return AVERROR_PATCHWELCOME; >> - } else { >> - // warn for damaged frames >> - av_log(s->avctx, AV_LOG_WARNING, "Canvas change detected. The output will be damaged.\n"); >> - } >> - } >> - >> - s->avctx->pix_fmt = format; >> - canvas->format = format; >> - canvas->width = s->canvas_width; >> - canvas->height = s->canvas_height; >> - >> - // VP8 decoder changed the width and height in AVCodecContext. >> - // Change it back to the canvas size. >> - ret = ff_set_dimensions(s->avctx, s->canvas_width, s->canvas_height); >> - if (ret < 0) >> - return ret; >> - >> - ff_thread_release_ext_buffer(s->avctx, &s->canvas_frame); >> - ret = ff_thread_get_ext_buffer(s->avctx, &s->canvas_frame, AV_GET_BUFFER_FLAG_REF); >> - if (ret < 0) >> - return ret; >> - >> - if (canvas->format == AV_PIX_FMT_ARGB) { >> - height = canvas->height; >> - memset(canvas->data[0], 0, height * canvas->linesize[0]); >> - } else /* if (canvas->format == AV_PIX_FMT_YUVA420P) */ { >> - const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(canvas->format); >> - for (int comp = 0; comp < desc->nb_components; comp++) { >> - int plane = desc->comp[comp].plane; >> - >> - if (comp == 1 || comp == 2) >> - height = AV_CEIL_RSHIFT(canvas->height, desc->log2_chroma_h); >> - else >> - height = FFALIGN(canvas->height, 1 << desc->log2_chroma_h); >> - >> - memset(canvas->data[plane], s->transparent_yuva[plane], >> - height * canvas->linesize[plane]); >> - } >> - } >> - >> - return 0; >> -} >> - >> /* >> * Blend src1 (foreground) and src2 (background) into dest, in ARGB format. >> * width, height are the dimensions of src1 > > You add this function in the preceding patch. So why don't you do it > properly instead of fixing this up later? Done for Anton's remark in the last iteration to make the patch reviewable. webp_decode_frame() was renamed into webp_decode_frame_common() which makes patch 3/4 very unreadable as the - lines do not correspond to the + lines, making it very hard/impossible to review. Moving init_canvas_frame() out of the way into another place in the file resolves this mangling up of the patch so that the changes in webp:decode_frame_common() become readable again. I could totally stash patch 4/4 into 3/4 before pushing if you want the history less readable but cleaner. Thanks, Thilo _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".