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 <yakoy...@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;
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
