[FFmpeg-devel] [PATCH v9 2/6] avcodec/webp: separate VP8 decoding

[Thilo Borgmann]

On 03.02.24 14:53, Andreas Rheinhardt wrote:
> Thilo Borgmann via ffmpeg-devel:
>> Am 28.01.24 um 11:29 schrieb Anton Khirnov:
>>> Quoting Thilo Borgmann via ffmpeg-devel (2024-01-25 16:39:19)
>>>> Am 25.01.24 um 11:04 schrieb Anton Khirnov:
>>>>> Quoting Thilo Borgmann via ffmpeg-devel (2023-12-31 13:30:14)
>>>>>> ---
>>>>>>     libavcodec/webp.c | 50
>>>>>> +++++++++++++++++++++++++++++++++++++++++------
>>>>>>     1 file changed, 44 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/webp.c b/libavcodec/webp.c
>>>>>> index 4fd107aa0c..58a20b73da 100644
>>>>>> --- a/libavcodec/webp.c
>>>>>> +++ b/libavcodec/webp.c
>>>>>> @@ -194,6 +194,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 */
>>>>>
>>>>> Nested codec contexts are in general highly undesirable and should be
>>>>> avoided whenever possible.
>>>>
>>>> AFAICT we do it that way in the other codecs as well (cri, ftr, imm5,
>>>> tdsc, tiff). So what do you suggest to do to avoid having it nested?
>>>
>>> Integrating the two decoders directly, as is done now.
>>>
>>> With nesting it is very tricky to handle all the corner cases properly,
>>> especially passing through all the options to the innner decoder, like
>>> direct rendering, other user callbacks, etc. It should only be done as a
>>> last resort and there should be a strong argument to do it this way.
>>
>> IIUC that was what the patch still did some some versions ago.
>> It brought us the data races in animated files, decoupling the decoder
>> solving the issue.
>>
> 
> If one keeps the codecs integrated, then one needs at the very least
> change the check for whether to call ff_thread_finish_setup() as I did:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2024-January/320490.html
> This will not be enough: E.g. changing the dimensions in VP8 code and
> then reverting that change in WebP (as has been done in the earlier
> version of your patch which made me propose that these decoders should
> be separated) will have to be avoided.

I've a version of the animated webp decoder with coupled vp8 decoder 
doing that size change and tsan is happy for me.

I had the impression ff_thread_finish_setup() blew it in the past which 
is now avoided - am I wrong? Once your patches landed I'll post v10 and 
we can check that again.

-Thilo