1018522 - Bad display of transition opacity since 21-May-2014 update with HWA disabled

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[obrufau]

Attachment: Demo

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140530030202

Steps to reproduce:

Open the attached demo and hover some elements.


Actual results:

Opacity changes suddenly, after transition-duration.


Expected results:

Elements should fade smoothly - there should be a transition.

This bug may have the same cause as Bug 1014764.

Good (20-May-2014): https://hg.mozilla.org/mozilla-central/rev/cb9f34f73ebe
Bad (21-May-2014): https://hg.mozilla.org/mozilla-central/rev/9d8d16695f6a
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb9f34f73ebe&tochange=9d8d16695f6a

Comment 1

[Paul Silaghi, QA [:pauly]]

Bug 1014764 is fixed in the latest nightly, while this one isn't.
32.0a1 (2014-06-01), win 7 x64

Comment 2

[Elbart]

Can only confirm with HWA disabled.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this Windows-only?  Is it a result of enabling OMTC on Windows in https://hg.mozilla.org/mozilla-central/rev/46d9ffb97fe3 ?

Comment 4

[Elbart]

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC+2) from comment #3)
> Is this Windows-only?  Is it a result of enabling OMTC on Windows in
> https://hg.mozilla.org/mozilla-central/rev/46d9ffb97fe3 ?

It's not happening with OMTC and HWA off, no.

Regression range (same for bug 1014764):

m-c:
Last good revision: 41a54c8add09 (2014-05-19)
First bad revision: cb9f34f73ebe (2014-05-20)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=41a54c8add09&tochange=cb9f34f73ebe

m-i:
Last good revision: 5bb98302ae5d
First bad revision: 2708630b4998
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5bb98302ae5d&tochange=2708630b4998

43a9f1fb84cb	Bas Schouten — Bug 1012198: Allow BasicCompositor to be used on windows. r=BenWa

Comment 5

[Elbart]

With layers.offmainthreadcomposition.force-basic;true, the range is:

Last good revision: 58c5a3427997 (2014-05-16)
First bad revision: 2893f60d5903 (2014-05-17)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58c5a3427997&tochange=2893f60d5903

Last good revision: 01e4e4f65079
First bad revision: c666f8f24d29
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=01e4e4f65079&tochange=c666f8f24d29

c666f8f24d29	Chris Lord — Bug 994088 - Only redraw everything on every frame with e10s in BasicCompositor. r=mattwoodrow This was a work-around for an X compositor bug.

Might need confirmation, though.

Comment 6

[Markus Stange [:mstange]]

I can reproduce this with BasicCompositor on Mac.

Comment 7

[Markus Stange [:mstange]]

LayerTreeInvalidation doesn't pick up the opacity change because it compares layer->GetLocalOpacity(), and the shadow opacity changes outside the transaction.

LayerTransactionParent::RecvUpdate does this:

[...]
  {
    AutoResolveRefLayers resolve(mShadowLayersManager->GetCompositionManager(this));
    layer_manager()->BeginTransaction();
  }
[...]
    // Attributes
    case Edit::TOpSetLayerAttributes: {
[...]
      layer->SetOpacity(common.opacity());
[...]
  {
    AutoResolveRefLayers resolve(mShadowLayersManager->GetCompositionManager(this));
    layer_manager()->EndTransaction(nullptr, nullptr, LayerManager::END_NO_IMMEDIATE_REDRAW);
  }

[...]

  mShadowLayersManager->ShadowLayersUpdated(this, aTransactionId, targetConfig,
                                            isFirstPaint, scheduleComposite, paintSequenceNumber);
[...]

and the shadow opacity is only updated during ShadowLayersUpdated, after EndTransaction has compared the layer properties.

Any ideas how to fix this? Can we just move the call to ShadowLayersUpdated before the EndTransaction call, or will that break things?

Comment 8

[Matt Woodrow (:mattwoodrow)]

I think that will be ok with a few changes. ShadowLayersUpdated currently assumes that we haven't resolved ref layers, and does it if required. Doing it twice will probably crash.

If we change that to always assume it's already done, and remove the AutoResolveRefLayers objects inside then we should be ok. I can see two of them in ShadowLayersUpdated, and one in CompositorParent::NotifyShadowTreeTransaction that would need to go.

Comment 9

[Markus Stange [:mstange]]

Attachment: v1

Comment 10

[Markus Stange [:mstange]]

Unfortunately the reftest doesn't fail with BasicCompositor without the patch.

Comment 11

[Markus Stange [:mstange]]

Wait a minute. I think I've seen a case where the layer manager didn't have a root in AsyncCompositionManager::ResolveRefLayers(), but in AssertRefLayersAreResolved() it suddenly had one. So that means that the new resolution place may not resolve all the layers that we used to resolve in the old places (which are now just AssertRefLayersAreResolved()). That makes me a bit uncomfortable.

Why don't we want to call mShadowLayersManager->ShadowLayersUpdated outside of (i.e. before) the AutoResolveRefLayers scope around the EndTransaction call?

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #11)
> Wait a minute. I think I've seen a case where the layer manager didn't have
> a root in AsyncCompositionManager::ResolveRefLayers(), but in
> AssertRefLayersAreResolved() it suddenly had one. So that means that the new
> resolution place may not resolve all the layers that we used to resolve in
> the old places (which are now just AssertRefLayersAreResolved()). That makes
> me a bit uncomfortable.
> 
> Why don't we want to call mShadowLayersManager->ShadowLayersUpdated outside
> of (i.e. before) the AutoResolveRefLayers scope around the EndTransaction
> call?

I can't think of a reason why that wouldn't work. Give it a shot?

Comment 13

[Markus Stange [:mstange]]

Attachment: v2

I thought there might have been a reason that you didn't suggest this in the first place. Seems to work, I'll start a tryserver run.

Comment 14

[Markus Stange [:mstange]]

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

Comment 15

[Markus Stange [:mstange]]

The new reftest fails on R6 on B2G ICS Emulator Opt - it looks like the invalidated rect is offset. But doesn't the emulator use CompositorOGL? And doesn't CompositorOGL reftesting snapshot the whole window every time?

Comment 16

[Matt Woodrow (:mattwoodrow)]

I think we're using CompositorOGL, but I'm not 100% sure. I wouldn't trust the emulators. CompositorOGL should snapshot the entire window always.

Comment 17

[Markus Stange [:mstange]]

The B2G reftest failure is probably bug 1000722 (margin on oop iframe in reftest suite).

Comment 18

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85e37b8bf188

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85e37b8bf188

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Marking 32 as unaffected based on comment 4. (OMTC is disabled in 32.)