[FFmpeg-devel] lavc/webp: Remove frame threading

Revealed by the patch to support animated webp, the current
frame threading implementation contains a data race.
vp8_lossy_decode_frame() calls ff_vp8_decode_frame() wich
calls ff_thread_finish_setup() to sync its internal slice threading.
The race is happens because vp8_lossy_decode_frame() has to touch
the AVCodecContext after it was passed to ff_vp8_decode_frame() and
ff_thread_finish_setup() had been called.

Therefore remove frame threading in webp and rely on slice threading
in VP8 only.
---
 libavcodec/webp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Thilo Borgmann via ffmpeg-devel:
> Revealed by the patch to support animated webp, the current
> frame threading implementation contains a data race.

No, it doesn't: The current implementation does not call
ff_thread_finish_setup() in vp8.c for webp:

    if (ffcodec(avctx->codec)->update_thread_context)
        ff_thread_finish_setup(avctx);

It seems that "the patch to support animated webp" (what patch are we
talking about?) adds an update_thread_context to the webp decoder,
thereby changing things and adding the data race.

> vp8_lossy_decode_frame() calls ff_vp8_decode_frame() wich
> calls ff_thread_finish_setup() to sync its internal slice threading.

Nonsense: ff_thread_finish_setup() is only for frame-threading.

> The race is happens because vp8_lossy_decode_frame() has to touch
> the AVCodecContext after it was passed to ff_vp8_decode_frame() and
> ff_thread_finish_setup() had been called.
> 
> Therefore remove frame threading in webp and rely on slice threading
> in VP8 only.

Also nonsense: The webp decoder does not support slice threading, so the
internal VP8 decoder won't ever use it (even though it supports it).

I am a bit confused here: On the one hand,
https://developers.google.com/speed/webp/docs/riff_container says that
it only uses VP8 key frame encoding; on the other hand, it has this
animation feature. Does this also only use VP8-intra coding (i.e. is the
non-intra part of animation just the blending of earlier frames?)? If it
does, then the webp decoder should be separated from the VP8 decoder
(i.e. it should use it according to the public API) and the sub-decoder
should only be used in single-threaded mode.

IMO removing frame-threading for ordinary WebP due to animated WebP is
unacceptable.

> ---
>  libavcodec/webp.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/webp.c b/libavcodec/webp.c
> index 54b3fde6dc..cde91aa7bb 100644
> --- a/libavcodec/webp.c
> +++ b/libavcodec/webp.c
> @@ -49,7 +49,6 @@
>  #include "decode.h"
>  #include "exif.h"
>  #include "get_bits.h"
> -#include "thread.h"
>  #include "tiff_common.h"
>  #include "vp8.h"
>  
> @@ -570,7 +569,7 @@ static int decode_entropy_coded_image(WebPContext *s, enum ImageRole role,
>      img->frame->height = h;
>  
>      if (role == IMAGE_ROLE_ARGB && !img->is_alpha_primary) {
> -        ret = ff_thread_get_buffer(s->avctx, img->frame, 0);
> +        ret = ff_get_buffer(s->avctx, img->frame, 0);
>      } else
>          ret = av_frame_get_buffer(img->frame, 1);
>      if (ret < 0)
> @@ -1564,6 +1563,6 @@ const FFCodec ff_webp_decoder = {
>      .init           = webp_decode_init,
>      FF_CODEC_DECODE_CB(webp_decode_frame),
>      .close          = webp_decode_close,
> -    .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_FRAME_THREADS,
> +    .p.capabilities = AV_CODEC_CAP_DR1,
>      .caps_internal  = FF_CODEC_CAP_ICC_PROFILES,
>  };

Am 22.10.23 um 16:30 schrieb Andreas Rheinhardt:
> Thilo Borgmann via ffmpeg-devel:
>> Revealed by the patch to support animated webp, the current
>> frame threading implementation contains a data race.
> 
> No, it doesn't: The current implementation does not call
> ff_thread_finish_setup() in vp8.c for webp:
> 
>      if (ffcodec(avctx->codec)->update_thread_context)
>          ff_thread_finish_setup(avctx);
> 
> It seems that "the patch to support animated webp" (what patch are we
> talking about?) adds an update_thread_context to the webp decoder,
> thereby changing things and adding the data race.

Oh yes that's true, no update_thread_context(), so not happening.


>> vp8_lossy_decode_frame() calls ff_vp8_decode_frame() wich
>> calls ff_thread_finish_setup() to sync its internal slice threading.
> 
> Nonsense: ff_thread_finish_setup() is only for frame-threading.

Indeed, thx. Though, even if I was too stupid to realize for what, it is called...


>> The race is happens because vp8_lossy_decode_frame() has to touch
>> the AVCodecContext after it was passed to ff_vp8_decode_frame() and
>> ff_thread_finish_setup() had been called.
>>
>> Therefore remove frame threading in webp and rely on slice threading
>> in VP8 only.
> 
> Also nonsense: The webp decoder does not support slice threading, so the
> internal VP8 decoder won't ever use it (even though it supports it).

Thx again, that's good to know.


> I am a bit confused here: On the one hand,
> https://developers.google.com/speed/webp/docs/riff_container says that
> it only uses VP8 key frame encoding; on the other hand, it has this
> animation feature. Does this also only use VP8-intra coding (i.e. is the
> non-intra part of animation just the blending of earlier frames?)?

That's how it works.

> If it
> does, then the webp decoder should be separated from the VP8 decoder
> (i.e. it should use it according to the public API) and the sub-decoder
> should only be used in single-threaded mode.

That would solve the issue.
How do I force the VP8 decoder to be single-threaded from the API?
Tried that and failed - well, you know how stupid I am...
Agreed, the internal VP8 decoding for lossless images should be gone from webp.c.


> IMO removing frame-threading for ordinary WebP due to animated WebP is
> unacceptable.

Once VP8 decoding happens single-threaded, this patch is void of course.

Thanks,
Thilo