[FFmpeg-devel] [PATCH] Use tkhd matrix for proper display in mov
Michael Niedermayer
michaelni
Wed Jul 23 19:43:28 CEST 2008
On Wed, Jul 23, 2008 at 12:26:39PM -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.
>
> I don't know how much patch-submitter testing counts for, but FWIW below is an outline of what I tested:
>
> I had 7 mp4s and 9 movs lying around (not counting the original problem video), none of which had any track header matrix but the default identity matrix. As expected, there was no change in the output videos.
>
> Then I modified the tkhd matrices of a mov and a mp4 which were 16x9 videos to make them 4x3, and a mov and a mp4 that were 4x3 videos to make them 16x9. Those four output videos had the correct modified aspect ratio.
fine, then patch ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20080723/652a7c51/attachment.pgp>
More information about the ffmpeg-devel
mailing list