[FFmpeg-devel] JPEG200 encoding : added option for changing default codeblock size

Submitted by francesco@bltitalia.com on Aug. 7, 2017, 8:26 a.m.

Details

Message ID 1502094381-16589-1-git-send-email-francesco@bltitalia.com
State New
Headers show

Commit Message

francesco@bltitalia.com Aug. 7, 2017, 8:26 a.m.
From: Francesco Cuzzocrea <francesco@bltitalia.com>

HI
I think this time I've made correctly a patch. As in previous mail, I added
option for changing codeblock size. I've inserted check on exponent sum as for
ISO/IEC FCD 15444-1



---
 libavcodec/j2kenc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Moritz Barsnick Aug. 15, 2017, 9:57 p.m.
On Mon, Aug 07, 2017 at 10:26:21 +0200, francesco@bltitalia.com wrote:
> Subject: [FFmpeg-devel] [PATCH] JPEG200 encoding : added option for changing default codeblock size

Is this your commit message? It's 2000, not 200. ;-) Actually, this
should read:
libavcodec/j2kenc: add option for changing codeblock size

> +    i = codsty->log2_cblk_width + codsty->log2_cblk_height -4;
> +    if ( i > 12 )
> +    {
> +      av_log(avctx, AV_LOG_ERROR, "Invalid values for codeblocks size\n");
> +      return -1;
> +    }

Bracket placement, indentation and whitespace all do not correspond to
ffmpeg style.

Apart from that: Isn't that a quite complicated way of saying
if (codsty->log2_cblk_width + codsty->log2_cblk_height > 16)
??

> +    { "log2_cblk_width",   "Codeblock Width",   OFFSET(codsty.log2_cblk_width),    AV_OPT_TYPE_INT,   { .i64 = 4           }, 0,     1<<10,           VE, },
> +    { "log2_cblk_height",  "Codeblock Height",  OFFSET(codsty.log2_cblk_height),   AV_OPT_TYPE_INT,   { .i64 = 4           }, 0,     1<<10,           VE, },

I would suggest to drop the capital letters in the option descriptions,
but it appears to follow the style of the other options, so fine by me.

And what's with the upper limits? They don't seem sane. If you choose
either option at one of those limits, the check above will fail
(1<<10 + 0 - 4 is waaaay larger than 12).

Looking at the spec, you are mixing exponent and value. I read
"Dimension of the code-blocks is always a power of 2 with the minimum
height and width being 4 and and maximum height and width being 1024."
Furthermore the sum of the exponents needs to be less than or equal to
12.

So, the variable being log2, the limits are obviously 2 and 10, the
default 4 (as before). And your check actually needs to read
if (codsty->log2_cblk_width + codsty->log2_cblk_height > 12)
.

Or am I missing something?


Moritz

Patch hide | download patch | download mbox

diff --git a/libavcodec/j2kenc.c b/libavcodec/j2kenc.c
index c8d3861..63096ca 100644
--- a/libavcodec/j2kenc.c
+++ b/libavcodec/j2kenc.c
@@ -1121,8 +1121,6 @@  FF_ENABLE_DEPRECATION_WARNINGS
     memset(codsty->log2_prec_heights, 15, sizeof(codsty->log2_prec_heights));
     codsty->nreslevels2decode=
     codsty->nreslevels       = 7;
-    codsty->log2_cblk_width  = 4;
-    codsty->log2_cblk_height = 4;
     codsty->transform        = s->pred ? FF_DWT53 : FF_DWT97_INT;
 
     qntsty->nguardbits       = 1;
@@ -1131,6 +1129,12 @@  FF_ENABLE_DEPRECATION_WARNINGS
         (s->tile_height & (s->tile_height-1))) {
         av_log(avctx, AV_LOG_WARNING, "Tile dimension not a power of 2\n");
     }
+    i = codsty->log2_cblk_width + codsty->log2_cblk_height -4;
+    if ( i > 12 )
+    {
+      av_log(avctx, AV_LOG_ERROR, "Invalid values for codeblocks size\n");
+      return -1;
+    }
 
     if (codsty->transform == FF_DWT53)
         qntsty->quantsty = JPEG2000_QSTY_NONE;
@@ -1188,6 +1192,8 @@  static const AVOption options[] = {
     { "pred",          "DWT Type",          OFFSET(pred),          AV_OPT_TYPE_INT,   { .i64 = 0           }, 0,         1,           VE, "pred"        },
     { "dwt97int",      NULL,                0,                     AV_OPT_TYPE_CONST, { .i64 = 0           }, INT_MIN, INT_MAX,       VE, "pred"        },
     { "dwt53",         NULL,                0,                     AV_OPT_TYPE_CONST, { .i64 = 0           }, INT_MIN, INT_MAX,       VE, "pred"        },
+    { "log2_cblk_width",   "Codeblock Width",   OFFSET(codsty.log2_cblk_width),    AV_OPT_TYPE_INT,   { .i64 = 4           }, 0,     1<<10,           VE, },
+    { "log2_cblk_height",  "Codeblock Height",  OFFSET(codsty.log2_cblk_height),   AV_OPT_TYPE_INT,   { .i64 = 4           }, 0,     1<<10,           VE, },
 
     { NULL }
 };