[FFmpeg-devel] [PATCH] Use tkhd matrix for proper display in mov
John Schmiederer
jschmiederer
Wed Jul 16 18:15:28 CEST 2008
>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.
-John
More information about the ffmpeg-devel
mailing list