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

[Michael Niedermayer]

On Wed, Jul 16, 2008 at 05:10:40PM -0400, John Schmiederer wrote:
> >> >> >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.
> [...]
> Ok, updated patch checks if it's the identity matrix, and if so skips the calculation and setting of SAR.

Iam ok with the patch IF it has been tested with several mov and mp4 files
and did not break anything.

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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- 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/20080718/e153775c/attachment.pgp>