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

RESOLVED FIXED in mozilla33

Status

()

Core
Layout: View Rendering
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: obrufau, Assigned: mstange)

Tracking

32 Branch
mozilla33
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32+ unaffected)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8432022 [details]
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
(Reporter)

Updated

4 years ago
See Also: → bug 1014764
Bug 1014764 is fixed in the latest nightly, while this one isn't.
32.0a1 (2014-06-01), win 7 x64
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

4 years ago
Can only confirm with HWA disabled.
Summary: Bad display of transition opacity since 21-May-2014 update → Bad display of transition opacity since 21-May-2014 update with HWA disabled
tracking-firefox32: --- → ?
Is this Windows-only?  Is it a result of enabling OMTC on Windows in https://hg.mozilla.org/mozilla-central/rev/46d9ffb97fe3 ?

Comment 4

4 years ago
(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

4 years ago
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.
I can reproduce this with BasicCompositor on Mac.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
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?
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
tracking-firefox32: ? → +
Unfortunately the reftest doesn't fail with BasicCompositor without the patch.
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?
(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?
Created attachment 8437994 [details] [diff] [review]
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.
Attachment #8437850 - Attachment is obsolete: true
Attachment #8437850 - Flags: review?(matt.woodrow)
Attachment #8437994 - Flags: review?(matt.woodrow)
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?
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.
Attachment #8437994 - Flags: review?(matt.woodrow) → review+
The B2G reftest failure is probably bug 1000722 (margin on oop iframe in reftest suite).
https://hg.mozilla.org/mozilla-central/rev/85e37b8bf188
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Marking 32 as unaffected based on comment 4. (OMTC is disabled in 32.)
status-firefox32: --- → unaffected
You need to log in before you can comment on or make changes to this bug.