| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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 [...]
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".
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 [...]
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".
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 --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; }
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(-)