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

[Moritz Barsnick]

On Mon, Aug 07, 2017 at 10:26:21 +0200, francesco at 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