diff mbox series

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

Message ID 20200702191939.6667-1-gautamramk@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,v7,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 July 2, 2020, 7:19 p.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     | 169 ++++++++++++++++++++++++++++++++++++++++
 libavcodec/version.h    |   4 +-
 8 files changed, 184 insertions(+), 2 deletions(-)
 create mode 100644 libavcodec/pgxdec.c

Comments

Michael Niedermayer July 3, 2020, 9:30 p.m. UTC | #1 
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

[...]
Nicolas George July 3, 2020, 10:38 p.m. UTC | #2 
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,
Gautam Ramakrishnan July 4, 2020, 10:21 a.m. UTC | #3 
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.
Nicolas George July 4, 2020, 12:02 p.m. UTC | #4 
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,
Nicolas George July 4, 2020, 12:12 p.m. UTC | #5 
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,
Gautam Ramakrishnan July 4, 2020, 12:45 p.m. UTC | #6 
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
Andreas Rheinhardt July 4, 2020, 1:18 p.m. UTC | #7 
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
Nicolas George July 4, 2020, 1:32 p.m. UTC | #8 
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,
Michael Niedermayer July 4, 2020, 4:33 p.m. UTC | #9 
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

[...]
Michael Niedermayer July 4, 2020, 4:38 p.m. UTC | #10 
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

[...]
Nicolas George July 4, 2020, 4:45 p.m. UTC | #11 
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,
Michael Niedermayer July 4, 2020, 6:11 p.m. UTC | #12 
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 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..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, \