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

Baptiste Coudurier baptiste.coudurier
Wed Jul 16 05:10:31 CEST 2008


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 ?

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc.                                http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA




More information about the ffmpeg-devel mailing list