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

[John Schmiederer]

> On Wed, May 28, 2008 at 11:14:31AM -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.
> > > > >
> > > > > > The video
> > > > > >
> > > http://samples.mplayerhq.hu/mov/tkhd_matrix/white_zombie_scrunch.m
> > > > > > ov plays at 160x240 when it should really be scaled to
> 320x240.
> > > > >
> > > > > > +    int i;
> > > > > > +    int width;
> > > > > > +    int height;
> > > > > > +    float disp_transform[3];
> > > > > > +    float display_matrix[3][3];
> > > > >
> > > > > please use integers, there is no need for floats.
> > > > >
> > > > Ok, I changed display_matrix to int, but left disp_transform as
> > > > float
> > > to avoid any possible overflow from multiplying large ints.
> > >
> > > use a*(int64_t)b
> > >
> > All right - no more nasty floats - I switched it to int64_t.
> > The only caveat is a float cast when calculating the aspect ratio,
> since multiplying an int_64 by an int_32 won't fit in 64bit precision.
> >
> > To Reimar's note "we do have function for multiplying two 16.16
> fixed-point numbers correctly"; I couldn't find those functions.  If
> anyone knows where that is off the top of their head, I would be
> appreciated.  The closest thing I found was libavutil/integer, and that
> didn't look used.
> [...]
>
> > +    //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.
Originally the multiplication was
disp_transform[0] = width * matrix[0][0] + height * matrix[1][0] + matrix[2][0];
disp_transform[1] = width * matrix[0][1] + height * matrix[1][1] + matrix[2][1];
disp_transform[2] = width * matrix[0][2] + height * matrix[1][2] + matrix[2][2];

But as you pointed out earlier, dividing the new width and height by disp_transform[2] was unnecessary, so I took out that last loop iteration altogether, as disp_transform[2] is no longer used.


> > +
> > +        //the sample aspect ratio is new width/height divided by old
> width/height
> > +        st->codec->sample_aspect_ratio = av_d2q(
> > +            ((float) disp_transform[0] * height) /
> > +            ((float) disp_transform[1] * width), INT_MAX);
>
> float has insufficient precission to convert it to a 32bit fraction
> reliably

Ok, upgraded the cast to double.
Thanks for the precise precision pointers.

-John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ff_mov_tkhd_matrix5.diff
Type: application/octet-stream
Size: 1909 bytes
Desc: ff_mov_tkhd_matrix5.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080528/1a6d09ad/attachment.obj>