diff mbox series

[FFmpeg-devel,v2,3/3] libavcodec/jpeg2000dec: Support for PPM marker

Message ID 20200718131622.6234-3-gautamramk@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v2,1/3] libavcodec/jpeg2000dec: Fix codeblock decode check | 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 18, 2020, 1:16 p.m. UTC 
From: Gautam Ramakrishnan <gautamramk@gmail.com>

This patch adds support for PPM marker for JPEG2000
decoder. It allows the samples p1_03.j2k and p1_05.j2k
to be decoded.
---
 libavcodec/jpeg2000dec.c | 111 +++++++++++++++++++++++++++++++++++----
 1 file changed, 101 insertions(+), 10 deletions(-)

Comments

Michael Niedermayer July 19, 2020, 6:35 a.m. UTC | #1 
On Sat, Jul 18, 2020 at 06:46:22PM +0530, gautamramk@gmail.com wrote:
> From: Gautam Ramakrishnan <gautamramk@gmail.com>
> 
> This patch adds support for PPM marker for JPEG2000
> decoder. It allows the samples p1_03.j2k and p1_05.j2k
> to be decoded.
> ---
>  libavcodec/jpeg2000dec.c | 111 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 101 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index 5e9e97eb6a..e37f834afe 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -71,6 +71,7 @@ typedef struct Jpeg2000POC {
>  typedef struct Jpeg2000TilePart {
>      uint8_t tile_index;                 // Tile index who refers the tile-part
>      const uint8_t *tp_end;
> +    GetByteContext header_tpg;          // bit stream of header if PPM header is used
>      GetByteContext tpg;                 // bit stream in tile-part
>  } Jpeg2000TilePart;
>  
> @@ -102,6 +103,13 @@ typedef struct Jpeg2000DecoderContext {
>      uint8_t         cbps[4];    // bits per sample in particular components
>      uint8_t         sgnd[4];    // if a component is signed
>      uint8_t         properties[4];
> +
> +    uint8_t         has_ppm;
> +    uint8_t         *packed_headers; // contains packed headers. Used only along with PPM marker
> +    int             packed_headers_size;
> +    GetByteContext  packed_headers_stream;

> +    uint8_t         in_tile_headers;

in_tile_headers could be moved into a seperate patch before the ppm addition
it could also be used for other things than ppm



> +
>      int             cdx[4], cdy[4];
>      int             precision;
>      int             ncomponents;
> @@ -928,6 +936,31 @@ static int get_plt(Jpeg2000DecoderContext *s, int n)
>      return 0;
>  }
>  
> +static int get_ppm(Jpeg2000DecoderContext *s, int n)
> +{
> +    void *new;
> +
> +    if (n < 3) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPM data.\n");
> +        return AVERROR_INVALIDDATA;
> +    }

> +    s->has_ppm = 1;

Is there a reason has_ppm is set before all error checks ?
this leaves the possibility for has_ppm to be set but get_ppm failing
it seems more consistant to me to only set it if get_ppm succeeded

[...]
Gautam Ramakrishnan July 19, 2020, 7:54 a.m. UTC | #2 
On Sun, Jul 19, 2020 at 12:05 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sat, Jul 18, 2020 at 06:46:22PM +0530, gautamramk@gmail.com wrote:
> > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> >
> > This patch adds support for PPM marker for JPEG2000
> > decoder. It allows the samples p1_03.j2k and p1_05.j2k
> > to be decoded.
> > ---
> >  libavcodec/jpeg2000dec.c | 111 +++++++++++++++++++++++++++++++++++----
> >  1 file changed, 101 insertions(+), 10 deletions(-)
> >
> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > index 5e9e97eb6a..e37f834afe 100644
> > --- a/libavcodec/jpeg2000dec.c
> > +++ b/libavcodec/jpeg2000dec.c
> > @@ -71,6 +71,7 @@ typedef struct Jpeg2000POC {
> >  typedef struct Jpeg2000TilePart {
> >      uint8_t tile_index;                 // Tile index who refers the tile-part
> >      const uint8_t *tp_end;
> > +    GetByteContext header_tpg;          // bit stream of header if PPM header is used
> >      GetByteContext tpg;                 // bit stream in tile-part
> >  } Jpeg2000TilePart;
> >
> > @@ -102,6 +103,13 @@ typedef struct Jpeg2000DecoderContext {
> >      uint8_t         cbps[4];    // bits per sample in particular components
> >      uint8_t         sgnd[4];    // if a component is signed
> >      uint8_t         properties[4];
> > +
> > +    uint8_t         has_ppm;
> > +    uint8_t         *packed_headers; // contains packed headers. Used only along with PPM marker
> > +    int             packed_headers_size;
> > +    GetByteContext  packed_headers_stream;
>
> > +    uint8_t         in_tile_headers;
>
> in_tile_headers could be moved into a seperate patch before the ppm addition
> it could also be used for other things than ppm
Yep, makes sense, I shall change this.
>
>
>
> > +
> >      int             cdx[4], cdy[4];
> >      int             precision;
> >      int             ncomponents;
> > @@ -928,6 +936,31 @@ static int get_plt(Jpeg2000DecoderContext *s, int n)
> >      return 0;
> >  }
> >
> > +static int get_ppm(Jpeg2000DecoderContext *s, int n)
> > +{
> > +    void *new;
> > +
> > +    if (n < 3) {
> > +        av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPM data.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
>
> > +    s->has_ppm = 1;
>
> Is there a reason has_ppm is set before all error checks ?
> this leaves the possibility for has_ppm to be set but get_ppm failing
> it seems more consistant to me to only set it if get_ppm succeeded
I shall change this as well
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If the United States is serious about tackling the national security threats
> related to an insecure 5G network, it needs to rethink the extent to which it
> values corporate profits and government espionage over security.-Bruce Schneier
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
index 5e9e97eb6a..e37f834afe 100644
--- a/libavcodec/jpeg2000dec.c
+++ b/libavcodec/jpeg2000dec.c
@@ -71,6 +71,7 @@  typedef struct Jpeg2000POC {
 typedef struct Jpeg2000TilePart {
     uint8_t tile_index;                 // Tile index who refers the tile-part
     const uint8_t *tp_end;
+    GetByteContext header_tpg;          // bit stream of header if PPM header is used
     GetByteContext tpg;                 // bit stream in tile-part
 } Jpeg2000TilePart;
 
@@ -102,6 +103,13 @@  typedef struct Jpeg2000DecoderContext {
     uint8_t         cbps[4];    // bits per sample in particular components
     uint8_t         sgnd[4];    // if a component is signed
     uint8_t         properties[4];
+
+    uint8_t         has_ppm;
+    uint8_t         *packed_headers; // contains packed headers. Used only along with PPM marker
+    int             packed_headers_size;
+    GetByteContext  packed_headers_stream;
+    uint8_t         in_tile_headers;
+
     int             cdx[4], cdy[4];
     int             precision;
     int             ncomponents;
@@ -928,6 +936,31 @@  static int get_plt(Jpeg2000DecoderContext *s, int n)
     return 0;
 }
 
+static int get_ppm(Jpeg2000DecoderContext *s, int n)
+{
+    void *new;
+
+    if (n < 3) {
+        av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPM data.\n");
+        return AVERROR_INVALIDDATA;
+    }
+    s->has_ppm = 1;
+    bytestream2_get_byte(&s->g); //Zppm is skipped and not used
+    new = av_realloc(s->packed_headers,
+                     s->packed_headers_size + n - 3);
+    if (new) {
+        s->packed_headers = new;
+    } else
+        return AVERROR(ENOMEM);
+    memset(&s->packed_headers_stream, 0, sizeof(s->packed_headers_stream));
+    memcpy(s->packed_headers + s->packed_headers_size,
+           s->g.buffer, n - 3);
+    s->packed_headers_size += n - 3;
+    bytestream2_skip(&s->g, n - 3);
+
+    return 0;
+}
+
 static int get_ppt(Jpeg2000DecoderContext *s, int n)
 {
     Jpeg2000Tile *tile;
@@ -1039,8 +1072,19 @@  static int getlblockinc(Jpeg2000DecoderContext *s)
     return res;
 }
 
-static inline void select_stream(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
+static inline void select_header(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
                                  int *tp_index)
+{
+    s->g = tile->tile_part[*tp_index].header_tpg;
+    if (bytestream2_get_bytes_left(&s->g) == 0 && s->bit_index == 8) {
+        if (*tp_index < FF_ARRAY_ELEMS(tile->tile_part) - 1) {
+            s->g = tile->tile_part[++(*tp_index)].tpg;
+        }
+    }
+}
+
+static inline void select_stream(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
+                                 int *tp_index, Jpeg2000CodingStyle *codsty)
 {
     s->g = tile->tile_part[*tp_index].tpg;
     if (bytestream2_get_bytes_left(&s->g) == 0 && s->bit_index == 8) {
@@ -1048,8 +1092,12 @@  static inline void select_stream(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
             s->g = tile->tile_part[++(*tp_index)].tpg;
         }
     }
-    if (bytestream2_peek_be32(&s->g) == JPEG2000_SOP_FIXED_BYTES)
-        bytestream2_skip(&s->g, JPEG2000_SOP_BYTE_LENGTH);
+    if (codsty->csty & JPEG2000_CSTY_SOP) {
+        if (bytestream2_peek_be32(&s->g) == JPEG2000_SOP_FIXED_BYTES)
+            bytestream2_skip(&s->g, JPEG2000_SOP_BYTE_LENGTH);
+        else
+            av_log(s->avctx, AV_LOG_ERROR, "SOP marker not found. instead %X\n", bytestream2_peek_be32(&s->g));
+    }
 }
 
 static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile, int *tp_index,
@@ -1064,10 +1112,12 @@  static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
         return 0;
     rlevel->band[0].prec[precno].decoded_layers = layno + 1;
     // Select stream to read from
-    if (tile->has_ppt)
+    if (s->has_ppm)
+        select_header(s, tile, tp_index);
+    else if (tile->has_ppt)
         s->g = tile->packed_headers_stream;
     else
-        select_stream(s, tile, tp_index);
+        select_stream(s, tile, tp_index, codsty);
 
     if (!(ret = get_bits(s, 1))) {
         jpeg2000_flush(s);
@@ -1178,9 +1228,12 @@  static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
     }
 
     // Save state of stream
-    if (tile->has_ppt) {
+    if (s->has_ppm) {
+        tile->tile_part[*tp_index].header_tpg = s->g;
+        select_stream(s, tile, tp_index, codsty);
+    } else if (tile->has_ppt) {
         tile->packed_headers_stream = s->g;
-        select_stream(s, tile, tp_index);
+        select_stream(s, tile, tp_index, codsty);
     }
     for (bandno = 0; bandno < rlevel->nbands; bandno++) {
         Jpeg2000Band *band = rlevel->band + bandno;
@@ -1228,10 +1281,20 @@  static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
     return 0;
 
 skip_data:
-    if (tile->has_ppt)
+    if (codsty->csty & JPEG2000_CSTY_EPH) {
+        if (bytestream2_peek_be16(&s->g) == JPEG2000_EPH)
+            bytestream2_skip(&s->g, 2);
+        else
+            av_log(s->avctx, AV_LOG_ERROR, "EPH marker not found. instead %X\n", bytestream2_peek_be32(&s->g));
+    }
+    if (s->has_ppm) {
+        tile->tile_part[*tp_index].header_tpg = s->g;
+        select_stream(s, tile, tp_index, codsty);
+    } else if (tile->has_ppt) {
         tile->packed_headers_stream = s->g;
-    else
-        tile->tile_part[*tp_index].tpg = s->g;
+        select_stream(s, tile, tp_index, codsty);
+    }
+    tile->tile_part[*tp_index].tpg = s->g;
     return 0;
 }
 
@@ -2064,6 +2127,9 @@  static void jpeg2000_dec_cleanup(Jpeg2000DecoderContext *s)
             s->tile[tileno].packed_headers_size = 0;
         }
     }
+    av_freep(&s->packed_headers);
+    s->packed_headers_size = 0;
+    memset(&s->packed_headers_stream, 0, sizeof(s->packed_headers_stream));
     av_freep(&s->tile);
     memset(s->codsty, 0, sizeof(s->codsty));
     memset(s->qntsty, 0, sizeof(s->qntsty));
@@ -2114,6 +2180,11 @@  static int jpeg2000_read_main_headers(Jpeg2000DecoderContext *s)
                 return AVERROR_INVALIDDATA;
             }
 
+            if (s->has_ppm) {
+                uint32_t tp_header_size = bytestream2_get_be32u(&s->packed_headers_stream);
+                bytestream2_init(&tp->header_tpg, s->packed_headers_stream.buffer, tp_header_size);
+                bytestream2_skip(&s->packed_headers_stream, tp_header_size);
+            }
             if (tile->has_ppt && tile->tp_idx == 0) {
                 bytestream2_init(&tile->packed_headers_stream, tile->packed_headers, tile->packed_headers_size);
             }
@@ -2165,6 +2236,12 @@  static int jpeg2000_read_main_headers(Jpeg2000DecoderContext *s)
             ret = get_poc(s, len, poc);
             break;
         case JPEG2000_SOT:
+            if (!s->in_tile_headers) {
+                s->in_tile_headers = 1;
+                if (s->has_ppm) {
+                    bytestream2_init(&s->packed_headers_stream, s->packed_headers, s->packed_headers_size);
+                }
+            }
             if (!(ret = get_sot(s, len))) {
                 av_assert1(s->curtileno >= 0);
                 codsty = s->tile[s->curtileno].codsty;
@@ -2190,8 +2267,22 @@  static int jpeg2000_read_main_headers(Jpeg2000DecoderContext *s)
             // Packet length, tile-part header
             ret = get_plt(s, len);
             break;
+        case JPEG2000_PPM:
+            // Packed headers, main header
+            if (s->in_tile_headers) {
+                av_log(s->avctx, AV_LOG_ERROR, "PPM Marker can only be in Main header\n");
+                return AVERROR_INVALIDDATA;
+            }
+            ret = get_ppm(s, len);
+            break;
         case JPEG2000_PPT:
             // Packed headers, tile-part header
+            if (s->has_ppm) {
+                av_log(s->avctx, AV_LOG_ERROR,
+                       "Cannot have both PPT and PPM marker.\n");
+                return AVERROR_INVALIDDATA;
+            }
+
             ret = get_ppt(s, len);
             break;
         default: