Bug 79145 – [chromium] Make opaque paint tracker aware of SkCanvas layers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

The current tracker ignored layers entirely. But layers can be used to apply things like alpha. So we make it know about layers, and reapply the opaque rect in a given layer into the next layer down when restore() is called, using the paint/clip/transform applied to that layer.

[Dana Jansens]

The following layout tests were rebaselined in a previous change where we tracked an opaque white paint. Turns out that was in a SkCanvas layer, and the original baselines match again after this CL. So I'm including the original baselines again.

compositing/overflow/clip-content-under-overflow-controls-expected.png
compositing/scrollbar-painting-expected.png

[Dana Jansens]

Image clipping is done via a layer, so we can remove the special case code around image clipping in OpaqueRegionSkia.

[Dana Jansens]

Created attachment 128062 [details]
Patch

Ooh ok, comment #2 was a lie. The layout tests are right.

[James Robinson]

Comment on attachment 128062 [details]
Patch

This looks like Stephen's territory to me.

[Stephen White]

Comment on attachment 128062 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:261
> +    AffineTransform restoreTransform = restoringToRootLayer ? transform : AffineTransform();

This is a little confusing to me.  Shouldn't this be the other way around (root layer = empty transform, other = m_opaqueRegionTransform)?  or does m_opaqueRegionTransform always represent the root layer transform?

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:271
> +    if (!deviceClip.isRect())
> +        return;

Are you going to need to handle non-rect regions at some point?  Or does WebKit never create them?

[Dana Jansens]

Comment on attachment 128062 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:261
>> +    AffineTransform restoreTransform = restoringToRootLayer ? transform : AffineTransform();
> 
> This is a little confusing to me.  Shouldn't this be the other way around (root layer = empty transform, other = m_opaqueRegionTransform)?  or does m_opaqueRegionTransform always represent the root layer transform?

You're right I should change this. We store the root layer in "chromium layer" coordinate space. I thought I'd need to transform other canvas layers but I don't so I should store them all in the same space. Thanks.

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:271
>> +        return;
> 
> Are you going to need to handle non-rect regions at some point?  Or does WebKit never create them?

I don't expect we'll bother to go there, at least any time soon. WebKit can (probably?) create them but the complexity of dealing with them gets much larger.

[Dana Jansens]

Created attachment 128542 [details]
Patch

The work to pop a layer is a bit more (extra mapRect() call), but all opaque rects are stored in the target coordinate space for consistency.

[Stephen White]

Comment on attachment 128542 [details]
Patch

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

OK with me, assuming enne is ok with it.

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:268
> +    // We have a rect in target space, move it to device space.
> +    if (!transform.isInvertible())
> +        return;
> +    SkMatrix targetToDeviceTransform = transform.inverse();
> +    if (!targetToDeviceTransform.mapRect(&opaqueLayerRect))
> +        return;

Nit:  Could the cons of the LayerIter above be moved below these early-outs?  Not a big deal, but kind of nice to early-out as early as possible.

[Dana Jansens]

Created attachment 128549 [details]
Patch

nit done!

[Adrienne Walker]

Comment on attachment 128549 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:99
> +    canvasPainter.skiaContext()->setTrackOpaqueRegion(false);

I don't understand this change and the SkPictureCanvas updater change? Why is this needed with this patch?

[Dana Jansens]

Comment on attachment 128549 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:99
>> +    canvasPainter.skiaContext()->setTrackOpaqueRegion(false);
> 
> I don't understand this change and the SkPictureCanvas updater change? Why is this needed with this patch?

Ah! Because the canvasPainter has a save() and restore().
The save() is done in the constructor, so setTrackOpaqueRegion() is not true, and the tracker doesn't know about it.
The restore() is done in the destructor, and if the tracker tries to execute it, then it sees unbalanced save/restore. It can work but i put an assert to catch this. Turning off tracking here makes any clean up not cause unbalance with the opaque tracking.

[Adrienne Walker]

Comment on attachment 128549 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:99
>>> +    canvasPainter.skiaContext()->setTrackOpaqueRegion(false);
>> 
>> I don't understand this change and the SkPictureCanvas updater change? Why is this needed with this patch?
> 
> Ah! Because the canvasPainter has a save() and restore().
> The save() is done in the constructor, so setTrackOpaqueRegion() is not true, and the tracker doesn't know about it.
> The restore() is done in the destructor, and if the tracker tries to execute it, then it sees unbalanced save/restore. It can work but i put an assert to catch this. Turning off tracking here makes any clean up not cause unbalance with the opaque tracking.

This is just a drive-by opinion, but that seems really fragile to have to case about whether flags were true at various points in time during save/restores.

Is it reasonable to instead maybe not assert and early-out, or maybe perform some logic in all cases to be consistent, or maybe to set the right value up front prior to calling save() in the first place and disallow changing it at the wrong time?

[Dana Jansens]

Well, I was just thinking about a shorter answer to this:

We want to track what is done in paintContents() only. Leaving tracking on after makes us track things we're not interested in.

The code does not break if you restore() more than you save(). But I put an assert since it's not valid. GraphicsContext::restore() has LOG_ERROR when this happens there. So maybe just take the assert out?

[Dana Jansens]

Created attachment 128594 [details]
Patch

ok no longer need the setTracking(false).

PlatformContextSkia will always keep track of layers so that no matter when you enable tracking, you are in a valid state. The tracking is pretty much a no-op when tracking is not on, other than append/remove from vector.

[Adrienne Walker]

Comment on attachment 128594 [details]
Patch

Thanks! LGTM.

[Stephen White]

Comment on attachment 128594 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:236
> +    m_opaqueRectStack.append(SkRect::MakeEmpty());

It seems like m_opaqueRect should be made empty here.

[Dana Jansens]

Created attachment 129530 [details]
Patch

Avoid the whole didSave/willRestore business, and the whole stack thing. Use whatever layers are on the SkCanvas directly when tracking a draw.

[Stephen White]

Comment on attachment 129530 [details]
Patch

Me likey!  Much simpler.  Will leave for enne to take a look.

[Adrienne Walker]

(In reply to comment #18)
> (From update of attachment 129530 [details])
> Me likey!  Much simpler.  Will leave for enne to take a look.

I don't understand enough about SkCanvas layers to review the changes in this latest patch.  This is all you, Stephen.

[Stephen White]

Comment on attachment 129530 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:265
> +            if (!targetRect.intersect(SkIntToScalar(clipBounds.fLeft), SkIntToScalar(clipBounds.fTop), SkIntToScalar(clipBounds.fRight), SkIntToScalar(clipBounds.fBottom)))
> +                return;

Hmmm.. are you sure you want to early-return here?  It seems like we won't be marking the rect either way if any one of the layers has non-intersecting bounds.  Is that intentional?

[Dana Jansens]

(In reply to comment #20)
> (From update of attachment 129530 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129530&action=review
> 
> > Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:265
> > +            if (!targetRect.intersect(SkIntToScalar(clipBounds.fLeft), SkIntToScalar(clipBounds.fTop), SkIntToScalar(clipBounds.fRight), SkIntToScalar(clipBounds.fBottom)))
> > +                return;
> 
> Hmmm.. are you sure you want to early-return here?  It seems like we won't be marking the rect either way if any one of the layers has non-intersecting bounds.  Is that intentional?

Yeh, it means the draw is being clipped out of existence so there's nothing to track. It's similar to the clip check above.

[Stephen White]

Comment on attachment 129530 [details]
Patch

OK then.  r=me

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 129530 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 129530 [details]
Patch

Rejecting attachment 129530 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ed at Tools/Scripts/update-webkit line 164.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9
ate pixel count for frame rate control

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/11776188

[W. James MacLean]

Created attachment 129702 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 129702 [details]
Patch for landing

Rejecting attachment 129702 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11757475

[WebKit Review Bot]

Comment on attachment 129702 [details]
Patch for landing

Rejecting attachment 129702 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11776478

[WebKit Review Bot]

Comment on attachment 129702 [details]
Patch for landing

Rejecting attachment 129702 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11766573

[Dana Jansens]

Created attachment 129735 [details]
Patch

Try to bypass this cq bot..

[WebKit Review Bot]

Comment on attachment 129735 [details]
Patch

Clearing flags on attachment: 129735

Committed r109514: <http://trac.webkit.org/changeset/109514>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.