[FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix
Michael Niedermayer
michael at niedermayer.cc
Sat Oct 8 03:55:20 EEST 2016
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 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
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/1daebfc4/attachment.sig>
More information about the ffmpeg-devel
mailing list