diff mbox series

[FFmpeg-devel,v8,1/3] libavcodec/jpeg2000dec.c: Split function and modify structs for PPM marker support

Message ID 20200330162155.6901-1-gautamramk@gmail.com
State New
Headers show
Series [FFmpeg-devel,v8,1/3] libavcodec/jpeg2000dec.c: Split function and modify structs for PPM marker support
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 March 30, 2020, 4:21 p.m. UTC
From: Gautam Ramakrishnan <gautamramk@gmail.com>

---
 libavcodec/jpeg2000dec.c | 48 ++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 7 deletions(-)

Comments

Michael Niedermayer March 31, 2020, 12:40 a.m. UTC | #1
On Mon, Mar 30, 2020 at 09:51:53PM +0530, gautamramk@gmail.com wrote:
> From: Gautam Ramakrishnan <gautamramk@gmail.com>
> 
> ---
>  libavcodec/jpeg2000dec.c | 48 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index 7103cd6ceb..9d52969821 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;
> @@ -935,21 +939,32 @@ static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
>      int bandno, cblkno, ret, nb_code_blocks;
>      int cwsno;
>  
> +// This part decodes the packet header
> +
>      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;
> +    // Select appropriate stream to read from
> +    if (tile->has_ppt) {
> +        s->g = tile->packed_headers_stream;
> +    } else {
> +        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);
>      }
>  
> -    if (bytestream2_peek_be32(&s->g) == JPEG2000_SOP_FIXED_BYTES)
> -        bytestream2_skip(&s->g, JPEG2000_SOP_BYTE_LENGTH);
> -
>      if (!(ret = get_bits(s, 1))) {
>          jpeg2000_flush(s);
> +        // Save state of stream
> +        if (tile->has_ppt)
> +            tile->packed_headers_stream = s->g;
> +        else
> +            tile->tile_part[*tp_index].tpg = s->g;
>          return 0;
>      } else if (ret < 0)
>          return ret;
> @@ -1056,6 +1071,23 @@ 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));
>      }
>  
> +    if (tile->has_ppt)
> +        tile->packed_headers_stream = s->g;
> +    else
> +        tile->tile_part[*tp_index].tpg = s->g;
> +
> +// This part decodes the packet data.
> +    // Select appropriate stream to read from
> +    s->g = tile->tile_part[*tp_index].tpg;
> +    if (tile->has_ppt) {
> +        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);
> +    }
>      for (bandno = 0; bandno < rlevel->nbands; bandno++) {
>          Jpeg2000Band *band = rlevel->band + bandno;
>          Jpeg2000Prec *prec = band->prec + precno;
> @@ -1097,6 +1129,8 @@ 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;
>  }

has_ppt is never set in this also it is specific to the new functional code.
it and all branches which depend on it being 1 should be in the 2nd patch.

The function split should be in the first patch instead

Its probably easiest if you first merge the 2 patches and then remove all
ppt feature specific code which was added. what remains should be the first
patch. the diff from that to the final result would be the 2nd.
At least i think that would bring it closer to what the idea is with this
split. you / we need to take a look at how this looks after doing it.
There may still be obvious things that can be cleaned up ...

thx

[...]
Gautam Ramakrishnan March 31, 2020, 4:31 a.m. UTC | #2
On Tue, Mar 31, 2020 at 6:10 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Mon, Mar 30, 2020 at 09:51:53PM +0530, gautamramk@gmail.com wrote:
> > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> >
> > ---
> >  libavcodec/jpeg2000dec.c | 48 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 41 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > index 7103cd6ceb..9d52969821 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;
> > @@ -935,21 +939,32 @@ static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
> >      int bandno, cblkno, ret, nb_code_blocks;
> >      int cwsno;
> >
> > +// This part decodes the packet header
> > +
> >      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;
> > +    // Select appropriate stream to read from
> > +    if (tile->has_ppt) {
> > +        s->g = tile->packed_headers_stream;
> > +    } else {
> > +        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);
> >      }
> >
> > -    if (bytestream2_peek_be32(&s->g) == JPEG2000_SOP_FIXED_BYTES)
> > -        bytestream2_skip(&s->g, JPEG2000_SOP_BYTE_LENGTH);
> > -
> >      if (!(ret = get_bits(s, 1))) {
> >          jpeg2000_flush(s);
> > +        // Save state of stream
> > +        if (tile->has_ppt)
> > +            tile->packed_headers_stream = s->g;
> > +        else
> > +            tile->tile_part[*tp_index].tpg = s->g;
> >          return 0;
> >      } else if (ret < 0)
> >          return ret;
> > @@ -1056,6 +1071,23 @@ 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));
> >      }
> >
> > +    if (tile->has_ppt)
> > +        tile->packed_headers_stream = s->g;
> > +    else
> > +        tile->tile_part[*tp_index].tpg = s->g;
> > +
> > +// This part decodes the packet data.
> > +    // Select appropriate stream to read from
> > +    s->g = tile->tile_part[*tp_index].tpg;
> > +    if (tile->has_ppt) {
> > +        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);
> > +    }
> >      for (bandno = 0; bandno < rlevel->nbands; bandno++) {
> >          Jpeg2000Band *band = rlevel->band + bandno;
> >          Jpeg2000Prec *prec = band->prec + precno;
> > @@ -1097,6 +1129,8 @@ 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;
> >  }
>
> has_ppt is never set in this also it is specific to the new functional code.
> it and all branches which depend on it being 1 should be in the 2nd patch.
>
> The function split should be in the first patch instead
>
Sure, now its clear. Shall do that

> Its probably easiest if you first merge the 2 patches and then remove all
> ppt feature specific code which was added. what remains should be the first
> patch. the diff from that to the final result would be the 2nd.
> At least i think that would bring it closer to what the idea is with this
> split. you / we need to take a look at how this looks after doing it.
> There may still be obvious things that can be cleaned up ...
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No snowflake in an avalanche ever feels responsible. -- Voltaire
> _______________________________________________
> 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".



--
-------------
Gautam |
diff mbox series

Patch

diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
index 7103cd6ceb..9d52969821 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;
@@ -935,21 +939,32 @@  static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
     int bandno, cblkno, ret, nb_code_blocks;
     int cwsno;
 
+// This part decodes the packet header
+
     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;
+    // Select appropriate stream to read from
+    if (tile->has_ppt) {
+        s->g = tile->packed_headers_stream;
+    } else {
+        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);
     }
 
-    if (bytestream2_peek_be32(&s->g) == JPEG2000_SOP_FIXED_BYTES)
-        bytestream2_skip(&s->g, JPEG2000_SOP_BYTE_LENGTH);
-
     if (!(ret = get_bits(s, 1))) {
         jpeg2000_flush(s);
+        // Save state of stream
+        if (tile->has_ppt)
+            tile->packed_headers_stream = s->g;
+        else
+            tile->tile_part[*tp_index].tpg = s->g;
         return 0;
     } else if (ret < 0)
         return ret;
@@ -1056,6 +1071,23 @@  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));
     }
 
+    if (tile->has_ppt)
+        tile->packed_headers_stream = s->g;
+    else
+        tile->tile_part[*tp_index].tpg = s->g;
+
+// This part decodes the packet data.
+    // Select appropriate stream to read from
+    s->g = tile->tile_part[*tp_index].tpg;
+    if (tile->has_ppt) {
+        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);
+    }
     for (bandno = 0; bandno < rlevel->nbands; bandno++) {
         Jpeg2000Band *band = rlevel->band + bandno;
         Jpeg2000Prec *prec = band->prec + precno;
@@ -1097,6 +1129,8 @@  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;
 }