[FFmpeg-devel] [PATCH 2/2] libavformat: add WebP demuxer

[Zlomek, Josef]

On Thu, Jul 9, 2020 at 11:44 AM Nicolas George <george at nsup.org> wrote:

> Josef Zlomek (12020-07-08):
>
> > + at item min_delay
> > +Set the minimum valid delay between frames in milliseconds.
> > +Range is 0 to 60000. Default value is 10.
> > +
> > + at item max_webp_delay
> > +Set the maximum valid delay between frames in milliseconds.
> > +Range is 0 to 16777215. Default value is 16777215 (over four hours),
> > +the maximum value allowed by the specification.
> > +
> > + at item default_delay
> > +Set the default delay between frames in milliseconds.
> > +Range is 0 to 60000. Default value is 100.
>
> Make these durations, with option type AV_OPT_TYPE_DURATION and internal
> semantic in microseconds.
>

OK.

> + at item ignore_loop
> > +WebP files can contain information to loop a certain number of times (or
> > +infinitely). If @option{ignore_loop} is set to 1, then the loop setting
> > +from the input will be ignored and looping will not occur. If set to 0,
> > +then looping will occur and will cycle the number of times according to
> > +the WebP. Default value is 1.
>
> Make it boolean.
>

OK. I have just copied the options from gif demuxer.

Why default to true?
>

The loop count in the WebP files is usually 0 = forever.
If decoding/transcoding the file, people probably do not want to loop the
file forever, exhausting the disk capacity.
Also when reading from pipe, the whole file would have to be buffered in
memory to be able to loop.
So in my opinion, it is better to ignore loop count by default. Also gif
demuxer ignores loop by default..

> +    while (1) {
> > +        chunk_type = avio_rl32(pb);
> > +        chunk_size = avio_rl32(pb);
> > +        if (chunk_size == UINT32_MAX)
> > +            return AVERROR_INVALIDDATA;
> > +        chunk_size += chunk_size & 1;
>
> chunk_size needs to be validated better than that. Otherwise,
> chunk_size-10 or such can underflow and that could be bad.
>

Here it is just a validation that chunk_size will not overflow when making
it even.
I will add validations to the cases of the switch statement below.

> +        if (avio_feof(pb))
> > +            break;
> > +
> > +        switch (chunk_type) {
> > +        case MKTAG('V', 'P', '8', 'X'):
> > +            avio_skip(pb, 4);
> > +            width  = avio_rl24(pb) + 1;
> > +            height = avio_rl24(pb) + 1;
> > +            break;
> > +        case MKTAG('V', 'P', '8', ' '):
> > +            avio_skip(pb, 6);
> > +            width  = avio_rl16(pb) & 0x3fff;
> > +            height = avio_rl16(pb) & 0x3fff;
> > +            duration += wdc->delay;
> > +            nb_frames++;
> > +            avio_skip(pb, chunk_size - 10);
> > +            break;
> > +        case MKTAG('V', 'P', '8', 'L'):
> > +            avio_skip(pb, 1);
> > +            n = avio_rl32(pb);
> > +            width  = (n & 0x3fff) + 1;          /* first 14 bits */
> > +            height = ((n >> 14) & 0x3fff) + 1;  /* next 14 bits */
> > +            duration += wdc->delay;
> > +            nb_frames++;
> > +            avio_skip(pb, chunk_size - 5);
> > +            break;
> > +        case MKTAG('A', 'N', 'M', 'F'):
> > +            avio_skip(pb, 6);
> > +            width  = avio_rl24(pb) + 1;
> > +            height = avio_rl24(pb) + 1;
> > +            wdc->delay = avio_rl24(pb);
> > +            if (wdc->delay < wdc->min_delay)
> > +                wdc->delay = wdc->default_delay;
> > +            wdc->delay = FFMIN(wdc->delay, wdc->max_delay);
> > +            duration += wdc->delay;
> > +            nb_frames++;
> > +            avio_skip(pb, chunk_size - 15);
> > +            break;
> > +        default:
> > +            avio_skip(pb, chunk_size);
> > +        }
>
[...]

> +    pkt->stream_index = 0;
> > +    pkt->duration = is_frame ? wdc->delay : 0;
>
> What is the packet, if it is not a frame?
>

It may be metadata (chunk "XMP " or "EXIF") or garbage at the end of the
file.
-- 
Josef