[FFmpeg-devel] [PATCH] Use tkhd matrix for proper display in mov

Michael Niedermayer michaelni
Wed Jul 16 18:26:27 CEST 2008


On Wed, Jul 16, 2008 at 12:15:28PM -0400, John Schmiederer wrote:
> >On Wed, Jul 16, 2008 at 11:48:20AM -0400, John Schmiederer wrote:
> >> >
> >> >Hi,
> >> >
> >> >Michael Niedermayer wrote:
> >> >> On Thu, May 29, 2008 at 10:49:22AM -0400, John Schmiederer wrote:
> >> >>>>>>>>>> On Tue, May 27, 2008 at 02:49:24PM -0400, John Schmiederer
> >> >>>>>> wrote:
> >> >>>>>>>>>>>>> Attached is a patch to account for the transformation
> >> >>>>>>>>>>>>> matrix
> >> >>>>>>>>>> contained in the tkhd atom for proper display width/height.
> >> >>>>> [...]
> >> >>>>>
> >> >>>>>>>>> +    //transform the display width/height according to the
> >> >>>> matrix
> >> >>>>>>>>> +    if (width && height) {
> >> >>>>>>>>> +        for (i = 0; i < 2; i++)
> >> >>>>>>>>> +            disp_transform[i] =
> >> >>>>>>>>> +                (int64_t) width * display_matrix[0][i] +
> >> >>>>>>>>> +                (int64_t) height * display_matrix[1][i] +
> >> >>>>>>>>> +                display_matrix[2][i];
> >> >>>>>>>> This is not what the original patch did.
> >> >>>>>>> It may look a little different, but the functionality has not
> >> >>>>>> changed.
> >> >>>>>>
> >> >>>>>> You removed the scaling by 1<<16 and 1<<30 the code is no
> >> >>>>>> longer doing the same. The relative scaling of w*matrix[0] and
> >> >>>>>> matrix[2] has changed They are added together so the result is
> >> >>>>>> more than
> >> >just
> >> >>>>>> wrong by a constant scale factor.
> >> >>>>> Argh!  You're right, the math is off.
> >> >>>>> Although I maintain that the functionality didn't change from
> >> >>>>> the first patch - that was wrong, too. =) I forgot that width
> >> >>>>> and
> >> >height
> >> >>>>> were 16.16 fixed, so instead of multiplying by [width height 1]
> >> >>>>> it should have always been [width height 1<<16]
> >> >>>>>
> >> >>>>> The 1<<16 vs 1<<30 scaling is no longer an issue though, as all
> >> >>>>> the
> >> >>>> 1<<30 scaled terms were in that last column of the display matrix
> >> >that
> >> >>>> I no longer read in or use (display_matrix[*][2], not
> >> >>>> display_matrix[2][*]).
> >> >>>>> So in this updated patch all the multiplied terms are in the
> >> >>>>> same
> >> >>>> scale.
> >> >>>>
> >> >>>> [...]
> >> >>>>
> >> >>>>> +    //transform the display width/height according to the
> >matrix
> >> >>>>> +    // to keep the same scale, use [width height 1<<16]
> >> >>>>> +    if (width && height) {
> >> >>>>> +        for (i = 0; i < 2; i++)
> >> >>>>> +            disp_transform[i] =
> >> >>>>> +                (int64_t) width * display_matrix[0][i] +
> >> >>>>> +                (int64_t) height * display_matrix[1][i] +
> >> >>>>> +                (int64_t) (display_matrix[2][i] << 16);
> >> >>>> with that order of operations display_matrix[2][i] << 16 can
> >> >overflow
> >> >>> Oops - I misapplied those parentheses to fix a "parentheses around
> >> >>> +
> >> >or - inside shift" warning.
> >> >>> Attached patch does it right this time.
> >> >>>
> >> >>>> also things can be vertically aligned like:
> >> >>>>                 (int64_t) width  * display_matrix[0][i] +
> >> >>>>                 (int64_t) height * display_matrix[1][i] +
> >> >>>>
> >> >>>> looks more pretty ...
> >> >>> Agreed.
> >> >>>
> >> >>>> anyway iam mostly ok with the patch after this, maybe baptiste
> >> >>>> has
> >> >some
> >> >>>> further comments though ...
> >> >>> Thanks again for all the help.
> >> >>
> >> >> patch looks ok if someone tested it with a few mov files
> >> >>
> >> >
> >> >Is it possible to have the exact quote from the specifications
> >> >describing the correct interpretation of these values ?
> >> >
> >> >Is this valid in mov or/and isom ?
> >>
> >> The tkhd matrix is defined in both the QuickTime (mov) documentation
> >and the ISO standard, as such I believe it is valid for both:
> >[...]
> >> "[...] Not all
> >> derived specifications use matrices; if they are not used, they shall
> >be set to the identity matrix, If a matrix is used, the point (p,q) is
> >transformed into (p', q') using the matrix as follows:
> >
> >Or in other words the matrix may or may not contain valid values in a
> >.mp4 I guess checking if the matrix is the identity matrix in .mp4 would
> >be required at minimum in addition to the patch.
> 
> I read it a little differently: the matrix will either have values to transform the track, which is valid, or it will have the identity matrix, which is also valid.
> If the matrix is the identity matrix, then (p', q') == (p, q), or in our terms the new width/height will be the same as the old width/height.

I know mp4 does not say it clearly, but just look at an mp4 file
channels=2 while it has 6 channels
Thats the way .mp4 is designed, values can be set correctly or to the default
value.
Specs based upon the base spec can require values to be set correctly but
they tend not to require it.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080716/af939cdf/attachment.pgp>



More information about the ffmpeg-devel mailing list