Open Bug 931587 Opened 6 years ago Updated 4 years ago

Cleanup of 2d canvas class

Categories

(Core :: Canvas: 2D, defect)

x86
macOS
defect
Not set

Tracking

()

ASSIGNED

People

(Reporter: cabanier, Assigned: cabanier)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Depends on: 830734
Attached patch First patch (obsolete) — Splinter Review
Assignee: nobody → cabanier
Status: NEW → ASSIGNED
Attached patch First patch (obsolete) — Splinter Review
removed some stale functions
Attachment #823043 - Attachment is obsolete: true
Attachment #823072 - Attachment is obsolete: true
Attachment #823134 - Attachment is obsolete: true
Attachment #823137 - Flags: review?(roc)
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 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.
Attachment #823137 - Flags: review?(roc) → review-
(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.
Depends on: 932500
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
Attachment #823137 - Flags: review- → review?(roc)
Where is PathBuilder::AdjustMatrix defined? Some other patch?
(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)
(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.
Attachment #823137 - Attachment is obsolete: true
Attachment #823137 - Flags: review?(roc)
Attachment #824889 - Flags: review?(roc)
Attachment #824889 - Attachment is obsolete: true
Attachment #824889 - Flags: review?(roc)
Attachment #824897 - Attachment is obsolete: true
Attachment #824902 - Flags: review?(roc)
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.
(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));
(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.
(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
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.
You need to log in before you can comment on or make changes to this bug.