[FFmpeg-devel] [PATCH v3 1/4] avcodec/webp: compatibilize with avformat/webpdec

[Martin Reboredo]

Andreas Rheinhardt:
> Martin Reboredo:
> > Andreas Rheinhardt:
> >> Martin Reboredo:
> >>> The demuxer implementation splits some RIFF chunks (`RIFF`/`VP8X`/`ANMF` + frame chunk) or sends the picture chunks separately.
> >>> The internal WebP decoder waits for a complete file instead and by consequence it needs to be modified to support this kind of fractioned input.
> >>>
> >>> Fixes FATE tests with WebP.
> 
> If this fixes fate tests, then you need to update the fate reference
> files in the patch that changes fate output.
> 

In this case the fix was meant for passing FATE not fixing it by itself, I will clarify in the next update.

> >>>
> >>> Signed-off-by: Martin Reboredo <yakoyoku at gmail.com>
> >>> ---
> >>>  libavcodec/webp.c | 41 ++++++++++++++++++++++++++++++++---------
> >>>  1 file changed, 32 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/libavcodec/webp.c b/libavcodec/webp.c
> >>> index 3efd4438d9..7858d69481 100644
> >>> --- a/libavcodec/webp.c
> >>> +++ b/libavcodec/webp.c
> >>> @@ -40,6 +40,7 @@
> >>>   *   - XMP metadata
> >>>   */
> >>>  
> >>> +#include "libavformat/internal.h"
> >>>  #include "libavutil/imgutils.h"
> >>>  
> >>>  #define BITSTREAM_READER_LE
> >>> @@ -191,6 +192,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 */
> >>> +    int read_header;                    /* RIFF header has been read */
> >>>      int initialized;                    /* set once the VP8 context is initialized */
> >>>      int has_alpha;                      /* has a separate alpha chunk */
> >>>      enum AlphaCompression alpha_compression; /* compression type for alpha chunk */
> >>> @@ -1353,17 +1355,36 @@ static int webp_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
> >>>      if (bytestream2_get_bytes_left(&gb) < 12)
> >>>          return AVERROR_INVALIDDATA;
> >>>  
> >>> -    if (bytestream2_get_le32(&gb) != MKTAG('R', 'I', 'F', 'F')) {
> >>> -        av_log(avctx, AV_LOG_ERROR, "missing RIFF tag\n");
> >>> -        return AVERROR_INVALIDDATA;
> >>> -    }
> >>> -
> >>> +    chunk_type = bytestream2_get_le32(&gb);
> >>>      chunk_size = bytestream2_get_le32(&gb);
> >>> -    if (bytestream2_get_bytes_left(&gb) < chunk_size)
> >>> -        return AVERROR_INVALIDDATA;
> >>>  
> >>> -    if (bytestream2_get_le32(&gb) != MKTAG('W', 'E', 'B', 'P')) {
> >>> -        av_log(avctx, AV_LOG_ERROR, "missing WEBP tag\n");
> >>> +    for (int i = 0; !s->read_header && i < 4; i++) {
> >>> +        ff_lock_avformat();
> >>
> >> 1. Why are you locking avformat? What is this lock supposed to guard?
> >> 2. You can't lock avformat from avcodec at all: The ff_-prefix means
> >> that this function is intra-library (in this case libavformat) only. It
> >> will work with static builds, but it won't work with shared builds.
> > 
> > The locking mechanism was meant for parsing the WebP header that is not a single image i.e. sending the animated packages. For simple files a situation like
> > this is not going to happen because the entire file was in the package, but for animations the parsing turns out to be difficult while using concurrency.
> > I could do a much better implementation, at the moment I don't have any better fixes for it.
> > 
>
> This does not answer my first question at all: What is it supposed to
> guard? What data race would there be if you didn't lock at all?
> (I do not see why you need a lock at all.)

The lock purpose was meant to synchronize the WebP header parsing with the arriving packages in the case of feeding an animated image (behaviour introduced by
my demuxer implementation).

Taking a better look on this makes me think that the internal codec (avcodec/webp) does an unnecessary parsing plus demuxing step that could be realized by
libavformat itself. A much better procedure might be achieved by sending only the RIFF chunks that contain decode parameters and/or frame data to the decoder
and omit sending the RIFF header altogether as we already know that the file is a valid WebP file to begin with. This of course involves tweaking
avformat/webpenc too (alongside my patchset of course). Any thoughts on this?

> > As for the libavformat lock guard it was a _horrible_ production error from my side, I will submit the fix.
> > 
> >>> +
> >>> +        if (s->read_header) {
> >>> +            ff_unlock_avformat();
> >>> +            break;
> >>> +        }
> >>> +
> >>> +        if (chunk_type == MKTAG('R', 'I', 'F', 'F')) {
> >>> +            int left = bytestream2_get_bytes_left(&gb);
> >>> +            if (left < chunk_size && left != 22) {
> >>> +                ff_unlock_avformat();
> >>> +                return AVERROR_INVALIDDATA;