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

[Michael Niedermayer]

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 at 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     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161008/4c664453/attachment.sig>