FloatQuad::isRectilinear() returns false for 180degree rotations

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

isRectilinear can't handle rotations that cause slight rounding errors.  Instead of testing floats for equality, we should test that they are sufficiently close.

This changes the test to check that floats are equal to 13 decimal places.

[Dana Jansens]

Created attachment 116392 [details]
Patch

[David Reveman]

Comment on attachment 116392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116392&action=review

Good catch! This will also increase the number of cases where we can avoid the anti-aliasing shader.

> Source/WebCore/platform/graphics/FloatQuad.cpp:90
> +    return a - b < 0.00000000000001 && a - b > -0.00000000000001;

I'd put that magic value in a constant:
const float epsilon = 0.00000000000001;
that's enough to make sure there's no confusion about its use. Maybe it also doesn't have to be that small? It just has to be small enough to not affect rendering, right? We have to make a similar check in CCTiledLayerImpl::drawTiles. I used 1 / 1024 there.

and maybe "return fabs(a - b) < epsilon" would be cleaner?

> Source/WebCore/platform/graphics/FloatQuad.cpp:93
>  bool FloatQuad::isRectilinear() const

Maybe it's a bad idea to do this approximation in isRectilinear(). How about adding a FloatQuad::isCloseToRectilinear()?

[Dana Jansens]

(In reply to comment #2)
> (From update of attachment 116392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116392&action=review
> 
> Good catch! This will also increase the number of cases where we can avoid the anti-aliasing shader.
> 
> > Source/WebCore/platform/graphics/FloatQuad.cpp:90
> > +    return a - b < 0.00000000000001 && a - b > -0.00000000000001;
> 
> I'd put that magic value in a constant:
> const float epsilon = 0.00000000000001;

Sure.

> that's enough to make sure there's no confusion about its use. Maybe it also doesn't have to be that small? It just has to be small enough to not affect rendering, right? We have to make a similar check in CCTiledLayerImpl::drawTiles. I used 1 / 1024 there.

I guess this would be the right value to use in both cases: http://msdn.microsoft.com/en-us/library/6x7575x3(v=vs.71).aspx

> and maybe "return fabs(a - b) < epsilon" would be cleaner?

I think this has an extra floating point instead of boolean instruction (the fabs() vs &&), but I see it used numerous times in the code already so yup. Thanks.

> > Source/WebCore/platform/graphics/FloatQuad.cpp:93
> >  bool FloatQuad::isRectilinear() const
> 
> Maybe it's a bad idea to do this approximation in isRectilinear(). How about adding a FloatQuad::isCloseToRectilinear()?

Are there places that would want to check isRectilinear and expect a 180 degree rotation to fail ?

[Dana Jansens]

Created attachment 116411 [details]
use std::numeric_limits<float>::epsilon() for equality comparison

[David Reveman]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 116392 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=116392&action=review
> > 
> > Good catch! This will also increase the number of cases where we can avoid the anti-aliasing shader.
> > 
> > > Source/WebCore/platform/graphics/FloatQuad.cpp:90
> > > +    return a - b < 0.00000000000001 && a - b > -0.00000000000001;
> > 
> > I'd put that magic value in a constant:
> > const float epsilon = 0.00000000000001;
> 
> Sure.
> 
> > that's enough to make sure there's no confusion about its use. Maybe it also doesn't have to be that small? It just has to be small enough to not affect rendering, right? We have to make a similar check in CCTiledLayerImpl::drawTiles. I used 1 / 1024 there.
> 
> I guess this would be the right value to use in both cases: http://msdn.microsoft.com/en-us/library/6x7575x3(v=vs.71).aspx
> 
> > and maybe "return fabs(a - b) < epsilon" would be cleaner?
> 
> I think this has an extra floating point instead of boolean instruction (the fabs() vs &&), but I see it used numerous times in the code already so yup. Thanks.

Thanks, it's just for readability.

> 
> > > Source/WebCore/platform/graphics/FloatQuad.cpp:93
> > >  bool FloatQuad::isRectilinear() const
> > 
> > Maybe it's a bad idea to do this approximation in isRectilinear(). How about adding a FloatQuad::isCloseToRectilinear()?
> 
> Are there places that would want to check isRectilinear and expect a 180 degree rotation to fail ?

No need for a isCloseToRectilinear if you use std::numeric_limits<float>::epsilon() like in the current patch. That would only make sense if we rounded based on pixel precision. 

Current patch looks good to me.

[James Robinson]

Comment on attachment 116411 [details]
use std::numeric_limits<float>::epsilon() for equality comparison

View in context: https://bugs.webkit.org/attachment.cgi?id=116411&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (will be covered in layout tests for opaque flag: https://bugs.webkit.org/show_bug.cgi?id=70634)

really? you can't even right a unit test for this?

> Source/WebCore/platform/graphics/FloatQuad.cpp:89
> +static inline bool equal(float a, float b)

do we actually need this? are there actual cases where exact comparisons fail but this succeeds?

this function name isn't very good since it isn't actually testing for equality

> Source/WebCore/platform/graphics/FloatQuad.cpp:91
> +    return fabs(a - b) < std::numeric_limits<float>::epsilon();

WebKit style is to not use the 'std::' qualifier here, add an appropriate using definition at the top of the .cpp and use numeric_limits<> unqualified.

[Dana Jansens]

(In reply to comment #6)
> (From update of attachment 116411 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116411&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (will be covered in layout tests for opaque flag: https://bugs.webkit.org/show_bug.cgi?id=70634)
> 
> really? you can't even right a unit test for this?

I can make a chromium unit test sure. Just not looking to be redirected to fixing the general unit test framework to work with WebCore again.

> > Source/WebCore/platform/graphics/FloatQuad.cpp:89
> > +static inline bool equal(float a, float b)
> 
> do we actually need this? are there actual cases where exact comparisons fail but this succeeds?

Rotate a rect by 180 degrees and an == test will fail in the 12th decimal place or so. Thus is no longer considered opaque and won't use AA etc.

> this function name isn't very good since it isn't actually testing for equality

ok will change.

> > Source/WebCore/platform/graphics/FloatQuad.cpp:91
> > +    return fabs(a - b) < std::numeric_limits<float>::epsilon();
> 
> WebKit style is to not use the 'std::' qualifier here, add an appropriate using definition at the top of the .cpp and use numeric_limits<> unqualified.

ok! thanks.

[Dana Jansens]

> Source/WebCore/platform/graphics/FloatQuad.cpp:37:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]

Style check tells me to use the whole std namespace so I'll do that.

[Dana Jansens]

Created attachment 116761 [details]
Patch

[James Robinson]

Comment on attachment 116761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116761&action=review

Thanks for the test. R=me

> Source/WebKit/chromium/tests/FloatQuadTest.cpp:26
> +// FIXME: If we get the TestWebKitAPI framework to bring a full Frame + DOM stack
> +// in a portable way, this test should be shared with all ports!

this FIXME doesn't really belong in this test - it's not something to do with this file

> Source/WebKit/chromium/tests/FloatQuadTest.cpp:52
> +    TransformationMatrix nonRectilinearTrans[8];

are there any transformations other than rotations that are interesting for this logic?

[Dana Jansens]

(In reply to comment #10)
> this FIXME doesn't really belong in this test - it's not something to do with this file

Yep copy/paste error.

> are there any transformations other than rotations that are interesting for this logic?

TBH I'm not entirely sure so I'll throw in some scale+rotation and some reeeeeally small skews for good measure. Is there something in particular you were thinking of?

[Dana Jansens]

Created attachment 116790 [details]
Patch

[James Robinson]

Comment on attachment 116790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116790&action=review

> Source/WebKit/chromium/tests/FloatQuadTest.cpp:54
> +        EXPECT_EQ(true, quad.isRectilinear());

i believe gtest has EXPECT_TRUE and EXPECT_FALSE

> Source/WebKit/chromium/tests/FloatQuadTest.cpp:72
> +        EXPECT_EQ(false, quad.isRectilinear());

EXPECT_FALSE

[Dana Jansens]

Created attachment 116802 [details]
Patch

[James Robinson]

Comment on attachment 116802 [details]
Patch

Awesome! Thanks.

[WebKit Review Bot]

Comment on attachment 116802 [details]
Patch

Clearing flags on attachment: 116802

Committed r101300: <http://trac.webkit.org/changeset/101300>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.