Closed Bug 1159034 Opened 5 years ago Closed 5 years ago

No-alpha canvases cause uninitialized textures to be (0,0,0,1) not (0,0,0,0)

Categories

(Core :: Canvas: WebGL, defect)

38 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox-esr38 --- wontfix

People

(Reporter: jgilbert, Assigned: jgilbert)

References

()

Details

Attachments

(2 files)

Forwarded to me via Twitter:
https://github.com/GoodBoyDigital/pixi.js/issues/1636

Testcase in URL field:
http://jsfiddle.net/6r5e070w/

Correct: Blue circle on a green background.
Firefox+Windows+ANGLE issue: Blue circle on a black background.
(Works fine on Chrome on same system)

I traced the behavior diversion (vs Chrome) to the first drawElements, which I believe draws the circle. The draw buffer for this op should have been N*[0,0,0,0], since that's the default for a null-initialized texture. Instead, it was N*[0,0,0,1.0].

This comes from ForceClearFramebufferWithDefaultValues, which we currently use to initialize these null textures. This function has a branch for mNeedsFakeNoAlpha for handling no-alpha contexts on ANGLE. This branch should only be taken if we're operating on the backbuffer.

This issue persists back to 37 (currently Release).

This can be worked around by asking for alpha:true during context creation.
Attachment #8598303 - Flags: review?(dglastonbury)
Attachment #8598303 - Flags: review?(dglastonbury) → review+
https://hg.mozilla.org/mozilla-central/rev/5242ffa9e8d5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Dan's on PTO. Can you review this, David?
Attachment #8601233 - Flags: review?(dvander)
Comment on attachment 8598303 [details] [diff] [review]
0001-Only-fake-no-alpha-for-the-backbuffer.patch

Approval Request Comment
[Feature/regressing bug #]: clearing buffers with glClear()
[User impact if declined]: Some WebGL content will be broken.
[Describe test coverage new/current, TreeHerder]: Regression test is written and working on Try, landing soon.
[Risks and why]: No risk.
[String/UUID change made/needed]: None.
Attachment #8598303 - Flags: approval-mozilla-aurora?
Attachment #8601233 - Flags: review?(dvander) → review+
Jeff, what is the bug number for the test that you're landing soon? Let's wait for that to land before uplift. I'm happy to take this in early beta if we need to wait for the test.
Flags: needinfo?(jgilbert)
(In reply to Liz Henry (:lizzard) from comment #7)
> Jeff, what is the bug number for the test that you're landing soon? Let's
> wait for that to land before uplift. I'm happy to take this in early beta if
> we need to wait for the test.

It landed in this bug in comment 5 and 6.
Flags: needinfo?(jgilbert)
Comment on attachment 8598303 [details] [diff] [review]
0001-Only-fake-no-alpha-for-the-backbuffer.patch

Both patches approved for uplift to beta (39). This already landed on 40 so doesn't need uplift to aurora.
Attachment #8598303 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
This seems low enough damage that we should WONTFIX it for ESR38.
Jeff, one more question, do you want to write a release note for this fix for 39?
Flags: needinfo?(jgilbert)
(In reply to Liz Henry (:lizzard) from comment #12)
> Jeff, one more question, do you want to write a release note for this fix
> for 39?

I don't think it's a big enough fix to warrant that, but here's a blurb if we want something:
Fixed a bug where uninitialized textures defaulted to (0,0,0,1) instead of (0,0,0,0) on some platforms.
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.