diff mbox series

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

Message ID 20200825042007.18535-2-gautamramk@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,RFC,v4,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. 25, 2020, 4:20 a.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, 27 insertions(+), 16 deletions(-)

Comments

Michael Niedermayer Aug. 25, 2020, 7:48 a.m. UTC | #1
On Tue, Aug 25, 2020 at 09:50:04AM +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, 27 insertions(+), 16 deletions(-)

iam not sure the tag tree is working correctly before or after this

For example if i make this chnage:

--- a/libavcodec/j2kenc.c
+++ b/libavcodec/j2kenc.c
@@ -282,6 +282,7 @@ static void tag_tree_update(Jpeg2000TgtNode *node)
     while (node->parent){
         if (node->parent->val <= node->val)
             break;
+        abort();
         node->parent->val = node->val;
         node = node->parent;
         lev++;

all tests still pass, so iam not sure what sets the parent values or if
they are not if the code working with them is fully tested

thx

[...]
Gautam Ramakrishnan Aug. 25, 2020, 11:26 a.m. UTC | #2
On Tue, Aug 25, 2020 at 1:18 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Tue, Aug 25, 2020 at 09:50:04AM +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, 27 insertions(+), 16 deletions(-)
>
> iam not sure the tag tree is working correctly before or after this
>
> For example if i make this chnage:
>
> --- a/libavcodec/j2kenc.c
> +++ b/libavcodec/j2kenc.c
> @@ -282,6 +282,7 @@ static void tag_tree_update(Jpeg2000TgtNode *node)
>      while (node->parent){
>          if (node->parent->val <= node->val)
>              break;
> +        abort();
>          node->parent->val = node->val;
>          node = node->parent;
>          lev++;
>
> all tests still pass, so iam not sure what sets the parent values or if
> they are not if the code working with them is fully tested
Looks like the tag tree initializer is incorrect. This would not cause an
undecodable file, but however, leads to an extremely inefficient encoding
of the tag tree. I shall send a patch for this also.
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Observe your enemies, for they first find out your faults. -- Antisthenes
> _______________________________________________
> 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..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..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;