931587 - Cleanup of 2d canvas class

Status: RESOLVED INACTIVE

(Core :: Graphics: Canvas2D, defect)

Description

[Rik Cabanier]

It seems that the 2d rendering context could use some cleanup. There is some copying of path classes and it's tracking 2 different path builder which doesn't seem necessary.

With the addition of the path class, things could be simplified.

Comment 1

[Rik Cabanier]

Attachment: First patch

Comment 2

[Rik Cabanier]

Attachment: First patch

removed some stale functions

Comment 3

[Rik Cabanier]

Attachment: Cleanup of canvas' rendering context.

Comment 4

[Rik Cabanier]

Attachment: Cleanup of canvas' rendering context.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 823137 [details] [diff] [review]
Cleanup of canvas' rendering context.

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

Please use nullptr instead of NULL, brace single-line ifs and start local variable names with a lower case letter.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 823137 [details] [diff] [review]
Cleanup of canvas' rendering context.

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

I don't think you understand the role of mDSPathBuilder ... not surprising since it's not well-documented. The reason we have separate PathBuilders is that sometimes we have the path in canvas user space and sometimes we have it in device space. Which space it's in depends on how it's being used.

Comment 7

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 823137 [details] [diff] [review]
> Cleanup of canvas' rendering context.
> 
> Review of attachment 823137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think you understand the role of mDSPathBuilder ... not surprising
> since it's not well-documented. The reason we have separate PathBuilders is
> that sometimes we have the path in canvas user space and sometimes we have
> it in device space. Which space it's in depends on how it's being used.

I know why it was there.
The first pathbuilder is used if there are not transforms or save/restore operations. The other one is generated in all other cases. This results in unnecessary conversions from one path to the other as soon as you set a transform; it also make the code hard to read and introduces a bunch of if/else statements.
My patch is passing the try servers, except for a crash on D2D: https://tbpl.mozilla.org/?tree=Try&rev=18ab1c106692
Maybe there are applications that use canvasrenderingcontext somewhere else?

By changing the interface of 'path' slightly, I can turn it into a member (instead of a refptr). That should also remove a couple of indirections. Applying/unapplying the matrix to the path lazily will also make things faster.

Comment 8

[Rik Cabanier]

Comment on attachment 823137 [details] [diff] [review]
Cleanup of canvas' rendering context.

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

With the fix in D2D, all tests pass: https://tbpl.mozilla.org/?tree=Try&rev=904a26f64f43

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Where is PathBuilder::AdjustMatrix defined? Some other patch?

Comment 10

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Where is PathBuilder::AdjustMatrix defined? Some other patch?

It's part of the pathbuilder interface. The patch that fixes d2d is in https://bugzilla.mozilla.org/show_bug.cgi?id=932500
(I haven't looked at the issue with skia yet where AdjustMatrix changes the object you call it on)

Comment 11

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Where is PathBuilder::AdjustMatrix defined? Some other patch?

Sorry, I defined that function on the path object. Since it's so confusing, I renamed the variable.

Comment 12

[Rik Cabanier]

Attachment: Cleanup of canvas' rendering context.

Comment 13

[Rik Cabanier]

Attachment: Cleanup of canvas' rendering context.

Comment 14

[Rik Cabanier]

Attachment: Cleanup of canvas' rendering context.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem with this approach is that if you have JS that does something like this:
  for (var i = 0; i < N; ++i) {
    ctx.lineTo(...);
    ctx.rotate(...);
  }
this will now take O(N^2) time.

Comment 16

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> The problem with this approach is that if you have JS that does something
> like this:
>   for (var i = 0; i < N; ++i) {
>     ctx.lineTo(...);
>     ctx.rotate(...);
>   }
> this will now take O(N^2) time.

Sure, but that's not that common of a use case. People usually set the matrix and then draw.
Right now, if you have a save/restore or *any* type of transformation, the following code is executed:
  if (mDSPathBuilder) {
    RefPtr<Path> dsPath;
    dsPath = mDSPathBuilder->Finish();
    mDSPathBuilder = nullptr;

    Matrix inverse = mTarget->GetTransform();
    if (!inverse.Invert()) {
      NS_WARNING("Could not invert transform");
      return;
    }

    mPathBuilder =
      dsPath->TransformedCopyToBuilder(inverse, fillRule);
    mPath = mPathBuilder->Finish();
    mPathBuilder = nullptr;
  }
Which converts a path twice (3 times if a different fill rule was requested)

I checked the webkit code and it's similar to my changes, ie CanvasRenderingContext2D::translate(float tx, float ty):
    c->translate(tx, ty);
    m_path.transform(AffineTransform().translate(-tx, -ty));

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Rik Cabanier from comment #16)
> Sure, but that's not that common of a use case. People usually set the
> matrix and then draw.
> Right now, if you have a save/restore or *any* type of transformation, the
> following code is executed:
>   if (mDSPathBuilder) {
>     RefPtr<Path> dsPath;
>     dsPath = mDSPathBuilder->Finish();
>     mDSPathBuilder = nullptr;
> 
>     Matrix inverse = mTarget->GetTransform();
>     if (!inverse.Invert()) {
>       NS_WARNING("Could not invert transform");
>       return;
>     }
> 
>     mPathBuilder =
>       dsPath->TransformedCopyToBuilder(inverse, fillRule);
>     mPath = mPathBuilder->Finish();
>     mPathBuilder = nullptr;
>   }

This code is from EnsureUserSpacePath. EnsureUserSpacePath is not called during save/restore or transform changes.

Comment 18

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> This code is from EnsureUserSpacePath. EnsureUserSpacePath is not called
> during save/restore or transform changes.

yes, it's called at fill/stroke time, so this very common idiom:
  rotate()
  moveto()lineto()....fill()

will convert the path twice

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Only if there was a path already emitted before the rotate(), in which case Webkit will convert the path once and we'll convert it twice.

Are you really saying that "emit some path, (change the transform, emit some more path) N times, and then draw" is very common for N=1 but very uncommon for N>1? Because that would surprise me.

Comment 20

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

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