On Sat, Jul 04, 2020 at 02:12:01PM +0200, Nicolas George wrote:
> gautamr...@gmail.com (12020-07-03):
> > From: Gautam Ramakrishnan <gautamr...@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
signature.asc
Description: PGP signature
_______________________________________________ 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".
