[FFmpeg-devel] [PATCH v1 2/3] avcodec/jpeg2000dec: add support for HTJ2K block decoding

[Tomas Härdin]

> -#define JPEG2000_CTSY_HTJ2K_M   0xC0 // HT code blocks (Rec. ITU-T
> T.814 | ISO/IEC 15444-15) can be present
> +#define JPEG2000_CTSY_HTJ2K_M   0xC0 // HT code-blocks (Rec. ITU-T
> T.814 | ISO/IEC 15444-15) can be present

Stray hunk?

>      if (c->cblk_style != 0) { // cblk style
>          if (c->cblk_style & JPEG2000_CTSY_HTJ2K_M || c->cblk_style &
> JPEG2000_CTSY_HTJ2K_F) {
> -            av_log(s->avctx, AV_LOG_ERROR, "Support for High
> throughput JPEG 2000 is not yet available\n");
> -            return AVERROR_PATCHWELCOME;
> +            av_log(s->avctx,AV_LOG_TRACE,"High Throughput jpeg 2000
> codestream.\n");
> +            s->is_htj2k = 1;

Can't files use both HT and part 1 codeblocks?

> -                if ((ret = get_bits(s, av_log2(newpasses1) + cblk-
> >lblock)) < 0)
> -                    return ret;
> +                if (newpasses > 1 && s->is_htj2k) {
> +                    // Retrieve pass lengths for each pass
> +                    int href_passes =  (cblk->npasses + newpasses -
> 1) % 3;
> +                    int segment_passes = newpasses - href_passes;

Looks OK since newpasses >= 2

> +                    int pass_bound = 2;
> +                    int eb = 0;
> +                    int extra_bit = newpasses > 2 ? 1 : 0;
> +                    while (pass_bound <=segment_passes) {
> +                        eb++;
> +                        pass_bound +=pass_bound;
> +                    }

Looks like this could use av_log2()

> +/*
> + * Copyright 2019 - 2021, Osamu Watanabe

Is this based on another codebase?

> + * 2. Redistributions in binary form must reproduce the above
> copyright notice,
> + *    this list of conditions and the following disclaimer in the
> documentation
> + *    and/or other materials provided with the distribution.

Do we do this currently?

> +    if (buffer->bits_left > 32)
> +        return 0; // enough data, no need to pull in more bits

Could this be >= 32?

> +    tmp = AV_RB32(&array[position + 1]);

I presume array points such that reading negative positions is always
OK, even if somehow the forward bitstream is very short

> +/**
> + * Refill the bit-buffer reading new bits going forward
> + * in the stream while skipping over stuffed bits.
> + */
> +static void jpeg2000_bitbuf_refill_forward(StateVars *buffer, const
> uint8_t *array,
> +                                           uint32_t length)
> +{
> +    while (buffer->bits_left < 32) {
> +        buffer->tmp = 0xFF;
> +        buffer->bits = (buffer->last == 0xFF) ? 7 : 8;

I was going to say tmp and bits look like they could be local variables
but I see they're made use of elsewhere in the code.

Also this function looks like it could be sped up considerably, but
let's leave that out of this patchset.

> +/**
> + * Look ahead bit buffer without discarding bits.
> + */
> +av_always_inline
> +static uint64_t jpeg2000_bitbuf_peek_bits_lsb(StateVars *stream,
> uint8_t nbits)
> +{
> +    uint64_t mask = (1ull << nbits) - 1;

What if bits_left < nbits? Can it happen?
Looks like callers ensure this, but checking it would be more in line
with the other bit functions here

For forward reading I feel like we could reuse lavc's bitreader and
just do unstuffing after reading. Or?

/Tomas