[FFmpeg-devel] [PATCH] MMX/floating-point crash issue

[Michael Niedermayer]

On Wed, Jan 11, 2012 at 10:22:15PM -0800, Ray Simard wrote:
> Forgot to attach the patch...
> 
> 
> On 1/11/2012 10:18 PM, Ray Simard wrote:
> > The following requires the patch for the uninitialized variable above to
> > be applied first ("Odd, random-appearing crashes"), so I'm mentioning
> > this here, after that.
> > 
> > As Reimar Doeffinger pointed out, there is no reason to use doubles in
> > these loops. As it happens, the result is worse than needless overhead;
> > the assignment of the result of the MMX calculations called by the CMP
> > macro to the double floating-point members of mv without an intervening
> > call to emms_c() was causing bogus data to be assigned, resulting in the
> > crashes. In fact, the values in mv are never used for anything except
> > integer values.
> > 
> > (My earlier, rather naive suggestion, to add emms_c() calls in the
> > loops, should be disregarded.  It was a good test to verify the cause of
> > the data-corruption and crash problem, but not a solution.)
> > 
> > The attached patch implements his suggestion to replace the MotionVector
> > variables in these cases with an integer-only version. I've tried it out
> > and it seems to be working perfectly.  At the moment I'm running FATE on
> > it. It's very straightforward and I'd be very surprised if there were
> > any problem with it.
> > 
> > 
> > Note: This does not affect the MotionVector member of the Transform
> > struct passed to avfilter_transform(). That is still double.
> > Determining whether or not that should also be changed requires further
> > scrutiny.
> > 
> > 
> > One of the loops in question. Others are similar. The struct mv points
> > to was a MotionVector struct in which x and y were double.
> > 
> >     if (deshake->search == EXHAUSTIVE) {
> >         // Compare every possible position - this is sloooow!
> >         for (y = -deshake->ry; y <= deshake->ry; y++) {
> >             for (x = -deshake->rx; x <= deshake->rx; x++) {
> >                 diff = CMP(cx - x, cy - y);
> >                 if (diff < smallest) {
> >                     smallest = diff;
> >                     mv->x = x;  <=== BOGUS.
> >                     mv->y = y;  <===  ""
> >                 }
> >             }
> >         }
> > 
> > Ray Simard
> > rhs.ffmpeg at sylvan-glade.com
> > 
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > 
> 
> 

>  vf_deshake.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> e803ba9a35cfa5aed884a8561585be67b3c75f36  motion-vectors-to-int.patch
> >From 4376dad32d2f41b7ae4e2c12bcdb9ab26f48b27d Mon Sep 17 00:00:00 2001

> From: Ray Simard <rhs.ffmpeg at sylvan-glade.com>

That should be reimar as its his patch IIRC
you can set the author via git commit --amend --author reimar

otherwise it looks good

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

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120112/fb8530e4/attachment.asc>