T1: fix BYPASS/LAZY, TERMALL/RESTART and PTERM/ERTERM encoding modes. (#674)

There were a number of defects regarding when and how the termination of
passes had to done and the computation of their rate.

src/lib/openjp2/mqc.c

@@ -182,13 +182,10 @@ static opj_mqc_state_t mqc_states[47 * 2] = {
 
 static void opj_mqc_byteout(opj_mqc_t *mqc)
 {
-    /* avoid accessing uninitialized memory*/
+    /* bp is initialized to start - 1 in opj_mqc_init_enc() */
-    if (mqc->bp == mqc->start - 1) {
+    /* but this is safe, see opj_tcd_code_block_enc_allocate_data() */
-        mqc->bp++;
+    assert(mqc->bp >= mqc->start - 1);
-        *mqc->bp = (OPJ_BYTE)(mqc->c >> 19);
+    if (*mqc->bp == 0xff) {
-        mqc->c &= 0x7ffff;
-        mqc->ct = 8;
-    } else if (*mqc->bp == 0xff) {
         mqc->bp++;
         *mqc->bp = (OPJ_BYTE)(mqc->c >> 20);
         mqc->c &= 0xfffff;
@@ -296,12 +293,23 @@ OPJ_UINT32 opj_mqc_numbytes(opj_mqc_t *mqc)
 
 void opj_mqc_init_enc(opj_mqc_t *mqc, OPJ_BYTE *bp)
 {
-    /* TODO MSD: need to take a look to the v2 version */
+    /* To avoid the curctx pointer to be dangling, but not strictly */
+    /* required as the current context is always set before encoding */
     opj_mqc_setcurctx(mqc, 0);
+
+    /* As specified in Figure C.10 - Initialization of the encoder */
+    /* (C.2.8 Initialization of the encoder (INITENC)) */
     mqc->a = 0x8000;
     mqc->c = 0;
+    /* Yes, we point before the start of the buffer, but this is safe */
+    /* given opj_tcd_code_block_enc_allocate_data() */
     mqc->bp = bp - 1;
     mqc->ct = 12;
+    /* At this point we should test *(mqc->bp) against 0xFF, but this is not */
+    /* necessary, as this is only used at the beginning of the code block */
+    /* and our initial fake byte is set at 0 */
+    assert(*(mqc->bp) != 0xff);
+
     mqc->start = bp;
 }
 
@@ -316,60 +324,102 @@ void opj_mqc_encode(opj_mqc_t *mqc, OPJ_UINT32 d)
 
 void opj_mqc_flush(opj_mqc_t *mqc)
 {
+    /* C.2.9 Termination of coding (FLUSH) */
+    /* Figure C.11 – FLUSH procedure */
     opj_mqc_setbits(mqc);
     mqc->c <<= mqc->ct;
     opj_mqc_byteout(mqc);
     mqc->c <<= mqc->ct;
     opj_mqc_byteout(mqc);
 
+    /* It is forbidden that a coding pass ends with 0xff */
     if (*mqc->bp != 0xff) {
+        /* Advance pointer so that opj_mqc_numbytes() returns a valid value */
         mqc->bp++;
     }
 }
 
+#define BYPASS_CT_INIT  0xDEADBEEF
+
 void opj_mqc_bypass_init_enc(opj_mqc_t *mqc)
 {
+    /* This function is normally called after at least one opj_mqc_flush() */
+    /* which will have advance mqc->bp by at least 2 bytes beyond its */
+    /* initial position */
+    assert(mqc->bp >= mqc->start);
     mqc->c = 0;
-    mqc->ct = 8;
+    /* in theory we should initialize to 8, but use this special value */
-    /*if (*mqc->bp == 0xff) {
+    /* as a hint that opj_mqc_bypass_enc() has never been called, so */
-    mqc->ct = 7;
+    /* as to avoid the 0xff 0x7f elimination trick in opj_mqc_bypass_flush_enc() */
-     } */
+    /* to trigger when we don't have output any bit during this bypass sequence */
+    /* Any value > 8 will do */
+    mqc->ct = BYPASS_CT_INIT;
+    /* Given that we are called after opj_mqc_flush(), the previous byte */
+    /* cannot be 0xff. */
+    assert(mqc->bp[-1] != 0xff);
 }
 
 void opj_mqc_bypass_enc(opj_mqc_t *mqc, OPJ_UINT32 d)
 {
+    if (mqc->ct == BYPASS_CT_INIT) {
+        mqc->ct = 8;
+    }
     mqc->ct--;
     mqc->c = mqc->c + (d << mqc->ct);
     if (mqc->ct == 0) {
-        mqc->bp++;
         *mqc->bp = (OPJ_BYTE)mqc->c;
         mqc->ct = 8;
+        /* If the previous byte was 0xff, make sure that the next msb is 0 */
         if (*mqc->bp == 0xff) {
             mqc->ct = 7;
         }
+        mqc->bp++;
         mqc->c = 0;
     }
 }
 
-OPJ_UINT32 opj_mqc_bypass_flush_enc(opj_mqc_t *mqc)
+OPJ_UINT32 opj_mqc_bypass_get_extra_bytes(opj_mqc_t *mqc, OPJ_BOOL erterm)
 {
-    OPJ_BYTE bit_padding;
+    return (mqc->ct < 7 ||
+            (mqc->ct == 7 && (erterm || mqc->bp[-1] != 0xff))) ? 1 : 0;
+}
 
-    bit_padding = 0;
+void opj_mqc_bypass_flush_enc(opj_mqc_t *mqc, OPJ_BOOL erterm)
-
+{
-    if (mqc->ct != 0) {
+    /* Is there any bit remaining to be flushed ? */
+    /* If the last output byte is 0xff, we can discard it, unless */
+    /* erterm is required (I'm not completely sure why in erterm */
+    /* we must output 0xff 0x2a if the last byte was 0xff instead of */
+    /* discarding it, but Kakadu requires it when decoding */
+    /* in -fussy mode) */
+    if (mqc->ct < 7 || (mqc->ct == 7 && (erterm || mqc->bp[-1] != 0xff))) {
+        OPJ_BYTE bit_value = 0;
+        /* If so, fill the remaining lsbs with an alternating sequence of */
+        /* 0,1,... */
+        /* Note: it seems the standard only requires that for a ERTERM flush */
+        /* and doesn't specify what to do for a regular BYPASS flush */
         while (mqc->ct > 0) {
             mqc->ct--;
-            mqc->c += (OPJ_UINT32)(bit_padding << mqc->ct);
+            mqc->c += (OPJ_UINT32)(bit_value << mqc->ct);
-            bit_padding = (bit_padding + 1) & 0x01;
+            bit_value = (OPJ_BYTE)(1U - bit_value);
         }
-        mqc->bp++;
         *mqc->bp = (OPJ_BYTE)mqc->c;
-        mqc->ct = 8;
+        /* Advance pointer so that opj_mqc_numbytes() returns a valid value */
-        mqc->c = 0;
+        mqc->bp++;
+    } else if (mqc->ct == 7 && mqc->bp[-1] == 0xff) {
+        /* Discard last 0xff */
+        assert(!erterm);
+        mqc->bp --;
+    } else if (mqc->ct == 8 && !erterm &&
+               mqc->bp[-1] == 0x7f && mqc->bp[-2] == 0xff) {
+        /* Tiny optimization: discard terminating 0xff 0x7f since it is */
+        /* interpreted as 0xff 0x7f [0xff 0xff] by the decoder, and given */
+        /* the bit stuffing, in fact as 0xff 0xff [0xff ..] */
+        /* Happens once on opj_compress -i ../MAPA.tif -o MAPA.j2k  -M 1 */
+        mqc->bp -= 2;
     }
 
-    return 1;
+    assert(mqc->bp[-1] != 0xff);
 }
 
 void opj_mqc_reset_enc(opj_mqc_t *mqc)
@@ -380,6 +430,7 @@ void opj_mqc_reset_enc(opj_mqc_t *mqc)
     opj_mqc_setstate(mqc, T1_CTXNO_ZC, 0, 4);
 }
 
+#ifdef notdef
 OPJ_UINT32 opj_mqc_restart_enc(opj_mqc_t *mqc)
 {
     OPJ_UINT32 correction = 1;
@@ -396,15 +447,23 @@ OPJ_UINT32 opj_mqc_restart_enc(opj_mqc_t *mqc)
 
     return correction;
 }
+#endif
 
 void opj_mqc_restart_init_enc(opj_mqc_t *mqc)
 {
     /* <Re-init part> */
-    opj_mqc_setcurctx(mqc, 0);
+
+    /* As specified in Figure C.10 - Initialization of the encoder */
+    /* (C.2.8 Initialization of the encoder (INITENC)) */
     mqc->a = 0x8000;
     mqc->c = 0;
     mqc->ct = 12;
-    mqc->bp--;
+    /* This function is normally called after at least one opj_mqc_flush() */
+    /* which will have advance mqc->bp by at least 2 bytes beyond its */
+    /* initial position */
+    mqc->bp --;
+    assert(mqc->bp >= mqc->start - 1);
+    assert(*mqc->bp != 0xff);
     if (*mqc->bp == 0xff) {
         mqc->ct = 13;
     }

src/lib/openjp2/mqc.h

@@ -142,36 +142,45 @@ void opj_mqc_flush(opj_mqc_t *mqc);
 /**
 BYPASS mode switch, initialization operation.
 JPEG 2000 p 505.
-<h2>Not fully implemented and tested !!</h2>
 @param mqc MQC handle
 */
 void opj_mqc_bypass_init_enc(opj_mqc_t *mqc);
+
+/** Return number of extra bytes to add to opj_mqc_numbytes() for the²
+    size of a non-terminating BYPASS pass
+@param mqc MQC handle
+@param erterm 1 if ERTERM is enabled, 0 otherwise
+*/
+OPJ_UINT32 opj_mqc_bypass_get_extra_bytes(opj_mqc_t *mqc, OPJ_BOOL erterm);
+
 /**
 BYPASS mode switch, coding operation.
 JPEG 2000 p 505.
-<h2>Not fully implemented and tested !!</h2>
 @param mqc MQC handle
 @param d The symbol to be encoded (0 or 1)
 */
 void opj_mqc_bypass_enc(opj_mqc_t *mqc, OPJ_UINT32 d);
 /**
 BYPASS mode switch, flush operation
-<h2>Not fully implemented and tested !!</h2>
 @param mqc MQC handle
-@return Returns 1 (always)
+@param erterm 1 if ERTERM is enabled, 0 otherwise
 */
-OPJ_UINT32 opj_mqc_bypass_flush_enc(opj_mqc_t *mqc);
+void opj_mqc_bypass_flush_enc(opj_mqc_t *mqc, OPJ_BOOL erterm);
 /**
 RESET mode switch
 @param mqc MQC handle
 */
 void opj_mqc_reset_enc(opj_mqc_t *mqc);
+
+#ifdef notdef
 /**
 RESTART mode switch (TERMALL)
 @param mqc MQC handle
 @return Returns 1 (always)
 */
 OPJ_UINT32 opj_mqc_restart_enc(opj_mqc_t *mqc);
+#endif
+
 /**
 RESTART mode switch (TERMALL) reinitialisation
 @param mqc MQC handle

src/lib/openjp2/t1.c

@@ -2089,6 +2089,37 @@ OPJ_BOOL opj_t1_encode_cblks(opj_t1_t *t1,
     return OPJ_TRUE;
 }
 
+/* Returns whether the pass (bpno, passtype) is terminated */
+static int opj_t1_enc_is_term_pass(opj_tcd_cblk_enc_t* cblk,
+                                   OPJ_UINT32 cblksty,
+                                   OPJ_INT32 bpno,
+                                   OPJ_UINT32 passtype)
+{
+    /* Is it the last cleanup pass ? */
+    if (passtype == 2 && bpno == 0) {
+        return OPJ_TRUE;
+    }
+
+    if (cblksty & J2K_CCP_CBLKSTY_TERMALL) {
+        return OPJ_TRUE;
+    }
+
+    if ((cblksty & J2K_CCP_CBLKSTY_LAZY)) {
+        /* For bypass arithmetic bypass, terminate the 4th cleanup pass */
+        if ((bpno == ((OPJ_INT32)cblk->numbps - 4)) && (passtype == 2)) {
+            return OPJ_TRUE;
+        }
+        /* and beyond terminate all the magnitude refinement passes (in raw) */
+        /* and cleanup passes (in MQC) */
+        if ((bpno < ((OPJ_INT32)(cblk->numbps) - 4)) && (passtype > 0)) {
+            return OPJ_TRUE;
+        }
+    }
+
+    return OPJ_FALSE;
+}
+
+
 /** mod fixed_quality */
 static void opj_t1_encode_cblk(opj_t1_t *t1,
                                opj_tcd_cblk_enc_t* cblk,
@@ -2116,6 +2147,11 @@ static void opj_t1_encode_cblk(opj_t1_t *t1,
     OPJ_BYTE type = T1_TYPE_MQ;
     OPJ_FLOAT64 tempwmsedec;
 
+#ifdef EXTRA_DEBUG
+    printf("encode_cblk(x=%d,y=%d,x1=%d,y1=%d,orient=%d,compno=%d,level=%d\n",
+           cblk->x0, cblk->y0, cblk->x1, cblk->y1, orient, compno, level);
+#endif
+
     mqc->lut_ctxno_zc_orient = lut_ctxno_zc + orient * 256;
 
     max = 0;
@@ -2140,10 +2176,18 @@ static void opj_t1_encode_cblk(opj_t1_t *t1,
 
     for (passno = 0; bpno >= 0; ++passno) {
         opj_tcd_pass_t *pass = &cblk->passes[passno];
-        OPJ_UINT32 correction = 3;
         type = ((bpno < ((OPJ_INT32)(cblk->numbps) - 4)) && (passtype < 2) &&
                 (cblksty & J2K_CCP_CBLKSTY_LAZY)) ? T1_TYPE_RAW : T1_TYPE_MQ;
 
+        /* If the previous pass was terminating, we need to reset the encoder */
+        if (passno > 0 && cblk->passes[passno - 1].term) {
+            if (type == T1_TYPE_RAW) {
+                opj_mqc_bypass_init_enc(mqc);
+            } else {
+                opj_mqc_restart_init_enc(mqc);
+            }
+        }
+
         switch (passtype) {
         case 0:
             opj_t1_enc_sigpass(t1, bpno, &nmsedec, type, cblksty);
@@ -2165,35 +2209,32 @@ static void opj_t1_encode_cblk(opj_t1_t *t1,
                                         stepsize, numcomps, mct_norms, mct_numcomps) ;
         cumwmsedec += tempwmsedec;
         tile->distotile += tempwmsedec;
+        pass->distortiondec = cumwmsedec;
 
-        /* Code switch "RESTART" (i.e. TERMALL) */
+        if (opj_t1_enc_is_term_pass(cblk, cblksty, bpno, passtype)) {
-        if ((cblksty & J2K_CCP_CBLKSTY_TERMALL) && !((passtype == 2) &&
+            /* If it is a terminated pass, terminate it */
-                (bpno - 1 < 0))) {
             if (type == T1_TYPE_RAW) {
+                opj_mqc_bypass_flush_enc(mqc, cblksty & J2K_CCP_CBLKSTY_PTERM);
+            } else {
+                if (cblksty & J2K_CCP_CBLKSTY_PTERM) {
+                    opj_mqc_erterm_enc(mqc);
+                } else {
                     opj_mqc_flush(mqc);
-                correction = 1;
+                }
-                /* correction = mqc_bypass_flush_enc(); */
-            } else {            /* correction = mqc_restart_enc(); */
-                opj_mqc_flush(mqc);
-                correction = 1;
             }
             pass->term = 1;
+            pass->rate = opj_mqc_numbytes(mqc);
         } else {
-            if (((bpno < ((OPJ_INT32)(cblk->numbps) - 4) && (passtype > 0))
+            /* Non terminated pass */
-                    || ((bpno == ((OPJ_INT32)cblk->numbps - 4)) && (passtype == 2))) &&
+            OPJ_UINT32 rate_extra_bytes;
-                    (cblksty & J2K_CCP_CBLKSTY_LAZY)) {
             if (type == T1_TYPE_RAW) {
-                    opj_mqc_flush(mqc);
+                rate_extra_bytes = opj_mqc_bypass_get_extra_bytes(
-                    correction = 1;
+                                       mqc, (cblksty & J2K_CCP_CBLKSTY_PTERM));
-                    /* correction = mqc_bypass_flush_enc(); */
-                } else {        /* correction = mqc_restart_enc(); */
-                    opj_mqc_flush(mqc);
-                    correction = 1;
-                }
-                pass->term = 1;
             } else {
+                rate_extra_bytes = 3;
+            }
             pass->term = 0;
-            }
+            pass->rate = opj_mqc_numbytes(mqc) + rate_extra_bytes;
         }
 
         if (++passtype == 3) {
@@ -2201,45 +2242,54 @@ static void opj_t1_encode_cblk(opj_t1_t *t1,
             bpno--;
         }
 
-        if (pass->term && bpno > 0) {
-            type = ((bpno < ((OPJ_INT32)(cblk->numbps) - 4)) && (passtype < 2) &&
-                    (cblksty & J2K_CCP_CBLKSTY_LAZY)) ? T1_TYPE_RAW : T1_TYPE_MQ;
-            if (type == T1_TYPE_RAW) {
-                opj_mqc_bypass_init_enc(mqc);
-            } else {
-                opj_mqc_restart_init_enc(mqc);
-            }
-        }
-
-        pass->distortiondec = cumwmsedec;
-        pass->rate = opj_mqc_numbytes(mqc) + correction;    /* FIXME */
-
         /* Code-switch "RESET" */
         if (cblksty & J2K_CCP_CBLKSTY_RESET) {
             opj_mqc_reset_enc(mqc);
         }
     }
 
-    /* Code switch "ERTERM" (i.e. PTERM) */
-    if (cblksty & J2K_CCP_CBLKSTY_PTERM) {
-        opj_mqc_erterm_enc(mqc);
-    } else /* Default coding */ if (!(cblksty & J2K_CCP_CBLKSTY_LAZY)) {
-        opj_mqc_flush(mqc);
-    }
-
     cblk->totalpasses = passno;
 
+    if (cblk->totalpasses) {
+        /* Make sure that pass rates are increasing */
+        OPJ_UINT32 last_pass_rate = opj_mqc_numbytes(mqc);
+        for (passno = cblk->totalpasses; passno > 0;) {
+            opj_tcd_pass_t *pass = &cblk->passes[--passno];
+            if (pass->rate > last_pass_rate) {
+                pass->rate = last_pass_rate;
+            } else {
+                last_pass_rate = pass->rate;
+            }
+        }
+    }
+
     for (passno = 0; passno < cblk->totalpasses; passno++) {
         opj_tcd_pass_t *pass = &cblk->passes[passno];
-        if (pass->rate > opj_mqc_numbytes(mqc)) {
+
-            pass->rate = opj_mqc_numbytes(mqc);
+        /* Prevent generation of FF as last data byte of a pass*/
-        }
+        /* For terminating passes, the flushing procedure ensured this already */
-        /*Preventing generation of FF as last data byte of a pass*/
+        assert(pass->rate > 0);
-        if ((pass->rate > 1) && (cblk->data[pass->rate - 1] == 0xFF)) {
+        if ((cblk->data[pass->rate - 1] == 0xFF)) {
             pass->rate--;
         }
         pass->len = pass->rate - (passno == 0 ? 0 : cblk->passes[passno - 1].rate);
     }
+
+#ifdef EXTRA_DEBUG
+    printf(" len=%d\n", (cblk->totalpasses) ? opj_mqc_numbytes(mqc) : 0);
+
+    /* Check that there not 0xff >=0x90 sequences */
+    if (cblk->totalpasses) {
+        OPJ_UINT32 i;
+        OPJ_UINT32 len = opj_mqc_numbytes(mqc);
+        for (i = 1; i < len; ++i) {
+            if (cblk->data[i - 1] == 0xff && cblk->data[i] >= 0x90) {
+                printf("0xff %02x at offset %d\n", cblk->data[i], i - 1);
+                abort();
+            }
+        }
+    }
+#endif
 }
 
 #if 0

src/lib/openjp2/tcd.c

@@ -1162,7 +1162,8 @@ static OPJ_BOOL opj_tcd_code_block_enc_allocate_data(opj_tcd_cblk_enc_t *
 
     if (l_data_size > p_code_block->data_size) {
         if (p_code_block->data) {
-            opj_free(p_code_block->data - 1); /* again, why -1 */
+            /* We refer to data - 1 since below we incremented it */
+            opj_free(p_code_block->data - 1);
         }
         p_code_block->data = (OPJ_BYTE*) opj_malloc(l_data_size + 1);
         if (! p_code_block->data) {
@@ -1171,6 +1172,10 @@ static OPJ_BOOL opj_tcd_code_block_enc_allocate_data(opj_tcd_cblk_enc_t *
         }
         p_code_block->data_size = l_data_size;
 
+        /* We reserve the initial byte as a fake byte to a non-FF value */
+        /* and increment the data pointer, so that opj_mqc_init_enc() */
+        /* can do bp = data - 1, and opj_mqc_byteout() can safely dereference */
+        /* it. */
         p_code_block->data[0] = 0;
         p_code_block->data += 1; /*why +1 ?*/
     }
@@ -1914,6 +1919,8 @@ static void opj_tcd_code_block_enc_deallocate(opj_tcd_precinct_t * p_precinct)
 
         for (cblkno = 0; cblkno < l_nb_code_blocks; ++cblkno)  {
             if (l_code_block->data) {
+                /* We refer to data - 1 since below we incremented it */
+                /* in opj_tcd_code_block_enc_allocate_data() */
                 opj_free(l_code_block->data - 1);
                 l_code_block->data = 00;
             }

tests/nonregression/test_suite.ctest.in

@@ -149,7 +149,20 @@ opj_compress -i @INPUT_NR_PATH@/flower-minisblack-15.tif -o @TEMP_PATH@/flower-m
 # issue 843 Crash with invalid ppm file
 !opj_compress -i @INPUT_NR_PATH@/issue843.ppm -o @TEMP_PATH@/issue843.ppm.jp2
 
+# Test all 6 coding options individually
 opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_vsc.j2k -M 8
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_bypass.j2k -M 1
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_reset.j2k -M 2
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_termall.j2k -M 4
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_pterm.j2k -M 16
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_segsym.j2k -M 32
+
+# Test a few combinations of coding options
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_bypass_termall.j2k -M 5
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_bypass_pterm.j2k -M 17
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_termall_pterm.j2k -M 20
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_bypass_termall_pterm.j2k -M 21
+opj_compress -i @INPUT_NR_PATH@/Bretagne2.ppm -o @TEMP_PATH@/Bretagne2_bypass_vsc_reset_termall_pterm_segsym.j2k -M 63
 
 # DECODER TEST SUITE
 opj_decompress -i  @INPUT_NR_PATH@/Bretagne2.j2k -o @TEMP_PATH@/Bretagne2.j2k.pgx