| Message ID | 20200825135545.16438-2-gautamramk@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,RFC,v5,1/3] libavcodec/jpeg2000: Make tag tree functions non static | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Tue, Aug 25, 2020 at 07:25:44PM +0530, gautamramk@gmail.com wrote: > From: Gautam Ramakrishnan <gautamramk@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 thx [...]
On Wed, Aug 26, 2020 at 4:04 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Tue, Aug 25, 2020 at 07:25:44PM +0530, gautamramk@gmail.com wrote: > > From: Gautam Ramakrishnan <gautamramk@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. > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Whats the most studid thing your enemy could do ? Blow himself up > Whats the most studid thing you could do ? Give up your rights and > freedom because your enemy blew himself up. > > _______________________________________________ > 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".
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@niedermayer.cc> wrote: > > > > On Tue, Aug 25, 2020 at 07:25:44PM +0530, gautamramk@gmail.com wrote: > > > From: Gautam Ramakrishnan <gautamramk@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. Thanks [...]
On Wed, Aug 26, 2020 at 12:44 PM Michael Niedermayer <michael@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@niedermayer.cc> wrote: > > > > > > On Tue, Aug 25, 2020 at 07:25:44PM +0530, gautamramk@gmail.com wrote: > > > > From: Gautam Ramakrishnan <gautamramk@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? > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > No human being will ever know the Truth, for even if they happen to say it > by chance, they would not even known they had done so. -- Xenophanes > _______________________________________________ > 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".
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@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@niedermayer.cc> wrote: > > > > > > > > On Tue, Aug 25, 2020 at 07:25:44PM +0530, gautamramk@gmail.com wrote: > > > > > From: Gautam Ramakrishnan <gautamramk@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 [...]
diff --git a/libavcodec/j2kenc.c b/libavcodec/j2kenc.c index 16863f8e8c..87acd2d5c9 100644 --- a/libavcodec/j2kenc.c +++ b/libavcodec/j2kenc.c @@ -242,27 +242,36 @@ static void j2k_flush(Jpeg2000EncoderContext *s) static void tag_tree_code(Jpeg2000EncoderContext *s, Jpeg2000TgtNode *node, int threshold) { Jpeg2000TgtNode *stack[30]; - int sp = 1, curval = 0; - stack[0] = node; + int sp = -1, curval = 0; - node = node->parent; - while(node){ - if (node->vis){ - curval = node->val; - break; - } - node->vis++; - stack[sp++] = node; + while(node->parent){ + stack[++sp] = node; node = node->parent; } - while(--sp >= 0){ - if (stack[sp]->val >= threshold){ + + while (1) { + if (curval > node->temp_val) + node->temp_val = curval; + else { + curval = node->temp_val; + } + + if (node->val >= threshold) { put_bits(s, 0, threshold - curval); - break; + curval = threshold; + } else { + put_bits(s, 0, node->val - curval); + curval = node->val; + if (!node->vis) { + put_bits(s, 1, 1); + node->vis = 1; + } } - put_bits(s, 0, stack[sp]->val - curval); - put_bits(s, 1, 1); - curval = stack[sp]->val; + + node->temp_val = curval; + if (sp < 0) + break; + node = stack[sp--]; } } 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); for (cblkno = 0; cblkno < prec->nb_codeblocks_width * prec->nb_codeblocks_height; cblkno++) { Jpeg2000Cblk *cblk = prec->cblk + cblkno; cblk->length = 0; diff --git a/libavcodec/jpeg2000.h b/libavcodec/jpeg2000.h index c3437b02fe..a9f2e01632 100644 --- a/libavcodec/jpeg2000.h +++ b/libavcodec/jpeg2000.h @@ -127,6 +127,7 @@ typedef struct Jpeg2000T1Context { typedef struct Jpeg2000TgtNode { uint8_t val; + uint8_t temp_val; uint8_t vis; struct Jpeg2000TgtNode *parent; } Jpeg2000TgtNode; @@ -291,6 +292,6 @@ static inline int needs_termination(int style, int passno) { } int32_t ff_tag_tree_size(int w, int h); -void ff_tag_tree_zero(Jpeg2000TgtNode *t, int w, int h); +void ff_tag_tree_zero(Jpeg2000TgtNode *t, int w, int h, int val); #endif /* AVCODEC_JPEG2000_H */
From: Gautam Ramakrishnan <gautamramk@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. --- libavcodec/j2kenc.c | 41 +++++++++++++++++++++++++---------------- libavcodec/jpeg2000.c | 9 +++++---- libavcodec/jpeg2000.h | 3 ++- 3 files changed, 32 insertions(+), 21 deletions(-)