>> >> >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.
-John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ff_mov_tkhd_matrix8.diff
Type: application/octet-stream
Size: 2276 bytes
Desc: ff_mov_tkhd_matrix8.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080716/f52e5cb8/attachment.obj>