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

[Michael Niedermayer]

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

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20080715/dbfcd40c/attachment.pgp>