diff mbox

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

Message ID 20161007193146.93184-1-vittorio.giovara@gmail.com
State Superseded
Headers show

Commit Message

Vittorio Giovara Oct. 7, 2016, 7:31 p.m. UTC
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 if not identity, apply it when appropriate,
handling the case when trak display matrix is identity and when it is not.

Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
---
Updated according review.
Vittorio

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

Comments

Michael Niedermayer Oct. 8, 2016, 12:38 a.m. UTC | #1
On Fri, Oct 07, 2016 at 03:31:46PM -0400, 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 if not identity, apply it when appropriate,
> handling the case when trak display matrix is identity and when it is not.
> 
> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> ---
> Updated according review.
> Vittorio
> 
>  libavformat/isom.h                      |  2 ++
>  libavformat/mov.c                       | 63 +++++++++++++++++++++++++++------
>  tests/fate/mov.mak                      |  6 +++-
>  tests/ref/fate/mov-movie-display-matrix | 10 ++++++
>  4 files changed, 70 insertions(+), 11 deletions(-)
>  create mode 100644 tests/ref/fate/mov-movie-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..307ce08 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,9 +3804,24 @@ 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 int64_t
> +#define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
> +
> +// int64_t to fixed point
> +#define CONV_INT2FP(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];
> @@ -3855,13 +3876,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  
>      // 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(display_matrix)) {
>          double rotate;
>  
>          av_freep(&sc->display_matrix);
> @@ -3884,13 +3899,41 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          }
>      }
>  
> +    // if movie display matrix is not identity, and if this is a video track
> +    if (IS_MATRIX_FULL(c->movie_display_matrix) && width && height) {
> +        // if trak display matrix was identity, just copy the movie one
> +        if (!sc->display_matrix) {
> +            sc->display_matrix = av_malloc(sizeof(int32_t) * 9);
> +            if (!sc->display_matrix)
> +                return AVERROR(ENOMEM);
> +
> +            for (i = 0; i < 3; i++)
> +                for (j = 0; j < 3; j++)
> +                    sc->display_matrix[i * 3 + j] = c->movie_display_matrix[i][j];

> +        } else { // otherwise multiply the two and store the result
> +            int64_t val = 0;
> +            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_FP2INT(display_matrix[i][e], sh) *
> +                               CONV_FP2INT(c->movie_display_matrix[e][j], sh);

This does not work (you are dividing the 32bit numbers down to 2 bit)
also its not tested by the fate testcase
i can just replace it by 0 and fate-mov-movie-display-matrix still
passes
the macros also lack some () protection giving probably unintended
results

i dont mind fixing it myself to work with int64 but i need a testcase

[...]
Michael Niedermayer Oct. 8, 2016, 12:55 a.m. UTC | #2
On Sat, Oct 08, 2016 at 02:38:27AM +0200, Michael Niedermayer wrote:
> On Fri, Oct 07, 2016 at 03:31:46PM -0400, 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 if not identity, apply it when appropriate,
> > handling the case when trak display matrix is identity and when it is not.
> > 
> > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> > ---
> > Updated according review.
> > Vittorio
> > 
> >  libavformat/isom.h                      |  2 ++
> >  libavformat/mov.c                       | 63 +++++++++++++++++++++++++++------
> >  tests/fate/mov.mak                      |  6 +++-
> >  tests/ref/fate/mov-movie-display-matrix | 10 ++++++
> >  4 files changed, 70 insertions(+), 11 deletions(-)
> >  create mode 100644 tests/ref/fate/mov-movie-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..307ce08 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,9 +3804,24 @@ 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 int64_t
> > +#define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
> > +
> > +// int64_t to fixed point
> > +#define CONV_INT2FP(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];
> > @@ -3855,13 +3876,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  
> >      // 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(display_matrix)) {
> >          double rotate;
> >  
> >          av_freep(&sc->display_matrix);
> > @@ -3884,13 +3899,41 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >          }
> >      }
> >  
> > +    // if movie display matrix is not identity, and if this is a video track
> > +    if (IS_MATRIX_FULL(c->movie_display_matrix) && width && height) {
> > +        // if trak display matrix was identity, just copy the movie one
> > +        if (!sc->display_matrix) {
> > +            sc->display_matrix = av_malloc(sizeof(int32_t) * 9);
> > +            if (!sc->display_matrix)
> > +                return AVERROR(ENOMEM);
> > +
> > +            for (i = 0; i < 3; i++)
> > +                for (j = 0; j < 3; j++)
> > +                    sc->display_matrix[i * 3 + j] = c->movie_display_matrix[i][j];
> 
> > +        } else { // otherwise multiply the two and store the result
> > +            int64_t val = 0;
> > +            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_FP2INT(display_matrix[i][e], sh) *
> > +                               CONV_FP2INT(c->movie_display_matrix[e][j], sh);
> 
> This does not work (you are dividing the 32bit numbers down to 2 bit)
> also its not tested by the fate testcase
> i can just replace it by 0 and fate-mov-movie-display-matrix still
> passes
> the macros also lack some () protection giving probably unintended
> results
> 
> i dont mind fixing it myself to work with int64 but i need a testcase

to explain a bit more how to do it with int64 instead of double floats
int32 can be multiplied together to form int64 with a new fixed point
without rounding or shifts
then added up and then at the end shifted down with rounding to get
back to the fixed point needed for the final destination

[...]
Vittorio Giovara Oct. 8, 2016, 2:38 a.m. UTC | #3
On Fri, Oct 7, 2016 at 8:38 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Fri, Oct 07, 2016 at 03:31:46PM -0400, 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 if not identity, apply it when appropriate,
>> handling the case when trak display matrix is identity and when it is not.
>>
>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
>> ---
>
>> +        } else { // otherwise multiply the two and store the result
>> +            int64_t val = 0;
>> +            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_FP2INT(display_matrix[i][e], sh) *
>> +                               CONV_FP2INT(c->movie_display_matrix[e][j], sh);
>
> This does not work (you are dividing the 32bit numbers down to 2 bit)

I don't understand this comment, are you referring to the fact that
the first two columns of the display matrix are 16.16, while the third
column is 2:30?

> also its not tested by the fate testcase
> i can just replace it by 0 and fate-mov-movie-display-matrix still
> passes

Yes, the sample I shared only has the movie display matrix, not
trak+movie. I can create another sample if you insist, but a fate test
would test little more than the matrix multiplication loops.

> the macros also lack some () protection giving probably unintended
> results

Can you tell me how many more () would they need?

From the subsequent email

> to explain a bit more how to do it with int64 instead of double floats
> int32 can be multiplied together to form int64 with a new fixed point
> without rounding or shifts
> then added up and then at the end shifted down with rounding to get
> back to the fixed point needed for the final destination

The problem is that the display matrix fields have different mantissa
length, the first two columns are 16.16, the last one is 2:30. I think
that method would work if all elements were the same length no?
Michael Niedermayer Oct. 8, 2016, 5:39 p.m. UTC | #4
On Fri, Oct 07, 2016 at 10:38:22PM -0400, Vittorio Giovara wrote:
> On Fri, Oct 7, 2016 at 8:38 PM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > On Fri, Oct 07, 2016 at 03:31:46PM -0400, 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 if not identity, apply it when appropriate,
> >> handling the case when trak display matrix is identity and when it is not.
> >>
> >> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> >> ---
> >
> >> +        } else { // otherwise multiply the two and store the result
> >> +            int64_t val = 0;
> >> +            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_FP2INT(display_matrix[i][e], sh) *
> >> +                               CONV_FP2INT(c->movie_display_matrix[e][j], sh);
> >
> > This does not work (you are dividing the 32bit numbers down to 2 bit)
> 
> I don't understand this comment, are you referring to the fact that
> the first two columns of the display matrix are 16.16, while the third
> column is 2:30?

you have 32bit integers and sh can be 30, in that case you throw away
30 of 32 bits before doing anything with the number


> 
> > also its not tested by the fate testcase
> > i can just replace it by 0 and fate-mov-movie-display-matrix still
> > passes
> 
> Yes, the sample I shared only has the movie display matrix, not
> trak+movie. I can create another sample if you insist, but a fate test
> would test little more than the matrix multiplication loops.

yes


> 
> > the macros also lack some () protection giving probably unintended
> > results
> 
> Can you tell me how many more () would they need?

#define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)

is missing at least 2 pairs

#define CONV_FP2INT(x, sh) (((int64_t) (x)) / (1 << (sh)))

also the rounding is not optimal it should likely be rounding to
nearest
the division can be replaced by a shift while correcting the rounding


> 
> From the subsequent email
> 
> > to explain a bit more how to do it with int64 instead of double floats
> > int32 can be multiplied together to form int64 with a new fixed point
> > without rounding or shifts
> > then added up and then at the end shifted down with rounding to get
> > back to the fixed point needed for the final destination
> 
> The problem is that the display matrix fields have different mantissa
> length, the first two columns are 16.16, the last one is 2:30. I think
> that method would work if all elements were the same length no?

iam not sure the original double code is correct, but it used the same
shift for each output, just changing between outputs.

if that was correct it should work fine, if it was not correct then
some extra shift is needed after the multiplication and before the
addition to get things to match


[...]
Vittorio Giovara Oct. 13, 2016, 10:25 p.m. UTC | #5
On Sat, Oct 8, 2016 at 1:39 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Fri, Oct 07, 2016 at 10:38:22PM -0400, Vittorio Giovara wrote:
>> On Fri, Oct 7, 2016 at 8:38 PM, Michael Niedermayer
>> <michael@niedermayer.cc> wrote:
>> > On Fri, Oct 07, 2016 at 03:31:46PM -0400, 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 if not identity, apply it when appropriate,
>> >> handling the case when trak display matrix is identity and when it is not.
>> >>
>> >> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
>> >> ---
>> >
>> >> +        } else { // otherwise multiply the two and store the result
>> >> +            int64_t val = 0;
>> >> +            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_FP2INT(display_matrix[i][e], sh) *
>> >> +                               CONV_FP2INT(c->movie_display_matrix[e][j], sh);
>> >
>> > This does not work (you are dividing the 32bit numbers down to 2 bit)
>>
>> I don't understand this comment, are you referring to the fact that
>> the first two columns of the display matrix are 16.16, while the third
>> column is 2:30?
>
> you have 32bit integers and sh can be 30, in that case you throw away
> 30 of 32 bits before doing anything with the number

yes but that value will be between 0 and 0.999999, which can be safely ignored.

>> > also its not tested by the fate testcase
>> > i can just replace it by 0 and fate-mov-movie-display-matrix still
>> > passes
>>
>> Yes, the sample I shared only has the movie display matrix, not
>> trak+movie. I can create another sample if you insist, but a fate test
>> would test little more than the matrix multiplication loops.
>
> yes

ok i'll simplify the code altogether

>> > the macros also lack some () protection giving probably unintended
>> > results
>>
>> Can you tell me how many more () would they need?
>
> #define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
>
> is missing at least 2 pairs
>
> #define CONV_FP2INT(x, sh) (((int64_t) (x)) / (1 << (sh)))

thanks, applied

> also the rounding is not optimal it should likely be rounding to
> nearest
> the division can be replaced by a shift while correcting the rounding

the rounding is fine, as i said the error is below 1 which is
acceptable for transform operations
also i really doubt using int64 is useful here, given that the numbers
are immediately converted to double when computing disp_transform and
hypot. I actually fear that using int64 will increase the error so I'm
tempted to drop it.
diff mbox

Patch

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..307ce08 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,9 +3804,24 @@  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 int64_t
+#define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
+
+// int64_t to fixed point
+#define CONV_INT2FP(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];
@@ -3855,13 +3876,7 @@  static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     // 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(display_matrix)) {
         double rotate;
 
         av_freep(&sc->display_matrix);
@@ -3884,13 +3899,41 @@  static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
         }
     }
 
+    // if movie display matrix is not identity, and if this is a video track
+    if (IS_MATRIX_FULL(c->movie_display_matrix) && width && height) {
+        // if trak display matrix was identity, just copy the movie one
+        if (!sc->display_matrix) {
+            sc->display_matrix = av_malloc(sizeof(int32_t) * 9);
+            if (!sc->display_matrix)
+                return AVERROR(ENOMEM);
+
+            for (i = 0; i < 3; i++)
+                for (j = 0; j < 3; j++)
+                    sc->display_matrix[i * 3 + j] = c->movie_display_matrix[i][j];
+        } else { // otherwise multiply the two and store the result
+            int64_t val = 0;
+            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_FP2INT(display_matrix[i][e], sh) *
+                               CONV_FP2INT(c->movie_display_matrix[e][j], sh);
+                    }
+                    sc->display_matrix[i * 3 + j] = CONV_INT2FP(val, sh);
+                    val = 0;
+                }
+            }
+        }
+    }
+
     // transform the display width/height according to the matrix
     // to keep the same scale, use [width height 1<<16]
     if (width && height && sc->display_matrix) {
         double disp_transform[2];
 
         for (i = 0; i < 2; i++)
-            disp_transform[i] = hypot(display_matrix[i][0], display_matrix[i][1]);
+            disp_transform[i] = hypot(sc->display_matrix[i * 3],
+                                      sc->display_matrix[i * 3 + 1]);
 
         if (disp_transform[0] > 0       && disp_transform[1] > 0 &&
             disp_transform[0] < (1<<24) && disp_transform[1] < (1<<24) &&
diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
index 4b5885b..3210ea6 100644
--- a/tests/fate/mov.mak
+++ b/tests/fate/mov.mak
@@ -4,7 +4,8 @@  FATE_MOV = fate-mov-3elist \
            fate-mov-1elist-noctts \
            fate-mov-elist-starts-ctts-2ndsample \
            fate-mov-1elist-ends-last-bframe \
-           fate-mov-2elist-elist1-ends-bframe
+           fate-mov-2elist-elist1-ends-bframe \
+           fate-mov-movie-display-matrix \
 
 FATE_SAMPLES_AVCONV += $(FATE_MOV)
 
@@ -26,3 +27,6 @@  fate-mov-1elist-ends-last-bframe: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-1e
 
 # Makes sure that we handle timestamps of packets in case of multiple edit lists with one of them ending on a B-frame correctly.
 fate-mov-2elist-elist1-ends-bframe: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-2elist-elist1-ends-bframe.mov
+
+# File contains a movie display matrix
+fate-mov-movie-display-matrix: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/moviedispmat.mp4 -frames 1
diff --git a/tests/ref/fate/mov-movie-display-matrix b/tests/ref/fate/mov-movie-display-matrix
new file mode 100644
index 0000000..4ca0308
--- /dev/null
+++ b/tests/ref/fate/mov-movie-display-matrix
@@ -0,0 +1,10 @@ 
+#format: frame checksums
+#version: 2
+#hash: MD5
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 540x576
+#sar 0: 93207/65536
+#stream#, dts,        pts, duration,     size, hash
+0,          0,          0,        1,   466560, e3ec6c8f40c864b41edbdd438d642ac0