[FFmpeg-devel] [PATCH] libopenjpegenc: make dci compliant j2c

[Michael Bradshaw]

On Fri, Feb 1, 2013 at 5:00 AM, Jean First <jeanfirst at gmail.com> wrote:
> From: Michaël Cinquin <mc at michaelcinquin.com>
>
> Signed-off-by: Jean First <jeanfirst at gmail.com>
> ---
>
>  I just fixed the trailing white spaces
>
>  libavcodec/libopenjpegenc.c |   43 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
> index 13e8ef9..70e608a 100644
> --- a/libavcodec/libopenjpegenc.c
> +++ b/libavcodec/libopenjpegenc.c
> @@ -55,6 +55,7 @@ typedef struct {
>      int disto_alloc;
>      int fixed_alloc;
>      int fixed_quality;
> +    int shr;

Please remove the changes from the shr patch. That is, please change
this patch to only be the if statement below (which I'm about to
review)

> @@ -182,6 +184,36 @@ static av_cold int libopenjpeg_encode_init(AVCodecContext *avctx)
>      ctx->enc_params.tcp_numlayers = ctx->numlayers;
>      ctx->enc_params.tcp_rates[0] = FFMAX(avctx->compression_level, 0) * 2;
>
> +    if(ctx->cinema_mode > 0){

Space after if and after ) (same thing for the if statement below).
Also, indentation should be by 4 spaces (throughout this patch) to be
consistent with the rest of the file.

> +      ctx->enc_params.irreversible = 1;
> +      ctx->enc_params.tcp_mct = 1;
> +      ctx->enc_params.tile_size_on = 0;
> +      /* no subsampling */
> +      ctx->enc_params.cp_tdx=1;
> +      ctx->enc_params.cp_tdy=1;

Ok

> +      ctx->enc_params.subsampling_dx = 1;
> +      ctx->enc_params.subsampling_dy = 1;

Default

> +      /* Tile and Image shall be at (0,0) */
> +      ctx->enc_params.cp_tx0 = 0;
> +      ctx->enc_params.cp_ty0 = 0;
> +      ctx->enc_params.image_offset_x0 = 0;
> +      ctx->enc_params.image_offset_y0 = 0;

Default

> +      /* Codeblock size= 32*32 */
> +      ctx->enc_params.cblockw_init = 32;
> +      ctx->enc_params.cblockh_init = 32;
> +      ctx->enc_params.csty |= 0x01;

Ok

> +      /* No ROI */
> +      ctx->enc_params.roi_compno = -1;

Default

> +
> +      if(ctx->enc_params.prog_order != CPRL){
> +        av_log(avctx, AV_LOG_ERROR, "Prog_order forced to CPRL\n");
> +        ctx->enc_params.prog_order = CPRL;
> +      }
> +      ctx->enc_params.tp_flag = 'C';
> +      ctx->enc_params.tp_on = 1;
> +    }
> +
> +

Extra blank line (should be just one).


Note: I've marked things "Default" if they set the parameters to the
same values that opj_set_default_encoder_parameters() does. I'm not
saying you have to remove these; it's not a bad idea to explicitly
force things for compliance's sake. I'm just making a note and
documenting it for myself.


I see this pretty much came from cinema_parameters() in image_to_j2k.
Is there anything in cinema_setup_encoder() that should also be
included? Particularly, setting parameters->max_comp_size (we may want
to set it to COMP_24_CS/
COMP_48_CS depending on CINEMA2K_24/CINEMA2K_48/CINEMA4K_24 to force
openjpeg to respect the limitation; it defaults to 0 so openjpeg
ignores it) or parameters->numpocs (they seem to set this and
parameters->POC for CINEMA4K_24)? I'm assuming since the user can set
parameters->tcp_rates we don't have to force it to be something
specific.

Thanks!