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

[Thilo Borgmann]

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?


> And even if a nested decoder is unavoidable for some reason, it need a
> lot more justification than none.

It was nested in v9, you complained, so that was removed in v10.
It came back in v11 and is in ever since without complains from you - so silent agreement was assumed.
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?

Do you think it is reason enough? Or you still want this to use the ff_ internal API?

-Thilo


[1] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2024-February/thread.html