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

[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.

> 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,
>  };