1377595 - Don't use blob images with content that must be painted on the content process

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Nicolas Silva [:nical]]

Things like native theme widgets have to be painted on the content process, so we end up rasterizing them into an image and serializing the image in the blob image recording which is not great.

Comment 1

[Nicolas Silva [:nical]]

Attachment: WIP: annotate display items that should be painted on the content side.

Comment 2

[Nicolas Silva [:nical]]

Attachment: don't use blob images for native widgets

This patch allows us to decide whether to use blob images depending on a MustPaintOnContentSide flag exposed by each display item. If any of the display item assigned to a given painted layer data has this flag, then the painted layer data is marked as preferring content side painting and the information is ultimately passed to the webrender layer manager through PaintedLayerCreationHint to decide whether to create a regular painted layer or serialize the drawing commands with blob image.

PaintedLayerCreationHint had to be moved out of LayerManager for the MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS macro to work. The most notable change is that people now need to be careful about not using == with PaintedLayerCreationHint and check for the presence or absence of specif bits instead.

There might be other items to flag for content side drawing but as far as I can see from my initial testing, this patch gets all of the native theme stuff I have come across out of the blob image path.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Hook up invalidation

Without this, the painted layer gets repainted continously every frame.

Comment 4

[Jamie Nicol [:jnicol]]

Comment on attachment 8883299 [details] [diff] [review]
don't use blob images for native widgets

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

Looks good to me!

Typo in commit message: specif should be specific

::: gfx/layers/Layers.h
@@ +159,5 @@
> + * Hints that can be used during PaintedLayer creation to influence the type
> + * or properties of the layer created.
> + *
> + * NONE: No hint.
> + * SCROLLABLE: This layer may represent scrollable content.

Missing comment for CONTENT_SIDE_PAINT

::: layout/painting/FrameLayerBuilder.cpp
@@ +632,5 @@
>     * with visible backface.
>     */
>    bool mBackfaceHidden;
>    /**
> +   * Set if it is better to render this layer on the content process, for

Nit: "It is better to" is weaker than "must" used in the variable name and elsewhere. Which one is it?

@@ +2354,3 @@
>      GetLayerCreationHint(aData->mAnimatedGeometryRoot);
>  
> +  if (aData->mMustPaintOnContentSide) {

Should this be added to GetLayerCreationHint instead? Maybe rewrite that function to |= the scrollable flags without returning early.

Comment 5

[Nicolas Silva [:nical]]

> Nit: "It is better to" is weaker than "must" used in the variable name and elsewhere. Which one is it?

Either way the item must be rendered on the content side, but the rest of the layer can be rendered elsewhere. The hint here is that it's likely that if an item must be rendered on the content side then it is probably better to render the rest of the layer there as well to avoid paying the cost of an extra serialization+desarilzation+blit of the pixels that had to be painted on the content side.

> Should this be added to GetLayerCreationHint instead? Maybe rewrite that function to |= the scrollable flags without returning early.

It'd be nicer but it looks like we don't always have access to the PaintedLayerData when calling this so I went for the simpler option.

Comment 6

[Nicolas Silva [:nical]]

I renamed PaintedLayerData::mMustPaintOnContentSide into mShouldPaintOnContentSide to avoid the mismatch between the variable name and the comment.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Hook up invalidation v2

Almost there. Turns out we need to do a bit more than apply the valid region, since the painted content can be invalidated when the size of the layer's texture changes.

I am still hitting the assertion MOZ_ASSERT(GetInvalidRegion().IsEmpty()); in a few tests, not sure why yet. Also this seems to affect the anti-aliasing in  layout/reftests/image-rect/background-zoom-1.html which breaks the test.
On the flip side this (accidentally) fixes 4 tests related to invalidation.

Comment 8

[Nicolas Silva [:nical]]

Attachment: Four new passing tests

Comment 9

[Nicolas Silva [:nical]]

Attachment: Hook up invalidation v3

Need to keep track of the painted rect and not just the size since if the visible region has the same size but with a different origin we should also repaint the whole thing.

Comment 10

[Nicolas Silva [:nical]]

Matt, Halp! I don't fully understand the assertion MOZ_ASSERT(GetInvalidRegion().IsEmpty()); in https://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/gfx/layers/wr/WebRenderPaintedLayer.cpp#144

The assertion blows up in a lot of tests if we try to apply the painted region to the layer's valid region (see patch https://bugzilla.mozilla.org/attachment.cgi?id=8885246&action=diff)

What's not clear to me is that if the visible region of the layer does not overlap with the invalid region, it should be possible to take this branch with an non-empty invalid region. So I am just tempted to remove the assertion. A try push with the valid region tracked and without the assertion, came back without outstanding glitch (except one reftest[1] that could be fuzzed maybe, I am not sure).


[1] https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/B3i9bks9To-gcWRfqQ6FSA/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 11

[Nicolas Silva [:nical]]

Attachment: Invalidation v4

Another attempt, that is more explicit about what really happens under the hood, and a bit more documented.

Comment 12

[Matt Woodrow (:mattwoodrow)]

I agree with your analysis, that assertion seems wrong to me.

You could make it assert that the intersection of the invalid and visible regions are empty instead?

Comment 13

[Nicolas Silva [:nical]]

As for the remaining reftest that needs fuzzing, I think that we should just fuzz it and that the reason the test is failing is similar to 1128229 in which this test is intermittent when timing affects whether or not OptimizeSourceSurface is called in an image container. This path queue makes it so that we will tend to keep around the same image in the WebRenderPaintedLayer's ImageContainer whereas we used to re-build it every frame before that.

Comment 14

[Nicolas Silva [:nical]]

Attachment: Hook up invalidation v5

Comment 15

[Nicolas Silva [:nical]]

Attachment: Fuzz a test.

Comment 16

[Nicolas Silva [:nical]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6686dfccf2ab40f4ed16801d889a0e849b02171e

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4dc716c401c
Avoid using blob images for items that must be painted on the content side. r=jnicol
https://hg.mozilla.org/integration/mozilla-inbound/rev/59479f0872ec
Track the valid region in WebRenderPaintedLayer. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea065f577c1c
fuzz a test. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ee58818e46
Mark four tests as passing with webrender.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out on suspicion of causing reftest failures on Android "(waiting for MozAfterPaint)":

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc8b6cff9a42e0e3b63adecfae62a254bfa6625

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e8ee58818e46e0d0f4fffccf430701e888dacf5a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=success
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=114821217&repo=mozilla-inbound

[task 2017-07-17T12:49:12.272671Z] 12:49:12     INFO -  REFTEST TEST-START | http://10.0.2.2:8854/tests/layout/reftests/bidi/dirAuto/dynamicDirAuto-setLTR-Auto1.html == http://10.0.2.2:8854/tests/layout/reftests/bidi/dirAuto/dynamicDirAuto-refLTR-LTR.html
[task 2017-07-17T12:49:12.272925Z] 12:49:12     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/bidi/dirAuto/dynamicDirAuto-setLTR-Auto1.html | 611 / 942 (64%)
[task 2017-07-17T12:54:16.775871Z] 12:54:16     INFO -  REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/bidi/dirAuto/dynamicDirAuto-setLTR-Auto1.html | load failed: timed out waiting for pending paint count to reach zero (waiting for MozAfterPaint)

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59479f0872ec
https://hg.mozilla.org/mozilla-central/rev/ea065f577c1c
https://hg.mozilla.org/mozilla-central/rev/e8ee58818e46

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d12dd6f27a5
Mark display items that should be painted on the content side. r=jnicol

Comment 21

[Nicolas Silva [:nical]]

Attachment: Fix ClientTiledPaintedLayer::IsOptimizedFor

This appears to be what caused the reftest issues on android (try green with this fix). This patch will be folded into the original patch, but I upload it separately here to avoid making you re-review the entire thing.

Comment 22

[Jamie Nicol [:jnicol]]

Comment on attachment 8888261 [details] [diff] [review]
Fix ClientTiledPaintedLayer::IsOptimizedFor

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

Just the comment change. As discussed on irc, webrender layer recycling might not work properly but that can be a follow up.

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +600,5 @@
>  
>  bool
>  ClientTiledPaintedLayer::IsOptimizedFor(PaintedLayerCreationHint aHint)
>  {
> +  // On OSX, the creation hint is used to determine whether to use a tiled

Please change "the creation hint" to "the SCROLLABLE flag".

I think this comment is wrong and that this affects android too.  There's nothing OS X specific in this function any more, nor in ClientTiledPaintedLayer::RenderLayer() where we create either a single or multi tiled content client.

Also maybe it should say "... used to determine whether to use a multi-tiled content client".

Your choice whether to fix those last two points, but please fix the first.

Comment 23

[Nicolas Silva [:nical]]

Attachment: consistently create the hint and don't recycle blob painted layer if new item is incompatible

I ended up moving the logic from GetLayerCreationHint into IsLayerScrollable and reimplementing the former on top of the latter with two overrides. There's still a tiny bit of duplication but at least it stays close.

Comment 24

[Jamie Nicol [:jnicol]]

Comment on attachment 8888301 [details] [diff] [review]
consistently create the hint and don't recycle blob painted layer if new item is incompatible

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

I think we have a problem here. ProcessDisplayItems() only checks AttemptToRecyclePaintedLayer() until it first succeeds. So in ProcessDisplayItems() if the first display item is mShouldPaintOnContentSide=false we will recycle the PaintedLayerBlob happily, but then a subsequent display item could have mShouldPaintOnContentSide=true and be added to that layer.

Maybe we could delay attempting to recycle the layer until FinishPaintedLayerData(), but I'm not sure what consequences that would have.

Comment 25

[Nicolas Silva [:nical]]

Darnit. I'll think about it some more. We most likely won't ship webrender with layers so I would rather not have to do something complicated to fix this.

Comment 26

[Nicolas Silva [:nical]]

I think that we should either land this as is with a big comment explaining that the code makes a best effort at avoiding to use native widgets with blob images but that it will still happens in some scenarios such as the one you described (and it will work, just a bit slower than when the heuristics work in our favor), or not land it at all and count on shipping webrender "layers-free".
The advantage of landing this is that we can work on the performance of various things like blob image performance in parallel of the layers-free work.
In any case, I would rather not add any overhead or significant complexity to FrameLayerBuilder for this since it will most likely not by the time we ship.

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7d12dd6f27a5

Comment 28

[Jamie Nicol [:jnicol]]

Attachment: Attempt to recycle painted layers only after their entire contents is known

Markus just to bring you up to speed - Nical has added a PaintedLayerCreationHint to indicate whether a layer must be painted on the content side or can use a blob image, based on which display items it contains.

This works fine for creating a new painted layer as by that time the entire contents of the layer is known. However, the decision to recycle a layer is made much earlier. We might decide to recycle a layer, and then process a display item which would have changed the hint, but by that time it is too late. If we move the recycling to later then we should be able to make the right decision.

If this has any negative consequences we'd rather not land it, since it won't be needed for layers-free. But it seems okay to me.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1645e22b13e58eb84bd2e713892580b9e842434e

Comment 29

[Markus Stange [:mstange]]

Comment on attachment 8888827 [details] [diff] [review]
Attempt to recycle painted layers only after their entire contents is known

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

I'd rather not land this - I think this patch means that data->mAssignedDisplayItems will always grow to contain all display items for a layer, and then we'll iterate over that array again in FinishPaintedLayerData. At the moment, we only do that double iteration when all display items are new.

Or am I misunderstanding the patch?

Comment 30

[Jamie Nicol [:jnicol]]

No, you understand it correctly. That wouldn't be good.

It would be able to avoid that by creating a list of candidate layers for recycling as each display item is assigned to a PaintedLayerData. Then only iterate that list rather than every display item in FinishPaintedLayerData.

But this is getting quite complicated for something which we won't need. So I'd say nical just land the patch without this.

Comment 31

[Markus Stange [:mstange]]

I agree.

Comment 32

[Markus Stange [:mstange]]

This can probably be simplified a lot now that we only have layers-free. Nicolas, are you planning to get back to this soon or is this bug available to be picked up by somebody else?

Comment 33

[Nicolas Silva [:nical]]

This bug is available to be picked up.

Comment 34

[Nicolas Silva [:nical]]

I'm giving this another go.

Comment 35

[Nicolas Silva [:nical]]

Attachment: Don't use blob images for items that must be painted on the content side in layers-free

This works well, although we may have to refine which items should implement MustPaintOnContentSide as I am seeing some blob in a few places where I expected the thing to be a native widget.

Comment 36

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8926930 [details] [diff] [review]
Don't use blob images for items that must be painted on the content side in layers-free

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

Couple of minor nits but I don't feel strongly about them. r+

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +434,5 @@
>      aItem->Paint(aDisplayListBuilder, context);
>      break;
>    }
>  
> +  if (aHighlight.isSome()) {

We usually just do |if (aHighlight) {| since it has an overloaded conversion to bool.

@@ +457,5 @@
>                                                LayoutDeviceRect& aImageRect)
>  {
> +  bool useBlobImage = gfxPrefs::WebRenderBlobImages() && !aItem->MustPaintOnContentSide();
> +  Maybe<gfx::Color> highlight = gfxPrefs::WebRenderHighlightPaintedLayers() ?
> +    Some(useBlobImage ? gfx::Color(1.0, 0.0, 0.0, 0.5) : gfx::Color(1.0, 1.0, 0.0, 0.5)) : Nothing();

Just for readability I'd prefer splitting this into two:
Maybe<gfx::Color> highlight;
if (gfxPrefs::WebRenderHighlightPaintedLayers()) {
  highlight = Some(...);
}

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8745afb06952
Don't use blob images for items that must be painted on the content side. r=kats

Comment 38

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8745afb06952

Comment 39

[Jeff Muizelaar [:jrmuizel]]

So it's worth noting that this will cause increased invalidation for items that must be painted on the content side. i.e as items scroll into view they'll now repaint. This is probably not desirable and we should fix it..

Comment 40

[Nicolas Silva [:nical]]

Attachment: Don't clip native themed items

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4327949e300
Don't clip native themed item images to avoid over invalidation. r=jrmuizel

Comment 42

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b4327949e300