Bug 73040

Summary: FloatQuad::isRectilinear() returns false for 180degree rotations
Product: WebKit Reporter: Dana Jansens <danakj@chromium.org>
Component: New BugsAssignee: Dana Jansens <danakj@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: akling@apple.com, backer@chromium.org, jamesr@chromium.org, piman@chromium.org, reveman@chromium.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 70634, 72965    
Attachments:
Description Flags
Patch
none
use std::numeric_limits<float>::epsilon() for equality comparison
none
Patch
none
Patch
none
Patch none

Description From 2011-11-23 12:04:10 PST
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.
------- Comment #1 From 2011-11-23 12:07:05 PST -------
Created an attachment (id=116392) [details]
Patch
------- Comment #2 From 2011-11-23 12:42:21 PST -------
(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;
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()?
------- Comment #3 From 2011-11-23 13:18:16 PST -------
(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.

> > 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 ?
------- Comment #4 From 2011-11-23 13:30:19 PST -------
Created an attachment (id=116411) [details]
use std::numeric_limits<float>::epsilon() for equality comparison
------- Comment #5 From 2011-11-23 14:12:09 PST -------
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 116392 [details] [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.
------- Comment #6 From 2011-11-27 11:07:46 PST -------
(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?

> 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.
------- Comment #7 From 2011-11-28 07:10:20 PST -------
(In reply to comment #6)
> (From update of attachment 116411 [details] [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.
------- Comment #8 From 2011-11-28 07:59:04 PST -------
> 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.
------- Comment #9 From 2011-11-28 08:02:57 PST -------
Created an attachment (id=116761) [details]
Patch
------- Comment #10 From 2011-11-28 10:54:39 PST -------
(From update of attachment 116761 [details])
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?
------- Comment #11 From 2011-11-28 11:24:25 PST -------
(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?
------- Comment #12 From 2011-11-28 11:26:45 PST -------
Created an attachment (id=116790) [details]
Patch
------- Comment #13 From 2011-11-28 12:00:07 PST -------
(From update of attachment 116790 [details])
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
------- Comment #14 From 2011-11-28 12:50:32 PST -------
Created an attachment (id=116802) [details]
Patch
------- Comment #15 From 2011-11-28 12:55:09 PST -------
(From update of attachment 116802 [details])
Awesome! Thanks.
------- Comment #16 From 2011-11-28 16:17:13 PST -------
(From update of attachment 116802 [details])
Clearing flags on attachment: 116802

Committed r101300: <http://trac.webkit.org/changeset/101300>
------- Comment #17 From 2011-11-28 16:17:18 PST -------
All reviewed patches have been landed.  Closing bug.