Black background is drawing

VERIFIED FIXED in Firefox 35

Status

()

Core
Graphics
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Alice0775 White, Assigned: jgilbert)

Tracking

({regression})

33 Branch
mozilla35
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox32 unaffected, firefox33+ wontfix, firefox34+ wontfix, firefox35 verified, firefox36 verified)

Details

(URL)

Attachments

(7 attachments, 5 obsolete attachments)

55.02 KB, image/png
Details
434.98 KB, application/zip
Details
11.91 KB, text/html
Details
22.49 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.11 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
8.81 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.88 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

3 years ago
Component: Canvas: WebGL → Graphics
(Reporter)

Comment 1

3 years ago
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
Vlad, does it ring a bell?
Flags: needinfo?(vladimir)
Graphic is so important on Firefox now, tracking.
tracking-firefox33: ? → +
tracking-firefox34: ? → +
(Reporter)

Comment 4

3 years ago
Not responding,
So, It is time to backed out the offending patches.
Milan, do you think we should backout the patches here? Thanks
Flags: needinfo?(milan)
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?
Flags: needinfo?(wlitwinczyk)
Flags: needinfo?(milan)
Flags: needinfo?(dglastonbury)
(Reporter)

Comment 7

3 years ago
(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
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.
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.
Walter, Let me know if you need help.
Flags: needinfo?(dglastonbury)
(Assignee)

Updated

3 years ago
Created attachment 8483171 [details]
grav.zip

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

Comment 12

3 years ago
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.
Flags: needinfo?(wlitwinczyk)
Flags: needinfo?(vladimir)
(Assignee)

Comment 13

3 years ago
Created attachment 8483232 [details] [diff] [review]
dst-alpha-blending

This appears to fix it locally.
Try run at:
 https://tbpl.mozilla.org/?tree=Try&rev=6aeffb468b5a
Assignee: nobody → jgilbert
Attachment #8483232 - Flags: review?(dglastonbury)
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.
Attachment #8483232 - Flags: review?(dglastonbury) → review+
Jeff, are you going to land this patch soon? Thanks
Flags: needinfo?(jgilbert)
(Assignee)

Comment 16

3 years ago
(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.
Flags: needinfo?(jgilbert)
Jeff, any news on this? Thanks
Flags: needinfo?(jgilbert)
(Assignee)

Comment 18

3 years ago
Created attachment 8494029 [details]
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.
Flags: needinfo?(jgilbert)
(Assignee)

Comment 19

3 years ago
Created attachment 8494047 [details] [diff] [review]
rgbx-config
Attachment #8483232 - Attachment is obsolete: true
Attachment #8494047 - Flags: review?(dglastonbury)
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.
Attachment #8494047 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 21

3 years ago
Created attachment 8494859 [details] [diff] [review]
rgbx-config

Bitrot.
Attachment #8494047 - Attachment is obsolete: true
Attachment #8494859 - Flags: review+
(Assignee)

Comment 22

3 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8bba4620826b
(Assignee)

Comment 23

3 years ago
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.
Attachment #8494859 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8bba4620826b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Jeff, this bug is marked as affecting 33 too (beta). Are you going to ask for an uplift to beta? Thanks
status-firefox35: --- → fixed
Flags: needinfo?(jgilbert)
Depends on: 1072935
(Reporter)

Updated

3 years ago
Depends on: 1073112
(Assignee)

Comment 26

3 years ago
Created attachment 8495539 [details] [diff] [review]
patch 2: Actually iterate through the configs
Attachment #8495539 - Flags: review?(dglastonbury)
Flags: needinfo?(jgilbert)
(Assignee)

Comment 27

3 years ago
Created attachment 8495541 [details] [diff] [review]
patch 3: ANGLE BGRX should be like BGR8 not BGRA8
Attachment #8495541 - Flags: review?(dglastonbury)
(Assignee)

Comment 28

3 years ago
Reopened for perf regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8495539 - Flags: review?(dglastonbury) → review+
Attachment #8495541 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 30

3 years ago
  https://hg.mozilla.org/integration/mozilla-inbound/rev/5da1e1c4c706
  https://hg.mozilla.org/integration/mozilla-inbound/rev/dc8fac10012d
(Assignee)

Comment 31

3 years ago
(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?
Flags: needinfo?(sledru)
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
Flags: needinfo?(jgilbert)
It's failing on Windows, too.
Jeff, beta 8 is going to build next Monday (during the afternoon PST). It would be nice if the patch could land before that.
Flags: needinfo?(sledru)
(Assignee)

Comment 35

3 years ago
Created attachment 8496231 [details] [diff] [review]
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.
Attachment #8495541 - Attachment is obsolete: true
Attachment #8496231 - Flags: review?(dglastonbury)
Flags: needinfo?(jgilbert)
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?
Attachment #8496231 - Flags: review?(dglastonbury) → review+
Jeff, could you land it asap and request for the uplifts? Thanks
Flags: needinfo?(jgilbert)
(Assignee)

Comment 38

3 years ago
(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.
Flags: needinfo?(jgilbert)
(Assignee)

Comment 39

3 years ago
(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.
(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!
(Assignee)

Comment 41

3 years ago
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).
(Assignee)

Updated

3 years ago
Attachment #8494859 - Flags: approval-mozilla-aurora?
Taking a step back, why did this bug suddenly start happening? ANGLE update or has it always been lurking?
(Assignee)

Comment 43

3 years ago
(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.
(Assignee)

Comment 44

3 years ago
Created attachment 8497910 [details] [diff] [review]
patch 3: Don't require no-alpha on ANGLE
Attachment #8496231 - Attachment is obsolete: true
Attachment #8497910 - Flags: review?(dglastonbury)
(Assignee)

Comment 45

3 years ago
Created attachment 8497911 [details] [diff] [review]
patch 4: Work around not having no-alpha on ANGLE
Attachment #8497911 - Flags: review?(dglastonbury)
(Assignee)

Comment 46

3 years ago
 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=700d62f0a8a8
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?
Attachment #8497910 - Flags: review?(dglastonbury) → review-
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?
Attachment #8497911 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 49

3 years ago
Created attachment 8498423 [details] [diff] [review]
patch 3: Don't require no-alpha on ANGLE

Yep, oops.
Attachment #8497910 - Attachment is obsolete: true
Attachment #8498423 - Flags: review?(dglastonbury)
(Assignee)

Comment 50

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=59d4f4ce01ae
(Assignee)

Comment 51

3 years ago
(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.
(Assignee)

Comment 52

3 years ago
...I don't think the call-site makes sense if we just have `ScopedMask autoMask(*this)`.
Attachment #8498423 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 53

3 years ago
 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
https://hg.mozilla.org/mozilla-central/rev/0155f960161f
https://hg.mozilla.org/mozilla-central/rev/45472bfa3f16
https://hg.mozilla.org/mozilla-central/rev/e17f9996e092
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
This is too late for 33 but we would be happy to take in aurora (34).
Thanks
status-firefox33: affected → wontfix
Flags: needinfo?(jgilbert)
Flags: qe-verify+
Jeff - Can you please submit an uplift request to get this fix into 34? As of Monday this is a request for Beta.
Flags: needinfo?(jgilbert)
(Reporter)

Updated

3 years ago
Depends on: 1081497
(Assignee)

Comment 57

3 years ago
(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.
A regression is already reported. See bug 1081497.
(Assignee)

Comment 59

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

Yep, that's what I meant, sorry.

Updated

3 years ago
Depends on: 1089140
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.
status-firefox34: affected → wontfix
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.
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
status-firefox36: --- → verified

Updated

3 years ago
Depends on: 1122465
(Reporter)

Updated

3 years ago
Depends on: 1123297
(Reporter)

Updated

3 years ago
Depends on: 1125833

Updated

3 years ago
Depends on: 1125445
You need to log in before you can comment on or make changes to this bug.