diff mbox series

[FFmpeg-devel,v13] libavcodec/jpeg2000dec.c: Add support for PPT marker

Message ID 20200405104328.15484-1-gautamramk@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v13] libavcodec/jpeg2000dec.c: Add support for PPT marker
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Gautam Ramakrishnan April 5, 2020, 10:43 a.m. UTC
From: Gautam Ramakrishnan <gautamramk@gmail.com>

This patch adds functional changes to support the
PPT marker. This patch fixes bug ticket #4610.
---
 libavcodec/jpeg2000dec.c | 88 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 11 deletions(-)

Comments

Michael Niedermayer April 5, 2020, 7:45 p.m. UTC | #1
On Sun, Apr 05, 2020 at 04:13:28PM +0530, gautamramk@gmail.com wrote:
> From: Gautam Ramakrishnan <gautamramk@gmail.com>
> 
> This patch adds functional changes to support the
> PPT marker. This patch fixes bug ticket #4610.
> ---
>  libavcodec/jpeg2000dec.c | 88 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 77 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index 732d88e6fc..2af3c61c37 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -83,6 +83,10 @@ typedef struct Jpeg2000Tile {
>      Jpeg2000QuantStyle  qntsty[4];
>      Jpeg2000POC         poc;
>      Jpeg2000TilePart    tile_part[32];
> +    uint8_t             has_ppt;                // whether this tile has a ppt marker
> +    uint8_t             *packed_headers;        // contains packed headers. Used only along with PPT marker
> +    int                 packed_headers_size;    // size in bytes of the packed headers
> +    GetByteContext      packed_headers_stream;  // byte context corresponding to packed headers
>      uint16_t tp_idx;                    // Tile-part index
>      int coord[2][2];                    // border coordinates {{x0, x1}, {y0, y1}}
>  } Jpeg2000Tile;
> @@ -855,6 +859,37 @@ static int get_plt(Jpeg2000DecoderContext *s, int n)
>      return 0;
>  }
>  
> +static int get_ppt(Jpeg2000DecoderContext *s, int n)
> +{
> +    Jpeg2000Tile *tile;
> +
> +    if (s->curtileno < 0)
> +        return AVERROR_INVALIDDATA;
> +
> +    tile = &s->tile[s->curtileno];
> +    if (tile->tp_idx != 0) {
> +        av_log(s->avctx, AV_LOG_ERROR,
> +               "PPT marker can occur only on first tile part of a tile.\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    tile->has_ppt = 1;  // this tile has a ppt marker
> +    bytestream2_get_byte(&s->g); // Zppt is skipped and not used


> +    if (!tile->packed_headers)
> +        tile->packed_headers = av_malloc(n - 3);
> +    else
> +        tile->packed_headers = av_realloc(tile->packed_headers,
> +                                          tile->packed_headers_size + n - 3);

why is the special case with av_malloc() needed ?


> +    if (!tile->packed_headers)
> +        return AVERROR(ENOMEM);

this is looks like a memleak as the original array has been overwritten but
not deallocated

thx

[...]
Gautam Ramakrishnan April 6, 2020, 3:53 a.m. UTC | #2
On Mon, Apr 6, 2020 at 1:16 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sun, Apr 05, 2020 at 04:13:28PM +0530, gautamramk@gmail.com wrote:
> > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> >
> > This patch adds functional changes to support the
> > PPT marker. This patch fixes bug ticket #4610.
> > ---
> >  libavcodec/jpeg2000dec.c | 88 +++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 77 insertions(+), 11 deletions(-)
> >
> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > index 732d88e6fc..2af3c61c37 100644
> > --- a/libavcodec/jpeg2000dec.c
> > +++ b/libavcodec/jpeg2000dec.c
> > @@ -83,6 +83,10 @@ typedef struct Jpeg2000Tile {
> >      Jpeg2000QuantStyle  qntsty[4];
> >      Jpeg2000POC         poc;
> >      Jpeg2000TilePart    tile_part[32];
> > +    uint8_t             has_ppt;                // whether this tile has a ppt marker
> > +    uint8_t             *packed_headers;        // contains packed headers. Used only along with PPT marker
> > +    int                 packed_headers_size;    // size in bytes of the packed headers
> > +    GetByteContext      packed_headers_stream;  // byte context corresponding to packed headers
> >      uint16_t tp_idx;                    // Tile-part index
> >      int coord[2][2];                    // border coordinates {{x0, x1}, {y0, y1}}
> >  } Jpeg2000Tile;
> > @@ -855,6 +859,37 @@ static int get_plt(Jpeg2000DecoderContext *s, int n)
> >      return 0;
> >  }
> >
> > +static int get_ppt(Jpeg2000DecoderContext *s, int n)
> > +{
> > +    Jpeg2000Tile *tile;
> > +
> > +    if (s->curtileno < 0)
> > +        return AVERROR_INVALIDDATA;
> > +
> > +    tile = &s->tile[s->curtileno];
> > +    if (tile->tp_idx != 0) {
> > +        av_log(s->avctx, AV_LOG_ERROR,
> > +               "PPT marker can occur only on first tile part of a tile.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    tile->has_ppt = 1;  // this tile has a ppt marker
> > +    bytestream2_get_byte(&s->g); // Zppt is skipped and not used
>
>
> > +    if (!tile->packed_headers)
> > +        tile->packed_headers = av_malloc(n - 3);
> > +    else
> > +        tile->packed_headers = av_realloc(tile->packed_headers,
> > +                                          tile->packed_headers_size + n - 3);
>
> why is the special case with av_malloc() needed ?
>
The if else is to check whether this is the first time this tile is being
allocated its packed headers. If packed headers does not exist
exist, create it. The else condition is just to add a new packed
header to it.
>
> > +    if (!tile->packed_headers)
> > +        return AVERROR(ENOMEM);
>
> this is looks like a memleak as the original array has been overwritten but
> not deallocated
This was a check for the malloc failure
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> In fact, the RIAA has been known to suggest that students drop out
> of college or go to community college in order to be able to afford
> settlements. -- The RIAA
> _______________________________________________
> 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".
Michael Niedermayer April 6, 2020, 12:11 p.m. UTC | #3
On Mon, Apr 06, 2020 at 09:23:26AM +0530, Gautam Ramakrishnan wrote:
> On Mon, Apr 6, 2020 at 1:16 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Sun, Apr 05, 2020 at 04:13:28PM +0530, gautamramk@gmail.com wrote:
> > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > >
> > > This patch adds functional changes to support the
> > > PPT marker. This patch fixes bug ticket #4610.
> > > ---
> > >  libavcodec/jpeg2000dec.c | 88 +++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 77 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > > index 732d88e6fc..2af3c61c37 100644
> > > --- a/libavcodec/jpeg2000dec.c
> > > +++ b/libavcodec/jpeg2000dec.c
> > > @@ -83,6 +83,10 @@ typedef struct Jpeg2000Tile {
> > >      Jpeg2000QuantStyle  qntsty[4];
> > >      Jpeg2000POC         poc;
> > >      Jpeg2000TilePart    tile_part[32];
> > > +    uint8_t             has_ppt;                // whether this tile has a ppt marker
> > > +    uint8_t             *packed_headers;        // contains packed headers. Used only along with PPT marker
> > > +    int                 packed_headers_size;    // size in bytes of the packed headers
> > > +    GetByteContext      packed_headers_stream;  // byte context corresponding to packed headers
> > >      uint16_t tp_idx;                    // Tile-part index
> > >      int coord[2][2];                    // border coordinates {{x0, x1}, {y0, y1}}
> > >  } Jpeg2000Tile;
> > > @@ -855,6 +859,37 @@ static int get_plt(Jpeg2000DecoderContext *s, int n)
> > >      return 0;
> > >  }
> > >
> > > +static int get_ppt(Jpeg2000DecoderContext *s, int n)
> > > +{
> > > +    Jpeg2000Tile *tile;
> > > +
> > > +    if (s->curtileno < 0)
> > > +        return AVERROR_INVALIDDATA;
> > > +
> > > +    tile = &s->tile[s->curtileno];
> > > +    if (tile->tp_idx != 0) {
> > > +        av_log(s->avctx, AV_LOG_ERROR,
> > > +               "PPT marker can occur only on first tile part of a tile.\n");
> > > +        return AVERROR_INVALIDDATA;
> > > +    }
> > > +
> > > +    tile->has_ppt = 1;  // this tile has a ppt marker
> > > +    bytestream2_get_byte(&s->g); // Zppt is skipped and not used
> >
> >
> > > +    if (!tile->packed_headers)
> > > +        tile->packed_headers = av_malloc(n - 3);
> > > +    else
> > > +        tile->packed_headers = av_realloc(tile->packed_headers,
> > > +                                          tile->packed_headers_size + n - 3);
> >
> > why is the special case with av_malloc() needed ?
> >
> The if else is to check whether this is the first time this tile is being
> allocated its packed headers. If packed headers does not exist
> exist, create it. The else condition is just to add a new packed
> header to it.

what is the problem with

av_realloc(tile->packed_headers,  tile->packed_headers_size + n - 3);
when tile->packed_headers is NULL and tile->packed_headers_size is 0 for the
first allocation ?


thx

[...]
Gautam Ramakrishnan April 6, 2020, 6:31 p.m. UTC | #4
On Mon, Apr 6, 2020 at 5:41 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Mon, Apr 06, 2020 at 09:23:26AM +0530, Gautam Ramakrishnan wrote:
> > On Mon, Apr 6, 2020 at 1:16 AM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Sun, Apr 05, 2020 at 04:13:28PM +0530, gautamramk@gmail.com wrote:
> > > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > > >
> > > > This patch adds functional changes to support the
> > > > PPT marker. This patch fixes bug ticket #4610.
> > > > ---
> > > >  libavcodec/jpeg2000dec.c | 88 +++++++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 77 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > > > index 732d88e6fc..2af3c61c37 100644
> > > > --- a/libavcodec/jpeg2000dec.c
> > > > +++ b/libavcodec/jpeg2000dec.c
> > > > @@ -83,6 +83,10 @@ typedef struct Jpeg2000Tile {
> > > >      Jpeg2000QuantStyle  qntsty[4];
> > > >      Jpeg2000POC         poc;
> > > >      Jpeg2000TilePart    tile_part[32];
> > > > +    uint8_t             has_ppt;                // whether this tile has a ppt marker
> > > > +    uint8_t             *packed_headers;        // contains packed headers. Used only along with PPT marker
> > > > +    int                 packed_headers_size;    // size in bytes of the packed headers
> > > > +    GetByteContext      packed_headers_stream;  // byte context corresponding to packed headers
> > > >      uint16_t tp_idx;                    // Tile-part index
> > > >      int coord[2][2];                    // border coordinates {{x0, x1}, {y0, y1}}
> > > >  } Jpeg2000Tile;
> > > > @@ -855,6 +859,37 @@ static int get_plt(Jpeg2000DecoderContext *s, int n)
> > > >      return 0;
> > > >  }
> > > >
> > > > +static int get_ppt(Jpeg2000DecoderContext *s, int n)
> > > > +{
> > > > +    Jpeg2000Tile *tile;
> > > > +
> > > > +    if (s->curtileno < 0)
> > > > +        return AVERROR_INVALIDDATA;
> > > > +
> > > > +    tile = &s->tile[s->curtileno];
> > > > +    if (tile->tp_idx != 0) {
> > > > +        av_log(s->avctx, AV_LOG_ERROR,
> > > > +               "PPT marker can occur only on first tile part of a tile.\n");
> > > > +        return AVERROR_INVALIDDATA;
> > > > +    }
> > > > +
> > > > +    tile->has_ppt = 1;  // this tile has a ppt marker
> > > > +    bytestream2_get_byte(&s->g); // Zppt is skipped and not used
> > >
> > >
> > > > +    if (!tile->packed_headers)
> > > > +        tile->packed_headers = av_malloc(n - 3);
> > > > +    else
> > > > +        tile->packed_headers = av_realloc(tile->packed_headers,
> > > > +                                          tile->packed_headers_size + n - 3);
> > >
> > > why is the special case with av_malloc() needed ?
> > >
> > The if else is to check whether this is the first time this tile is being
> > allocated its packed headers. If packed headers does not exist
> > exist, create it. The else condition is just to add a new packed
> > header to it.
>
> what is the problem with
>
> av_realloc(tile->packed_headers,  tile->packed_headers_size + n - 3);
> when tile->packed_headers is NULL and tile->packed_headers_size is 0 for the
> first allocation ?
>
Oh!! Thanks for clearing that out. I did not know realloc could be
used like that.
Was a bit skeptical about writing it that way. I'll test that out and resubmit.
>
> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> In fact, the RIAA has been known to suggest that students drop out
> of college or go to community college in order to be able to afford
> settlements. -- The RIAA
> _______________________________________________
> 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 732d88e6fc..2af3c61c37 100644
--- a/libavcodec/jpeg2000dec.c
+++ b/libavcodec/jpeg2000dec.c
@@ -83,6 +83,10 @@  typedef struct Jpeg2000Tile {
     Jpeg2000QuantStyle  qntsty[4];
     Jpeg2000POC         poc;
     Jpeg2000TilePart    tile_part[32];
+    uint8_t             has_ppt;                // whether this tile has a ppt marker
+    uint8_t             *packed_headers;        // contains packed headers. Used only along with PPT marker
+    int                 packed_headers_size;    // size in bytes of the packed headers
+    GetByteContext      packed_headers_stream;  // byte context corresponding to packed headers
     uint16_t tp_idx;                    // Tile-part index
     int coord[2][2];                    // border coordinates {{x0, x1}, {y0, y1}}
 } Jpeg2000Tile;
@@ -855,6 +859,37 @@  static int get_plt(Jpeg2000DecoderContext *s, int n)
     return 0;
 }
 
+static int get_ppt(Jpeg2000DecoderContext *s, int n)
+{
+    Jpeg2000Tile *tile;
+
+    if (s->curtileno < 0)
+        return AVERROR_INVALIDDATA;
+
+    tile = &s->tile[s->curtileno];
+    if (tile->tp_idx != 0) {
+        av_log(s->avctx, AV_LOG_ERROR,
+               "PPT marker can occur only on first tile part of a tile.\n");
+        return AVERROR_INVALIDDATA;
+    }
+
+    tile->has_ppt = 1;  // this tile has a ppt marker
+    bytestream2_get_byte(&s->g); // Zppt is skipped and not used
+    if (!tile->packed_headers)
+        tile->packed_headers = av_malloc(n - 3);
+    else
+        tile->packed_headers = av_realloc(tile->packed_headers,
+                                          tile->packed_headers_size + n - 3);
+    if (!tile->packed_headers)
+        return AVERROR(ENOMEM);
+    memcpy(tile->packed_headers + tile->packed_headers_size,
+           s->g.buffer, n - 3);
+    tile->packed_headers_size += n - 3;
+    bytestream2_skip(&s->g, n - 3);
+
+    return 0;
+}
+
 static int init_tile(Jpeg2000DecoderContext *s, int tileno)
 {
     int compno;
@@ -927,6 +962,19 @@  static int getlblockinc(Jpeg2000DecoderContext *s)
     return res;
 }
 
+static inline void select_stream(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
+                                 int *tp_index)
+{
+    s->g = tile->tile_part[*tp_index].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;
+        }
+    }
+    if (bytestream2_peek_be32(&s->g) == JPEG2000_SOP_FIXED_BYTES)
+        bytestream2_skip(&s->g, JPEG2000_SOP_BYTE_LENGTH);
+}
+
 static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile, int *tp_index,
                                   Jpeg2000CodingStyle *codsty,
                                   Jpeg2000ResLevel *rlevel, int precno,
@@ -938,19 +986,15 @@  static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
     if (layno < rlevel->band[0].prec[precno].decoded_layers)
         return 0;
     rlevel->band[0].prec[precno].decoded_layers = layno + 1;
-
-    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;
-        }
-    }
-
-    if (bytestream2_peek_be32(&s->g) == JPEG2000_SOP_FIXED_BYTES)
-        bytestream2_skip(&s->g, JPEG2000_SOP_BYTE_LENGTH);
+    // Select stream to read from
+    if (tile->has_ppt)
+        s->g = tile->packed_headers_stream;
+    else
+        select_stream(s, tile, tp_index);
 
     if (!(ret = get_bits(s, 1))) {
         jpeg2000_flush(s);
-        return 0;
+        goto skip_data;
     } else if (ret < 0)
         return ret;
 
@@ -1056,6 +1100,11 @@  static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
             av_log(s->avctx, AV_LOG_ERROR, "EPH marker not found. instead %X\n", bytestream2_peek_be32(&s->g));
     }
 
+    // Save state of stream
+    if (tile->has_ppt) {
+        tile->packed_headers_stream = s->g;
+        select_stream(s, tile, tp_index);
+    }
     for (bandno = 0; bandno < rlevel->nbands; bandno++) {
         Jpeg2000Band *band = rlevel->band + bandno;
         Jpeg2000Prec *prec = band->prec + precno;
@@ -1097,6 +1146,15 @@  static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
             av_freep(&cblk->lengthinc);
         }
     }
+    // Save state of stream
+    tile->tile_part[*tp_index].tpg = s->g;
+    return 0;
+
+skip_data:
+    if (tile->has_ppt)
+        tile->packed_headers_stream = s->g;
+    else
+        tile->tile_part[*tp_index].tpg = s->g;
     return 0;
 }
 
@@ -1929,6 +1987,11 @@  static int jpeg2000_read_main_headers(Jpeg2000DecoderContext *s)
                 av_log(s->avctx, AV_LOG_ERROR, "Invalid tpend\n");
                 return AVERROR_INVALIDDATA;
             }
+
+            if (tile->has_ppt && tile->tp_idx == 0) {
+                bytestream2_init(&tile->packed_headers_stream, tile->packed_headers, tile->packed_headers_size);
+            }
+
             bytestream2_init(&tp->tpg, s->g.buffer, tp->tp_end - s->g.buffer);
             bytestream2_skip(&s->g, tp->tp_end - s->g.buffer);
 
@@ -1995,6 +2058,10 @@  static int jpeg2000_read_main_headers(Jpeg2000DecoderContext *s)
             // Packet length, tile-part header
             ret = get_plt(s, len);
             break;
+        case JPEG2000_PPT:
+            // Packed headers, tile-part header
+            ret = get_ppt(s, len);
+            break;
         default:
             av_log(s->avctx, AV_LOG_ERROR,
                    "unsupported marker 0x%.4"PRIX16" at pos 0x%X\n",
@@ -2024,7 +2091,6 @@  static int jpeg2000_read_bitstream_packets(Jpeg2000DecoderContext *s)
         if ((ret = init_tile(s, tileno)) < 0)
             return ret;
 
-        s->g = tile->tile_part[0].tpg;
         if ((ret = jpeg2000_decode_packets(s, tile)) < 0)
             return ret;
     }