diff mbox series

[FFmpeg-devel,RFC,v3,2/5] libavcodec/j2kenc: Fix tag tree coding

Message ID 20200824174034.17837-2-gautamramk@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,RFC,v3,1/5] libavcodec/jpeg2000: Make tag tree functions non static
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Gautam Ramakrishnan Aug. 24, 2020, 5:40 p.m. UTC
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 |  1 +
 libavcodec/jpeg2000.h |  1 +
 3 files changed, 26 insertions(+), 17 deletions(-)

Comments

Michael Niedermayer Aug. 24, 2020, 10:38 p.m. UTC | #1
On Mon, Aug 24, 2020 at 11:10:31PM +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.
> ---
>  libavcodec/j2kenc.c   | 41 ++++++++++++++++++++++++-----------------
>  libavcodec/jpeg2000.c |  1 +
>  libavcodec/jpeg2000.h |  1 +
>  3 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/libavcodec/j2kenc.c b/libavcodec/j2kenc.c
> index 16863f8e8c..d9777fe07f 100644
> --- a/libavcodec/j2kenc.c
> +++ b/libavcodec/j2kenc.c
> @@ -242,27 +242,34 @@ 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){
> -            put_bits(s, 0, threshold - curval);
> -            break;
> +
> +    while (1) {
> +        if (curval > node->temp_val)
> +            node->temp_val = curval;
> +        else {
> +            curval = node->temp_val;
>          }
> -        put_bits(s, 0, stack[sp]->val - curval);
> -        put_bits(s, 1, 1);
> -        curval = stack[sp]->val;
> +        while (curval < threshold) {
> +            if (curval >= node->val) {
> +                if (!node->vis) {
> +                    node->vis = 1;
> +                    put_bits(s, 1, 1);
> +                }
> +                break;
> +            }
> +            put_bits(s, 0, 1);
> +            curval++;
> +        }

why is the put_bits(s, 0, stack[sp]->val - curval);
changed into 1 bit at a time calls ?
a single call should still work with the modified loop
a single call may be faster ...

thx

[...]
Gautam Ramakrishnan Aug. 25, 2020, 3:37 a.m. UTC | #2
On Tue, Aug 25, 2020 at 4:08 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Mon, Aug 24, 2020 at 11:10:31PM +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.
> > ---
> >  libavcodec/j2kenc.c   | 41 ++++++++++++++++++++++++-----------------
> >  libavcodec/jpeg2000.c |  1 +
> >  libavcodec/jpeg2000.h |  1 +
> >  3 files changed, 26 insertions(+), 17 deletions(-)
> >
> > diff --git a/libavcodec/j2kenc.c b/libavcodec/j2kenc.c
> > index 16863f8e8c..d9777fe07f 100644
> > --- a/libavcodec/j2kenc.c
> > +++ b/libavcodec/j2kenc.c
> > @@ -242,27 +242,34 @@ 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){
> > -            put_bits(s, 0, threshold - curval);
> > -            break;
> > +
> > +    while (1) {
> > +        if (curval > node->temp_val)
> > +            node->temp_val = curval;
> > +        else {
> > +            curval = node->temp_val;
> >          }
> > -        put_bits(s, 0, stack[sp]->val - curval);
> > -        put_bits(s, 1, 1);
> > -        curval = stack[sp]->val;
> > +        while (curval < threshold) {
> > +            if (curval >= node->val) {
> > +                if (!node->vis) {
> > +                    node->vis = 1;
> > +                    put_bits(s, 1, 1);
> > +                }
> > +                break;
> > +            }
> > +            put_bits(s, 0, 1);
> > +            curval++;
> > +        }
>
> why is the put_bits(s, 0, stack[sp]->val - curval);
> changed into 1 bit at a time calls ?
> a single call should still work with the modified loop
> a single call may be faster ...
My bad, I oversimplified the implementation as it was hard to debug.
Now since I am sure its correct, i'll remove the loop.
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Dictatorship naturally arises out of democracy, and the most aggravated
> form of tyranny and slavery out of the most extreme liberty. -- Plato
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavcodec/j2kenc.c b/libavcodec/j2kenc.c
index 16863f8e8c..d9777fe07f 100644
--- a/libavcodec/j2kenc.c
+++ b/libavcodec/j2kenc.c
@@ -242,27 +242,34 @@  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){
-            put_bits(s, 0, threshold - curval);
-            break;
+
+    while (1) {
+        if (curval > node->temp_val)
+            node->temp_val = curval;
+        else {
+            curval = node->temp_val;
         }
-        put_bits(s, 0, stack[sp]->val - curval);
-        put_bits(s, 1, 1);
-        curval = stack[sp]->val;
+        while (curval < threshold) {
+            if (curval >= node->val) {
+                if (!node->vis) {
+                    node->vis = 1;
+                    put_bits(s, 1, 1);
+                }
+                break;
+            }
+            put_bits(s, 0, 1);
+            curval++;
+        }
+        node->temp_val = curval;
+        if (sp < 0)
+            break;
+        node = stack[sp--];
     }
 }
 
diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c
index 26e09fbe38..3d3e7ec313 100644
--- a/libavcodec/jpeg2000.c
+++ b/libavcodec/jpeg2000.c
@@ -88,6 +88,7 @@  void ff_tag_tree_zero(Jpeg2000TgtNode *t, int w, int h)
 
     for (i = 0; i < siz; i++) {
         t[i].val = 0;
+        t[i].temp_val = 0;
         t[i].vis = 0;
     }
 }
diff --git a/libavcodec/jpeg2000.h b/libavcodec/jpeg2000.h
index c3437b02fe..ad58b1ae88 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;