[FFmpeg-devel] [RFC PATCH v5 2/3] libavcodec/j2kenc: Fix tag tree coding

[Michael Niedermayer]

On Wed, Aug 26, 2020 at 03:04:57PM +0530, Gautam Ramakrishnan wrote:
> On Wed, Aug 26, 2020 at 12:44 PM Michael Niedermayer
> <michael at niedermayer.cc> wrote:
> >
> > On Wed, Aug 26, 2020 at 08:38:30AM +0530, Gautam Ramakrishnan wrote:
> > > On Wed, Aug 26, 2020 at 4:04 AM Michael Niedermayer
> > > <michael at niedermayer.cc> wrote:
> > > >
> > > > On Tue, Aug 25, 2020 at 07:25:44PM +0530, gautamramk at gmail.com wrote:
> > > > > From: Gautam Ramakrishnan <gautamramk at gmail.com>
> > > > >
> > > > > The implementation of tag tree encoding was incorrect.
> > > > > However, this error was not visible as the current j2k
> > > > > encoder encodes only 1 layer.
> > > > > This patch fixes tag tree coding for JPEG2000 such tag
> > > > > tree coding would work for multi layer encoding.
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c
> > > > > index 26e09fbe38..2e26bc5b00 100644
> > > > > --- a/libavcodec/jpeg2000.c
> > > > > +++ b/libavcodec/jpeg2000.c
> > > > > @@ -82,12 +82,13 @@ static Jpeg2000TgtNode *ff_jpeg2000_tag_tree_init(int w, int h)
> > > > >      return res;
> > > > >  }
> > > > >
> > > > > -void ff_tag_tree_zero(Jpeg2000TgtNode *t, int w, int h)
> > > > > +void ff_tag_tree_zero(Jpeg2000TgtNode *t, int w, int h, int val)
> > > > >  {
> > > > >      int i, siz = ff_tag_tree_size(w, h);
> > > > >
> > > > >      for (i = 0; i < siz; i++) {
> > > > > -        t[i].val = 0;
> > > > > +        t[i].val = val;
> > > > > +        t[i].temp_val = 0;
> > > > >          t[i].vis = 0;
> > > > >      }
> > > > >  }
> > > > > @@ -567,8 +568,8 @@ void ff_jpeg2000_reinit(Jpeg2000Component *comp, Jpeg2000CodingStyle *codsty)
> > > > >              Jpeg2000Band *band = rlevel->band + bandno;
> > > > >              for(precno = 0; precno < rlevel->num_precincts_x * rlevel->num_precincts_y; precno++) {
> > > > >                  Jpeg2000Prec *prec = band->prec + precno;
> > > > > -                ff_tag_tree_zero(prec->zerobits, prec->nb_codeblocks_width, prec->nb_codeblocks_height);
> > > > > -                ff_tag_tree_zero(prec->cblkincl, prec->nb_codeblocks_width, prec->nb_codeblocks_height);
> > > > > +                ff_tag_tree_zero(prec->zerobits, prec->nb_codeblocks_width, prec->nb_codeblocks_height, 0);
> > > > > +                ff_tag_tree_zero(prec->cblkincl, prec->nb_codeblocks_width, prec->nb_codeblocks_height, 0);
> > > >
> > > > this looks a bit like a somewhat unlrelated bugfix thats spread over this and
> > > > the next patch
> > > > if so, that should be moved into a seperate patch
> > > > the patches are already complex without an additional bugfix in them
> > > >
> > > This portion is a fix for what you pointed out yesterday saying that
> > > the parent of a the
> > > nodes are not getting set. I thought this would be an appropriate
> > > patch to fix this. Do you
> > > feel its better that this part is made separate though?
> >
> > > In my opinion, only this patch provides the bug fix and the next patch
> > > just uses the fix.
> > > Do correct me if I am wrong.
> >
> > IIUC the problem is that the tag trees are inefficient, so a bugfix would
> > change the output files and make them bit smaller.
> > the 1/3 & 2/3 patches do not change the output of any fate tests so they
> > do not seem to fix this on their own. The 3/3 patch adds multi layer stuff
> > so the bugfix seemed intermingled with that, which is what i thought was
> > not optimal. Its not the end of the world if these are mixed but it should
> > be for a very good reason then. For example if seperating the fix would be
> > a unreasonable amount of work and make the changes alot more complex ...
> > But in 99% of the cases bugfixes really should be seperate it makes
> > understanding chnages, testing changes and also things like future regression
> > testing easier.
> 
> So in that case, do I make this part a 4th patch in the series which fixes this
> inefficiency?

if thats easier then yes, otherwise i would do the bugfixes before new 
features

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200826/554d230b/attachment.sig>