Re: [FFmpeg-devel] [PATCHv3] mov: Evaluate the movie display matrix

2016-10-18 Thread Benoit Fouet
Hi,

On 14/10/2016 00:50, Vittorio Giovara wrote:
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index a15c8d1..e8da77f 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c

[...]

> @@ -3798,16 +3804,33 @@ static int mov_read_meta(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return 0;
>  }
>  
> +// return 0 when matrix is identity, 1 otherwise
> +#define IS_MATRIX_FULL(matrix)   \
> +(matrix[0][0] != (1 << 16) ||\
> + matrix[1][1] != (1 << 16) ||\
> + matrix[2][2] != (1 << 30) ||\
> + matrix[0][1] || matrix[0][2] || \
> + matrix[1][0] || matrix[1][2] || \
> + matrix[2][0] || matrix[2][1])
> +

should be "(matrix)" everywhere
Also, reversing the logic would allow preventing the evaluation of all
conditions when the matrix is not identity (i.e. making it
IS_MATRIX_IDENTITY)

> +// fixed point to double
> +#define CONV_FP(x, sh) ((double) (x)) / (1 << (sh))
> +

((double) (x) / (1 << (sh)))

> +// double to fixed point
> +#define CONV_DB(x, sh) ((int32_t) ((x) * (1 << (sh
> +

Cheers,
-- 
Ben


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Re: [FFmpeg-devel] [PATCHv3] mov: Evaluate the movie display matrix

2016-10-17 Thread Vittorio Giovara
On Thu, Oct 13, 2016 at 6:50 PM, Vittorio Giovara
 wrote:
> This matrix needs to be applied after all others have (currently only
> display matrix from trak), but cannot be handled in movie box, since
> streams are not allocated yet. So store it in main context, and apply
> it when appropriate, that is after parsing the tkhd one.
>
> Signed-off-by: Vittorio Giovara 
> ---
> Reworked second matrix handling so that it is always applied, which makes
> the code easier to read and test. Updated according reviews, rolled back
> a couple of points for the reasons explained in the thread.
>
> Needs a new sample to be uploaded to fate,
> https://www.dropbox.com/s/qfio4bjhkpz3p4o/displaymatrix.mov?dl=0,
> the previous one can be deleted.
>
> Cheers,
> Vittorio
>
>  libavformat/isom.h|  2 ++
>  libavformat/mov.c | 53 
> +++
>  tests/fate/mov.mak|  6 -
>  tests/ref/fate/mov-display-matrix | 10 
>  4 files changed, 59 insertions(+), 12 deletions(-)
>  create mode 100644 tests/ref/fate/mov-display-matrix

Ping? Please cc me or i might miss replies.
Thanks
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


[FFmpeg-devel] [PATCHv3] mov: Evaluate the movie display matrix

2016-10-13 Thread Vittorio Giovara
This matrix needs to be applied after all others have (currently only
display matrix from trak), but cannot be handled in movie box, since
streams are not allocated yet. So store it in main context, and apply
it when appropriate, that is after parsing the tkhd one.

Signed-off-by: Vittorio Giovara 
---
Reworked second matrix handling so that it is always applied, which makes
the code easier to read and test. Updated according reviews, rolled back
a couple of points for the reasons explained in the thread.

Needs a new sample to be uploaded to fate,
https://www.dropbox.com/s/qfio4bjhkpz3p4o/displaymatrix.mov?dl=0,
the previous one can be deleted.

Cheers,
Vittorio

 libavformat/isom.h|  2 ++
 libavformat/mov.c | 53 +++
 tests/fate/mov.mak|  6 -
 tests/ref/fate/mov-display-matrix | 10 
 4 files changed, 59 insertions(+), 12 deletions(-)
 create mode 100644 tests/ref/fate/mov-display-matrix

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 2246fed..2aeb8fa 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -238,6 +238,8 @@ typedef struct MOVContext {
 uint8_t *decryption_key;
 int decryption_key_len;
 int enable_drefs;
+
+int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index a15c8d1..e8da77f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1211,6 +1211,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
+int i;
 int64_t creation_time;
 int version = avio_r8(pb); /* version */
 avio_rb24(pb); /* flags */
@@ -1238,7 +1239,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 avio_skip(pb, 10); /* reserved */
 
-avio_skip(pb, 36); /* display matrix */
+/* movie display matrix, store it in main context and use it later on */
+for (i = 0; i < 3; i++) {
+c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
+}
 
 avio_rb32(pb); /* preview time */
 avio_rb32(pb); /* preview duration */
@@ -3798,16 +3804,33 @@ static int mov_read_meta(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 return 0;
 }
 
+// return 0 when matrix is identity, 1 otherwise
+#define IS_MATRIX_FULL(matrix)   \
+(matrix[0][0] != (1 << 16) ||\
+ matrix[1][1] != (1 << 16) ||\
+ matrix[2][2] != (1 << 30) ||\
+ matrix[0][1] || matrix[0][2] || \
+ matrix[1][0] || matrix[1][2] || \
+ matrix[2][0] || matrix[2][1])
+
+// fixed point to double
+#define CONV_FP(x, sh) ((double) (x)) / (1 << (sh))
+
+// double to fixed point
+#define CONV_DB(x, sh) ((int32_t) ((x) * (1 << (sh
+
 static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
-int i;
+int i, j, e;
 int width;
 int height;
 int display_matrix[3][3];
+int res_display_matrix[3][3];
 AVStream *st;
 MOVStreamContext *sc;
 int version;
 int flags;
+double val = 0;
 
 if (c->fc->nb_streams < 1)
 return 0;
@@ -3853,15 +3876,22 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 sc->width = width >> 16;
 sc->height = height >> 16;
 
+// apply the moov display matrix
+for (i = 0; i < 3; i++) {
+for (j = 0; j < 3; j++) {
+int sh = j == 2 ? 30 : 16;
+for (e = 0; e < 3; e++) {
+val += CONV_FP(display_matrix[i][e], sh) *
+   CONV_FP(c->movie_display_matrix[e][j], sh);
+}
+res_display_matrix[i][j] = CONV_DB(val, sh);
+val = 0;
+}
+}
+
 // save the matrix and add rotate metadata when it is not the default
 // identity
-if (display_matrix[0][0] != (1 << 16) ||
-display_matrix[1][1] != (1 << 16) ||
-display_matrix[2][2] != (1 << 30) ||
-display_matrix[0][1] || display_matrix[0][2] ||
-display_matrix[1][0] || display_matrix[1][2] ||
-display_matrix[2][0] || display_matrix[2][1]) {
-int i, j;
+if (IS_MATRIX_FULL(res_display_matrix)) {
 double rotate;
 
 av_freep(&sc->display_matrix);
@@ -3871,7 +3901,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 for (i = 0; i < 3; i++)
 for (j = 0; j < 3; j++)
-sc->display_matrix[i * 3 + j] = display_matrix[i][j];
+sc->display_matrix[i * 3 + j] = res_display_matrix[i][j];
 
 rotate = av_display_rotation_get(sc->display_matrix);
 if (!isnan(rotate)) {
@@ -3890,7 +3920,8 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext