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

[Moritz Barsnick]

On Mon, Mar 30, 2020 at 10:36:31 +0530, gautamramk at gmail.com wrote:
> +    tile->has_ppt = 1;  // this tile has a ppt marker
> +/*    Zppt = */ bytestream2_get_byte(&s->g);

For readability, please place the explanation of what has been ignored
behind the statement. (This line looks commented out when not looking
closely enough.)

> +        tile->packed_headers = av_malloc_array(n - 3, sizeof(uint8_t));
> +        memcpy(tile->packed_headers, s->g.buffer, sizeof(uint8_t)*(n - 3));

sizeof(uint8_t) is always 1. If any of the types (src, dst) are subject
to change, please reference sizeof(variable) instead.

(Also other places in the patchset.)

> @@ -1088,6 +1128,8 @@ static int jpeg2000_decode_packet(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile,
>          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;

Unneccessary change.

> -    // Save state of stream
> +    // Save stream state

Didn't you introduce this comment in the previous patch? No use in
fixing something in a followup patch, just make it correct in the first
one. (Other comments as well, AFAICT.)

> +                            if ((ret = jpeg2000_decode_packet_header(s, tile, tp_index,
> +                                                              codsty, rlevel,
> +                                                              precno, layno,
> +                                                              qntsty->expn + (reslevelno ? 3 * (reslevelno - 1) + 1 : 0),
> +                                                              qntsty->nguardbits, &process_data)) < 0)

Wrong indentation (but we're moving really far to the right now ;->).
(Other parts as well.)

> +                            if ((ret = jpeg2000_decode_packet_data(s, tile, tp_index,
>                                                                codsty, rlevel,
>                                                                precno, layno,
>                                                                qntsty->expn + (reslevelno ? 3 * (reslevelno - 1) + 1 : 0),
>                                                                qntsty->nguardbits)) < 0)

Ditto, but this is part of the original, so the indentation can be
fixed in a subsequent patch.

> @@ -1419,6 +1520,7 @@ static int jpeg2000_decode_packets(Jpeg2000DecoderContext *s, Jpeg2000Tile *tile
>      int tp_index = 0;
>
>      s->bit_index = 8;
> +
>      if (tile->poc.nb_poc) {
>          for (i=0; i<tile->poc.nb_poc; i++) {
>              Jpeg2000POCEntry *e = &tile->poc.poc[i];

Unneccessary change.

Cheers,
Moritz