1109314 - Layer Tree Invalidation code doesn't run when compositing to draw target

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

We need to run this code even for draw targets so that we know to invalidate the retained render target for container layers.

Comment 1

[Benoit Girard (:BenWa)]

[Tracking Requested - why for this release]: Regression from bug 1068190. The trigger for the bug should be rare I imagine but we should still fix it.

Comment 2

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #1)
> Regression from bug 1068190.
Bug 1098495*

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=734dbb100c35

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8534170 [details] [diff] [review]
patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +295,1 @@
>      mInvalidRegion.Or(mInvalidRegion, mRenderBounds);

Doesn't this mean that we now invalidate everything if we're compositing without a target, e.g. during normal BasicCompositor OMTC?

But I don't really understand why the old code had } else if (!mTarget) { here, and not just } else {. Do you? How did we make sure we were painting everything when compositing to a DrawTarget?

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8534170 [details] [diff] [review]
patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +295,1 @@
>      mInvalidRegion.Or(mInvalidRegion, mRenderBounds);

This code is was making assumptions about what happens in begin transaction. It was assumed that if we didn't have a mClonedLayerTreeProperties we must have a DT or if we had it we didn't have a DT. I think the original code was just terrible. It really should of been:
 } else { MOZ_ASSERT(mTarget); ... }.

With this patch we run this code unconditionally and this logic no longer has this assumption about BeginTransactionXXX. Let me know if you still find it confusing.

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 8534170 [details] [diff] [review]
patch

Ok this passes try:
https://tbpl.mozilla.org/?tree=Try&rev=786498b4e431

Comment 8

[Benoit Girard (:BenWa)]

My comment above got the logic wrong. Marking as 'obsolete'.

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 8534170 [details] [diff] [review]
patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +295,1 @@
>      mInvalidRegion.Or(mInvalidRegion, mRenderBounds);

Actually I think with this patch this branch is no longer needed. We should always be tracking the invalid region regardless if we have mTarget or not.

Comment 10

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=c101d3911f01

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8535186 [details] [diff] [review]
patch v2

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

Does this pass tests?

The invalid region is the difference between the current layer tree and what we last composited. If we didn't composite to the same DT last time, then this invalid region won't be meaningful.

I think we want to keep the code setting the invalid region to the window bounds, and also make sure that we invalidate all cached ContainerLayers instead.

Comment 13

[Markus Stange [:mstange]]

In what scenarios do we composite into a DrawTarget?

Comment 14

[Matt Woodrow (:mattwoodrow)]

drawWindow, so usually reftests.

Comment 15

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> I think we want to keep the code setting the invalid region to the window
> bounds, and also make sure that we invalidate all cached ContainerLayers
> instead.

This would mean that performance would be worse, it's an extra path that different and it give us no practical way to test container layer recycling. Even with GTest we're still using a draw target.

I don't think we have a good reason to have invalidation depend on the draw target or not. At least not in the OGL case where we just read back. I'm not sure about the other backends.

Comment 16

[Benoit Girard (:BenWa)]

Attachment: irc discussion

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking!

Comment 18

[Benoit Girard (:BenWa)]

Updated container for bug 1109314.
To connect: ssh [e-mail] 1109314

For more information see: https://wiki.mozilla.org/DockerEnv

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Benoit, any plan wrt 36? Beta 8 is going to build today.

Comment 20

[Benoit Girard (:BenWa)]

I think we're going to let it slide, we have no evidence that this is impacting users. I'll come back to this when I find the time.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

OK, wontfix then. Merci!

Comment 22

[Benoit Girard (:BenWa)]

Quick update:
- This bug will only show up if we're taking a screenshot under very specific circumstances. I do plan on landing a test for this with the fix however to make sure we don't re-regress this.
- It's marked as tracking because I don't want us to forget about this bug but it's probably ok to ship this regression since no one has noticed a failure.
- I wont have time to fix it before I go on my leave and I don't think it's worth assigning someone else to look at it.
- I plan on finishing this after my leave this summer.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Given comment 22 and the timing of Benoit's leave, it sounds like we want to track this for 41 at the earliest.  No way to set that flag yet, so we will just leave needinfo on Benoit to set the tracking flag once 41 is available.

Comment 24

[Benoit Girard (:BenWa)]

For now this doesn't appear to be causing any issue. I don't think it's worth fixing. Let me know if you disagree.