Bug 217563 – [MotionMark] Computing the fast bounding rect of an arc should not materialize a CGPathRef

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

Gives us a ~13% win on the Canvas Arcs subtest after enabling GPU process for canvas rendering.

[Wenson Hsieh]

Created attachment 411023 [details]
WIP for EWS

[Wenson Hsieh]

Created attachment 411026 [details]
Patch

[Wenson Hsieh]

Created attachment 411028 [details]
Fix ChangeLog typo

[Darin Adler]

Comment on attachment 411028 [details]
Fix ChangeLog typo

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

> Source/WebCore/platform/graphics/Path.cpp:406
> +            approximateBounds.extend(arc.offset);

This doesn’t look right to me. Why is FloatRect::extend the appropriate algorithm here? Because extend takes the offset as a point and expands the rectangle to encompass that point. That does not seem like what an arc’s offset does. On the other hand, I guess I don’t really know what an arc’s offset does, so maybe this is correct!

[Wenson Hsieh]

(In reply to Darin Adler from comment #4)
> Comment on attachment 411028 [details]
> Fix ChangeLog typo
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411028&action=review
> 
> > Source/WebCore/platform/graphics/Path.cpp:406
> > +            approximateBounds.extend(arc.offset);
> 
> This doesn’t look right to me. Why is FloatRect::extend the appropriate
> algorithm here? Because extend takes the offset as a point and expands the
> rectangle to encompass that point. That does not seem like what an arc’s
> offset does. On the other hand, I guess I don’t really know what an arc’s
> offset does, so maybe this is correct!

Ah, so (IIRC) the `offset` member — if hasOffset is true — allows the ArcData to contain a starting point that will be connected to the arc itself. I suppose the name ArcData is misleading here — it’s more like “Arc (with an optional line segment prepended to it)”.

[Darin Adler]

Comment on attachment 411028 [details]
Fix ChangeLog typo

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

>>> Source/WebCore/platform/graphics/Path.cpp:406
>>> +            approximateBounds.extend(arc.offset);
>> 
>> This doesn’t look right to me. Why is FloatRect::extend the appropriate algorithm here? Because extend takes the offset as a point and expands the rectangle to encompass that point. That does not seem like what an arc’s offset does. On the other hand, I guess I don’t really know what an arc’s offset does, so maybe this is correct!
> 
> Ah, so (IIRC) the `offset` member — if hasOffset is true — allows the ArcData to contain a starting point that will be connected to the arc itself. I suppose the name ArcData is misleading here — it’s more like “Arc (with an optional line segment prepended to it)”.

We should rename "offset" then, because ... it’s not an offset from anything!

[Wenson Hsieh]

Comment on attachment 411028 [details]
Fix ChangeLog typo

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

>>>> Source/WebCore/platform/graphics/Path.cpp:406
>>>> +            approximateBounds.extend(arc.offset);
>>> 
>>> This doesn’t look right to me. Why is FloatRect::extend the appropriate algorithm here? Because extend takes the offset as a point and expands the rectangle to encompass that point. That does not seem like what an arc’s offset does. On the other hand, I guess I don’t really know what an arc’s offset does, so maybe this is correct!
>> 
>> Ah, so (IIRC) the `offset` member — if hasOffset is true — allows the ArcData to contain a starting point that will be connected to the arc itself. I suppose the name ArcData is misleading here — it’s more like “Arc (with an optional line segment prepended to it)”.
> 
> We should rename "offset" then, because ... it’s not an offset from anything!

Yep, that’s a good point! I’ll go ahead and rename it in this patch, as it doesn’t show up in a whole lot of places, and it’s a simple mechanical change. I’m thinking something like `initialMoveLocation` would make it much more obvious what effect this has.

[Darin Adler]

I see now that the whole concept of an arc being preceded by a straight line segment comes straight from CoreGraphics and other PostScript-influenced graphics APIs. It does say in CGPathAddArc documentation "possibly preceded by a straight line segment". So I am making my peace with tis.

Now that I understand this, I get that the name "offset" isn’t right. I think it should just be "start", just as in LineData. And also, as in LineData, there is no need for a "hasOffset" field. If there is no position set after opening the path, then the start is 0,0 as implemented in Path::addLineTo.

[Wenson Hsieh]

(In reply to Darin Adler from comment #8)
> I see now that the whole concept of an arc being preceded by a straight line
> segment comes straight from CoreGraphics and other PostScript-influenced
> graphics APIs. It does say in CGPathAddArc documentation "possibly preceded
> by a straight line segment". So I am making my peace with tis.
> 
> Now that I understand this, I get that the name "offset" isn’t right. I
> think it should just be "start", just as in LineData. And also, as in
> LineData, there is no need for a "hasOffset" field. If there is no position
> set after opening the path, then the start is 0,0 as implemented in
> Path::addLineTo.

`start` sounds good to me, I‘ll go with that.

As for what is currently `bool hasOffset`, that was added to deal with the fact that the act of calling `CGPathMoveToPoint()` with (0, 0) actually changes the bounds of the path:

Trying it out in a simple test app, I see:

    CGPathAddArc(path, NULL, 100, 100, 10, 0, 0.2, true);
    NSLog(@"> Bounds: %@", NSStringFromRect(CGPathGetBoundingBox(path)));

    > Bounds: {{90, 90}, {20, 20}}

...however:

    CGPathMoveToPoint(path, NULL, 0, 0);
    CGPathAddArc(path, NULL, 100, 100, 10, 0, 0.2, true);
    NSLog(@"> Bounds: %@", NSStringFromRect(CGPathGetBoundingBox(path)));

    > Bounds: {{0, 0}, {110, 110}}

[Wenson Hsieh]

Created attachment 411054 [details]
Patch for landing

[EWS]

Committed r268320: <https://trac.webkit.org/changeset/268320>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411054 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70183252>

[Darin Adler]

(In reply to Wenson Hsieh from comment #9)
> As for what is currently `bool hasOffset`, that was added to deal with the
> fact that the act of calling `CGPathMoveToPoint()` with (0, 0) actually
> changes the bounds of the path:
> 
> Trying it out in a simple test app, I see:
> 
>     CGPathAddArc(path, NULL, 100, 100, 10, 0, 0.2, true);
>     NSLog(@"> Bounds: %@", NSStringFromRect(CGPathGetBoundingBox(path)));
> 
>     > Bounds: {{90, 90}, {20, 20}}
> 
> ...however:
> 
>     CGPathMoveToPoint(path, NULL, 0, 0);
>     CGPathAddArc(path, NULL, 100, 100, 10, 0, 0.2, true);
>     NSLog(@"> Bounds: %@", NSStringFromRect(CGPathGetBoundingBox(path)));
> 
>     > Bounds: {{0, 0}, {110, 110}}

That seems like a mystery. Does stroking the path draw the line from 0,0 or not in that first case? If not, is there code that relies on this?

As a future refinement, I suggest using Optional rather than a separate boolean if the start point truly is optional.

[Wenson Hsieh]

(In reply to Darin Adler from comment #13)
> (In reply to Wenson Hsieh from comment #9)
> > As for what is currently `bool hasOffset`, that was added to deal with the
> > fact that the act of calling `CGPathMoveToPoint()` with (0, 0) actually
> > changes the bounds of the path:
> > 
> > Trying it out in a simple test app, I see:
> > 
> >     CGPathAddArc(path, NULL, 100, 100, 10, 0, 0.2, true);
> >     NSLog(@"> Bounds: %@", NSStringFromRect(CGPathGetBoundingBox(path)));
> > 
> >     > Bounds: {{90, 90}, {20, 20}}
> > 
> > ...however:
> > 
> >     CGPathMoveToPoint(path, NULL, 0, 0);
> >     CGPathAddArc(path, NULL, 100, 100, 10, 0, 0.2, true);
> >     NSLog(@"> Bounds: %@", NSStringFromRect(CGPathGetBoundingBox(path)));
> > 
> >     > Bounds: {{0, 0}, {110, 110}}
> 
> That seems like a mystery. Does stroking the path draw the line from 0,0 or
> not in that first case? If not, is there code that relies on this?

In the first case, stroking the path does not draw the line from (0, 0). However, it does in the second case. I believe there is a canvas layout test that exercises this (which is what originally led me to discover this corner case).

> 
> As a future refinement, I suggest using Optional rather than a separate
> boolean if the start point truly is optional.

I'll see if I can find a way to use Optional<FloatPoint> here. When I first wrote this, I used a separate `bool` to avoid pushing the size of this struct (currently, the separate bool flag is placed at the end of the struct, which keeps ArcData 32 bytes, with space for a couple more boolean flags).

[Darin Adler]

(In reply to Wenson Hsieh from comment #14)
> I'll see if I can find a way to use Optional<FloatPoint> here. When I first
> wrote this, I used a separate `bool` to avoid pushing the size of this
> struct (currently, the separate bool flag is placed at the end of the
> struct, which keeps ArcData 32 bytes, with space for a couple more boolean
> flags).

OK. I don’t want to waste your time on this; you have other important things to do!

But I have two parting thoughts:

Not sure that saving space in ArcData is important since BezierCurveData is bigger.

However, if we did want to keep pushing, we could probably make this optional in a way that does not require the separate byte, using Markable. While I don’t like the name of the Markable class template, it’s designed for cases where we want most of the semantics of Optional without taking extra space.

The usage isn’t super-elegant:

struct FloatPointMarkableTraits {
    constexpr static bool isEmptyValue(const FloatPoint& point) { return std::isnan(point.x()); }
    constexpr static FloatPoint emptyValue() { return { std::numeric_limits<float>::quiet_NaN(), 0 }; }
};
using OptionalFloatPoint = Markable<FloatPoint, FloatPointMarkableTraits>;

And since we still have the clockwise flag, it doesn’t actually save any space.

[Wenson Hsieh]

(In reply to Darin Adler from comment #15)
> (In reply to Wenson Hsieh from comment #14)
> > I'll see if I can find a way to use Optional<FloatPoint> here. When I first
> > wrote this, I used a separate `bool` to avoid pushing the size of this
> > struct (currently, the separate bool flag is placed at the end of the
> > struct, which keeps ArcData 32 bytes, with space for a couple more boolean
> > flags).
> 
> OK. I don’t want to waste your time on this; you have other important things
> to do!
> 
> But I have two parting thoughts:
> 
> Not sure that saving space in ArcData is important since BezierCurveData is
> bigger.
> 
> However, if we did want to keep pushing, we could probably make this
> optional in a way that does not require the separate byte, using Markable.
> While I don’t like the name of the Markable class template, it’s designed
> for cases where we want most of the semantics of Optional without taking
> extra space.
> 
> The usage isn’t super-elegant:
> 
> struct FloatPointMarkableTraits {
>     constexpr static bool isEmptyValue(const FloatPoint& point) { return
> std::isnan(point.x()); }
>     constexpr static FloatPoint emptyValue() { return {
> std::numeric_limits<float>::quiet_NaN(), 0 }; }
> };
> using OptionalFloatPoint = Markable<FloatPoint, FloatPointMarkableTraits>;

Wow, this is a really neat trick!! Thanks for the tip!

> 
> And since we still have the clockwise flag, it doesn’t actually save any
> space.

Hm... So the reason I was concerned about space is that the largest struct (BezierCurveData) is currently 32 bytes:

  +0 < 32> WebCore::BezierCurveData
  +0 <  8>     WebCore::FloatPoint startPoint
  +0 <  4>       float m_x
  +4 <  4>       float m_y
  +8 <  8>     WebCore::FloatPoint controlPoint1
  +8 <  4>       float m_x
 +12 <  4>       float m_y
 +16 <  8>     WebCore::FloatPoint controlPoint2
 +16 <  4>       float m_x
 +20 <  4>       float m_y
 +24 <  8>     WebCore::FloatPoint endPoint
 +24 <  4>       float m_x
 +28 <  4>       float m_y
Total byte size: 32
Total pad bytes: 0

...and I wanted to avoid bumping ArcData past this current 32-byte threshold. I tried making `start` an Optional<FloatPoint>, but kept ending up with 36 bytes due to the padding in the Optional :(

  +0 < 36> WebCore::ArcData
  +0 <  8>     WebCore::FloatPoint center
  +0 <  4>       float m_x
  +4 <  4>       float m_y
  +8 <  4>   float radius
 +12 <  4>   float startAngle
 +16 <  4>   float endAngle
 +20 < 12>     WTF::Optional<WebCore::FloatPoint> start
 +20 < 12>         WTF::OptionalBase<WebCore::FloatPoint> WTF::OptionalBase<WebCore::FloatPoint>
 +20 <  1>           bool init_
 +21 <  3>           <PADDING: 3 bytes>
 +24 <  8>           WTF::constexpr_storage_t<WebCore::FloatPoint> storage_
 +32 <  1>   bool clockwise
 +33 <  1>   bool endsAtStart
 +34 <  2>   <PADDING: 2 bytes>

I will investigate using `FloatPointMarkableTraits` to avoid this issue in a followup.