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

[Thilo Borgmann]

On 15.11.24 19:52, Thilo Borgmann via ffmpeg-devel wrote:
> 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?

Ping.

-Thilo