[FFmpeg-devel] [PATCH v13 2/8] avcodec/webp: separate VP8 decoding

[Anton Khirnov]

Quoting Thilo Borgmann via ffmpeg-devel (2024-11-15 19:52:05)
> Am 21.06.24 um 13:52 schrieb Anton Khirnov:
> > Quoting Thilo Borgmann via ffmpeg-devel (2024-06-21 12:43:17)
> >> From: Thilo Borgmann via ffmpeg-devel <ffmpeg-devel at ffmpeg.org>
> >>
> >> ---
> >>   libavcodec/webp.c | 50 +++++++++++++++++++++++++++++++++++++++++------
> >>   1 file changed, 44 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/libavcodec/webp.c b/libavcodec/webp.c
> >> index af4ebcec27..c52b9732b4 100644
> >> --- a/libavcodec/webp.c
> >> +++ b/libavcodec/webp.c
> >> @@ -195,6 +195,7 @@ typedef struct WebPContext {
> >>       AVFrame *alpha_frame;               /* AVFrame for alpha data decompressed from VP8L */
> >>       AVPacket *pkt;                      /* AVPacket to be passed to the underlying VP8 decoder */
> >>       AVCodecContext *avctx;              /* parent AVCodecContext */
> >> +    AVCodecContext *avctx_vp8;          /* wrapper context for VP8 decoder */
> > 
> > As I said before, nested decoders should be avoided whenever possible,
> > because properly forwarding everything between the caller and the nested
> > decoder is very tricky and almost never fully correct. For example, this
> > patch seems to break direct rendering.
> 
> How could I test that?

With ffmpeg CLI, the get_buffer() function in ffmpeg_dec.c should get
called from the decoder.

> However, the justification for adding it back was in the v10 discussion, yet probably it did not hit your and others mailboxes - as I find the corresponding mail in my mailbox (circulated via the ML, not a local copy) yet it does not appear in the archives [1].
> Maybe mails are lost without noticing longer than we thought...
> 
> The argumentation was as quoted:
> 
> >> [from Andreas] 
> >> 1. Up until now, when decoding a series of stand-alone WebP pictures,
> >> the multiple decoder instances don't wait for each other (because the
> >> WebP decoder had no update_thread_context callback). You added such a
> >> callback and now you are calling it after the main picture has already
> >> been decoded, effectively serializing everything. You can test this for
> >> yourself: Create lots of files via ffmpeg -i <input> -c:v libwebp -f
> >> webp%d.webp and decode them (don't use -stream_loop on a single input
> >> picture, as this will flush the decoder after every single picture, so
> >> that everything is always serialized).
> >> 2. To fix this, ff_thread_finish_setup() needs to be called as soon as
> >> possible. This means that you have to abandon the approach of letting
> >> the inner VP8 decoder set the frame dimensions and then overwriting them
> >> again in the WebP decoder.
> > 
> > so I tried to move the ff_thread_finish_setup() call and avoid updating the AVCodecContext after decoding.
> > 
> > With the integrated decoder like here, ff_vp8_decode_frame() writes into the AVCodecContext while decoding, so ff_thread_finish_setup() can only be called afterwards -> quasi sequentially. I do see decoding speed for many files at once dropping from 100x to 16x compared to master.
> > 
> > With the decoder decoupled, I can actually move ff_thread_finish_setup() before send_packet() (in contrast to v9) -> next thread can start before decoding the frame.
> > I don't see a penalty with that way on decoding many files at once compared to master, as expected.
> > 
> > Is that reason enough to actually decouple the vp8 decoder?

No, seems to me the right way to do this is split ff_vp8_decode_frame()
into header parsing and actual decoding, then the webp decoder can call
ff_thread_finish_setup() at the right place.

-- 
Anton Khirnov