[FFmpeg-devel] [PATCH 3/3] lavc/libopenjpegenc: add cinema_setup_encoder function to allow creation of dci compliant files

[Reimar Döffinger]

On Wed, Jan 28, 2015 at 04:41:26PM +0100, Jean First wrote:
> +            if (parameters->numresolution > 6) {
> +                parameters->numresolution = 6;
> +            }

FFMAX

> +            if (!((image->comps[0].w == 2048) || (image->comps[0].h == 1080))) {

w != 2048 && h != 1080
seems way more readable to me

> +        case CINEMA4K_24:
> +            if (parameters->numresolution < 1) {
> +                parameters->numresolution = 1;
> +            } else if (parameters->numresolution > 7) {
> +                parameters->numresolution = 7;
> +            }

av_clip

> +            if (!((image->comps[0].w == 4096) || (image->comps[0].h == 2160))) {

Same as above.

> +    switch (parameters->cp_cinema) {
> +        case CINEMA2K_24:
> +        case CINEMA4K_24:
> +            for (i = 0; i < parameters->tcp_numlayers; i++) {
> +                temp_rate = 0;
> +                if (ctx->tcp_rates[i] == 0) {
> +                    parameters->tcp_rates[0] = ((float) (image->numcomps * image->comps[0].w * image->comps[0].h * image->comps[0].prec)) /
> +                                               (CINEMA_24_CS * 8 * image->comps[0].dx * image->comps[0].dy);

Typo? Or why is this [0] and not [i]

> +                } else {
> +                    temp_rate = ((float) (image->numcomps * image->comps[0].w * image->comps[0].h * image->comps[0].prec)) /
> +                                (ctx->tcp_rates[i] * 8 * image->comps[0].dx * image->comps[0].dy);
> +                    if (temp_rate > CINEMA_24_CS) {
> +                        parameters->tcp_rates[i] = ((float) (image->numcomps * image->comps[0].w * image->comps[0].h * image->comps[0].prec)) /
> +                                                   (CINEMA_24_CS * 8 * image->comps[0].dx * image->comps[0].dy);

That is the same formula as in the == 0 and it is almost the same as for
temp_rate. This should be duplicated less.

> +                    } else {
> +                        parameters->tcp_rates[i] = ctx->tcp_rates[i];
> +                    }
> +                }

temp_rate is only used in the else block, so it should be declared
there.
The use of float can lead to random rounding errors, I'd consider
if this potentially could be done using integer arithmetic.
Or at least using double.

My best guess is that this code should look something like this:
parameters->tcp_rates[i] = rate_calc(CINEMA_24_CS);
if (ctx->tcp_rates[i] != 0 && rate_calc(ctx->tcp_rates[i]) <= CINEMA_24_CS)
    parameters->tcp_rates[i] = ctx->tcp_rates[i];

I _believe_ this is equivalent to your code above (with rate_calc
defined appropriately), but it still doesn't really make
sense to me, so unless there's some explanation added
I strongly suspect it is wrong.

> +            }
> +            parameters->max_comp_size = COMP_24_CS;
> +            break;
> +
> +        case CINEMA2K_48:

That looks like the same as the code for _24 except for one constant.
This should be done with less code duplication.