From 81c5311758a0ae1f1aea349a6ee0bca2a238fa79 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Fri, 9 Jun 2017 10:47:13 +0200 Subject: [PATCH] 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 | 111 +++++++++++++----- src/lib/openjp2/mqc.h | 19 ++- src/lib/openjp2/t1.c | 146 ++++++++++++++++-------- src/lib/openjp2/tcd.c | 9 +- tests/nonregression/test_suite.ctest.in | 13 +++ 5 files changed, 218 insertions(+), 80 deletions(-) diff --git a/src/lib/openjp2/mqc.c b/src/lib/openjp2/mqc.c index 8a792b60..92a0d318 100644 --- a/src/lib/openjp2/mqc.c +++ b/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*/ - if (mqc->bp == mqc->start - 1) { - mqc->bp++; - *mqc->bp = (OPJ_BYTE)(mqc->c >> 19); - mqc->c &= 0x7ffff; - mqc->ct = 8; - } else if (*mqc->bp == 0xff) { + /* bp is initialized to start - 1 in opj_mqc_init_enc() */ + /* but this is safe, see opj_tcd_code_block_enc_allocate_data() */ + assert(mqc->bp >= mqc->start - 1); + 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; - /*if (*mqc->bp == 0xff) { - mqc->ct = 7; - } */ + /* in theory we should initialize to 8, but use this special value */ + /* as a hint that opj_mqc_bypass_enc() has never been called, so */ + /* 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; - - if (mqc->ct != 0) { +void opj_mqc_bypass_flush_enc(opj_mqc_t *mqc, OPJ_BOOL erterm) +{ + /* 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); - bit_padding = (bit_padding + 1) & 0x01; + mqc->c += (OPJ_UINT32)(bit_value << mqc->ct); + bit_value = (OPJ_BYTE)(1U - bit_value); } - mqc->bp++; *mqc->bp = (OPJ_BYTE)mqc->c; - mqc->ct = 8; - mqc->c = 0; + /* Advance pointer so that opj_mqc_numbytes() returns a valid value */ + 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) { /* */ - 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; } diff --git a/src/lib/openjp2/mqc.h b/src/lib/openjp2/mqc.h index f21d46ef..3b52d95e 100644 --- a/src/lib/openjp2/mqc.h +++ b/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. -

Not fully implemented and tested !!

@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. -

Not fully implemented and tested !!

@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 -

Not fully implemented and tested !!

@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 diff --git a/src/lib/openjp2/t1.c b/src/lib/openjp2/t1.c index af520202..15ac0407 100644 --- a/src/lib/openjp2/t1.c +++ b/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 ((cblksty & J2K_CCP_CBLKSTY_TERMALL) && !((passtype == 2) && - (bpno - 1 < 0))) { + if (opj_t1_enc_is_term_pass(cblk, cblksty, bpno, passtype)) { + /* If it is a terminated pass, terminate it */ if (type == T1_TYPE_RAW) { - opj_mqc_flush(mqc); - correction = 1; - /* correction = mqc_bypass_flush_enc(); */ - } else { /* correction = mqc_restart_enc(); */ - opj_mqc_flush(mqc); - correction = 1; + 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); + } } pass->term = 1; + pass->rate = opj_mqc_numbytes(mqc); } else { - if (((bpno < ((OPJ_INT32)(cblk->numbps) - 4) && (passtype > 0)) - || ((bpno == ((OPJ_INT32)cblk->numbps - 4)) && (passtype == 2))) && - (cblksty & J2K_CCP_CBLKSTY_LAZY)) { - if (type == T1_TYPE_RAW) { - 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; + /* Non terminated pass */ + OPJ_UINT32 rate_extra_bytes; + if (type == T1_TYPE_RAW) { + rate_extra_bytes = opj_mqc_bypass_get_extra_bytes( + mqc, (cblksty & J2K_CCP_CBLKSTY_PTERM)); } else { - pass->term = 0; + 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); - } - /*Preventing generation of FF as last data byte of a pass*/ - if ((pass->rate > 1) && (cblk->data[pass->rate - 1] == 0xFF)) { + + /* Prevent generation of FF as last data byte of a pass*/ + /* For terminating passes, the flushing procedure ensured this already */ + assert(pass->rate > 0); + 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 diff --git a/src/lib/openjp2/tcd.c b/src/lib/openjp2/tcd.c index f1e1c1c3..e36496e6 100644 --- a/src/lib/openjp2/tcd.c +++ b/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; } diff --git a/tests/nonregression/test_suite.ctest.in b/tests/nonregression/test_suite.ctest.in index 82ed67c6..00c83b26 100644 --- a/tests/nonregression/test_suite.ctest.in +++ b/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