[FFmpeg-devel] [PATCH v7 1/2] libavcodec/pgxdec: Add PGX decoder

[Michael Niedermayer]

On Sat, Jul 04, 2020 at 02:12:01PM +0200, Nicolas George wrote:
> gautamramk at gmail.com (12020-07-03):
> > From: Gautam Ramakrishnan <gautamramk at gmail.com>
> > 
[...]
> > +static int pgx_get_number(AVCodecContext *avctx, GetByteContext *g, int *number) {
> > +    int ret = AVERROR_INVALIDDATA;
> > +    char digit;
> > +
> > +    *number = 0;
> > +    while (1) {
> > +        uint64_t temp;
> > +        if (!bytestream2_get_bytes_left(g))
> > +            return AVERROR_INVALIDDATA;
> > +        digit = bytestream2_get_byte(g);
> > +        if (digit == ' ' || digit == 0xA || digit == 0xD)
> > +            break;
> 

> > +        else if (digit < '0' || digit > '9')
> 
> Minor: we could have FFBETWEEN() for that.

that would make the code less readable IMHO

the same is true for the use of a macro for write_frame_*
it was IMHO more readable before the macro

now i dont want to confuse a GSOC student with such contraditionary
suggestions on a patch review so if you disagree i think we should
discuss this outside this review and tell Gautam only once we
agree what to do.


> 
> > +            return AVERROR_INVALIDDATA;
> > +
> > +        temp = (uint64_t)10 * (*number) + (digit - '0');
> > +        if (temp > INT_MAX)
> > +            return AVERROR_INVALIDDATA;
> > +        *number = temp;
> > +        ret = 0;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int pgx_decode_header(AVCodecContext *avctx, GetByteContext *g,
> > +                             int *depth, int *width, int *height,
> > +                             int *sign)
> > +{
> > +    int byte;
> > +
> > +    if (bytestream2_get_bytes_left(g) < 6) {
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    bytestream2_skip(g, 6);
> > +
> > +    // Is the component signed?
> > +    byte = bytestream2_peek_byte(g);
> > +    if (byte == '+') {
> > +        *sign = 0;
> > +        bytestream2_skip(g, 1);
> > +    } else if (byte == '-') {
> > +        *sign = 1;
> > +        bytestream2_skip(g, 1);
> 
> > +    } else if (byte == 0)
> 
> The coding style is !byte.

There are over 2000 occurances of "== 0" in the codebase and for a byte/char
i see nothing wrong with "== 0", its more a question of what the author
prefers and what looks more consistent in the specific case

for a true/false variable the if (!something) is more natural than == 0
but for something ultimately being nummeric the !byte feels a tiny bit
less natural to me

also IMHO where we test for multiple value with "==", the 0 looks
cleaner and more consistent than a !byte

But this is really nitpicky in either direction.
Gautam could have used byte == 0, 0 == byte, !byte, switch(byte) { case 0, ...
I think every of these would be perfectly fine and i would not ask for it to
be changed. Whatever the author preferres is fine with me


[...]
> > +                *(line + j) = val;                                                          \
> 
> *(line + j) is almost always better written line[j]

i agree


> 
> > +            }                                                                               \
> > +        }                                                                                   \
> > +    }                                                                                       \
> > +
> > +WRITE_FRAME(8, int8_t, byte)
> > +WRITE_FRAME(16, int16_t, be16)
> > +
> > +static int pgx_decode_frame(AVCodecContext *avctx, void *data,
> > +                            int *got_frame, AVPacket *avpkt)
> > +{
> > +    AVFrame *p    = data;
> > +    int ret;
> > +    int bpp;
> > +    int width, height, depth;
> > +    int sign = 0;
> > +    GetByteContext g;
> > +    bytestream2_init(&g, avpkt->data, avpkt->size);
> > +
> > +    if ((ret = pgx_decode_header(avctx, &g, &depth, &width, &height, &sign)) < 0)
> > +        return ret;
> > +
> > +    if ((ret = ff_set_dimensions(avctx, width, height)) < 0)
> > +        return ret;
> > +
> > +    if (depth <= 8) {
> > +        avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> > +        bpp = 8;
> > +    } else if (depth <= 16) {
> > +        avctx->pix_fmt = AV_PIX_FMT_GRAY16BE;
> > +        bpp = 16;
> > +    } else {
> > +        av_log(avctx, AV_LOG_ERROR, "Maximum depth of 16 bits supported.\n");
> > +        return AVERROR_PATCHWELCOME;
> > +    }
> 
> > +    if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3))
> 
> The multiplication can overflow.

ff_set_dimensions() should prevent this

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200704/08bd6f1b/attachment.sig>