Message ID | 20200702191939.6667-1-gautamramk@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel,v7,1/2] libavcodec/pgxdec: Add PGX decoder | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On Fri, Jul 03, 2020 at 12:49:38AM +0530, gautamramk@gmail.com wrote: > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > This patch adds a pgx decoder. > --- > Changelog | 1 + > doc/general.texi | 2 + > libavcodec/Makefile | 1 + > libavcodec/allcodecs.c | 1 + > libavcodec/codec_desc.c | 7 ++ > libavcodec/codec_id.h | 1 + > libavcodec/pgxdec.c | 169 ++++++++++++++++++++++++++++++++++++++++ > libavcodec/version.h | 4 +- > 8 files changed, 184 insertions(+), 2 deletions(-) > create mode 100644 libavcodec/pgxdec.c will apply patchset thx [...]
Michael Niedermayer (12020-07-03):
> will apply patchset
There were still significant problems with the code.
In particular, the handling of the signed case seems suspicious.
Regards,
On Sat, Jul 4, 2020 at 4:08 AM Nicolas George <george@nsup.org> wrote: > > Michael Niedermayer (12020-07-03): > > will apply patchset > > There were still significant problems with the code. > > In particular, the handling of the signed case seems suspicious. I tried printing the values before writing them for a file which has signed values. The values seem fine.
Gautam Ramakrishnan (12020-07-04): > I tried printing the values before writing them for a file which has > signed values. The values seem fine. Yes, but the computation relies on architecture- and undocumented compiler-specific implementations details. And there were other flaws. Michael pushed too soon. Regards,
gautamramk@gmail.com (12020-07-03): > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > This patch adds a pgx decoder. Reviewing even after push, because there are problems to fix. > --- > Changelog | 1 + > doc/general.texi | 2 + > libavcodec/Makefile | 1 + > libavcodec/allcodecs.c | 1 + > libavcodec/codec_desc.c | 7 ++ > libavcodec/codec_id.h | 1 + > libavcodec/pgxdec.c | 169 ++++++++++++++++++++++++++++++++++++++++ > libavcodec/version.h | 4 +- > 8 files changed, 184 insertions(+), 2 deletions(-) > create mode 100644 libavcodec/pgxdec.c > > diff --git a/Changelog b/Changelog > index a60e7d2eb8..1bb9931c0d 100644 > --- a/Changelog > +++ b/Changelog > @@ -4,6 +4,7 @@ releases are sorted from youngest to oldest. > version <next>: > - AudioToolbox output device > - MacCaption demuxer > +- PGX decoder > > > version 4.3: > diff --git a/doc/general.texi b/doc/general.texi > index ea34b963b5..53d556c56c 100644 > --- a/doc/general.texi > +++ b/doc/general.texi > @@ -751,6 +751,8 @@ following image formats are supported: > @tab Portable GrayMap image > @item PGMYUV @tab X @tab X > @tab PGM with U and V components in YUV 4:2:0 > +@item PGX @tab @tab X > + @tab PGX file decoder > @item PIC @tab @tab X > @tab Pictor/PC Paint > @item PNG @tab X @tab X > diff --git a/libavcodec/Makefile b/libavcodec/Makefile > index 5a6ea59715..12aa43fe51 100644 > --- a/libavcodec/Makefile > +++ b/libavcodec/Makefile > @@ -536,6 +536,7 @@ OBJS-$(CONFIG_PGM_ENCODER) += pnmenc.o > OBJS-$(CONFIG_PGMYUV_DECODER) += pnmdec.o pnm.o > OBJS-$(CONFIG_PGMYUV_ENCODER) += pnmenc.o > OBJS-$(CONFIG_PGSSUB_DECODER) += pgssubdec.o > +OBJS-$(CONFIG_PGX_DECODER) += pgxdec.o > OBJS-$(CONFIG_PICTOR_DECODER) += pictordec.o cga_data.o > OBJS-$(CONFIG_PIXLET_DECODER) += pixlet.o > OBJS-$(CONFIG_PJS_DECODER) += textdec.o ass.o > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c > index fa0c08d42e..a5048290f7 100644 > --- a/libavcodec/allcodecs.c > +++ b/libavcodec/allcodecs.c > @@ -238,6 +238,7 @@ extern AVCodec ff_pgm_encoder; > extern AVCodec ff_pgm_decoder; > extern AVCodec ff_pgmyuv_encoder; > extern AVCodec ff_pgmyuv_decoder; > +extern AVCodec ff_pgx_decoder; > extern AVCodec ff_pictor_decoder; > extern AVCodec ff_pixlet_decoder; > extern AVCodec ff_png_encoder; > diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c > index 9f8847544f..67e0a3055c 100644 > --- a/libavcodec/codec_desc.c > +++ b/libavcodec/codec_desc.c > @@ -1405,6 +1405,13 @@ static const AVCodecDescriptor codec_descriptors[] = { > .long_name = NULL_IF_CONFIG_SMALL("AVS2-P2/IEEE1857.4"), > .props = AV_CODEC_PROP_LOSSY, > }, > + { > + .id = AV_CODEC_ID_PGX, > + .type = AVMEDIA_TYPE_VIDEO, > + .name = "pgx", > + .long_name = NULL_IF_CONFIG_SMALL("PGX (JPEG2000 Test Format)"), > + .props = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS, > + }, > { > .id = AV_CODEC_ID_Y41P, > .type = AVMEDIA_TYPE_VIDEO, > diff --git a/libavcodec/codec_id.h b/libavcodec/codec_id.h > index d885962c9c..896ecb0ce0 100644 > --- a/libavcodec/codec_id.h > +++ b/libavcodec/codec_id.h > @@ -241,6 +241,7 @@ enum AVCodecID { > AV_CODEC_ID_SCREENPRESSO, > AV_CODEC_ID_RSCC, > AV_CODEC_ID_AVS2, > + AV_CODEC_ID_PGX, > > AV_CODEC_ID_Y41P = 0x8000, > AV_CODEC_ID_AVRP, > diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c > new file mode 100644 > index 0000000000..ee43278250 > --- /dev/null > +++ b/libavcodec/pgxdec.c > @@ -0,0 +1,169 @@ > +/* > + * PGX image format > + * Copyright (c) 2020 Gautam Ramakrishnan > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +#include "avcodec.h" > +#include "internal.h" > +#include "bytestream.h" > +#include "libavutil/imgutils.h" > + > +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. > + 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. > + goto error; > + > + byte = bytestream2_peek_byte(g); > + if (byte == ' ') > + bytestream2_skip(g, 1); > + else if (byte == 0) > + goto error; > + > + if (pgx_get_number(avctx, g, depth)) > + goto error; > + if (pgx_get_number(avctx, g, width)) > + goto error; > + if (pgx_get_number(avctx, g, height)) > + goto error; > + > + if (bytestream2_peek_byte(g) == 0xA) > + bytestream2_skip(g, 1); > + return 0; > + > +error: > + av_log(avctx, AV_LOG_ERROR, "Error in decoding header.\n"); > + return AVERROR_INVALIDDATA; > +} > + > +#define WRITE_FRAME(D, PIXEL, suffix) \ > + static inline void write_frame_ ##D(AVPacket *avpkt, AVFrame *frame, GetByteContext *g, \ > + int width, int height, int sign, int depth) \ > + { \ > + int i, j; \ > + for (i = 0; i < height; i++) { \ > + PIXEL *line = (PIXEL*)frame->data[0] + i*frame->linesize[0]/sizeof(PIXEL); \ It would have been more elegant and more efficient to keep the incrementation of line at the end of the loop than to keep its initialization here. > + for (j = 0; j < width; j++) { \ > + int val; \ > + if (sign) \ > + val = (PIXEL)bytestream2_get_ ##suffix(g) + (1 << (depth - 1)); \ Casting unsigned to negative signed is not portable. > + else \ > + val = bytestream2_get_ ##suffix(g); \ > + val <<= (D - depth); \ > + *(line + j) = val; \ *(line + j) is almost always better written line[j] > + } \ > + } \ > + } \ > + > +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. > + return AVERROR_INVALIDDATA; > + if ((ret = ff_get_buffer(avctx, p, 0)) < 0) > + return ret; > + p->pict_type = AV_PICTURE_TYPE_I; > + p->key_frame = 1; > + avctx->bits_per_raw_sample = depth; > + if (bpp == 8) > + write_frame_8(avpkt, p, &g, width, height, sign, depth); > + else if (bpp == 16) > + write_frame_16(avpkt, p, &g, width, height, sign, depth); > + *got_frame = 1; > + return 0; > +} > + > +AVCodec ff_pgx_decoder = { > + .name = "pgx", > + .long_name = NULL_IF_CONFIG_SMALL("PGX (JPEG2000 Test Format)"), > + .type = AVMEDIA_TYPE_VIDEO, > + .id = AV_CODEC_ID_PGX, > + .priv_data_size = 0, > + .decode = pgx_decode_frame, > + .capabilities = AV_CODEC_CAP_DR1, > +}; > diff --git a/libavcodec/version.h b/libavcodec/version.h > index bfe6abd92f..482cc6d6ba 100644 > --- a/libavcodec/version.h > +++ b/libavcodec/version.h > @@ -28,8 +28,8 @@ > #include "libavutil/version.h" > > #define LIBAVCODEC_VERSION_MAJOR 58 > -#define LIBAVCODEC_VERSION_MINOR 93 > -#define LIBAVCODEC_VERSION_MICRO 104 > +#define LIBAVCODEC_VERSION_MINOR 94 > +#define LIBAVCODEC_VERSION_MICRO 100 > > #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ > LIBAVCODEC_VERSION_MINOR, \ Regards,
On Sat, Jul 4, 2020 at 5:42 PM Nicolas George <george@nsup.org> wrote: > > gautamramk@gmail.com (12020-07-03): > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > This patch adds a pgx decoder. > > Reviewing even after push, because there are problems to fix. > > > --- > > Changelog | 1 + > > doc/general.texi | 2 + > > libavcodec/Makefile | 1 + > > libavcodec/allcodecs.c | 1 + > > libavcodec/codec_desc.c | 7 ++ > > libavcodec/codec_id.h | 1 + > > libavcodec/pgxdec.c | 169 ++++++++++++++++++++++++++++++++++++++++ > > libavcodec/version.h | 4 +- > > 8 files changed, 184 insertions(+), 2 deletions(-) > > create mode 100644 libavcodec/pgxdec.c > > > > diff --git a/Changelog b/Changelog > > index a60e7d2eb8..1bb9931c0d 100644 > > --- a/Changelog > > +++ b/Changelog > > @@ -4,6 +4,7 @@ releases are sorted from youngest to oldest. > > version <next>: > > - AudioToolbox output device > > - MacCaption demuxer > > +- PGX decoder > > > > > > version 4.3: > > diff --git a/doc/general.texi b/doc/general.texi > > index ea34b963b5..53d556c56c 100644 > > --- a/doc/general.texi > > +++ b/doc/general.texi > > @@ -751,6 +751,8 @@ following image formats are supported: > > @tab Portable GrayMap image > > @item PGMYUV @tab X @tab X > > @tab PGM with U and V components in YUV 4:2:0 > > +@item PGX @tab @tab X > > + @tab PGX file decoder > > @item PIC @tab @tab X > > @tab Pictor/PC Paint > > @item PNG @tab X @tab X > > diff --git a/libavcodec/Makefile b/libavcodec/Makefile > > index 5a6ea59715..12aa43fe51 100644 > > --- a/libavcodec/Makefile > > +++ b/libavcodec/Makefile > > @@ -536,6 +536,7 @@ OBJS-$(CONFIG_PGM_ENCODER) += pnmenc.o > > OBJS-$(CONFIG_PGMYUV_DECODER) += pnmdec.o pnm.o > > OBJS-$(CONFIG_PGMYUV_ENCODER) += pnmenc.o > > OBJS-$(CONFIG_PGSSUB_DECODER) += pgssubdec.o > > +OBJS-$(CONFIG_PGX_DECODER) += pgxdec.o > > OBJS-$(CONFIG_PICTOR_DECODER) += pictordec.o cga_data.o > > OBJS-$(CONFIG_PIXLET_DECODER) += pixlet.o > > OBJS-$(CONFIG_PJS_DECODER) += textdec.o ass.o > > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c > > index fa0c08d42e..a5048290f7 100644 > > --- a/libavcodec/allcodecs.c > > +++ b/libavcodec/allcodecs.c > > @@ -238,6 +238,7 @@ extern AVCodec ff_pgm_encoder; > > extern AVCodec ff_pgm_decoder; > > extern AVCodec ff_pgmyuv_encoder; > > extern AVCodec ff_pgmyuv_decoder; > > +extern AVCodec ff_pgx_decoder; > > extern AVCodec ff_pictor_decoder; > > extern AVCodec ff_pixlet_decoder; > > extern AVCodec ff_png_encoder; > > diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c > > index 9f8847544f..67e0a3055c 100644 > > --- a/libavcodec/codec_desc.c > > +++ b/libavcodec/codec_desc.c > > @@ -1405,6 +1405,13 @@ static const AVCodecDescriptor codec_descriptors[] = { > > .long_name = NULL_IF_CONFIG_SMALL("AVS2-P2/IEEE1857.4"), > > .props = AV_CODEC_PROP_LOSSY, > > }, > > + { > > + .id = AV_CODEC_ID_PGX, > > + .type = AVMEDIA_TYPE_VIDEO, > > + .name = "pgx", > > + .long_name = NULL_IF_CONFIG_SMALL("PGX (JPEG2000 Test Format)"), > > + .props = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS, > > + }, > > { > > .id = AV_CODEC_ID_Y41P, > > .type = AVMEDIA_TYPE_VIDEO, > > diff --git a/libavcodec/codec_id.h b/libavcodec/codec_id.h > > index d885962c9c..896ecb0ce0 100644 > > --- a/libavcodec/codec_id.h > > +++ b/libavcodec/codec_id.h > > @@ -241,6 +241,7 @@ enum AVCodecID { > > AV_CODEC_ID_SCREENPRESSO, > > AV_CODEC_ID_RSCC, > > AV_CODEC_ID_AVS2, > > + AV_CODEC_ID_PGX, > > > > AV_CODEC_ID_Y41P = 0x8000, > > AV_CODEC_ID_AVRP, > > diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c > > new file mode 100644 > > index 0000000000..ee43278250 > > --- /dev/null > > +++ b/libavcodec/pgxdec.c > > @@ -0,0 +1,169 @@ > > +/* > > + * PGX image format > > + * Copyright (c) 2020 Gautam Ramakrishnan > > + * > > + * This file is part of FFmpeg. > > + * > > + * FFmpeg is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later version. > > + * > > + * FFmpeg is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with FFmpeg; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > > + */ > > + > > +#include "avcodec.h" > > +#include "internal.h" > > +#include "bytestream.h" > > +#include "libavutil/imgutils.h" > > + > > +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. I shall use this. > > > + 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. Missed this out, shall change > > > + goto error; > > + > > + byte = bytestream2_peek_byte(g); > > + if (byte == ' ') > > + bytestream2_skip(g, 1); > > + else if (byte == 0) > > + goto error; > > + > > + if (pgx_get_number(avctx, g, depth)) > > + goto error; > > + if (pgx_get_number(avctx, g, width)) > > + goto error; > > + if (pgx_get_number(avctx, g, height)) > > + goto error; > > + > > + if (bytestream2_peek_byte(g) == 0xA) > > + bytestream2_skip(g, 1); > > + return 0; > > + > > +error: > > + av_log(avctx, AV_LOG_ERROR, "Error in decoding header.\n"); > > + return AVERROR_INVALIDDATA; > > +} > > + > > +#define WRITE_FRAME(D, PIXEL, suffix) \ > > + static inline void write_frame_ ##D(AVPacket *avpkt, AVFrame *frame, GetByteContext *g, \ > > + int width, int height, int sign, int depth) \ > > + { \ > > + int i, j; \ > > + for (i = 0; i < height; i++) { \ > > > + PIXEL *line = (PIXEL*)frame->data[0] + i*frame->linesize[0]/sizeof(PIXEL); \ > > It would have been more elegant and more efficient to keep the > incrementation of line at the end of the loop than to keep its > initialization here. If I got you right, you mean initialize 'line' outside the first for loop. Keep incrementing after every line is traversed. This is similar to the one in jpeg2000dec.c. Shall do that if that is what you meant. > > > + for (j = 0; j < width; j++) { \ > > + int val; \ > > + if (sign) \ > > > + val = (PIXEL)bytestream2_get_ ##suffix(g) + (1 << (depth - 1)); \ > > Casting unsigned to negative signed is not portable. Should manually take the 2's complement instead? Could write an inline function for that. > > > + else \ > > + val = bytestream2_get_ ##suffix(g); \ > > + val <<= (D - depth); \ > > > + *(line + j) = val; \ > > *(line + j) is almost always better written line[j] Shall change. > > > + } \ > > + } \ > > + } \ > > + > > +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. width and height could both be 32 bit. Typecasting to 64 bit might also overflow in that case. Any other alternatives? > > > + return AVERROR_INVALIDDATA; > > + if ((ret = ff_get_buffer(avctx, p, 0)) < 0) > > + return ret; > > + p->pict_type = AV_PICTURE_TYPE_I; > > + p->key_frame = 1; > > + avctx->bits_per_raw_sample = depth; > > + if (bpp == 8) > > + write_frame_8(avpkt, p, &g, width, height, sign, depth); > > + else if (bpp == 16) > > + write_frame_16(avpkt, p, &g, width, height, sign, depth); > > + *got_frame = 1; > > + return 0; > > +} > > + > > +AVCodec ff_pgx_decoder = { > > + .name = "pgx", > > + .long_name = NULL_IF_CONFIG_SMALL("PGX (JPEG2000 Test Format)"), > > + .type = AVMEDIA_TYPE_VIDEO, > > + .id = AV_CODEC_ID_PGX, > > + .priv_data_size = 0, > > + .decode = pgx_decode_frame, > > + .capabilities = AV_CODEC_CAP_DR1, > > +}; > > diff --git a/libavcodec/version.h b/libavcodec/version.h > > index bfe6abd92f..482cc6d6ba 100644 > > --- a/libavcodec/version.h > > +++ b/libavcodec/version.h > > @@ -28,8 +28,8 @@ > > #include "libavutil/version.h" > > > > #define LIBAVCODEC_VERSION_MAJOR 58 > > -#define LIBAVCODEC_VERSION_MINOR 93 > > -#define LIBAVCODEC_VERSION_MICRO 104 > > +#define LIBAVCODEC_VERSION_MINOR 94 > > +#define LIBAVCODEC_VERSION_MICRO 100 > > > > #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ > > LIBAVCODEC_VERSION_MINOR, \ > > Regards, > > -- > Nicolas George
Nicolas George: > gautamramk@gmail.com (12020-07-03): >> From: Gautam Ramakrishnan <gautamramk@gmail.com> >> >> This patch adds a pgx decoder. > > Reviewing even after push, because there are problems to fix. > >> --- >> Changelog | 1 + >> doc/general.texi | 2 + >> libavcodec/Makefile | 1 + >> libavcodec/allcodecs.c | 1 + >> libavcodec/codec_desc.c | 7 ++ >> libavcodec/codec_id.h | 1 + >> libavcodec/pgxdec.c | 169 ++++++++++++++++++++++++++++++++++++++++ >> libavcodec/version.h | 4 +- >> 8 files changed, 184 insertions(+), 2 deletions(-) >> create mode 100644 libavcodec/pgxdec.c >> [...] >> +#define WRITE_FRAME(D, PIXEL, suffix) \ >> + static inline void write_frame_ ##D(AVPacket *avpkt, AVFrame *frame, GetByteContext *g, \ >> + int width, int height, int sign, int depth) \ >> + { \ >> + int i, j; \ >> + for (i = 0; i < height; i++) { \ > >> + PIXEL *line = (PIXEL*)frame->data[0] + i*frame->linesize[0]/sizeof(PIXEL); \ > > It would have been more elegant and more efficient to keep the > incrementation of line at the end of the loop than to keep its > initialization here. > >> + for (j = 0; j < width; j++) { \ >> + int val; \ >> + if (sign) \ > >> + val = (PIXEL)bytestream2_get_ ##suffix(g) + (1 << (depth - 1)); \ > > Casting unsigned to negative signed is not portable. > It's not completely portable, but FFmpeg does not aim for complete portability: "Implementation defined behavior for signed integers is assumed to match the expected behavior for two’s complement". (see https://ffmpeg.org/developer.html#C-language-features) >> + else \ >> + val = bytestream2_get_ ##suffix(g); \ >> + val <<= (D - depth); \ > >> + *(line + j) = val; \ > > *(line + j) is almost always better written line[j] > >> + } \ >> + } \ >> + } \ >> + >> +WRITE_FRAME(8, int8_t, byte) >> +WRITE_FRAME(16, int16_t, be16) >> + - Andreas
Gautam Ramakrishnan (12020-07-04): > > Minor: we could have FFBETWEEN() for that. > I shall use this. You would need to add it first, which is ok by me. Note that (x >= a) && (x < b) can be written in a more efficient way: (unsigned)x - a < (unsigned)b - a > If I got you right, you mean initialize 'line' outside the first for loop. > Keep incrementing after every line is traversed. This is similar to the > one in jpeg2000dec.c. Shall do that if that is what you meant. Yes, that is exactly what I mean. > Should manually take the 2's complement instead? Could write > an inline function for that. I think we cannot avoid doing this. > > The multiplication can overflow. > width and height could both be 32 bit. Typecasting to 64 bit might > also overflow in that > case. Any other alternatives? We must check the operands before multiplying. Something like this: if (height > INT_MAX / width / bpp) Regards,
On Sat, Jul 04, 2020 at 02:12:01PM +0200, Nicolas George wrote: > gautamramk@gmail.com (12020-07-03): > > From: Gautam Ramakrishnan <gautamramk@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 [...]
On Sat, Jul 04, 2020 at 03:32:31PM +0200, Nicolas George wrote: > Gautam Ramakrishnan (12020-07-04): > > > Minor: we could have FFBETWEEN() for that. > > I shall use this. > > You would need to add it first, which is ok by me. > > Note that (x >= a) && (x < b) can be written in a more efficient way: > > (unsigned)x - a < (unsigned)b - a I dont think this FFBETWEEN macro is a great idea if someone used it with int64_t or with a float this would all not work and its easy to do that the name FFBETWEEN doesnt suggest that its int specific thx [...]
Michael Niedermayer (12020-07-04): > that would make the code less readable IMHO In this particular very simple case maybe. > the same is true for the use of a macro for write_frame_* > it was IMHO more readable before the macro Yes, each copy was slightly more readable than the merged macro. But now, bugs and enhancements have to be done only once. I say: small cost, bigger benefit. > 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. I do not think that calm discussion cause the risk to confuse, and the student deserves a say in this. > 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 I agree with this. > > > + if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3)) > > The multiplication can overflow. > ff_set_dimensions() should prevent this No, it should not, because it is not aware of the specifics. The default value for max_pixels is INT_MAX, but it is only a default value, applications or users can set it to a higher value. More importantly, ff_set_dimensions() does not know about bpp >> 3, doesn't know that the whole frame needs to fit in the size of AVPacket. Regards,
On Sat, Jul 04, 2020 at 06:45:02PM +0200, Nicolas George wrote: > Michael Niedermayer (12020-07-04): [...] > > > > + if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3)) > > > The multiplication can overflow. > > ff_set_dimensions() should prevent this > > No, it should not, because it is not aware of the specifics. The default > value for max_pixels is INT_MAX, but it is only a default value, > applications or users can set it to a higher value. More importantly, > ff_set_dimensions() does not know about bpp >> 3, doesn't know that the > whole frame needs to fit in the size of AVPacket. Currently av_image_check_size2() checks if ((int)w<=0 || (int)h<=0 || stride >= INT_MAX || stride*(uint64_t)(h+128) >= INT_MAX) { That uses INT_MAX and stride tries to include the maximum pixel size av_image_check_size2() could be used directly and the actual pixel format could be passed to it but it is still redundant and we dont do this in many other places. Also it wouldnt make a difference as ff_set_dimensions uses AV_PIX_FMT_NONE and would fail with resolutions such a av_image_check_size2() would succeeed If the goal would be to make ff_set_dimensions() work with cases beyond INT_MAX addressible then we should extend ff_set_dimensions() itself because requiring a seperate function to check this is dangerous as it can be forgotten. Also increasing the limit for all uses of ff_set_dimensions() is in itself also dangerous Its most robust is if the function setting the dimensions does the check already we could add a internal codec cap which disables the INT_MAX or a ff_set_dimensions2() all this is a little outside the scope of this patch review though there also may have been patches changing the INT_MAX limit, dont remember exactly thx [...]
diff --git a/Changelog b/Changelog index a60e7d2eb8..1bb9931c0d 100644 --- a/Changelog +++ b/Changelog @@ -4,6 +4,7 @@ releases are sorted from youngest to oldest. version <next>: - AudioToolbox output device - MacCaption demuxer +- PGX decoder version 4.3: diff --git a/doc/general.texi b/doc/general.texi index ea34b963b5..53d556c56c 100644 --- a/doc/general.texi +++ b/doc/general.texi @@ -751,6 +751,8 @@ following image formats are supported: @tab Portable GrayMap image @item PGMYUV @tab X @tab X @tab PGM with U and V components in YUV 4:2:0 +@item PGX @tab @tab X + @tab PGX file decoder @item PIC @tab @tab X @tab Pictor/PC Paint @item PNG @tab X @tab X diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 5a6ea59715..12aa43fe51 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -536,6 +536,7 @@ OBJS-$(CONFIG_PGM_ENCODER) += pnmenc.o OBJS-$(CONFIG_PGMYUV_DECODER) += pnmdec.o pnm.o OBJS-$(CONFIG_PGMYUV_ENCODER) += pnmenc.o OBJS-$(CONFIG_PGSSUB_DECODER) += pgssubdec.o +OBJS-$(CONFIG_PGX_DECODER) += pgxdec.o OBJS-$(CONFIG_PICTOR_DECODER) += pictordec.o cga_data.o OBJS-$(CONFIG_PIXLET_DECODER) += pixlet.o OBJS-$(CONFIG_PJS_DECODER) += textdec.o ass.o diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c index fa0c08d42e..a5048290f7 100644 --- a/libavcodec/allcodecs.c +++ b/libavcodec/allcodecs.c @@ -238,6 +238,7 @@ extern AVCodec ff_pgm_encoder; extern AVCodec ff_pgm_decoder; extern AVCodec ff_pgmyuv_encoder; extern AVCodec ff_pgmyuv_decoder; +extern AVCodec ff_pgx_decoder; extern AVCodec ff_pictor_decoder; extern AVCodec ff_pixlet_decoder; extern AVCodec ff_png_encoder; diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c index 9f8847544f..67e0a3055c 100644 --- a/libavcodec/codec_desc.c +++ b/libavcodec/codec_desc.c @@ -1405,6 +1405,13 @@ static const AVCodecDescriptor codec_descriptors[] = { .long_name = NULL_IF_CONFIG_SMALL("AVS2-P2/IEEE1857.4"), .props = AV_CODEC_PROP_LOSSY, }, + { + .id = AV_CODEC_ID_PGX, + .type = AVMEDIA_TYPE_VIDEO, + .name = "pgx", + .long_name = NULL_IF_CONFIG_SMALL("PGX (JPEG2000 Test Format)"), + .props = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS, + }, { .id = AV_CODEC_ID_Y41P, .type = AVMEDIA_TYPE_VIDEO, diff --git a/libavcodec/codec_id.h b/libavcodec/codec_id.h index d885962c9c..896ecb0ce0 100644 --- a/libavcodec/codec_id.h +++ b/libavcodec/codec_id.h @@ -241,6 +241,7 @@ enum AVCodecID { AV_CODEC_ID_SCREENPRESSO, AV_CODEC_ID_RSCC, AV_CODEC_ID_AVS2, + AV_CODEC_ID_PGX, AV_CODEC_ID_Y41P = 0x8000, AV_CODEC_ID_AVRP, diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c new file mode 100644 index 0000000000..ee43278250 --- /dev/null +++ b/libavcodec/pgxdec.c @@ -0,0 +1,169 @@ +/* + * PGX image format + * Copyright (c) 2020 Gautam Ramakrishnan + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include "avcodec.h" +#include "internal.h" +#include "bytestream.h" +#include "libavutil/imgutils.h" + +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') + 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) + goto error; + + byte = bytestream2_peek_byte(g); + if (byte == ' ') + bytestream2_skip(g, 1); + else if (byte == 0) + goto error; + + if (pgx_get_number(avctx, g, depth)) + goto error; + if (pgx_get_number(avctx, g, width)) + goto error; + if (pgx_get_number(avctx, g, height)) + goto error; + + if (bytestream2_peek_byte(g) == 0xA) + bytestream2_skip(g, 1); + return 0; + +error: + av_log(avctx, AV_LOG_ERROR, "Error in decoding header.\n"); + return AVERROR_INVALIDDATA; +} + +#define WRITE_FRAME(D, PIXEL, suffix) \ + static inline void write_frame_ ##D(AVPacket *avpkt, AVFrame *frame, GetByteContext *g, \ + int width, int height, int sign, int depth) \ + { \ + int i, j; \ + for (i = 0; i < height; i++) { \ + PIXEL *line = (PIXEL*)frame->data[0] + i*frame->linesize[0]/sizeof(PIXEL); \ + for (j = 0; j < width; j++) { \ + int val; \ + if (sign) \ + val = (PIXEL)bytestream2_get_ ##suffix(g) + (1 << (depth - 1)); \ + else \ + val = bytestream2_get_ ##suffix(g); \ + val <<= (D - depth); \ + *(line + j) = val; \ + } \ + } \ + } \ + +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)) + return AVERROR_INVALIDDATA; + if ((ret = ff_get_buffer(avctx, p, 0)) < 0) + return ret; + p->pict_type = AV_PICTURE_TYPE_I; + p->key_frame = 1; + avctx->bits_per_raw_sample = depth; + if (bpp == 8) + write_frame_8(avpkt, p, &g, width, height, sign, depth); + else if (bpp == 16) + write_frame_16(avpkt, p, &g, width, height, sign, depth); + *got_frame = 1; + return 0; +} + +AVCodec ff_pgx_decoder = { + .name = "pgx", + .long_name = NULL_IF_CONFIG_SMALL("PGX (JPEG2000 Test Format)"), + .type = AVMEDIA_TYPE_VIDEO, + .id = AV_CODEC_ID_PGX, + .priv_data_size = 0, + .decode = pgx_decode_frame, + .capabilities = AV_CODEC_CAP_DR1, +}; diff --git a/libavcodec/version.h b/libavcodec/version.h index bfe6abd92f..482cc6d6ba 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -28,8 +28,8 @@ #include "libavutil/version.h" #define LIBAVCODEC_VERSION_MAJOR 58 -#define LIBAVCODEC_VERSION_MINOR 93 -#define LIBAVCODEC_VERSION_MICRO 104 +#define LIBAVCODEC_VERSION_MINOR 94 +#define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ LIBAVCODEC_VERSION_MINOR, \
From: Gautam Ramakrishnan <gautamramk@gmail.com> This patch adds a pgx decoder. --- Changelog | 1 + doc/general.texi | 2 + libavcodec/Makefile | 1 + libavcodec/allcodecs.c | 1 + libavcodec/codec_desc.c | 7 ++ libavcodec/codec_id.h | 1 + libavcodec/pgxdec.c | 169 ++++++++++++++++++++++++++++++++++++++++ libavcodec/version.h | 4 +- 8 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 libavcodec/pgxdec.c