1452829 - Canvas is not optimized for clipping to rectangles

Status: NEW

(Core :: Graphics, enhancement, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

jrmuizel and I discovered this during Episode 23 of The Joy of Profiling[1].

According to jrmuizel, this is where we should check to see if the mPath is a rect, and then use PushClipRect instead of PushClip, which should be cheaper:

https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/dom/canvas/CanvasRenderingContext2D.cpp#3401-3407

[1]: https://air.mozilla.org/the-joy-of-profiling-episode-23/

Comment 1

[Daniel Holbert [:dholbert]]

Hi Maire!  If we've got any layout engineers looking for things to work on, this might be a good optimization to point them at.  (Technically in DOM, but on the fringes of layout/graphics.)

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Plus this is super easy.

Comment 3

[Daniel Almeida]

Hello! I'd like to work on this, can I get this assigned to me?

Comment 4

[Daniel Almeida]

Sorry, did I miss anything simple? I don't actually see a method that returns whether or not a gfx::Path is a rectangle :/

Comment 5

[Jeff Muizelaar [:jrmuizel]]

There's no method like that. It should be possible to track this similar to how gfxContext does it in: https://searchfox.org/mozilla-central/source/gfx/thebes/gfxContext.cpp#272

i.e. add mPathIsRect and mRect members to CanvasRenderingContext2D and track the state in a similar way.

Comment 6

[Daniel Almeida]

Attachment: 0001-bug-1452829-optimize-for-clipping-to-rectangles.patch

Comment 7

[Daniel Almeida]

Attachment: 0001-bug-1452829-optimize-for-clipping-to-rectangles.patch

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8970562 [details] [diff] [review]
0001-bug-1452829-optimize-for-clipping-to-rectangles.patch

Review of attachment 8970562 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3542,5 @@
>      mDSPathBuilder->LineTo(mTarget->GetTransform().TransformPoint(Point(aX, aY + aH)));
>      mDSPathBuilder->Close();
>    }
> +
> +  mPathIsRect = true;

mPathIsRect never gets set to false. You'll need to do that when the path isn't a rect anymore.

::: dom/canvas/CanvasRenderingContext2D.h
@@ +848,4 @@
>    RefPtr<mozilla::gfx::PathBuilder> mDSPathBuilder;
>    RefPtr<mozilla::gfx::PathBuilder> mPathBuilder;
>    bool mPathTransformWillUpdate;
> +  bool mPathIsRect;

You'll need to initialize mPathIsRect to false in the constructor of CanvasRenderingContext2D.

Comment 9

[Daniel Almeida]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 8970562 [details] [diff] [review]
> 0001-bug-1452829-optimize-for-clipping-to-rectangles.patch
> 
> Review of attachment 8970562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/CanvasRenderingContext2D.cpp
> @@ +3542,5 @@
> >      mDSPathBuilder->LineTo(mTarget->GetTransform().TransformPoint(Point(aX, aY + aH)));
> >      mDSPathBuilder->Close();
> >    }
> > +
> > +  mPathIsRect = true;
> 
> mPathIsRect never gets set to false. You'll need to do that when the path
> isn't a rect anymore.
> 

Most of the methods in which mPathIsRect was set to false in gfxContext aren't present in CanvasRenderingContext2D. Should I assume the path isn't a rect if there was a call to either Arc, ArcTo or Ellipse?

By the way, where can I educate myself some more about this topic?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

So in the gfxContext the main function that clears mPathIsRect is EnsurePathBuilder().

We can use EnsureWritablePath() for the same mechanism. i.e. Rect() will look something like:

  if (!mPathBuilder && !mPathIsRect) {
    mPathIsRect = true;
    mRect = rec;
    return;
  }

  EnsureWritablePath();

  if (mPathBuilder) {
    mPathBuilder->MoveTo(Point(aX, aY));
    mPathBuilder->LineTo(Point(aX + aW, aY));
    mPathBuilder->LineTo(Point(aX + aW, aY + aH));
    mPathBuilder->LineTo(Point(aX, aY + aH));
    mPathBuilder->Close();
  } else {
    mDSPathBuilder->MoveTo(mTarget->GetTransform().TransformPoint(Point(aX, aY)));
    mDSPathBuilder->LineTo(mTarget->GetTransform().TransformPoint(Point(aX + aW, aY)));
    mDSPathBuilder->LineTo(mTarget->GetTransform().TransformPoint(Point(aX + aW, aY + aH)));
    mDSPathBuilder->LineTo(mTarget->GetTransform().TransformPoint(Point(aX, aY + aH)));
    mDSPathBuilder->Close();
  }


And EnsureWritablePath() will need an additional piece like the following to make sure we end up with the rectangle in the path:

if (mPathIsRect) {
  if (mPathBuilder) {
    mPathBuilder->MoveTo(Point(aX, aY));
    mPathBuilder->LineTo(Point(aX + aW, aY));
    mPathBuilder->LineTo(Point(aX + aW, aY + aH));
    mPathBuilder->LineTo(Point(aX, aY + aH));
    mPathBuilder->Close();
  } else {
    mDSPathBuilder->MoveTo(mTarget->GetTransform().TransformPoint(Point(aX, aY)));
    mDSPathBuilder->LineTo(mTarget->GetTransform().TransformPoint(Point(aX + aW, aY)));
    mDSPathBuilder->LineTo(mTarget->GetTransform().TransformPoint(Point(aX + aW, aY + aH)));
    mDSPathBuilder->LineTo(mTarget->GetTransform().TransformPoint(Point(aX, aY + aH)));
    mDSPathBuilder->Close();
  }
}

If this doesn't make sense I can elaborate further. Just ask/need info me. Hopefully, it won't take so long to respond.

Comment 11

[Daniel Almeida]

Attachment: 0001-bug-1452829-optimize-for-clipping-to-rectangles.patch

Jeff,

See if that's what you're asking.


I did not get this part:

>>And EnsureWritablePath() will need an additional piece like the following to make sure we end up with the rectangle in the path:

Why would this method ever need to make sure we end up with a rectangle? mPathIsRect is already set on CanvasRenderingContext2D::Rect

Comment 12

[Daniel Almeida]

Also, should I assign mRect to something else when mPathIsRect gets set to false?

Comment 13

[Jeff Muizelaar [:jrmuizel]]

(In reply to Daniel  Almeida from comment #11
> 
> Why would this method ever need to make sure we end up with a rectangle?
> mPathIsRect is already set on CanvasRenderingContext2D::Rect

You're current code will break something that does this:

ctx.rect(0, 0, 5, 5);
ctx.rect(10, 10, 10, 10);
ctx.fill();

This should fill both rectangles. With your code the first rectangle will not be included in the path.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to Daniel  Almeida from comment #12)
> Also, should I assign mRect to something else when mPathIsRect gets set to
> false?

I don't think that will be necessary.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Here's an updated link to the video https://mozilla.hosted.panopto.com/Panopto/Pages/Embed.aspx?id=bbfcab37-f94b-4ed1-becd-ac2b01301cfc
We saw this in chart.js: https://github.com/bgrins/xbl-analysis/blob/592f092cea74506e3a411324c2ff5979e251448b/static/chart.js#L14116