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

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(-)

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

Hi Moritz
I check the code and it seems the error born from a previous version of 
ISO/IEC15444-1 I have
(I used a final draft not the final document and the definition was 
quite fuzzy).  Checking final
document I found that you are right.
According to this specification (Table A-18 Width or Height exponent of 
the code-blocks for the SPcod
and SPcoc parameters) code-block exponent width  xcb and height ycb are 
defined as:

xcb = value + 2   and  ycb = value+2

so xcb+ycb <=12 is the correct check. I will correct the patch.

Francesco


On 15/08/2017 23.57, Moritz Barsnick wrote:
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel