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

[Caleb Etemesi <etemesicaleb@gmail.com>]

On Wed, Apr 5, 2023 at 3:34 PM Tomas Härdin <git@haerdin.se> wrote:

>
>
> >      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?

Yes, will update patch to allow for this

>
>
> -                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()
> Will change
>


> > +/*
> > + * 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?


I'm not sure, haven't seen any part that directly uses someone else's work,
the tables
were generated for his project so I thought it was sensible to credit him

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

Any reason for that?


> > +    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
>
Yes ,  we can only read three bytes backwards, and we always ensure we have
more than three bytes in the buffer,we check for length(Lcup) being greater
than 2 in ff_jpeg2000_decode_htj2k


> > +/**
> > + * 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.
>
Yes.

> > +/**
> > + * 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
>
So should we remove checks from callers and place them inside the function
or ?


> For forward reading I feel like we could reuse lavc's bitreader and
> just do unstuffing after reading. Or?
>
Plausible, but may be slower.


> /Tomas
>
> _______________________________________________
> 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".
>
_______________________________________________
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".