[FFmpeg-devel] [PATCH v5 4/7] libavcodec/webp: add support for animated WebP decoding

[Thilo Borgmann]

Am 22.11.23 um 03:54 schrieb Cosmin Stejerean via ffmpeg-devel:
> On Nov 20, 2023, at 5:14 PM, James Almer <jamrial at gmail.com> wrote:
> 
> On 11/20/2023 4:22 PM, Thilo Borgmann via ffmpeg-devel wrote:
> +    if (*got_frame) {
> +        if (!(s->vp8x_flags & VP8X_FLAG_ANIMATION)) {
> +            // no animation, output the decoded frame
> +            av_frame_move_ref(p, s->frame);
> +            ret = ff_attach_decode_data(p);
> +            if (ret < 0)
> +                return ret;
> 
> If this frame's buffers were allocated with ff_get_buffer() (or its threaded version), then a call to ff_attach_decode_data() is not necessary.
> 
> The frame returned by avcodec_receive_frame doesn’t have private_ref set on it, and eventually this null private_ref would get propagated to the other frame objects (even if allocated with ff_get_buffer) by av_frame_copy_props, and later on this would fail the assert that frames returned by codecs with AV_CODEC_CAP_DR1 have private_ref set on them.


> However this patch is dealing with it in the wrong place, moving ff_attach_decode_data to right after avcodec_receive_frame would both make this more clear and fix the assert issues (currently there’s an unnecessary call to ff_attach_decode_data on an already allocated frame in a code path that doesn’t go through vp8_lossy_decode_frame).
> 
> I verified that with that change on top of the current patch set it would run the webp fate tests cleanly. I’m not sure if this is the best way to properly reference a frame that came from avcodec_receive_frame inside of another codec. But it does seem to work and a cursory search didn’t reveal a better API for this.

Moving ff_attach_decode_data() right after avcodec_receive_frame() solved FATE & the asserts even for --assert-level=2.

Will post a v6 according to this soon, unless there's more feedback on the buffer creation.

Thanks,
Thilo