922875 - Stencil test fails even if there is no stencil buffer in the current FBO.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jukka Jylänki]

This bug was found when testing for https://bugzilla.mozilla.org/show_bug.cgi?id=783914 .

To reproduce, visit

I. https://dl.dropboxusercontent.com/u/40949268/Bugs/firefox_enable_stencil_test_bug.html

II. https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_D16_UNORM&-depthrenderbuffer&-stencilformat&TextureFormat_NONE

or
III. https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-colorformat&TextureFormat_NONE&-depthformat&TextureFormat_NONE&-stencilformat&TextureFormat_NONE

Result:
in each case, the screen stays black.

Expected:
There should be content rendered on screen.

This is actually a regression.

Fails on Firefox 24.0 Stable, Firefox Nightly 26.0a1 (2013-09-15) and Firefox Trunk (as of today), but works on Firefox 23.0.1 Stable.

See also bug https://bugzilla.mozilla.org/show_bug.cgi?id=747498 , which might be related, but this cannot be a direct result of that bug, due to testcase II above, which uses an offscreen FBO.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jukka Jylänki from comment #0)
> This is actually a regression.
> 
> Fails on Firefox 24.0 Stable, Firefox Nightly 26.0a1 (2013-09-15) and
> Firefox Trunk (as of today), but works on Firefox 23.0.1 Stable.

Thanks for finding this useful piece of information! Hopefully we can get a narrower regression window from our QA people.

Question: you filed this as x86_64, do you mean 64bit firefox or 32bit-firefox-running-on-64bit-windows?

(If the former, as it's not officially supported, I'd like to know if the bug reproduces in the latter).

For what it's worth, this does not reproduce on linux 64bit / mesa even after the patches in bug 922810, so there really is something Windows-specific there.

Comment 2

[Jukka Jylänki]

I did use only 32-bit Firefoxes to test, the operating system is 64-bit.

Comment 3

[Jason Smith [:jsmith]]

If you are looking for a regression window here, you probably want the desktop QA team, as this looks like a bug on the desktop side.

Comment 4

[Jukka Jylänki]

Btw, as mentioned in comment 50 in https://bugzilla.mozilla.org/show_bug.cgi?id=783914#c50 , there are two separate bugs related to stenciling. This bug report 922875 covers the case I from that comment 50.

I don't have a handwritten WebGL repro of the second stenciling bug (case II in comment 50), but only those two Emscripten applications. When looking for a regression window, I would be interested to know if there is any Firefox version where either of the two links from comment 50 work (work==render a periodically blinking cube instead of a blank screen):

a. https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_D24_UNORM_S8_UINT&-depthrenderbuffer&-stencilformat&TextureFormat_NONE

b. https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_NONE&-stencilformat&TextureFormat_S8_UINT&-stencilrenderbuffer

I was not been able to test either of the above links with Firefox 23 stable, since the Firefox autoupdater updated me to 24 stable before I was able to do it.

Comment 5

[Alice0775 White]

I can reproduce if HWA disabled

Comment 6

[Alice0775 White]

(In reply to Alice0775 White from comment #5)
> I can reproduce if HWA disabled

Sorry, please ignore.

Comment 7

[u279076]

Ioana, please find someone on your team to find the regression window for this issue.

Comment 8

[Kelsey Gilbert [:jgilbert]]

This might be an ANGLE issue, possibly related to how closely coupled depth-stencil textures are, and that ANGLE doesn't have a native non-stencil depth type. (IIRC)

Comment 9

[Kelsey Gilbert [:jgilbert]]

Try webgl.prefer-native-gl:true.

Comment 10

[Manuela Muntean [Away]]

> I don't have a handwritten WebGL repro of the second stenciling bug (case II
> in comment 50), but only those two Emscripten applications. When looking for
> a regression window, I would be interested to know if there is any Firefox
> version where either of the two links from comment 50 work (work==render a
> periodically blinking cube instead of a blank screen):
> 
> a.
> https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-
> colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-
> depthformat&TextureFormat_D24_UNORM_S8_UINT&-depthrenderbuffer&-
> stencilformat&TextureFormat_NONE
> 
> b.
> https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-
> colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-
> depthformat&TextureFormat_NONE&-stencilformat&TextureFormat_S8_UINT&-
> stencilrenderbuffer
> 
> I was not been able to test either of the above links with Firefox 23
> stable, since the Firefox autoupdater updated me to 24 stable before I was
> able to do it.

I was able to reproduce the issue with both above links. Here are the results of my investigation:

1) I can see the black screen with the following releases: all from 6.0.2 until 24
2) with releases 4 and 5.0.1, the periodically blinking cube isn't rendered, but I don't see the black screen either, all I see is a an empty rectangle, with black margins

Since webGL is released since Firefox 4, and I think that on Firefox 4 there is also a faulty behavior, I don't think this is a regression.

Note: all 3 links from comment 0 work for me, with both Firefox 23.0.1 and 24.

I'm removing the "qawanted" keyword based on the results exposed in this comment, but please add it back if there is anything else QA can help with. Thanks!

Comment 11

[jjylanki]

Thanks Manuela!

Rechecking with the link I. https://dl.dropboxusercontent.com/u/40949268/Bugs/firefox_enable_stencil_test_bug.html , I get:

Firefox 23.0.1: Works
Firefox 24: Works
Nightly Firefox 27.01a: Does not work

So there does seem to be something that broke at some point.

Comment 12

[u279076]

Jukka, what about Firefox 25 and 26?

Comment 13

[Jukka Jylänki]

Beta 25.0: Works.
Aurora 26.0a2: Does not work.

(I notice I sent email from two different addresses, just to clarify, Jukka Jylänki == [e-mail])

Comment 14

[u279076]

So it would seem this is a regression sometime in the Firefox 26 timeframe. Manuela, can you narrow this down for Jukka?

Comment 15

[Manuela Muntean [Away]]

Rechecking with the link I. https://dl.dropboxusercontent.com/u/40949268/Bugs/firefox_enable_stencil_test_bug.html , I get the following regression range:

Last good nightly: 2013-09-07
First bad nightly: 2013-09-08

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3697f962bb7b&tochan
ge=4ca898d7db5f

Comment 16

[Jukka Jylänki]

Thanks Manuela, extremely helpful!

This change looks very related at least: http://hg.mozilla.org/mozilla-central/rev/356866ae2f68

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Jeff - can you investigate if this is related to bug 883478 landing and if so, what are the options here? How big of an impact might this be to users?

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Clearing regression window wanted since comment 15 appears to fulfill that request.

Comment 19

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

Can somebody back out bug 883478 and verify that is the cause?

Comment 20

[Alice0775 White]

Regression window(m-i) with the link I. https://dl.dropboxusercontent.com/u/40949268/Bugs/firefox_enable_stencil_test_bug.html
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2be3551a5d80
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130906171346
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/356866ae2f68
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130906172546
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2be3551a5d80&tochange=356866ae2f68

Regressed by:
356866ae2f68	Jeff Gilbert — Bug 883478 - Update ANGLE to pull from 13-08-02. r=upstream,bjacob,bas

Comment 21

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #19)
> Can somebody back out bug 883478 and verify that is the cause?

No, don't back this out. This just needs me to look at it, which I haven't had time to do.

Comment 22

[Kelsey Gilbert [:jgilbert]]

The workaround for this should be really easy: Just disable STENCIL_TEST when the render target doesn't have a stencil buffer.

Comment 23

[Kelsey Gilbert [:jgilbert]]

(In reply to [e-mail] [:lsblakk] from comment #17)
> Jeff - can you investigate if this is related to bug 883478 landing and if
> so, what are the options here? How big of an impact might this be to users?

This should have a relatively low impact, but is easy enough to workaround, and the workaround is small enough to uplift.

Comment 24

[Jukka Jylänki]

Agreed, this is not the type of bug that will block progression, and it is not currently blocking us. These kind of discrepancies can probably bite the worst only when porting large codebases that happen to rely on this behavior, and having to hunt down incorrect rendering result without getting any clues as to what is going on.

Comment 25

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

(In reply to Jeff Gilbert [:jgilbert] from comment #21)
> (In reply to Milan Sreckovic [:milan] from comment #19)
> > Can somebody back out bug 883478 and verify that is the cause?
> 
> No, don't back this out. This just needs me to look at it, which I haven't
> had time to do.

I meant "do a local build with this change backed out".  I didn't mean land the backout :)

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: possible workaround, needs testing

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Jeff - did you get a chance to test the workaround so we could uplift it to beta in this next week and make sure it's getting wider usage before shipping?

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Too late now for FF26 - leaving tracking for 27 & 28 to get the workaround landed.

Comment 29

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 821631 [details] [diff] [review]
patch: possible workaround, needs testing

This fixes it locally.

Comment 30

[Kelsey Gilbert [:jgilbert]]

Yep, this fixes it. We'll just uplift where we can.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Also, this means we should add a test for this.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 821631 [details] [diff] [review]
patch: possible workaround, needs testing

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

Is this working around a bug in a particular GL implementation? In ANGLE's libGLESv2? In a driver?

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

What is 'webgl-correctness' vs. the existing 'webgl-conformance' ?

Comment 34

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Benoit Jacob [:bjacob] from comment #32)
> Is this working around a bug in a particular GL implementation? In ANGLE's
> libGLESv2? In a driver?

Answering my own question:

The GL ES 2.0.25 spec, 4.1.4 Stencil Test, Page 96, says: "If there is no stencil buffer, no stencil modification can occur, and it is as if the stencil tests always pass, regardless of any calls to StencilFunc."

So this is definitely an ANGLE bug (which fits well comment 20 tracing it to an ANGLE update).

This already implies that the patch should at least have a if (WorkAroundDriverBugs()).

Since the workaround patch is quite complicated (with this conditional-create-new-scoped-helper-on-the-heap-but-maybe-not business), and since ANGLE is something that we get to fix in our tree, it is very much worth investigating if we could fix it there.

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 821631 [details] [diff] [review]
patch: possible workaround, needs testing

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

::: content/canvas/src/WebGLContextVertices.cpp
@@ +527,5 @@
> +
> +    if (hasStencil)
> +        return nullptr;
> +
> +    return new ScopedGLState(gl, LOCAL_GL_STENCIL_TEST, false);;

Double ;;

Also, I'm scared to have a function new'ing something, returning a plain pointer to it, and not having a name that hints toward that.

A simple way out would be to return a RefPtr but I don't suppose that these things are refcounted.

Does mfbt/Scoped.h have a helper for returning a scoped ptr from a function?

Anyway, the change proposed below (do this setup/cleanup imperatively in Draw*_check/cleanup functions) would remove the need to worry about that.

@@ +544,5 @@
>          return;
>  
> +    // Workaround bug 922875.
> +    ScopedDeletePtr<ScopedGLState> scopedDisableStencil;
> +    scopedDisableStencil = ScopedDisableStencilIfNeeded();

This should be done in a {...} scope to undo it at the exact right time.

Currently, it's applied after the DrawArrays_check so I suppose that it should be unapplied _before_ the Draw_cleanup(), and not after as it's currently done.

In fact, the only thing that should be inside that scope is the fDrawArrays call.

With a reorg of a Draw*_check functions (which IMO is needed anyways) this could then have to be coded only once for all types of draw calls.

I also wonder why this setup/cleanup code is not put in DrawArrays_check / Draw_cleanup like other similar things. Scoped helpers are nice, but here they evidently aren't trivial to use (due to the conditional nature of the workaround).

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

Also, please file an ANGLE bug!

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

Jeff - can you update the patch here with the feedback so we can try to get this into FF28 while on Beta, if low risk enough?

Comment 38

[Kelsey Gilbert [:jgilbert]]

This bug is being ornery. It'll take another day to go over it and check through what's actually going wrong.

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

We're out of time for FF28, wontfixing and marking affected but not tracking for upcoming versions.  When the patch is ready nominate for uplift and we can consider it based on risk/reward and timing with the cycle.

Comment 40

[u279076]

Jukka, is this still an issue?

Comment 41

[Jukka Jylänki]

Rechecked the test cases from commment 0 today on the following test setup, and the issue does still persist. Works correctly in Chrome and IE11.

HASWELL
-------

Custom built desktop PC
Windows 8.1 64-bit
3.0 GHz Intel 8-Core i7-5960X
16GB of RAM
3840x2160 pixels display @ 60 Hz
System DirectX version: DirectX 11.0

NVIDIA GeForce GTX 980, 12GB of RAM, driver version 358.50 (2015-10-07)
	- nvd3dumx.dll, nvwgf2umx.dll:
		- version 10.18.13.5850 (2015-10-03)
		- D3D Version 11.1, WDDM 1.3, WHQL approved
Firefox Nightly 45.0a1 (2015-11-03)

Comment 42

[u279076]

Thanks Jukka, I'm updating the status flags to reflect.

Jeff, it's been over a year so I assume you didn't make any progress. Can I get you to have a look at this again and give a status update?

Comment 43

[Kelsey Gilbert [:jgilbert]]

This sounds like the bug fixed by:
https://bugzilla.mozilla.org/show_bug.cgi?id=1247764