1048108 - Black background is drawing

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Alice0775 White]

Attachment: Screenshot

[Tracking Requested - why for this release]: regression

Steps To Reproduce:
1. Open http://gregtatum.com/poems/gravity/1/
2. Click "LOW"

Actual Results:
 Black background is drawing

Expected Results:
 Transparent


Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/225cc2ec2231
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140704081823
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/401bc7471c24
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140704085121
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=225cc2ec2231&tochange=401bc7471c24

Regressed by: Bug 1010371 - Update ANGLE, 2014-06

Comment 1

[Alice0775 White]

Graphics
--------

Adapter Description: ATI Radeon HD 4300/4500 Series
Adapter Drivers: aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
Adapter RAM: 512
ClearType Parameters: Gamma: 2200 Pixel Structure: R ClearType Level: 50 Enhanced Contrast: 200
Device ID: 0x954f
Direct2D Enabled: true
DirectWrite Enabled: true (6.2.9200.16492)
Driver Date: 4-29-2013
Driver Version: 8.970.100.1100
GPU #2 Active: false
GPU Accelerated Windows: 1/1 Direct3D 11 (OMTC)
Subsys ID: 00000000
Vendor ID: 0x1002
WebGL Renderer: Google Inc. -- ANGLE (ATI Radeon HD 4300/4500 Series Direct3D9Ex vs_3_0 ps_3_0)
windowLayerManagerRemote: true
AzureCanvasBackend: direct2d
AzureContentBackend: direct2d
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Vlad, does it ring a bell?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Graphic is so important on Firefox now, tracking.

Comment 4

[Alice0775 White]

Not responding,
So, It is time to backed out the offending patches.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Milan, do you think we should backout the patches here? Thanks

Comment 6

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

Since this isn't a recent landing, and a fairly significant one with some functionality we're using, we'll try to sort this out in the next 10 days; if not resolved by 9/12, we will back it out.
Walter, Dan, can you tag team on this?  Alice, does disabling OMTC or Direct2D help this problem go away?

Comment 7

[Alice0775 White]

(In reply to Milan Sreckovic [:milan] from comment #6)
> Alice, does disabling OMTC or Direct2D help this problem go away?


It works if set layers.acceleration.disabled = true

Comment 8

[Walter Litwinczyk [:walter]]

I only have Win8 to test on at the moment, but the problem is still there if I set 'layers.acceleration.disabled = true'. It works if I set 'webgl.disable-angle = true', but then it renders differently than it does on Linux. It looks like there's some blending, which turns to black (but background is transparent) on Windows and turns bright yellow on Linux.

Comment 9

[Walter Litwinczyk [:walter]]

So I updated my nightly version (which is an older version of 34) to 35 and the blending problem is gone, but the black boxes are still there, taking a look into it.

Comment 10

[u480271]

Walter, Let me know if you need help.

Comment 11

[Walter Litwinczyk [:walter]]

Attachment: grav.zip

I pulled down the website to make it easier to modify/test

Comment 12

[Kelsey Gilbert [:jgilbert]]

This is fixed if you edit phaser.js around line 4911:
Replace:
PIXI.blendModesWebGL[PIXI.blendModes.ADD]           = [gl.SRC_ALPHA, gl.DST_ALPHA];
With:
PIXI.blendModesWebGL[PIXI.blendModes.ADD]           = [gl.SRC_ALPHA, gl.ONE];

I believe this is because ANGLE is not properly handling blending for surfaces with no alpha channel. This should actually be a simple fix inside ANGLE. This can be worked around in our code, as well.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: dst-alpha-blending

This appears to fix it locally.
Try run at:
 https://tbpl.mozilla.org/?tree=Try&rev=6aeffb468b5a

Comment 14

[u480271]

Comment on attachment 8483232 [details] [diff] [review]
dst-alpha-blending

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

Surprising amount of code for something simple but looks OK to me.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Jeff, are you going to land this patch soon? Thanks

Comment 16

[Kelsey Gilbert [:jgilbert]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #15)
> Jeff, are you going to land this patch soon? Thanks

Sorry, I've been trying to create a testcase for this. My initial try at replicating the issue in a test was unsuccessful, which can indicate the real problem is not precisely what I thought it was.

I just succeeded at reproducing the issue in a testcase, so I should have a better idea what's up shortly.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Jeff, any news on this? Thanks

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase

Alright, here's a testcase.
I believe we have a couple workarounds in place that obscure the core bug here.

It looks like we ask for an RGBX PBuffer config from ANGLE, but it gives us an RGBA config. We need to weed out the RGBA configs when we want just RGBX.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: rgbx-config

Comment 20

[u480271]

Comment on attachment 8494047 [details] [diff] [review]
rgbx-config

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

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +350,2 @@
>      mConfig = ChooseConfig(mProdGL, mEGL, mReadCaps);
> +    if (mConfig == EGL_NO_CONFIG) {

I thought we omitted { } for single line bodies.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Attachment: rgbx-config

Bitrot.

Comment 22

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/8bba4620826b

Comment 23

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8494859 [details] [diff] [review]
rgbx-config

Approval Request Comment
[Feature/regressing bug #]: Unknown
[User impact if declined]: Bad rendering on some WebGL content.
[Describe test coverage new/current, TBPL]: This patch adds tests.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bba4620826b

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Jeff, this bug is marked as affecting 33 too (beta). Are you going to ask for an uplift to beta? Thanks

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Actually iterate through the configs

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: ANGLE BGRX should be like BGR8 not BGRA8

Comment 28

[Kelsey Gilbert [:jgilbert]]

Reopened for perf regressions.

Comment 29

[u480271]

Comment on attachment 8495539 [details] [diff] [review]
patch 2: Actually iterate through the configs

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

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +287,5 @@
>      // The requests passed to ChooseConfig are treated as minimums. If you ask
>      // for 0 bits of alpha, we might still get 8 bits.
>      EGLConfig config = EGL_NO_CONFIG;
>      for (int i = 0; i < foundConfigs; i++) {
> +        EGLConfig cur = configs[i];

Oh. yeah. hmmm.

Comment 30

[Kelsey Gilbert [:jgilbert]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/5da1e1c4c706
  https://hg.mozilla.org/integration/mozilla-inbound/rev/dc8fac10012d

Comment 31

[Kelsey Gilbert [:jgilbert]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #25)
> Jeff, this bug is marked as affecting 33 too (beta). Are you going to ask
> for an uplift to beta? Thanks

Yeah, sure. Since this causes bad rendering in real-world demos, we'd like to take it as far forward as we can. We need to take it one step at a time here, though. How far along is Beta?

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for build bustage on b2g: https://hg.mozilla.org/integration/mozilla-inbound/rev/1763f5d09746

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2572547&repo=mozilla-inbound

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

It's failing on Windows, too.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Jeff, beta 8 is going to build next Monday (during the afternoon PST). It would be nice if the patch could land before that.

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: ANGLE BGRX should be like BGR8 not BGRA8

Adding a format is harder than I thought.

My original patch just pretended it was GL_RGB8_OES, but it seems like that's asking for r/b-swap issues down the line.

Comment 36

[u480271]

Comment on attachment 8496231 [details] [diff] [review]
patch 3: ANGLE BGRX should be like BGR8 not BGRA8

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

::: gfx/angle/src/common/angleutils.h
@@ +142,5 @@
>  #define VENDOR_ID_AMD 0x1002
>  #define VENDOR_ID_INTEL 0x8086
>  #define VENDOR_ID_NVIDIA 0x10DE
>  
> +#define GL_BGRX8_ANGLEX 0x6ABB

Is it safe just to make these up?

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Jeff, could you land it asap and request for the uplifts? Thanks

Comment 38

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #36)
> Comment on attachment 8496231 [details] [diff] [review]
> patch 3: ANGLE BGRX should be like BGR8 not BGRA8
> 
> Review of attachment 8496231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/angle/src/common/angleutils.h
> @@ +142,5 @@
> >  #define VENDOR_ID_AMD 0x1002
> >  #define VENDOR_ID_INTEL 0x8086
> >  #define VENDOR_ID_NVIDIA 0x10DE
> >  
> > +#define GL_BGRX8_ANGLEX 0x6ABB
> 
> Is it safe just to make these up?

"They started it." I believe I have everything plumbed right, but it's pretty hard to tell.

Comment 39

[Kelsey Gilbert [:jgilbert]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #37)
> Jeff, could you land it asap and request for the uplifts? Thanks

I would upgrade the risk of regression here to medium, so I don't know if we really want to land on Beta. I'll be working on getting it landed on trunk and hopefully Aurora.

Comment 40

[Sylvestre Ledru [:Sylvestre]]

(In reply to Jeff Gilbert [:jgilbert] from comment #39)
> I would upgrade the risk of regression here to medium, so I don't know if we
> really want to land on Beta.
Probably not then!

Comment 41

[Kelsey Gilbert [:jgilbert]]

I filed an ANGLE bug for this: https://code.google.com/p/angleproject/issues/detail?id=764

I have had a surprising lack of success at adding an RGBX format. The way ANGLE deals with texture formats is arcane, and it appears to convert between GL and D3D repeatedly. I do not think I am the right person to fix this in ANGLE.

For now, we should try to maintain colorMask(1,1,1,0), except for our initial clear, which should actually clear to [0,0,0,1] with colorMask(1,1,1,1).

Comment 42

[u480271]

Taking a step back, why did this bug suddenly start happening? ANGLE update or has it always been lurking?

Comment 43

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #42)
> Taking a step back, why did this bug suddenly start happening? ANGLE update
> or has it always been lurking?

I don't actually know. I know we've had no-alpha-on-alpha-canvas workarounds in the past, so I would not be surprised if this is effectively a bug in our workaround.

Comment 44

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Don't require no-alpha on ANGLE

Comment 45

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: Work around not having no-alpha on ANGLE

Comment 46

[Kelsey Gilbert [:jgilbert]]

 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=700d62f0a8a8

Comment 47

[u480271]

Comment on attachment 8497910 [details] [diff] [review]
patch 3: Don't require no-alpha on ANGLE

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

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +299,5 @@
> +
> +        // We can't enforce alpha on ANGLE yet because of:
> +        // https://code.google.com/p/angleproject/issues/detail?id=764
> +        if (!gl->IsANGLE()) {
> +            if (!DoesAttribBitsMatchCapBool(egl, cur, LOCAL_EGL_DEPTH_SIZE,

You've written EGL_DEPTH_SIZE. Do you mean EGL_ALPHA_SIZE?

Comment 48

[u480271]

Comment on attachment 8497911 [details] [diff] [review]
patch 4: Work around not having no-alpha on ANGLE

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

::: dom/canvas/WebGLContext.cpp
@@ +1819,5 @@
>  
> +////////////////////////////////////////////////////////////////////////////////
> +
> +
> +WebGLContext::ScopedMaskWorkaround::ScopedMaskWorkaround(WebGLContext& webgl)

I don't think we need to encode workaround in the name of the class. It's a ScopedMask RAII helper.

::: dom/canvas/WebGLContext.h
@@ +1335,5 @@
> +
> +        static bool NeedsChange(WebGLContext& webgl) {
> +            return webgl.mNeedsFakeNoAlpha &&
> +                   webgl.mColorWriteMask[3] != false;
> +        }

What if, between constructing and destructing an ScopedMask RAII, someone manipulates the color mask directly. Should that be handled?

Comment 49

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Don't require no-alpha on ANGLE

Yep, oops.

Comment 50

[Kelsey Gilbert [:jgilbert]]

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

Comment 51

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #48)
> Comment on attachment 8497911 [details] [diff] [review]
> patch 4: Work around not having no-alpha on ANGLE
> 
> Review of attachment 8497911 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContext.cpp
> @@ +1819,5 @@
> >  
> > +////////////////////////////////////////////////////////////////////////////////
> > +
> > +
> > +WebGLContext::ScopedMaskWorkaround::ScopedMaskWorkaround(WebGLContext& webgl)
> 
> I don't think we need to encode workaround in the name of the class. It's a
> ScopedMask RAII helper.
I don't think the call-site makes sense if we just have 
> 
> ::: dom/canvas/WebGLContext.h
> @@ +1335,5 @@
> > +
> > +        static bool NeedsChange(WebGLContext& webgl) {
> > +            return webgl.mNeedsFakeNoAlpha &&
> > +                   webgl.mColorWriteMask[3] != false;
> > +        }
> 
> What if, between constructing and destructing an ScopedMask RAII, someone
> manipulates the color mask directly. Should that be handled?

This should not happen, and it's unlikely enough that I don't think it's worth asserting.

Comment 52

[Kelsey Gilbert [:jgilbert]]

...I don't think the call-site makes sense if we just have `ScopedMask autoMask(*this)`.

Comment 53

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/0155f960161f
 https://hg.mozilla.org/integration/mozilla-inbound/rev/45472bfa3f16
 https://hg.mozilla.org/integration/mozilla-inbound/rev/e17f9996e092

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0155f960161f
https://hg.mozilla.org/mozilla-central/rev/45472bfa3f16
https://hg.mozilla.org/mozilla-central/rev/e17f9996e092

Comment 55

[Sylvestre Ledru [:Sylvestre]]

This is too late for 33 but we would be happy to take in aurora (34).
Thanks

Comment 56

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

Jeff - Can you please submit an uplift request to get this fix into 34? As of Monday this is a request for Beta.

Comment 57

[Kelsey Gilbert [:jgilbert]]

(In reply to Lawrence Mandel [:lmandel] from comment #56)
> Jeff - Can you please submit an uplift request to get this fix into 34? As
> of Monday this is a request for Beta.

Let's check the regression before trying to uplift this. These patches definitely have a medium chance of causing regressions.

Comment 58

[Masatoshi Kimura [:emk]]

A regression is already reported. See bug 1081497.

Comment 59

[Kelsey Gilbert [:jgilbert]]

(In reply to Masatoshi Kimura [:emk] from comment #58)
> A regression is already reported. See bug 1081497.

Yep, that's what I meant, sorry.

Comment 60

[Liz Henry (:lizzard) (relman/hg->git project)]

It looks like this has missed the time window to be part of the 34 release as we are about to go to build. Marking wontfix for 34.

Comment 61

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 34 Beta 10 (20141117202603) using Windows 7 x64.

Verified fixed on Latest Firefox 35.0a2 (2014-11-24) and Latest Firefox 36.0a1 (2014-11-24) using Windows 7 x64.

Setting this bug Verified since the other issues related with this bug are separately tracked.