diff mbox series

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

Message ID 20200627070518.8175-1-gautamramk@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,1/2] libavcodec/pgxdec: Add PGX decoder | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Gautam Ramakrishnan June 27, 2020, 7:05 a.m. UTC 
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     | 184 ++++++++++++++++++++++++++++++++++++++++
 libavcodec/version.h    |   2 +-
 8 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 libavcodec/pgxdec.c

Comments

Michael Niedermayer June 27, 2020, 2:24 p.m. UTC | #1 
On Sat, Jun 27, 2020 at 12:35:17PM +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     | 184 ++++++++++++++++++++++++++++++++++++++++
>  libavcodec/version.h    |   2 +-
>  8 files changed, 198 insertions(+), 1 deletion(-)
>  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..df5f5028cd
> --- /dev/null
> +++ b/libavcodec/pgxdec.c
> @@ -0,0 +1,184 @@
> +/*
> + * 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"
> +
> +typedef struct PGXContext {
> +    GetByteContext  g;
> +    int depth;
> +    int sign;
> +    int width;
> +    int height;
> +} PGXContext;
> +
> +static int pgx_get_number(AVCodecContext *avctx, PGXContext * const s) {
> +    int ret = -1;
> +    char digit;
> +
> +    while (1) {
> +        if (!bytestream2_get_bytes_left(&s->g))
> +            return AVERROR_INVALIDDATA;
> +        digit = bytestream2_get_byte(&s->g);
> +        if (digit == ' ' || digit == 0xA || digit == 0xD) {
> +            break;
> +        } else if (digit < '0' || digit > '9') {
> +            return AVERROR_INVALIDDATA;
> +        }
> +        if (ret < 0)
> +            ret = 0;

> +        ret = 10 * ret + (digit - '0');

this could overflow


> +    }
> +
> +    if (ret >= 0)
> +        return ret;
> +    return AVERROR_INVALIDDATA;
> +}
> +
> +static int ff_pgx_decode_header(AVCodecContext *avctx, PGXContext * s)
> +{
> +    const char *header_start = "PG ML ";
> +    int byte;
> +
> +    if (bytestream2_get_bytes_left(&s->g) < 13) {
> +        return AVERROR_INVALIDDATA;
> +    }
> +    // Checks whether file starts with "PG ML "
> +    if (memcmp(header_start, s->g.buffer, 6))
> +        return AVERROR_INVALIDDATA;
> +    bytestream2_skip(&s->g, 6);
> +
> +    // Is the component signed?
> +    byte = bytestream2_peek_byte(&s->g);
> +    if (byte == '+') {
> +        s->sign = 0;
> +        bytestream2_skip(&s->g, 1);
> +    } else if (byte == '-') {
> +        s->sign = 1;
> +        bytestream2_skip(&s->g, 1);
> +    }
> +
> +    if (bytestream2_peek_byte(&s->g) == ' ')
> +        bytestream2_skip(&s->g, 1);
> +    s->depth = pgx_get_number(avctx, s);
> +    s->width = pgx_get_number(avctx, s);
> +    s->height = pgx_get_number(avctx, s);
> +    if (bytestream2_peek_byte(&s->g) == 0xA)
> +        bytestream2_skip(&s->g, 1);

pgx_get_number can return errors
the code using it does not check for them



> +    return 0;
> +}
> +
> +static inline int write_frame_8(AVPacket *avpkt, AVFrame *frame, PGXContext * const s)
> +{
> +    int i, j;
> +    for (i = 0; i < s->height; i++) {
> +        uint8_t *line = frame->data[0] + i * frame->linesize[0];
> +        for (j = 0; j < s->width; j++) {
> +            int val;
> +            if (bytestream2_get_bytes_left(&s->g) < 1)
> +                return AVERROR_INVALIDDATA;
> +            if (s->sign) {
> +                val = bytestream2_get_byte(&s->g) + (1 << (s->depth - 1));
> +                val <<= (8 - s->depth);
> +                *(line + j) = val;
> +            } else {
> +                val = bytestream2_get_byteu(&s->g);
> +                val <<= (8 - s->depth);
> +                *(line + j) = val;
> +            }
> +        }
> +        line += frame->linesize[0];
> +    }
> +    return 0;
> +}
> +
> +static inline int write_frame_16(AVPacket *avpkt, AVFrame *frame, PGXContext * const s)
> +{
> +    int i, j;
> +    for (i = 0; i < s->height; i++) {
> +        uint16_t *line = (uint16_t*)frame->data[0] + i*frame->linesize[0]/2;
> +        for (j = 0; j < s->width; j++) {
> +            int val;
> +            if (bytestream2_get_bytes_left(&s->g) < 2)
> +                return AVERROR_INVALIDDATA;
> +            if (s->sign) {
> +                val = (int16_t)bytestream2_get_be16(&s->g) + (1 << (s->depth - 1));
> +                val <<= (16 - s->depth);
> +                *(line + j) = val;
> +            } else {
> +                val = bytestream2_get_be16u(&s->g);
> +                val <<= (16 - s->depth);
> +                *(line + j) = val;
> +            }
> +        }
> +        line += frame->linesize[0]/2;
> +    }
> +    return 0;
> +}
> +

> +static int pgx_decode_frame(AVCodecContext *avctx, void *data,
> +                            int *got_frame, AVPacket *avpkt)
> +{
> +    PGXContext *s = avctx->priv_data;
> +    AVFrame *p    = data;
> +    int ret;
> +    int bpp;
> +    bytestream2_init(&s->g, avpkt->data, avpkt->size);
> +    if ((ret = ff_pgx_decode_header(avctx, s)) < 0)
> +        return ret;
> +
> +    if (ret = ff_set_dimensions(avctx, s->width, s->height) < 0)
> +        return ret;
> +
> +    if (s->depth <= 8) {
> +        avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> +        bpp = 8;

depth = 0 is valid ?, it would end in here

thx

[...]
Nicolas George June 27, 2020, 2:28 p.m. UTC | #2 
Michael Niedermayer (12020-06-27):
> this could overflow

The original code had a test to limit the size of numbers to 9 digits,
guaranteeing no overflow. I wonder where it went.

Regards,
Gautam Ramakrishnan June 27, 2020, 3:36 p.m. UTC | #3 
On Sat, Jun 27, 2020 at 7:55 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sat, Jun 27, 2020 at 12:35:17PM +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     | 184 ++++++++++++++++++++++++++++++++++++++++
> >  libavcodec/version.h    |   2 +-
> >  8 files changed, 198 insertions(+), 1 deletion(-)
> >  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..df5f5028cd
> > --- /dev/null
> > +++ b/libavcodec/pgxdec.c
> > @@ -0,0 +1,184 @@
> > +/*
> > + * 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"
> > +
> > +typedef struct PGXContext {
> > +    GetByteContext  g;
> > +    int depth;
> > +    int sign;
> > +    int width;
> > +    int height;
> > +} PGXContext;
> > +
> > +static int pgx_get_number(AVCodecContext *avctx, PGXContext * const s) {
> > +    int ret = -1;
> > +    char digit;
> > +
> > +    while (1) {
> > +        if (!bytestream2_get_bytes_left(&s->g))
> > +            return AVERROR_INVALIDDATA;
> > +        digit = bytestream2_get_byte(&s->g);
> > +        if (digit == ' ' || digit == 0xA || digit == 0xD) {
> > +            break;
> > +        } else if (digit < '0' || digit > '9') {
> > +            return AVERROR_INVALIDDATA;
> > +        }
> > +        if (ret < 0)
> > +            ret = 0;
>
> > +        ret = 10 * ret + (digit - '0');
>
> this could overflow
>
>
> > +    }
> > +
> > +    if (ret >= 0)
> > +        return ret;
> > +    return AVERROR_INVALIDDATA;
> > +}
> > +
> > +static int ff_pgx_decode_header(AVCodecContext *avctx, PGXContext * s)
> > +{
> > +    const char *header_start = "PG ML ";
> > +    int byte;
> > +
> > +    if (bytestream2_get_bytes_left(&s->g) < 13) {
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +    // Checks whether file starts with "PG ML "
> > +    if (memcmp(header_start, s->g.buffer, 6))
> > +        return AVERROR_INVALIDDATA;
> > +    bytestream2_skip(&s->g, 6);
> > +
> > +    // Is the component signed?
> > +    byte = bytestream2_peek_byte(&s->g);
> > +    if (byte == '+') {
> > +        s->sign = 0;
> > +        bytestream2_skip(&s->g, 1);
> > +    } else if (byte == '-') {
> > +        s->sign = 1;
> > +        bytestream2_skip(&s->g, 1);
> > +    }
> > +
> > +    if (bytestream2_peek_byte(&s->g) == ' ')
> > +        bytestream2_skip(&s->g, 1);
> > +    s->depth = pgx_get_number(avctx, s);
> > +    s->width = pgx_get_number(avctx, s);
> > +    s->height = pgx_get_number(avctx, s);
> > +    if (bytestream2_peek_byte(&s->g) == 0xA)
> > +        bytestream2_skip(&s->g, 1);
>
> pgx_get_number can return errors
> the code using it does not check for them
>
>
>
> > +    return 0;
> > +}
> > +
> > +static inline int write_frame_8(AVPacket *avpkt, AVFrame *frame, PGXContext * const s)
> > +{
> > +    int i, j;
> > +    for (i = 0; i < s->height; i++) {
> > +        uint8_t *line = frame->data[0] + i * frame->linesize[0];
> > +        for (j = 0; j < s->width; j++) {
> > +            int val;
> > +            if (bytestream2_get_bytes_left(&s->g) < 1)
> > +                return AVERROR_INVALIDDATA;
> > +            if (s->sign) {
> > +                val = bytestream2_get_byte(&s->g) + (1 << (s->depth - 1));
> > +                val <<= (8 - s->depth);
> > +                *(line + j) = val;
> > +            } else {
> > +                val = bytestream2_get_byteu(&s->g);
> > +                val <<= (8 - s->depth);
> > +                *(line + j) = val;
> > +            }
> > +        }
> > +        line += frame->linesize[0];
> > +    }
> > +    return 0;
> > +}
> > +
> > +static inline int write_frame_16(AVPacket *avpkt, AVFrame *frame, PGXContext * const s)
> > +{
> > +    int i, j;
> > +    for (i = 0; i < s->height; i++) {
> > +        uint16_t *line = (uint16_t*)frame->data[0] + i*frame->linesize[0]/2;
> > +        for (j = 0; j < s->width; j++) {
> > +            int val;
> > +            if (bytestream2_get_bytes_left(&s->g) < 2)
> > +                return AVERROR_INVALIDDATA;
> > +            if (s->sign) {
> > +                val = (int16_t)bytestream2_get_be16(&s->g) + (1 << (s->depth - 1));
> > +                val <<= (16 - s->depth);
> > +                *(line + j) = val;
> > +            } else {
> > +                val = bytestream2_get_be16u(&s->g);
> > +                val <<= (16 - s->depth);
> > +                *(line + j) = val;
> > +            }
> > +        }
> > +        line += frame->linesize[0]/2;
> > +    }
> > +    return 0;
> > +}
> > +
>
> > +static int pgx_decode_frame(AVCodecContext *avctx, void *data,
> > +                            int *got_frame, AVPacket *avpkt)
> > +{
> > +    PGXContext *s = avctx->priv_data;
> > +    AVFrame *p    = data;
> > +    int ret;
> > +    int bpp;
> > +    bytestream2_init(&s->g, avpkt->data, avpkt->size);
> > +    if ((ret = ff_pgx_decode_header(avctx, s)) < 0)
> > +        return ret;
> > +
> > +    if (ret = ff_set_dimensions(avctx, s->width, s->height) < 0)
> > +        return ret;
> > +
> > +    if (s->depth <= 8) {
> > +        avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> > +        bpp = 8;
>
> depth = 0 is valid ?, it would end in here
>
> thx

Thanks Michael, I shall fix these and resubmit.
Gautam Ramakrishnan June 27, 2020, 3:39 p.m. UTC | #4 
On Sat, Jun 27, 2020 at 7:58 PM Nicolas George <george@nsup.org> wrote:
>
> Michael Niedermayer (12020-06-27):
> > this could overflow
>
> The original code had a test to limit the size of numbers to 9 digits,
> guaranteeing no overflow. I wonder where it went.
>
I used 9 digits because I was using a fixed size string to store the number
and then used strol. Now as I removed the usage of strol, it wasnt necessary.
Do you thing using a loop of fixed iterations is a good idea?  Is it better if
I use the for loop again?
Nicolas George June 29, 2020, 12:57 p.m. UTC | #5 
Gautam Ramakrishnan (12020-06-27):
> I used 9 digits because I was using a fixed size string to store the number
> and then used strol. Now as I removed the usage of strol, it wasnt necessary.
> Do you thing using a loop of fixed iterations is a good idea?  Is it better if
> I use the for loop again?

You need to write it as the format demands. If there is an official
limit on the size of the numbers, then use that. Otherwise, if it is
possible to write the width as 00000000000000000000000000000640, then
your parser should accept that, and find another way to avoid the
overflow. Checking for INT_MAX/10 before multiplying seems like an easy
and obvious solution.

Regards,
Gautam Ramakrishnan June 29, 2020, 2:06 p.m. UTC | #6 
On Mon, Jun 29, 2020 at 6:28 PM Nicolas George <george@nsup.org> wrote:
>
> Gautam Ramakrishnan (12020-06-27):
> > I used 9 digits because I was using a fixed size string to store the number
> > and then used strol. Now as I removed the usage of strol, it wasnt necessary.
> > Do you thing using a loop of fixed iterations is a good idea?  Is it better if
> > I use the for loop again?
>
> You need to write it as the format demands. If there is an official
> limit on the size of the numbers, then use that. Otherwise, if it is
> possible to write the width as 00000000000000000000000000000640, then
> your parser should accept that, and find another way to avoid the
> overflow. Checking for INT_MAX/10 before multiplying seems like an easy
> and obvious solution.
>
I did not think of this. I shall account for this.
diff mbox series

Patch

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..df5f5028cd
--- /dev/null
+++ b/libavcodec/pgxdec.c
@@ -0,0 +1,184 @@ 
+/*
+ * 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"
+
+typedef struct PGXContext {
+    GetByteContext  g;
+    int depth;
+    int sign;
+    int width;
+    int height;
+} PGXContext;
+
+static int pgx_get_number(AVCodecContext *avctx, PGXContext * const s) {
+    int ret = -1;
+    char digit;
+
+    while (1) {
+        if (!bytestream2_get_bytes_left(&s->g))
+            return AVERROR_INVALIDDATA;
+        digit = bytestream2_get_byte(&s->g);
+        if (digit == ' ' || digit == 0xA || digit == 0xD) {
+            break;
+        } else if (digit < '0' || digit > '9') {
+            return AVERROR_INVALIDDATA;
+        }
+        if (ret < 0)
+            ret = 0;
+        ret = 10 * ret + (digit - '0');
+    }
+
+    if (ret >= 0)
+        return ret;
+    return AVERROR_INVALIDDATA;
+}
+
+static int ff_pgx_decode_header(AVCodecContext *avctx, PGXContext * s)
+{
+    const char *header_start = "PG ML ";
+    int byte;
+
+    if (bytestream2_get_bytes_left(&s->g) < 13) {
+        return AVERROR_INVALIDDATA;
+    }
+    // Checks whether file starts with "PG ML "
+    if (memcmp(header_start, s->g.buffer, 6))
+        return AVERROR_INVALIDDATA;
+    bytestream2_skip(&s->g, 6);
+
+    // Is the component signed?
+    byte = bytestream2_peek_byte(&s->g);
+    if (byte == '+') {
+        s->sign = 0;
+        bytestream2_skip(&s->g, 1);
+    } else if (byte == '-') {
+        s->sign = 1;
+        bytestream2_skip(&s->g, 1);
+    }
+
+    if (bytestream2_peek_byte(&s->g) == ' ')
+        bytestream2_skip(&s->g, 1);
+    s->depth = pgx_get_number(avctx, s);
+    s->width = pgx_get_number(avctx, s);
+    s->height = pgx_get_number(avctx, s);
+    if (bytestream2_peek_byte(&s->g) == 0xA)
+        bytestream2_skip(&s->g, 1);
+    return 0;
+}
+
+static inline int write_frame_8(AVPacket *avpkt, AVFrame *frame, PGXContext * const s)
+{
+    int i, j;
+    for (i = 0; i < s->height; i++) {
+        uint8_t *line = frame->data[0] + i * frame->linesize[0];
+        for (j = 0; j < s->width; j++) {
+            int val;
+            if (bytestream2_get_bytes_left(&s->g) < 1)
+                return AVERROR_INVALIDDATA;
+            if (s->sign) {
+                val = bytestream2_get_byte(&s->g) + (1 << (s->depth - 1));
+                val <<= (8 - s->depth);
+                *(line + j) = val;
+            } else {
+                val = bytestream2_get_byteu(&s->g);
+                val <<= (8 - s->depth);
+                *(line + j) = val;
+            }
+        }
+        line += frame->linesize[0];
+    }
+    return 0;
+}
+
+static inline int write_frame_16(AVPacket *avpkt, AVFrame *frame, PGXContext * const s)
+{
+    int i, j;
+    for (i = 0; i < s->height; i++) {
+        uint16_t *line = (uint16_t*)frame->data[0] + i*frame->linesize[0]/2;
+        for (j = 0; j < s->width; j++) {
+            int val;
+            if (bytestream2_get_bytes_left(&s->g) < 2)
+                return AVERROR_INVALIDDATA;
+            if (s->sign) {
+                val = (int16_t)bytestream2_get_be16(&s->g) + (1 << (s->depth - 1));
+                val <<= (16 - s->depth);
+                *(line + j) = val;
+            } else {
+                val = bytestream2_get_be16u(&s->g);
+                val <<= (16 - s->depth);
+                *(line + j) = val;
+            }
+        }
+        line += frame->linesize[0]/2;
+    }
+    return 0;
+}
+
+static int pgx_decode_frame(AVCodecContext *avctx, void *data,
+                            int *got_frame, AVPacket *avpkt)
+{
+    PGXContext *s = avctx->priv_data;
+    AVFrame *p    = data;
+    int ret;
+    int bpp;
+    bytestream2_init(&s->g, avpkt->data, avpkt->size);
+    if ((ret = ff_pgx_decode_header(avctx, s)) < 0)
+        return ret;
+
+    if (ret = ff_set_dimensions(avctx, s->width, s->height) < 0)
+        return ret;
+
+    if (s->depth <= 8) {
+        avctx->pix_fmt = AV_PIX_FMT_GRAY8;
+        bpp = 8;
+    } else if (s->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 ((ret = ff_get_buffer(avctx, p, 0)) < 0)
+        return ret;
+    *got_frame = 1;
+    p->pict_type = AV_PICTURE_TYPE_I;
+    p->key_frame = 1;
+    avctx->bits_per_raw_sample = s->depth;
+    if (bpp == 8)
+        write_frame_8(avpkt, p, s);
+    else if (bpp == 16)
+        write_frame_16(avpkt, p, s);
+    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 = sizeof(PGXContext),
+    .decode         = pgx_decode_frame,
+    .capabilities   = AV_CODEC_CAP_DR1,
+};
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 03593026b3..482cc6d6ba 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -28,7 +28,7 @@ 
 #include "libavutil/version.h"
 
 #define LIBAVCODEC_VERSION_MAJOR  58
-#define LIBAVCODEC_VERSION_MINOR  93
+#define LIBAVCODEC_VERSION_MINOR  94
 #define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \