Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000: Add support for High-Throughput JPEG 2000 (HTJ2K) decoding.

[Caleb Etemesi <etemesicaleb@gmail.com>]

>> Please separate struct renaming into a separate patch

Will do in the next patch

>> Corresponds to µ_start, right?

Not sure what  µ_start here relates to, but this is the zero bit plane
information in *B.10.5*
of the jpeg2000 spec (ITU Rec. T.800 (06/2019))

>> Could also be called Lcup and Lref. But fine I suppose.

Will change in the next patch

>> No time to review this atm, hopefully later though

No problem.


>> Why are these tables in here when they're only used by jpeg2000htdec.c?

I wasn't aware of any table placing conventions and my personal preference
is usually to place them
inside header files.

On Tue, Sep 20, 2022 at 4:49 PM Tomas Härdin <tjoppen@acc.umu.se> wrote:

> tor 2022-09-08 klockan 23:49 +0300 skrev etemesicaleb@gmail.com:
> >
> > --- a/libavcodec/j2kenc.c
> > +++ b/libavcodec/j2kenc.c
> > @@ -106,7 +106,7 @@ static const int dwt_norms[2][4][10] = { //
> > [dwt_type][band][rlevel] (multiplied
> >  typedef struct {
> >     Jpeg2000Component *comp;
> >     double *layer_rates;
> > -} Jpeg2000Tile;
> > +} Jpeg2000EncTile;
> >
> >  typedef struct {
> >      AVClass *class;
> > @@ -131,7 +131,7 @@ typedef struct {
> >      Jpeg2000CodingStyle codsty;
> >      Jpeg2000QuantStyle  qntsty;
> >
> > -    Jpeg2000Tile *tile;
> > +    Jpeg2000EncTile *tile;
>
> Please separate struct renaming into a separate patch
>
> >  typedef struct Jpeg2000Cblk {
> >      uint8_t npasses;
> >      uint8_t ninclpasses; // number coding of passes included in
> > codestream
> > @@ -181,6 +181,7 @@ typedef struct Jpeg2000Cblk {
> >      uint16_t *lengthinc;
> >      uint8_t nb_lengthinc;
> >      uint8_t lblock;
> > +    uint8_t zbp;         // Zero bit planes
>
> Corresponds to µ_start, right?
>
> >      uint8_t *data;
> >      size_t data_allocated;
> >      int nb_terminations;
> > @@ -189,6 +190,7 @@ typedef struct Jpeg2000Cblk {
> >      Jpeg2000Pass *passes;
> >      Jpeg2000Layer *layers;
> >      int coord[2][2]; // border coordinates {{x0, x1}, {y0, y1}}
> > +    int pass_lengths[2];
>
> Could also be called Lcup and Lref. But fine I suppose.
>
> >  } Jpeg2000Cblk; // code block
> >
> >  typedef struct Jpeg2000Prec {
> > @@ -227,6 +229,103 @@ typedef struct Jpeg2000Component {
> >      uint8_t roi_shift; // ROI scaling value for the component
> >  } Jpeg2000Component;
> >
> > +#define JP2_SIG_TYPE    0x6A502020
> > +#define JP2_SIG_VALUE   0x0D0A870A
> > +#define JP2_CODESTREAM  0x6A703263
> > +#define JP2_HEADER      0x6A703268
> > +
> > +#define HAD_COC 0x01
> > +#define HAD_QCC 0x02
> > +
> > +#define MAX_POCS 32
>
> Similarly with code movements. I can see this move adds is_htj2k to
> Jpeg2000DecoderContext. Please separate such things. Makes reviewing
> functional changes much easier.
>
> > -static inline void tile_codeblocks(const Jpeg2000DecoderContext *s,
> > Jpeg2000Tile *tile)
> > +static inline void tile_codeblocks(const Jpeg2000DecoderContext *s,
> > Jpeg2000DecTile *tile)
> >  {
> >      Jpeg2000T1Context t1;
> >
> >      int compno, reslevelno, bandno;
> > +    int subbandno;
> >
> >      /* Loop on tile components */
> >      for (compno = 0; compno < s->ncomponents; compno++) {
> > -        Jpeg2000Component *comp     = tile->comp + compno;
> > -        Jpeg2000CodingStyle *codsty = tile->codsty + compno;
> > +        Jpeg2000Component *comp        = tile->comp + compno;
> > +        Jpeg2000CodingStyle *codsty    = tile->codsty + compno;
> > +        Jpeg2000QuantStyle *quantsty   = tile->qntsty + compno;
>
> Cosmetic and functional changes don't mix
>
> >          int coded = 0;
> > +        subbandno = 0;
> >
> >          t1.stride = (1<<codsty->log2_cblk_width) + 2;
> >
> > @@ -1959,7 +1881,7 @@ static inline void tile_codeblocks(const
> > Jpeg2000DecoderContext *s, Jpeg2000Tile
> >          for (reslevelno = 0; reslevelno < codsty->nreslevels2decode;
> > reslevelno++) {
> >              Jpeg2000ResLevel *rlevel = comp->reslevel + reslevelno;
> >              /* Loop on bands */
> > -            for (bandno = 0; bandno < rlevel->nbands; bandno++) {
> > +            for (bandno = 0; bandno < rlevel->nbands;
> > bandno++,subbandno++) {
> >                  int nb_precincts, precno;
> >                  Jpeg2000Band *band = rlevel->band + bandno;
> >                  int cblkno = 0, bandpos;
> > @@ -1979,12 +1901,21 @@ static inline void tile_codeblocks(const
> > Jpeg2000DecoderContext *s, Jpeg2000Tile
> >                      for (cblkno = 0;
> >                           cblkno < prec->nb_codeblocks_width * prec-
> > >nb_codeblocks_height;
> >                           cblkno++) {
> > -                        int x, y;
> > +                        int x, y, ret, magp;
> >                          Jpeg2000Cblk *cblk = prec->cblk + cblkno;
> > -                        int ret = decode_cblk(s, codsty, &t1, cblk,
> > -                                    cblk->coord[0][1] - cblk-
> > >coord[0][0],
> > -                                    cblk->coord[1][1] - cblk-
> > >coord[1][0],
> > -                                    bandpos, comp->roi_shift);
> > +                        // Annex E (Equation E-2) ISO/IEC 15444-
> > 1:2019
> > +                        magp = quantsty->expn[subbandno] + quantsty-
> > >nguardbits - 1;
>
> Is expn[] allocated for regular j2k? I'd move this inside the is_htj2k
> block below
>
> > +
> > +                        if (s->is_htj2k)
> > +                            ret = decode_htj2k(s, codsty, &t1, cblk,
> > +                                               cblk->coord[0][1] -
> > cblk->coord[0][0],
> > +                                               cblk->coord[1][1] -
> > cblk->coord[1][0],
> > +                                               magp, comp-
> > >roi_shift);
>
> > --- /dev/null
> > +++ b/libavcodec/jpeg2000htdec.c
>
> No time to review this atm, hopefully later though
>
> > --- /dev/null
> > +++ b/libavcodec/jpeg2000htdec.h
> >
> > +/**
> > + * @brief  CtxVLC tables, borrowed from openhtj2k (
> > https://github.com/osamu620/OpenHTJ2K) (credits to Osamu Watanabe)
> > + */
> > +static const uint16_t dec_CxtVLC_table1[1024] = {
> > +static const uint16_t dec_CxtVLC_table0[1024] = {
>
> Why are these tables in here when they're only used by jpeg2000htdec.c?
>
> /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".