1337761 - Implement WebRenderLayerManager::EndEmptyTransaction

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Nicolas Silva [:nical]]

I am not sure exactly what need to be done there, but WebRenderLayerManager::EndEmptyTransaction doesn't do anything and always returns false, which generates loads of warnings. Both BasicLayerManager and ClientLayerManager do a lot of things in this method.

Comment 1

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

Related: bug 1337130 comment 6

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Implement empty transactions for WRLayerManager

ClientLayerManager and BasicLayerManager both implement empty transactions, which just repaint the invalid region for a painted layer. Both return false on an empty transaction if we have an invalid region to paint but no callback to FLB to actually paint the layer. This patch implements the same thing and cleans up WebRenderPaintedLayer a bit by refactoring the existing code and deleting the PaintThebes method which we never used.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Skip reftest

This test was already intermittent locally for me, it just is appearing on try now and more often locally. I'll fix it in a follow up.

Comment 4

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

Comment on attachment 8854244 [details] [diff] [review]
Skip reftest

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

::: layout/reftests/web-animations/reftest.list
@@ +1,4 @@
>  test-pref(dom.animations-api.core.enabled,true) == 1246046-1.html green-box.html
>  test-pref(dom.animations-api.core.enabled,true) == 1267937-1.html 1267937-ref.html
> +# see bug 1337761. Intermittent with WR enabled
> +test-pref(dom.animations-api.core.enabled,true) skip-if(webrender) == 1298742-1.html 1298742-ref.html

Please use random-if instead of skip-if here. skip-if will fail to catch regressions that manifest as crashes. And please file a new bug for this and put that bug's bug number in the comment.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Random if reftest

Comment 6

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

Comment on attachment 8854483 [details] [diff] [review]
Random if reftest

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

Do we have any idea why it's failing?

Comment 7

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Comment on attachment 8854483 [details] [diff] [review]
> Random if reftest
> 
> Review of attachment 8854483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have any idea why it's failing?

Not yet, still digging.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8854242 [details] [diff] [review]
Implement empty transactions for WRLayerManager

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

Hmm, so the existing recordings are only still valid if the don't rely on Layer properties (or if the properties they rely on don't change, but we don't track that).

It looks like they do, as we pass the Layer* to CreateWebRenderCommands, and that we use Layer properties in RelativeToParent.

RelativeToParent doesn't make sense to me btw, we're walking up the layer tree to find a stacking context and adjusting our rect using the topleft of the visible area of that Layer? Each Layer exists in an infinite coordinate space, and the visible region is just an annotation of what part of that plane is currently (maybe) visible. The only way to convert between Layer coordinate spaces is using the transform.

Anyway, I'm not sure we can guarantee that the coordinate conversion between the current layer is guaranteed to be constant during an empty transaction. It's worth checking though. If it is, I think we should pass the relative offset/transform into BuildWebRenderCommands, rather than the Layer*, so it's easy to verify that none of the overloads are reading other values.

Unrelated, but the WebRenderPaintedLayer changes scares me a bit. Are we 100% committing to doing PaintedLayers using the 'blob' format? That seems like a non-trivial amount of work, and not something that I'd want blocking WRs first release.

Comment 9

[Mason Chang [Inactive] [:mchang]]

After digging and talking, here are some notes that may or may not be true.

Jeff thinks that we cannot guarantee that the coordinate conversion between the current layer and parent will not change. But we can maybe get around this by updating the transform on the stacking contexts instead.

RelativeToParent kinda maybe makes sense? In the WR layer manager, every layer seems to be creating a stacking context. So instead of using transforms to correctly transform each layer, WR Layer Manager creates a stacking context with the layer transform and writes out the display items with the transform already applied in the stacking context. The parent stacking context should be a container layer, and so the transforms are applied correctly I think.

> Unrelated, but the WebRenderPaintedLayer changes scares me a bit. Are we
> 100% committing to doing PaintedLayers using the 'blob' format? That seems
> like a non-trivial amount of work, and not something that I'd want blocking
> WRs first release.

Maybe? I'm not sure, but the WebRenderPaintedLayer changes here is just a refactoring and deletion of dead code.

Comment 10

[Mason Chang [Inactive] [:mchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> Comment on attachment 8854242 [details] [diff] [review]
> RelativeToParent doesn't make sense to me btw, we're walking up the layer
> tree to find a stacking context and adjusting our rect using the topleft of
> the visible area of that Layer? Each Layer exists in an infinite coordinate
> space, and the visible region is just an annotation of what part of that
> plane is currently (maybe) visible. The only way to convert between Layer
> coordinate spaces is using the transform.

Maybe kats knows since IIRC he added them? Although today the blame log blames him since he moved the code from phabricator to m-c.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> Unrelated, but the WebRenderPaintedLayer changes scares me a bit. Are we
> 100% committing to doing PaintedLayers using the 'blob' format? That seems
> like a non-trivial amount of work, and not something that I'd want blocking
> WRs first release.

We're not 100% committing, but using the existing PaintedLayer code and surface sharing and making that work with webrender is not very attractive either. Going with blob images will hopefully put us in a better place in the longer term so we're exploring that now.

Comment 12

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

(In reply to Mason Chang [:mchang] from comment #10)
> Maybe kats knows since IIRC he added them? Although today the blame log
> blames him since he moved the code from phabricator to m-c.

What you said in comment 9 is basically correct, as far as I remember. When we create a stacking context in WR all the things inside that stacking context are relative to the stacking context. So RelativeToParent tries to set that up correctly on the layer side. That being said it currently also looks in FrameMetrics which is unnecessary since we're not creating scroll layers properly right now.

Comment 13

[Mason Chang [Inactive] [:mchang]]

Attachment: Implement empty transactions for WRLayerManager

Implements empty transactions using the LayerManager::Mutated notifications. WRLayerManager tracks layers that have changed. The displayitemlayer checks if those layers have been mutated or not.

The WebRenderPaintedLayer changes are just a refactoring, there isn't any functional change other than, if we don't have a painted layer callback, we have to bail out.

In a follow up, I'll change DisplayItemLayer::CreateWebRenderCommands to not reference the DisplayItemLayer and just use the transform instead.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8857218 [details] [diff] [review]
Implement empty transactions for WRLayerManager

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

::: gfx/layers/wr/WebRenderPaintedLayer.cpp
@@ +150,5 @@
> +  }
> +
> +  // We have something to paint but can't. This usually happens only in
> +  // empty transactions
> +  if (!GetInvalidRegion().IsEmpty() && !Manager()->GetPaintedLayerCallback()) {

I think you need to compute 'region to draw' as the difference between the visible region and the valid region.

Changes in visible region don't necessarily add to the invalid region iirc.

@@ +155,5 @@
> +    Manager()->SetTransactionIncomplete();
> +    return;
> +  }
> +
> +  if (Manager()->GetPaintedLayerCallback()) {

I think this condition should be if (!regionToDraw.IsEmpty()), so that we only update the ImageClient if something has changed within this layer.

Comment 15

[Mason Chang [Inactive] [:mchang]]

Attachment: Don't send external images to the parent side if a transaction is incomplete

We were getting flickering / crashes because in WebRenderBridgeParent::HandleDPEnd, we'd set a new display list which would clear the screen. However, if we have an incomplete transaction, sometimes the command list would be empty, so WR would just clear the screen for that transaction.

This patch doesn't send send an SendDPEnd in this case. However, we created some WRImage keys along some layers as each layer doesn't know if the transaction is incomplete. 

The only wrinkle here is, I think we have to create the read locks before beginning a transaction so layers can write to them. We still have to clear them even if the transaction failed, so I moved that up. From reading the code, I think we can create FwdTransactions and skip a number if a transaction failed [1]. 

I think it's also safe to SendInitReadLocks to the parent side and clear the child side before sending the SendDPEnd here since most transactions are async. So the client side can lose the references as long as it SendInitReadLocks first. If these aren't true, I can convert them back.

[1] http://searchfox.org/mozilla-central/source/gfx/layers/wr/WebRenderBridgeChild.cpp#411

Comment 16

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bfd85db6cd86c7783dfbc6d7f287076f9caf812

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/8a3a9bb42322
Implement WebRenderLayerManager::EndEmptyTransaction. r=mattwoodrow
https://hg.mozilla.org/projects/graphics/rev/735695011771
Part 2: Don't send external images to the parent side if a transaction is incomplete. r=nical

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/8b33bdca334e
Mark tests as random-if for intermittent reftest failure. r=kats

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/8a3a9bb42322
https://hg.mozilla.org/mozilla-central/rev/735695011771
https://hg.mozilla.org/mozilla-central/rev/8b33bdca334e