diff mbox series

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

Message ID 20200331120242.28648-2-gautamramk@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,v9,1/3] libavcodec/jpeg2000dec.c: Split function for PPT 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 31, 2020, 12:02 p.m. UTC
From: Gautam Ramakrishnan <gautamramk@gmail.com>

This patch adds functional changes to support the
PPT marker.
---
 libavcodec/jpeg2000dec.c | 85 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 76 insertions(+), 9 deletions(-)

Comments

Michael Niedermayer April 1, 2020, 12:46 a.m. UTC | #1
On Tue, Mar 31, 2020 at 05:32:41PM +0530, gautamramk@gmail.com wrote:
> From: Gautam Ramakrishnan <gautamramk@gmail.com>
> 
> This patch adds functional changes to support the
> PPT marker.
> ---
>  libavcodec/jpeg2000dec.c | 85 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 76 insertions(+), 9 deletions(-)

can you explain why the jpeg2000_decode_packet* function is / needs to be
split for this ?
iam asking as it duplicates quite some code in the previous patch

thanks

[...]
Gautam Ramakrishnan April 1, 2020, 2:31 a.m. UTC | #2
On Wed, Apr 1, 2020 at 6:17 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Tue, Mar 31, 2020 at 05:32:41PM +0530, gautamramk@gmail.com wrote:
> > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> >
> > This patch adds functional changes to support the
> > PPT marker.
> > ---
> >  libavcodec/jpeg2000dec.c | 85 +++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 76 insertions(+), 9 deletions(-)
>
> can you explain why the jpeg2000_decode_packet* function is / needs to be
> split for this ?
> iam asking as it duplicates quite some code in the previous patch
>
so, if we can represent packet header as H and data as D,
without a PPT marker, the packet data is represented as
HDHDHDHD.....
However, with the PPT marker, this is changed to
HHHHHHHHHDDDDDDD.....
The jpeg2000_decode_packet() decodes a packet in the HD format.
However, to decode packets in packed form (this change should apply
even if PPM marker support is added), I thought it would be better if we
break the function to decode header and data separately.
> thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If a bugfix only changes things apparently unrelated to the bug with no
> further explanation, that is a good sign that the bugfix is wrong.
> _______________________________________________
> 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 1, 2020, 7:28 p.m. UTC | #3
On Wed, Apr 01, 2020 at 08:01:48AM +0530, Gautam Ramakrishnan wrote:
> On Wed, Apr 1, 2020 at 6:17 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Tue, Mar 31, 2020 at 05:32:41PM +0530, gautamramk@gmail.com wrote:
> > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > >
> > > This patch adds functional changes to support the
> > > PPT marker.
> > > ---
> > >  libavcodec/jpeg2000dec.c | 85 +++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 76 insertions(+), 9 deletions(-)
> >
> > can you explain why the jpeg2000_decode_packet* function is / needs to be
> > split for this ?
> > iam asking as it duplicates quite some code in the previous patch
> >
> so, if we can represent packet header as H and data as D,
> without a PPT marker, the packet data is represented as
> HDHDHDHD.....
> However, with the PPT marker, this is changed to
> HHHHHHHHHDDDDDDD.....
> The jpeg2000_decode_packet() decodes a packet in the HD format.
> However, to decode packets in packed form (this change should apply
> even if PPM marker support is added), I thought it would be better if we
> break the function to decode header and data separately.

Is there any plan/need to call the 2 functions differently than always
first one and immedeatly the 2nd ?
Because if they are always called in order then the calling code would be
duplicated a few times
The data they read isnt one after the other but still they seem to be
called the same way as before ...

Thanks

[...]
Gautam Ramakrishnan April 2, 2020, 2:54 a.m. UTC | #4
On Thu, Apr 2, 2020 at 12:58 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Wed, Apr 01, 2020 at 08:01:48AM +0530, Gautam Ramakrishnan wrote:
> > On Wed, Apr 1, 2020 at 6:17 AM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Tue, Mar 31, 2020 at 05:32:41PM +0530, gautamramk@gmail.com wrote:
> > > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > > >
> > > > This patch adds functional changes to support the
> > > > PPT marker.
> > > > ---
> > > >  libavcodec/jpeg2000dec.c | 85 +++++++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 76 insertions(+), 9 deletions(-)
> > >
> > > can you explain why the jpeg2000_decode_packet* function is / needs to be
> > > split for this ?
> > > iam asking as it duplicates quite some code in the previous patch
> > >
> > so, if we can represent packet header as H and data as D,
> > without a PPT marker, the packet data is represented as
> > HDHDHDHD.....
> > However, with the PPT marker, this is changed to
> > HHHHHHHHHDDDDDDD.....
> > The jpeg2000_decode_packet() decodes a packet in the HD format.
> > However, to decode packets in packed form (this change should apply
> > even if PPM marker support is added), I thought it would be better if we
> > break the function to decode header and data separately.
>
> Is there any plan/need to call the 2 functions differently than always
> first one and immedeatly the 2nd ?
Hmm, I did not think of this. At the moment, I do not think so. Imo, even the
PPM marker addition should not make a difference. I really doubt that the
calling order will change. Does it look like a good idea to merge the function
back?
> Because if they are always called in order then the calling code would be
> duplicated a few times
> The data they read isnt one after the other but still they seem to be
> called the same way as before ...
>
> Thanks
>
> [...]
> --
> 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".
Michael Niedermayer April 2, 2020, 9:34 p.m. UTC | #5
On Thu, Apr 02, 2020 at 08:24:13AM +0530, Gautam Ramakrishnan wrote:
> On Thu, Apr 2, 2020 at 12:58 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Wed, Apr 01, 2020 at 08:01:48AM +0530, Gautam Ramakrishnan wrote:
> > > On Wed, Apr 1, 2020 at 6:17 AM Michael Niedermayer
> > > <michael@niedermayer.cc> wrote:
> > > >
> > > > On Tue, Mar 31, 2020 at 05:32:41PM +0530, gautamramk@gmail.com wrote:
> > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > > > >
> > > > > This patch adds functional changes to support the
> > > > > PPT marker.
> > > > > ---
> > > > >  libavcodec/jpeg2000dec.c | 85 +++++++++++++++++++++++++++++++++++-----
> > > > >  1 file changed, 76 insertions(+), 9 deletions(-)
> > > >
> > > > can you explain why the jpeg2000_decode_packet* function is / needs to be
> > > > split for this ?
> > > > iam asking as it duplicates quite some code in the previous patch
> > > >
> > > so, if we can represent packet header as H and data as D,
> > > without a PPT marker, the packet data is represented as
> > > HDHDHDHD.....
> > > However, with the PPT marker, this is changed to
> > > HHHHHHHHHDDDDDDD.....
> > > The jpeg2000_decode_packet() decodes a packet in the HD format.
> > > However, to decode packets in packed form (this change should apply
> > > even if PPM marker support is added), I thought it would be better if we
> > > break the function to decode header and data separately.
> >
> > Is there any plan/need to call the 2 functions differently than always
> > first one and immedeatly the 2nd ?
> Hmm, I did not think of this. At the moment, I do not think so. Imo, even the
> PPM marker addition should not make a difference. I really doubt that the
> calling order will change. Does it look like a good idea to merge the function
> back?

i think so, it would be less duplicated code

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
index 66361fa90a..23e43ff052 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,39 @@  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
+/*    Zppt = */ bytestream2_get_byte(&s->g); // Zppt is skipped and not used
+    if (!tile->packed_headers) {
+        tile->packed_headers = av_malloc_array(n - 3, 1);
+        memcpy(tile->packed_headers, s->g.buffer, n - 3);
+        tile->packed_headers_size = n - 3;
+    } else {
+        tile->packed_headers = av_realloc_array(tile->packed_headers,
+                                                tile->packed_headers_size + n - 3,
+                                                1);
+        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;
@@ -941,20 +978,24 @@  static int jpeg2000_decode_packet_header(Jpeg2000DecoderContext *s, Jpeg2000Tile
         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 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);
         *process_data = 0;
-        return 0;
+        goto end;
     } else if (ret < 0)
         return ret;
 
@@ -1059,6 +1100,12 @@  static int jpeg2000_decode_packet_header(Jpeg2000DecoderContext *s, Jpeg2000Tile
         else
             av_log(s->avctx, AV_LOG_ERROR, "EPH marker not found. instead %X\n", bytestream2_peek_be32(&s->g));
     }
+end:
+    // 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;
 }
 
@@ -1071,6 +1118,16 @@  static int jpeg2000_decode_packet_data(Jpeg2000DecoderContext *s, Jpeg2000Tile *
     int bandno, cblkno, nb_code_blocks;
     int cwsno;
 
+    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;
@@ -1112,6 +1169,8 @@  static int jpeg2000_decode_packet_data(Jpeg2000DecoderContext *s, Jpeg2000Tile *
             av_freep(&cblk->lengthinc);
         }
     }
+    // Save state of stream
+    tile->tile_part[*tp_index].tpg = s->g;
     return 0;
 }
 
@@ -2011,6 +2070,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);
 
@@ -2073,6 +2137,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",
@@ -2102,7 +2170,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;
     }