Fix ellipse hit testing in the non-circle case

Fix ellipse hit testing in the non-circle case

RenderSVGEllipse::shapeDependentStrokeContains() was incorrectly
considering the outer and inner offset curves of the ellipse's stroke
(the outer and inner boundary curves) to be the ellipses formed by
increasing/decreasing the radii by half the stroke width; however, the
offset curves are only ellipses when the ellipse is a circle. This would
cause problems where shapeDependentStrokeContains() would incorrectly
return false for points that actually are in the stroke of the ellipse.

Except in the case of continuous-stroke circles, we now always fall back
to the graphics library for the stroke hit test result.

BUG=393143
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189109

[dtrebbien, 2015-01-25 17:03:39]

dtrebbien@gmail.com changed reviewers:
+ ed@opera.com, fs@opera.com, pdr@chromium.org

[dtrebbien, 2015-01-25 17:09:06]

On 2015/01/25 17:03:58, dtrebbien wrote:

I had a note in the CL description about
PerformanceTests/SVG/SvgHitTesting.html, but I don't think that this is
necessary, so I have removed it.  The 7.25% figure came from testing Debug
builds.  When testing Release builds, I get around 4%.

[pdr., 2015-01-25 20:54:12]

On 2015/01/25 at 17:09:06, dtrebbien wrote:
> On 2015/01/25 17:03:58, dtrebbien wrote:
> 
> I had a note in the CL description about
PerformanceTests/SVG/SvgHitTesting.html, but I don't think that this is
necessary, so I have removed it.  The 7.25% figure came from testing Debug
builds.  When testing Release builds, I get around 4%.

This is excellent code, well commented, etc. I fear this is too much complexity
to add this high in the pipeline though, as Skia should do the exact computation
you've derived here. If the fast-path optimization doesn't work for non-circle
ellipses, can we just fall back to Skia?

[dtrebbien, 2015-01-25 23:58:55]

On 2015/01/25 20:54:12, pdr wrote:
> On 2015/01/25 at 17:09:06, dtrebbien wrote:
> > On 2015/01/25 17:03:58, dtrebbien wrote:
> > 
> > I had a note in the CL description about
> PerformanceTests/SVG/SvgHitTesting.html, but I don't think that this is
> necessary, so I have removed it.  The 7.25% figure came from testing Debug
> builds.  When testing Release builds, I get around 4%.
> 
> This is excellent code, well commented, etc. I fear this is too much
complexity
> to add this high in the pipeline though, as Skia should do the exact
computation
> you've derived here. If the fast-path optimization doesn't work for non-circle
> ellipses, can we just fall back to Skia?

Yes, I think that it will work to fall back to Skia for the stroke hit test. 
The concern is how this will affect performance.  I will check
PerformanceTests/SVG/SvgHitTesting.html to see what the impact is...

[dtrebbien, 2015-01-26 01:00:51]

On 2015/01/25 23:58:55, dtrebbien wrote:
> On 2015/01/25 20:54:12, pdr wrote:
> > On 2015/01/25 at 17:09:06, dtrebbien wrote:
> > > On 2015/01/25 17:03:58, dtrebbien wrote:
> > > 
> > > I had a note in the CL description about
> > PerformanceTests/SVG/SvgHitTesting.html, but I don't think that this is
> > necessary, so I have removed it.  The 7.25% figure came from testing Debug
> > builds.  When testing Release builds, I get around 4%.
> > 
> > This is excellent code, well commented, etc. I fear this is too much
> complexity
> > to add this high in the pipeline though, as Skia should do the exact
> computation
> > you've derived here. If the fast-path optimization doesn't work for
non-circle
> > ellipses, can we just fall back to Skia?
> 
> Yes, I think that it will work to fall back to Skia for the stroke hit test. 
> The concern is how this will affect performance.  I will check
> PerformanceTests/SVG/SvgHitTesting.html to see what the impact is...

It turns out to be not that bad.  I am seeing that the performance test is only
around 10% slower on my dev machine.

I will prepare a new Patch Set...

[dtrebbien, 2015-01-26 12:16:31]

On 2015/01/26 01:00:51, dtrebbien wrote:
> On 2015/01/25 23:58:55, dtrebbien wrote:
> > On 2015/01/25 20:54:12, pdr wrote:
> > > On 2015/01/25 at 17:09:06, dtrebbien wrote:
> > > > On 2015/01/25 17:03:58, dtrebbien wrote:
> > > > 
> > > > I had a note in the CL description about
> > > PerformanceTests/SVG/SvgHitTesting.html, but I don't think that this is
> > > necessary, so I have removed it.  The 7.25% figure came from testing Debug
> > > builds.  When testing Release builds, I get around 4%.
> > > 
> > > This is excellent code, well commented, etc. I fear this is too much
> > complexity
> > > to add this high in the pipeline though, as Skia should do the exact
> > computation
> > > you've derived here. If the fast-path optimization doesn't work for
> non-circle
> > > ellipses, can we just fall back to Skia?
> > 
> > Yes, I think that it will work to fall back to Skia for the stroke hit test.

> > The concern is how this will affect performance.  I will check
> > PerformanceTests/SVG/SvgHitTesting.html to see what the impact is...
> 
> It turns out to be not that bad.  I am seeing that the performance test is
only
> around 10% slower on my dev machine.
> 
> I will prepare a new Patch Set...

I have uploaded a new Patch Set.

[fs, 2015-01-26 12:25:21]

https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGEllipse.cpp (left):

https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGEllipse.cpp:73: m_fillBoundingBox =
FloatRect(m_center.x() - m_radii.width(), m_center.y() - m_radii.height(), 2 *
m_radii.width(), 2 * m_radii.height());
This code is still valid, no?

[dtrebbien, 2015-01-26 12:55:12]

On 2015/01/26 12:25:21, fs wrote:
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGEllipse.cpp (left):
> 
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGEllipse.cpp:73: m_fillBoundingBox =
> FloatRect(m_center.x() - m_radii.width(), m_center.y() - m_radii.height(), 2 *
> m_radii.width(), 2 * m_radii.height());
> This code is still valid, no?

Yes, it's still valid.  I figured that because we need to call
RenderSVGShape::updateShapeFromElement() to set the path data, and this also
sets the stroke/fill/hitTestStroke bounding boxes, that it could be removed
because it was duplicated work.

[fs, 2015-01-26 13:17:35]

On 2015/01/26 12:55:12, dtrebbien wrote:
> On 2015/01/26 12:25:21, fs wrote:
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > File Source/core/rendering/svg/RenderSVGEllipse.cpp (left):
> > 
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > Source/core/rendering/svg/RenderSVGEllipse.cpp:73: m_fillBoundingBox =
> > FloatRect(m_center.x() - m_radii.width(), m_center.y() - m_radii.height(), 2
*
> > m_radii.width(), 2 * m_radii.height());
> > This code is still valid, no?
> 
> Yes, it's still valid.  I figured that because we need to call
> RenderSVGShape::updateShapeFromElement() to set the path data, and this also
> sets the stroke/fill/hitTestStroke bounding boxes, that it could be removed
> because it was duplicated work.

We don't need the path up front - i.e. we can defer creating it until needed.

[dtrebbien, 2015-01-27 16:35:04]

On 2015/01/26 13:17:35, fs wrote:
> On 2015/01/26 12:55:12, dtrebbien wrote:
> > On 2015/01/26 12:25:21, fs wrote:
> > >
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > > File Source/core/rendering/svg/RenderSVGEllipse.cpp (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > > Source/core/rendering/svg/RenderSVGEllipse.cpp:73: m_fillBoundingBox =
> > > FloatRect(m_center.x() - m_radii.width(), m_center.y() - m_radii.height(),
2
> *
> > > m_radii.width(), 2 * m_radii.height());
> > > This code is still valid, no?
> > 
> > Yes, it's still valid.  I figured that because we need to call
> > RenderSVGShape::updateShapeFromElement() to set the path data, and this also
> > sets the stroke/fill/hitTestStroke bounding boxes, that it could be removed
> > because it was duplicated work.
> 
> We don't need the path up front - i.e. we can defer creating it until needed.

I have uploaded a new Patch Set with this change to lazily create the path.

[fs, 2015-01-27 16:39:58]

On 2015/01/27 16:35:04, dtrebbien wrote:
> On 2015/01/26 13:17:35, fs wrote:
> > On 2015/01/26 12:55:12, dtrebbien wrote:
> > > On 2015/01/26 12:25:21, fs wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > > > File Source/core/rendering/svg/RenderSVGEllipse.cpp (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > > > Source/core/rendering/svg/RenderSVGEllipse.cpp:73: m_fillBoundingBox =
> > > > FloatRect(m_center.x() - m_radii.width(), m_center.y() -
m_radii.height(),
> 2
> > *
> > > > m_radii.width(), 2 * m_radii.height());
> > > > This code is still valid, no?
> > > 
> > > Yes, it's still valid.  I figured that because we need to call
> > > RenderSVGShape::updateShapeFromElement() to set the path data, and this
also
> > > sets the stroke/fill/hitTestStroke bounding boxes, that it could be
removed
> > > because it was duplicated work.
> > 
> > We don't need the path up front - i.e. we can defer creating it until
needed.
> 
> I have uploaded a new Patch Set with this change to lazily create the path.

LGTM

[dtrebbien, 2015-01-27 16:43:19]

On 2015/01/27 16:39:58, fs wrote:
> On 2015/01/27 16:35:04, dtrebbien wrote:
> > On 2015/01/26 13:17:35, fs wrote:
> > > On 2015/01/26 12:55:12, dtrebbien wrote:
> > > > On 2015/01/26 12:25:21, fs wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > > > > File Source/core/rendering/svg/RenderSVGEllipse.cpp (left):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > > > > Source/core/rendering/svg/RenderSVGEllipse.cpp:73: m_fillBoundingBox =
> > > > > FloatRect(m_center.x() - m_radii.width(), m_center.y() -
> m_radii.height(),
> > 2
> > > *
> > > > > m_radii.width(), 2 * m_radii.height());
> > > > > This code is still valid, no?
> > > > 
> > > > Yes, it's still valid.  I figured that because we need to call
> > > > RenderSVGShape::updateShapeFromElement() to set the path data, and this
> also
> > > > sets the stroke/fill/hitTestStroke bounding boxes, that it could be
> removed
> > > > because it was duplicated work.
> > > 
> > > We don't need the path up front - i.e. we can defer creating it until
> needed.
> > 
> > I have uploaded a new Patch Set with this change to lazily create the path.
> 
> LGTM

I am going to wait to check the CQ bit until r189035 rolls (just to make sure
that there won't be merge conflicts, or upload a new Patch Set if so).

blink-deps-roller is currently trying to roll 189000:189033: 
https://codereview.chromium.org/user/blink-deps-roller

[fs, 2015-01-27 17:09:18]

On 2015/01/27 16:43:19, dtrebbien wrote:
> On 2015/01/27 16:39:58, fs wrote:
> > On 2015/01/27 16:35:04, dtrebbien wrote:
> > > On 2015/01/26 13:17:35, fs wrote:
> > > > On 2015/01/26 12:55:12, dtrebbien wrote:
> > > > > On 2015/01/26 12:25:21, fs wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > > > > > File Source/core/rendering/svg/RenderSVGEllipse.cpp (left):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/877553002/diff/20001/Source/core/rendering/sv...
> > > > > > Source/core/rendering/svg/RenderSVGEllipse.cpp:73: m_fillBoundingBox
=
> > > > > > FloatRect(m_center.x() - m_radii.width(), m_center.y() -
> > m_radii.height(),
> > > 2
> > > > *
> > > > > > m_radii.width(), 2 * m_radii.height());
> > > > > > This code is still valid, no?
> > > > > 
> > > > > Yes, it's still valid.  I figured that because we need to call
> > > > > RenderSVGShape::updateShapeFromElement() to set the path data, and
this
> > also
> > > > > sets the stroke/fill/hitTestStroke bounding boxes, that it could be
> > removed
> > > > > because it was duplicated work.
> > > > 
> > > > We don't need the path up front - i.e. we can defer creating it until
> > needed.
> > > 
> > > I have uploaded a new Patch Set with this change to lazily create the
path.
> > 
> > LGTM
> 
> I am going to wait to check the CQ bit until r189035 rolls (just to make sure
> that there won't be merge conflicts, or upload a new Patch Set if so).
> 
> blink-deps-roller is currently trying to roll 189000:189033: 
> https://codereview.chromium.org/user/blink-deps-roller

SGTM

[dtrebbien, 2015-01-28 12:06:20]

The CQ bit was checked by dtrebbien@gmail.com

[commit-bot: I haz the power, 2015-01-28 12:07:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877553002/40001

[commit-bot: I haz the power, 2015-01-28 13:20:41]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189109

[dtrebbien, 2015-01-29 19:42:14]

On 2015/01/28 13:20:41, I haz the power (commit-bot) wrote:
> Committed patchset #3 (id:40001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=189109

Hi fs and pdr,

A new issue has been opened for the resulting performance decrease: 
http://crbug.com/453164

How should Issue 453164 be handled?

[pdr., 2015-01-29 19:50:30]

On 2015/01/29 at 19:42:14, dtrebbien wrote:
> On 2015/01/28 13:20:41, I haz the power (commit-bot) wrote:
> > Committed patchset #3 (id:40001) as
> > https://src.chromium.org/viewvc/blink?view=rev&revision=189109
> 
> Hi fs and pdr,
> 
> A new issue has been opened for the resulting performance decrease: 
http://crbug.com/453164
> 
> How should Issue 453164 be handled?

I responded. Don't be scared of the perf sheriffs, they are just here to help us
:)