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

[Pierre-Anthony Lemieux <pal@sandflow.com>]

On Wed, Apr 5, 2023 at 6:45 AM Tomas Härdin <git@haerdin.se> wrote:
>
> 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.

There are several other files where some of the content originated
from BSD-licensed code, and thus include the BSD license.

>
> >
> > > +    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
>
> _______________________________________________
> 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".