[FFmpeg-devel] [PATCH v7 1/2] libavcodec/pgxdec: Add PGX decoder
Gautam Ramakrishnan
gautamramk at gmail.com
Sat Jul 4 15:45:54 EEST 2020
On Sat, Jul 4, 2020 at 5:42 PM Nicolas George <george at nsup.org> wrote:
>
> gautamramk at gmail.com (12020-07-03):
> > From: Gautam Ramakrishnan <gautamramk at 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
> > + at 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
--
-------------
Gautam |
More information about the ffmpeg-devel
mailing list