Re: [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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jean First]

 code originates from image_to_j2k.c provided by openjpeg

Signed-off-by: Jean First jeanfi...@gmail.com
---
 libavcodec/libopenjpegenc.c | 118 
 1 file changed, 118 insertions(+)

diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
index b9a8bac..8c8bf67 100644
--- a/libavcodec/libopenjpegenc.c
+++ b/libavcodec/libopenjpegenc.c
@@ -26,6 +26,11 @@
 
 #define  OPJ_STATIC
 
+#define CINEMA_24_CS 1302083 /* Codestream length for 24fps */
+#define CINEMA_48_CS 651041  /* Codestream length for 48fps */
+#define COMP_24_CS 1041666   /* Maximum size per color component for 2K  4K @ 
24fps */
+#define COMP_48_CS 520833/* Maximum size per color component for 2K @ 
48fps */
+
 #include libavutil/avassert.h
 #include libavutil/common.h
 #include libavutil/imgutils.h
@@ -75,6 +80,25 @@ static void info_callback(const char *msg, void *data)
 av_log(data, AV_LOG_DEBUG, %s\n, msg);
 }
 
+static int initialise_4K_poc(opj_poc_t *POC, int numres)
+{
+POC[0].tile= 1;
+POC[0].resno0  = 0;
+POC[0].compno0 = 0;
+POC[0].layno1  = 1;
+POC[0].resno1  = numres - 1;
+POC[0].compno1 = 3;
+POC[0].prg1= CPRL;
+POC[1].tile= 1;
+POC[1].resno0  = numres - 1;
+POC[1].compno0 = 0;
+POC[1].layno1  = 1;
+POC[1].resno1  = numres;
+POC[1].compno1 = 3;
+POC[1].prg1= CPRL;
+return 2;
+}
+
 static void cinema_parameters(opj_cparameters_t *p)
 {
 p-tile_size_on = 0;
@@ -110,8 +134,98 @@ static void cinema_parameters(opj_cparameters_t *p)
 p-irreversible = 1;
 }
 
+static void cinema_setup_encoder(AVCodecContext *avctx, opj_image_t *image)
+{
+LibOpenJPEGContext *ctx = avctx-priv_data;
+opj_cparameters_t *parameters = ctx-enc_params;
+int i;
+float temp_rate;
+
+switch (parameters-cp_cinema) {
+case CINEMA2K_24:
+case CINEMA2K_48:
+if (parameters-numresolution  6) {
+parameters-numresolution = 6;
+}
+if (!((image-comps[0].w == 2048) || (image-comps[0].h == 1080))) 
{
+av_log(avctx, AV_LOG_WARNING, Image coordinates %d x %d is 
not 2K compliant.\nJPEG Digital Cinema Profile-3 
+(2K profile) compliance requires that at least one of 
coordinates match 2048 x 1080\n,
+image-comps[0].w, image-comps[0].h);
+parameters-cp_rsiz = STD_RSIZ;
+}
+break;
+
+case CINEMA4K_24:
+if (parameters-numresolution  1) {
+parameters-numresolution = 1;
+} else if (parameters-numresolution  7) {
+parameters-numresolution = 7;
+}
+if (!((image-comps[0].w == 4096) || (image-comps[0].h == 2160))) 
{
+av_log(avctx, AV_LOG_WARNING, Image coordinates %d x %d is 
not 4K compliant.\nJPEG Digital Cinema Profile-4
+(4K profile) compliance requires that at least one of 
coordinates match 4096 x 2160\n,
+image-comps[0].w, image-comps[0].h);
+parameters-cp_rsiz = STD_RSIZ;
+}
+parameters-numpocs = initialise_4K_poc(parameters-POC, 
parameters-numresolution);
+break;
+case OFF:
+/* do nothing */
+break;
+}
+
+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);
+} 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);
+} else {
+parameters-tcp_rates[i] = ctx-tcp_rates[i];
+}
+}
+}
+parameters-max_comp_size = COMP_24_CS;
+break;
+
+case CINEMA2K_48:
+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_48_CS * 8 *