[FFmpeg-devel] [PATCH v1 4/4] avcodec/webp: make init_canvas_frame static

[Thilo Borgmann]

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