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

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

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

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