diff mbox series

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

Message ID 20200627181047.13466-1-gautamramk@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v2,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, 6:10 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     | 199 ++++++++++++++++++++++++++++++++++++++++
 libavcodec/version.h    |   2 +-
 8 files changed, 213 insertions(+), 1 deletion(-)
 create mode 100644 libavcodec/pgxdec.c

Comments

Carl Eugen Hoyos June 27, 2020, 7:11 p.m. UTC | #1
Am Sa., 27. Juni 2020 um 20:11 Uhr schrieb <gautamramk@gmail.com>:

> +static int ff_pgx_decode_header(AVCodecContext *avctx, PGXContext * s,
> +                                int *depth, int *width, int *height,
> +                                int *sign)
> +{
> +    const char *header_start = "PG ML ";
> +    int byte;
> +
> +    if (bytestream2_get_bytes_left(&s->g) < 13) {
> +        return AVERROR_INVALIDDATA;
> +    }

Do other decoders do this for 13 bytes or less?

> +    // Checks whether file starts with "PG ML "
> +    if (memcmp(header_start, s->g.buffer, 6))
> +        return AVERROR_INVALIDDATA;

If you really have to keep this why isn't it:
memcmp("PG ML ", s->g.buffer, 6)
which would make the ugly comment go away?

Carl Eugen
Gautam Ramakrishnan June 28, 2020, 10:45 a.m. UTC | #2
On Sun, Jun 28, 2020 at 12:41 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Am Sa., 27. Juni 2020 um 20:11 Uhr schrieb <gautamramk@gmail.com>:
>
> > +static int ff_pgx_decode_header(AVCodecContext *avctx, PGXContext * s,
> > +                                int *depth, int *width, int *height,
> > +                                int *sign)
> > +{
> > +    const char *header_start = "PG ML ";
> > +    int byte;
> > +
> > +    if (bytestream2_get_bytes_left(&s->g) < 13) {
> > +        return AVERROR_INVALIDDATA;
> > +    }
>
> Do other decoders do this for 13 bytes or less?
This could be improved. I modify it.
>
> > +    // Checks whether file starts with "PG ML "
> > +    if (memcmp(header_start, s->g.buffer, 6))
> > +        return AVERROR_INVALIDDATA;
>
> If you really have to keep this why isn't it:
> memcmp("PG ML ", s->g.buffer, 6)
> which would make the ugly comment go away?
I shall change this. I do not understand how the check can be
removed. It is compulsory that a PGX file must have these bytes
Nicolas George June 28, 2020, 10:50 a.m. UTC | #3
Gautam Ramakrishnan (12020-06-28):
> I shall change this. I do not understand how the check can be
> removed. It is compulsory that a PGX file must have these bytes

In case it was not clear, I think Carl Eugen was suggesting you include
the string directly in the comparison.

But you do not need that test, because you do not need the file to be a
100% valid PGX file. It is not the task of a decoder to enforce
conformance: its task is to decode.

Regards,
Gautam Ramakrishnan June 28, 2020, 12:36 p.m. UTC | #4
On Sun, Jun 28, 2020 at 4:21 PM Nicolas George <george@nsup.org> wrote:
>
> Gautam Ramakrishnan (12020-06-28):
> > I shall change this. I do not understand how the check can be
> > removed. It is compulsory that a PGX file must have these bytes
>
> In case it was not clear, I think Carl Eugen was suggesting you include
> the string directly in the comparison.
>
This was clear.
> But you do not need that test, because you do not need the file to be a
> 100% valid PGX file. It is not the task of a decoder to enforce
> conformance: its task is to decode.
>
This is what I do not understand. Suppose the file does not start
with the bytes "PG ML ", how is that to be handled?
Nicolas George June 28, 2020, 12:39 p.m. UTC | #5
Gautam Ramakrishnan (12020-06-28):
> This is what I do not understand. Suppose the file does not start
> with the bytes "PG ML ", how is that to be handled?

The same way as if it did.

Take a valid PGX file. Replace these six octets with FOOBAR: why should
it make any difference for your decoder? All the same information is
present.

Regards,
Gautam Ramakrishnan June 28, 2020, 2:23 p.m. UTC | #6
On Sun, Jun 28, 2020 at 6:09 PM Nicolas George <george@nsup.org> wrote:
>
> Gautam Ramakrishnan (12020-06-28):
> > This is what I do not understand. Suppose the file does not start
> > with the bytes "PG ML ", how is that to be handled?
>
> The same way as if it did.
>
> Take a valid PGX file. Replace these six octets with FOOBAR: why should
> it make any difference for your decoder? All the same information is
> present.
Oh got it, just need to ignore the text then. Thanks.
Carl Eugen Hoyos June 28, 2020, 2:27 p.m. UTC | #7
Am So., 28. Juni 2020 um 16:24 Uhr schrieb Gautam Ramakrishnan
<gautamramk@gmail.com>:

> Oh got it, just need to ignore the text then.

You *can*

Carl Eugen
Kieran Kunhya June 29, 2020, 7:38 a.m. UTC | #8
On Sun, 28 Jun 2020 at 15:27, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am So., 28. Juni 2020 um 16:24 Uhr schrieb Gautam Ramakrishnan
> <gautamramk@gmail.com>:
>
> > Oh got it, just need to ignore the text then.
>
> You *can*
>
> Carl Eugen
>

Why are you confusing Gautam like this?
If I were new to FFmpeg and working on this I would be really confused by
the lack of clarity.

IMO just let the memcmp stay, but at least make it clear which way he
should go.

Kieran
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..5f4704f31a
--- /dev/null
+++ b/libavcodec/pgxdec.c
@@ -0,0 +1,199 @@ 
+/*
+ * 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;
+} PGXContext;
+
+static int pgx_get_number(AVCodecContext *avctx, PGXContext * const s) {
+    int ret = -1;
+    char digit;
+    int numdigits = 0;
+
+    while (1) {
+        if (numdigits > 10) {
+            return -1;
+        }
+        if (!bytestream2_get_bytes_left(&s->g))
+            return -1;
+        digit = bytestream2_get_byte(&s->g);
+        if (digit == ' ' || digit == 0xA || digit == 0xD)
+            break;
+        else if (digit < '0' || digit > '9')
+            return -1;
+
+        if (ret < 0)
+            ret = 0;
+        ret = 10 * ret + (digit - '0');
+        numdigits++;
+    }
+
+    if (ret >= 0)
+        return ret;
+    return -1;
+}
+
+static int ff_pgx_decode_header(AVCodecContext *avctx, PGXContext * s,
+                                int *depth, int *width, int *height,
+                                int *sign)
+{
+    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 == '+') {
+        *sign = 0;
+        bytestream2_skip(&s->g, 1);
+    } else if (byte == '-') {
+        *sign = 1;
+        bytestream2_skip(&s->g, 1);
+    }
+
+    if (bytestream2_peek_byte(&s->g) == ' ')
+        bytestream2_skip(&s->g, 1);
+    *depth = pgx_get_number(avctx, s);
+    *width = pgx_get_number(avctx, s);
+    *height = pgx_get_number(avctx, s);
+    if (*depth <= 0 || *width <= 0 || *height <= 0)
+        goto error;
+
+    if (bytestream2_peek_byte(&s->g) == 0xA)
+        bytestream2_skip(&s->g, 1);
+    return 0;
+
+error:
+    av_log(avctx, AV_LOG_ERROR, "Error in decoding header.\n");
+    return AVERROR_INVALIDDATA;
+}
+
+static inline int write_frame_8(AVPacket *avpkt, AVFrame *frame, PGXContext * const s,
+                                int width, int height, int sign, int depth)
+{
+    int i, j;
+    for (i = 0; i < height; i++) {
+        uint8_t *line = frame->data[0] + i * frame->linesize[0];
+        for (j = 0; j < width; j++) {
+            int val;
+            if (bytestream2_get_bytes_left(&s->g) < 1)
+                return AVERROR_INVALIDDATA;
+            if (sign) {
+                val = bytestream2_get_byte(&s->g) + (1 << (depth - 1));
+                val <<= (8 - depth);
+                *(line + j) = val;
+            } else {
+                val = bytestream2_get_byteu(&s->g);
+                val <<= (8 - depth);
+                *(line + j) = val;
+            }
+        }
+        line += frame->linesize[0];
+    }
+    return 0;
+}
+
+static inline int write_frame_16(AVPacket *avpkt, AVFrame *frame, PGXContext * const s,
+                                 int width, int height, int sign, int depth)
+{
+    int i, j;
+    for (i = 0; i < height; i++) {
+        uint16_t *line = (uint16_t*)frame->data[0] + i*frame->linesize[0]/2;
+        for (j = 0; j < width; j++) {
+            int val;
+            if (bytestream2_get_bytes_left(&s->g) < 2)
+                return AVERROR_INVALIDDATA;
+            if (sign) {
+                val = (int16_t)bytestream2_get_be16(&s->g) + (1 << (depth - 1));
+                val <<= (16 - depth);
+                *(line + j) = val;
+            } else {
+                val = bytestream2_get_be16u(&s->g);
+                val <<= (16 - 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;
+    int width, height, depth;
+    int sign = 0;
+    bytestream2_init(&s->g, avpkt->data, avpkt->size);
+
+    if ((ret = ff_pgx_decode_header(avctx, s, &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 ((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 = depth;
+    if (bpp == 8)
+        write_frame_8(avpkt, p, s, width, height, sign, depth);
+    else if (bpp == 16)
+        write_frame_16(avpkt, p, s, width, height, sign, depth);
+    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, \