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.

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.

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

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.

> 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.

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

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.

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.

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

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

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?

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.

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

Pushed by nsilva@mozilla.com: 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.

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)

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

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

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 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.

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 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.

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.

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.

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 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?

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.

I agree.

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?

This bug is available to be picked up.

I'm giving this another go.

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 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(...); }

Pushed by nsilva@mozilla.com: 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

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..

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