[FFmpeg-devel] [PATCH] libavcodec/pgx: Added pgx decoder

[Carl Eugen Hoyos]

Am Do., 25. Juni 2020 um 09:23 Uhr schrieb Gautam Ramakrishnan
<gautamramk at gmail.com>:
>
> On Thu, Jun 25, 2020 at 12:50 AM Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:

> > > +    for (i = 0; i < 6; i++) {
> > > +        if (header_start[i] != (char)bytestream2_get_byteu(&s->g)) {
> > > +            return AVERROR_INVALIDDATA;
> > > +        }
> >
> > Use memcmp() or consider to drop this check:
> > If a user forces this decoder, it should not be necessary to depend on this.
> >
> I did not understand this

My personal believe is - although some decoders are doing this - that
this check is not useful.
If you keep the check, it should be done with memcmp().

[...]

> > > diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> >
> > I believe other developers will request that you split the demuxer
> > and the decoder patch.
> >
> The only issue is the demuxer refers to the decoder. How do I do this?

If you decide to split the patch, the first patch adds the decoder,
the second the demuxer.
Please increase MINOR versions for both libraries in the appropriate
patches.

[...]

> > > +static int pgx_probe(const AVProbeData *p)
> > > +{
> > > +    const uint8_t *b = p->buf;
> > > +    int ret = (AV_RB32(b) & 0xFFFFFF00) == 0x50472000;
> >
> > if (AV_RB24(b) != ...)
> > return 0;
> >
> > > +    ret = ret && av_match_ext(p->filename, "pgx");
> > > +    if (ret)
> > > +        return AVPROBE_SCORE_EXTENSION + 1;
> >
> > You should instead check if the file internally looks like pgx,
> > checking the extension is a final possibility for things that
> > are impossible to detect (which I think is not the case here).
> >
> The previous check does that. It checks if the first 3 bytes are PG and space.

Please check more than three bytes - use memcmp() - and please
remove the usage of av_match_ext(), it should only be used in rare
cases, this is not one of them.

Carl Eugen