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

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

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;