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

[Tomas Härdin]

ons 2023-04-05 klockan 16:16 +0300 skrev Caleb Etemesi:
> > > +/*
> > > + * 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

Definitely do credit. I wonder if there's other files in the project
with similar licenses and if we do print this stuff.

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

Avoids reading more bits in some cases, potentially speeding up the
decoder. Unless of course there are cases where 33 bits are absolutely
necessary.

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

Great

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

It would make the code smaller and prettier, and there's no repeated
calls to jpeg2000_bitbuf_peek_bits_lsb() that would benefit from only
checking the length outside the function. Were there a loop that needs
to peek multiple times then it could make sense, but there are none.

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

Let's leave it for later then.

/Tomas