783914 - WEBGL_depth_texture doesn't work using ANGLE

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Lord [:cwiiis]]

Currently, WEBGL_depth_texture doesn't work on Windows using ANGLE. This does work in Chrome Canary.

Patch incoming.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Add support for WEBGL_depth_texture with ANGLE

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Add support for WEBGL_depth_texture with ANGLE (v2)

Whoops, didn't notice IsANGLE() before.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 653309 [details] [diff] [review]
Add support for WEBGL_depth_texture with ANGLE (v2)

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

We *must* guarantee that webgl alloc'd buffers are cleared to zero, or fully initialized with user data.

::: content/canvas/src/WebGLContextGL.cpp
@@ +5705,5 @@
>      internalformat = InternalFormatForFormatAndType(format, type, gl->IsGLES2());
>  
>      GLenum error = LOCAL_GL_NO_ERROR;
>  
> +    if (byteLength && data) {

Isn't this redundant?

@@ +5739,5 @@
> +            // We need some zero pages, because GL doesn't guarantee the
> +            // contents of a texture allocated with NULL data.
> +            // Hopefully calloc will just mmap zero pages here.
> +            // ANGLE is excluded from this, as it doesn't support uploading to
> +            // depth textures, and will return an error if you try.

We can't skip this step.
Since we can't upload anything, this means we have to guarantee that we zero it by other means. We'll need to construct a framebuffer to hold this texture, and use glClear() to clear it to zeros. Unfortunately, I believe that for depth textures, we'll need to also create a dummy same-sized color renderbuffer or the framebuffer will not be complete.

While we're doing that, we might as well add support for using glClear for color textures, instead of calloc'ing and uploading data.

Comment 4

[conor]

Hey guys, I see there hasn't been any movement on this for a while, is there a different bug number that it has moved to?  We would really like to get depth texture support on windows.

Comment 5

[Kelsey Gilbert [:jgilbert]]

We have most of the parts we need, it's just tricky to get this working on ANGLE:
We can't upload zero'd data ourselves. We will need to bind it to a framebuffer and glClear it to what we want. (We want to switch to doing this for all null teximage2d calls anyways, and this should be relatively straight-forward)

There may be an issue with how ANGLE depth textures are actually depth-stencil, but ANGLE may successfully hide this from us.

I'll keep this on my radar and put a patch together when I have time.

Comment 6

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

Jeff- Alternatively, if people care about it more than we do in the immediate future, this bug might be a good candidate for crowdsourcing...? I mean, clearing arbitrary textures/renderbuffers by using a dummy framebuffer and program is tedious coding indeed, but there is a whole lots of people out there who know OpenGL ;-)

Comment 7

[conor]

Hmm, it seems depth texture support has gone away on Mac now as well, was that done intentionally so that Windows and Mac functionality would match?

Comment 8

[Kelsey Gilbert [:jgilbert]]

(In reply to conor from comment #7)
> Hmm, it seems depth texture support has gone away on Mac now as well, was
> that done intentionally so that Windows and Mac functionality would match?

This would certainly not be the intention. I don't think we have deliberately removed such functionality unless we found some concrete issue with it or its implementation.

Comment 9

[conor]

I've only confirmed that it stopped working on MacBook Pro Retina, tested on two of them. Unfortunately those are the only Macs we have at the office.

Comment 10

[Chris Lord [:cwiiis]]

I probably shouldn't be assigned to this, someone from gfx team (preferably with more Windows dev experience) should take a look.

Comment 11

[conor]

I hate to be this guy, but I am going to poke at this issue again:

Is this ever going to be fixed? Our app (www.cloudparty.com) relies heavily on the depth texture extension for all of its graphics effects, and currently Firefox doesn't support it on Windows or on most Macs. Consequently we have been recommending that all of our users use Chrome instead because it look so much better. I would absolutely love for Firefox to reach parity graphics-wise, and there doesn't seem to be much reason it shouldn't.

Comment 12

[Chris Lord [:cwiiis]]

cc'ing :milan as this is a gfx issue.

Also a chrome-parity issue.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Naively, all we need to do, after we allocate an uninitialized depth texture `foo` is:

{
GLuint fb = 0;
gl->fGenFramebuffers(1, &fb);
{
ScopedBindFramebuffer autoBindFB(gl, fb);
gl->fFramebufferTexture2D(FRAMEBUFFER, DEPTH_ATTACHMENT, TEXTURE_2D, foo, 0);
MOZ_ASSERT(gl->fCheckFramebufferStatus(FRAMEBUFFER) == FRAMEBUFFER_COMPLETE);

GLfloat prevClearDepth = 0;
gl->fGetFloatv(GL_CLEAR_DEPTH_VALUE, &prevClearDepth);

gl->fClearDepth(0.0);
gl->fClear(GL_DEPTH_BUFFER_BIT);

gl->fClearDepth(prevClearDepth);
}
gl->fDeleteFramebuffers(1, &fb);
}

(symbols are basically pseudocode)

The only caveat might be that some systems may not treat an FB with just a depth as complete. (they'll hit the assert, or possible generate an INVALID_OPERATION at the Clear())

I say 'naively', as we should have most of these values shadowed at the webgl level, and so wouldn't need to query and save them for restoring. (Just restore from our already-stored values)

Comment 14

[Kelsey Gilbert [:jgilbert]]

Oh, and we might need to assure that depth mask and depth test enable state are allowing us to write to the depth texture.

Comment 15

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

Bas, any thoughts?

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #15)
> Bas, any thoughts?

I can give my thoughts.. But I'm not sure what the question is?

Jeff Gilbert is right as far as the problems with the OpenGL case for this is. Clearing a Depth buffer by its own is a little tricky. In D3D9 this is easily done by doing a clear with a D3DCLEAR_ZBUFFER flag and doesn't require a specific render target being set. But I doubt ANGLE will expose this to us, and in any case I'm not sure if it's the right way to do things.

I'm pretty sure I'm not providing any useful new information here though :-).

Comment 17

[Masatoshi Kimura [:emk]]

How does Chromium clear the depth buffer?

Comment 18

[Kelsey Gilbert [:jgilbert]]

(In reply to Masatoshi Kimura [:emk] from comment #17)
> How does Chromium clear the depth buffer?

Likely by attaching the extra buffers (color? stencil?) needed to make the framebuffer complete. (thus clearable)

Comment 19

[conor]

Chromium waits to clear any uninitialized attachments until the framebuffer is complete. Then they read the current color/stencil/depth masks and scissor test state, set them to writable, clear, then restore the masks and scissor.

Comment 20

[conor]

So this has been over a year now, are we ever going to see this implemented?

Comment 21

[Jukka Jylänki]

Desktop Chrome implements the WEBKIT_WEBGL_depth_texture extension: http://www.khronos.org/registry/webgl/extensions/WEBGL_depth_texture/ 

When this bug is resolved, will depth textures then be available on desktop Firefox via a 'MOZ_WEBGL_depth_texture' extension?

Comment 22

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

Taking.

Comment 23

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

(In reply to conor from comment #19)
> Chromium waits to clear any uninitialized attachments until the framebuffer
> is complete. Then they read the current color/stencil/depth masks and
> scissor test state, set them to writable, clear, then restore the masks and
> scissor.

That is what every browser has to do to implement WebGL sanely. Properly initializing renderbuffers is necessary to guarantee basic security, and GL framebuffer completion rules are not tight enough to be able to rely on logic that 'should' get it right, so the only sane way to implement WebGL on top of OpenGL has always been to do deferred initialization of renderbuffers. In our code, this is done here:

http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLFramebuffer.cpp#l386

Comment 24

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

In other words, the only thing that is new here is that we need to do this for a _texture_ as opposed to a _renderbuffer_, but the exitsting code doing it for renderbuffers will happily also do it for textures.

Comment 25

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

There is something that doesn't seem to be properly specified AFAICS in the current WEBGL_depth_texture spec, took it to the mailing list, meanwhile I'm proceeding with what I believe this should be specced as (uninitialized depth texture should IMO be treated as an incomplete texture, in the usual way, faked as rgba(0,0,0,0))

https://www.khronos.org/webgl/public-mailing-list/archives/1309/msg00046.html

Comment 26

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

Alright, interesting conclusions from that thread:
 - this is actually well-specified in 4.1 Resource Restrictions
 - ANGLE unconditionally supports depth-only FBOs so Jeff's proposed code in comment 13 is going to work, no need for a color attachment, and no need to worry about the intricacies of setting state right for the clearing to happen, as we already have that problem solved in GLContext::ClearSafely().

Comment 27

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

Attachment: ♫ Quand on n'a que ANGLE_depth_texture♫

(To be sung on the tune of:
  http://en.wikipedia.org/wiki/Quand_on_n%27a_que_l%27amour
)

Needs some testing on windows... I tried testing on linux by faking the extension situation there, but Mesa 8.0 won't accept a depth-only framebuffer.

Comment 28

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

https://tbpl.mozilla.org/?tree=Try&rev=2aa402eeb2d5

Comment 29

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

Attachment: ♫ Quand on n'a que ANGLE_depth_texture♫

Was leaking the framebuffer object, and not checking that its creation had succeeded.

Comment 30

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 811346 [details] [diff] [review]
♫ Quand on n'a que ANGLE_depth_texture♫

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3658,5 @@
>                                               "target must be TEXTURE_2D, "
>                                               "data must be nullptr, "
> +                                             "level must be zero"); // Haiku by unknown Zen master
> +        } else {
> +            return ErrorInvalidEnum("texImage2D: attempt to upload a depth texture "

s/upload/create/, since uploading depth textures is never allowed.

@@ +3742,5 @@
> +            // ANGLE_depth_texture does not allow uploading image data with texImage2D,
> +            // so our only way to clear this texture is to attach it to a FBO and clear that.
> +            // Fortunately, we should only get here on ANGLE, which supports depth-only FBOs.
> +            GLuint fb = 0;
> +            gl->fGenFramebuffers(1, &fb);

You never delete this.

@@ +3745,5 @@
> +            GLuint fb = 0;
> +            gl->fGenFramebuffers(1, &fb);
> +            ScopedBindFramebuffer autoBindFB(gl, fb);
> +            gl->fFramebufferTexture2D(LOCAL_GL_FRAMEBUFFER,
> +                                      LOCAL_GL_DEPTH_ATTACHMENT,

If this is a depth_stencil texture, we need to clear both aspects.

Comment 31

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

Attachment: ♫ Quand on n'a que ANGLE_depth_texture♫

DEPTH_STENCIL also need to be attached as STENCIL_ATTACHMENT for that part to be cleared.

Comment 32

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 811350 [details] [diff] [review]
♫ Quand on n'a que ANGLE_depth_texture♫

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3658,5 @@
>                                               "target must be TEXTURE_2D, "
>                                               "data must be nullptr, "
> +                                             "level must be zero"); // Haiku by unknown Zen master
> +        } else {
> +            return ErrorInvalidEnum("texImage2D: attempt to upload a depth texture "

s/upload/create/, since we can never upload data to these.

@@ +3744,5 @@
> +            // Fortunately, we should only get here on ANGLE, which supports depth-only FBOs.
> +            GLuint fb = 0;
> +            gl->fGenFramebuffers(1, &fb);
> +            if (!fb) {
> +                return ErrorOutOfMemory("texImage2D: sorry, failed to create a temporary framebuffer.");

We prooobably don't have to check this, but sure. I'm fairly sure there are other places where we do not check this.

@@ +3750,5 @@
> +
> +            {
> +                ScopedBindFramebuffer autoBindFB(gl, fb);
> +                gl->fFramebufferTexture2D(LOCAL_GL_FRAMEBUFFER,
> +                                          LOCAL_GL_DEPTH_ATTACHMENT,

Depth_stencil textures need both aspects cleared.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 811360 [details] [diff] [review]
♫ Quand on n'a que ANGLE_depth_texture♫

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3658,5 @@
>                                               "target must be TEXTURE_2D, "
>                                               "data must be nullptr, "
> +                                             "level must be zero"); // Haiku by unknown Zen master
> +        } else {
> +            return ErrorInvalidEnum("texImage2D: attempt to upload a depth texture "

s/upload/create/, since we can never upload to depth textures.

@@ +3747,5 @@
> +            if (!fb) {
> +                return ErrorOutOfMemory("texImage2D: sorry, failed to create a temporary framebuffer.");
> +            }
> +
> +            {

This scope is not strictly necessary, but fine.

Comment 34

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

Attachment: ♫ Quand on n'a que ANGLE_depth_texture♫

There was another catch! in the error case, we were returning without deleting the fb. carrying r+.

Comment 35

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

https://tbpl.mozilla.org/?tree=Try&rev=9230f707dfb4

Comment 36

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

Attachment: ♫ Quand on n'a que ANGLE_depth_texture♫

The previous patch didn't work, and the reason was that I was not doing any texImage2D on the depth texture, not even the one with null data to allocate it!

So with that, faking the ANGLE situation on my linux/mesa system, things work for me (depth-only FBOs are supported here, maybe support for them isn't so rare after all).

Re-requesting review because after trying many things, my patch ended up being significantly different, and hopefully cleaner, than the previous one.

https://tbpl.mozilla.org/?tree=Try&rev=3b2b0079e2eb

Comment 37

[Jukka Jylänki]

Awesome job on working through this Benoit!

I wrote a feature test for different depth and depth+stencil formats, perhaps it can be of some use. See here: https://dl.dropboxusercontent.com/u/40949268/emcc/depth_stencil_test/Cube_d.html

If you click on a link on the right that says it has a depth buffer, you should see the hidden edges behind the cube rendered with dashed lines. If depth buffering is not working, the all edges will look solid (here is a bad rendering that would have depth buffering disabled: https://dl.dropboxusercontent.com/u/40949268/emcc/depth_stencil_test/Cube_d.html?-depthformat&TextureFormat_UNKNOWN )

In the two links where stencil buffer is enabled, the cube should be blinking at one second intervals. The links were checked to work on Chrome, so this can be used to cross-check.

Comment 38

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

I get no rendering at all (stays blank). For example, with "TextureFormat_D24_UNORM_S8_UINT as depth+stencil" I get these errors in the Web Console:

10:03:37.570 GET https://dl.dropboxusercontent.com/u/40949268/emcc/depth_stencil_test/Cube_d.html [HTTP/1.1 200 OK 2546ms]
10:03:38.382 GET https://dl.dropboxusercontent.com/u/40949268/emcc/depth_stencil_test/jquery-1.8.3.min.js [HTTP/1.1 304 NOT MODIFIED 661ms]
10:03:41.592 SyntaxError: Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead Cube_d.html:111857
10:03:42.004 "run() called, but dependencies remain, so not running" Cube_d.html:67
10:03:42.196 GET https://dl.dropboxusercontent.com/u/40949268/emcc/depth_stencil_test/Cube_d.data [HTTP/1.1 304 NOT MODIFIED 401ms]
10:03:42.063 Unknown property 'box-sizing'.  Declaration dropped. Cube_d.html
10:03:42.886 "preload time: 882 ms" Cube_d.html:67
10:03:45.434 "Error: GlGetAttribLocation reports vertex attribute 'color' to be present at index 4, but GlGetActiveAttrib locates the same attribute to index 0! This GL implementation is broken!" Cube_d.html:67
10:03:45.441 "Error: GlGetAttribLocation reports vertex attribute 'pos' to be present at index 0, but GlGetActiveAttrib locates the same attribute to index 1! This GL implementation is broken!" Cube_d.html:67
10:03:45.527 "Error: Shader program expects a vertex attribute input at location 5: name: 'uv', type: (GLenum 0x1406), size: 1, but it was not enabled via a call to GlEnableVertexAttribArray(5) prior to rendering!" Cube_d.html:67
10:03:53.720 TypeError: Argument 2 of EventTarget.removeEventListener is not an object. Cube_d.html:1804

And the emscripten console says:

Emscripten main().
Calling application main().
glGenTextures+glTexImage2D for texture format TextureFormat_D24_UNORM_S8_UINT as render target: format: (GLenum 0x84FA), internalformat: (GLenum 0x84FA), type: (GLenum 0x84FA), width: 640, height: 480
No separate stencil buffer in use.
glGenTextures+glTexImage2D for texture format TextureFormat_R8G8B8A8_UNORM as render target: format: (GLenum 0x1401), internalformat: (GLenum 0x1401), type: (GLenum 0x1401), width: 640, height: 480
glGenTextures+glTexImage2D for texture format TextureFormat_R8G8B8A8_UNORM as render target: format: (GLenum 0x1401), internalformat: (GLenum 0x1401), type: (GLenum 0x1401), width: 640, height: 480
Starting main loop with refresh rate of 0.
Warning: VertexBuffer::BindToMaterial: Cannot find attribute location for vertex attribute "uv" in material that uses VS 'bias_shader.hlsl' and PS 'bias_shader.hlsl', when binding vertex buffer '(null)'!
GLES2 object 12:
GlIsShader: true
GL_SHADER_TYPE: GL_VERTEX_SHADER
GL_DELETE_STATUS: 0
GL_COMPILE_STATUS: 1
GL_INFO_LOG_LENGTH: 1
GLES2 object 14:
GL_DELETE_STATUS: 0
GL_LINK_STATUS: 1
GL_VALIDATE_STATUS: 0
GL_INFO_LOG_LENGTH: 1
GL_ATTACHED_SHADERS: 2
GL_ACTIVE_ATTRIBUTES: 2
GL_ACTIVE_UNIFORMS: 1
Active vertex attribute at index 0: color, type: GL_FLOAT_VEC3, size: 1.
Active vertex attribute at index 1: pos, type: GL_FLOAT_VEC4, size: 1.
Active uniform constant at index 0: worldViewProj, type: GL_FLOAT_MAT4, size: 1.
GlValidateProgram returned:

Uniform constants in shader program:
Uniform worldViewProj: size 1. type: GL_FLOAT_MAT4(35676). Location: 16
Compiled shader program has 2 active vertex attributes.
exit(0) called

Comment 39

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

Nevermind, please ignore comment 38 !

I was running a build with other patches applied.

Now with only the present patch applied, it works very well. Thanks for the testcase!

If you see any useful addition to make to the Khronos conformance test,

https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-depth-texture.html

then please file bugs / pull requests against https://github.com/KhronosGroup/WebGL

Comment 40

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 811565 [details] [diff] [review]
♫ Quand on n'a que ANGLE_depth_texture♫

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

Do you need this tested on windows again?

Comment 41

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

It doesn't hurt to test it, but assuming the claim that ANGLE supports depth-only FBOs is correct, it can hardly fail. Since it doesn't work atm on windows anyway, it doesn't hurt to land this and test post-landing, which is easier.

Comment 42

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7fb74cbc46

Comment 43

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #41)
> It doesn't hurt to test it, but assuming the claim that ANGLE supports
> depth-only FBOs is correct, it can hardly fail. Since it doesn't work atm on
> windows anyway, it doesn't hurt to land this and test post-landing, which is
> easier.

Huh? This enables it though, which is a claim that it *does* work. If it doesn't, people will mistakenly try to use it, and it will break. It doesn't look like we have any tests for this, either.

Comment 44

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

We do have some test: the conformance test, and Jukka's testcase above. I patched m-c locally to pretend it has ANGLE_depth_texture and not the regular depth_texture extension, and tested these two things. The main way that my testing could differ from a real Windows/ANGLE setup is if it were not true after all that ANGLE supports depth-only FBOs.

Comment 45

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fb7fb74cbc46

Comment 46

[Jukka Jylänki]

I updated to latest trunk today and rebuilt Firefox debug build. Running the conformance test page linked to above now gives me this: http://pastebin.com/H5pBb0AS

However, when I try to run the Cube_d.html test sample application I wrote above, I get a black screen on all test cases. I see the depth_texture being advertised in the extension list, should this now be working or is there some work pending until the support is in mozilla-central?

Comment 47

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

This should be working (as of the mozilla-central revision said in comment 45). If it's not, it's a bug. Let me boot to windows and see what is going on...

Comment 48

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

Indeed, this fails on https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-depth-texture.html

Either I can whip up a patch that throws in a color attachment... or I can finish my patch queue that sidesteps the whole problem by using a fake texture for uninitialized textures. Maybe do both.

Comment 49

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

Attachment: Throw in a color attachment

Jukka, does this fix it for you?

Results will be displayed on TBPL as they come in:
https://tbpl.mozilla.org/?tree=Try&rev=ae702d7f2d75

Once completed, builds and logs will be available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ae702d7f2d75

Comment 50

[Jukka Jylänki]

I created a more fine-grained testing bed for different texture formats to troubleshoot why the original version is getting black. The amount and combination of options in the new page is a bit confusing (even for me!) since not all combinations are possible, so I tried to document the invalid combinations carefully. (sorry for the UI being a bit odd, but try to bear with me!)

Looks like the depth textures do work properly. The new implementation that was written for depth_texture says these cases should work:

1. D16_UNORM depth render target with GL texImage2D: https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-disablestenciltest&-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_D16_UNORM&-stencilformat&TextureFormat_NONE&

2. D32_UNORM depth render target with GL texImage2D:
https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-disablestenciltest&-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_D32_UNORM&-stencilformat&TextureFormat_NONE&

3. D24_UNORM_S8_UINT depth render target with GL texImage2D:
https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-disablestenciltest&-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_D24_UNORM_S8_UINT&-stencilformat&TextureFormat_NONE&

In addition, the base GLES2 core spec says that this case should work (as it did before this patch:

A. D16_UNORM depth render target with GL Renderbuffer:
https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-disablestenciltest&-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_D16_UNORM&-depthrenderbuffer&-stencilformat&TextureFormat_NONE&

In addition, the WebGL 1.0 core spec section 6.6 says that this case should work (which it didn't before this patch, but now does):

B. D24_UNORM_S8_UINT depth+stencil render target with GL Renderbuffer:
https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-disablestenciltest&-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_D24_UNORM_S8_UINT&-depthrenderbuffer&-stencilformat&TextureFormat_NONE&

However there are two separate bugs that are most likely unrelated to the depth_texture extension I think that I am seeing, and that caused the black screen in the above test. They are both related to stenciling. To understand the checkbox 'Enable stencil comparison' on the test page, here is a snippet of the code that is run each frame:

bind offscreen FBO with stencil buffer (if present), or with STENCIL_ATTACHMENT = 0 if no stencil buffer is present; 
glClearStencil((timeInSeconds%2)?4:5); // Alternate stencil buffer reference value each second.
glClear(GL_STENCIL_BUFFER_BIT); // Clear stencil to our reference value.
if ('Enable stencil comparison' checkbox is checked)
{
   glEnable(GL_STENCIL_TEST); // Enable stencil test.
   glStencilFunc(GL_EQUAL, 4, 0xFF); // Pass pixels that have stencil value 4.
}
else
   glDisable(GL_STENCIL_TEST);

This is the first case that does not work:

I. Enable stencil test when there is no stencil attachment in an offscreen FBO:
https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-colorformat&TextureFormat_R8G8B8A8_UNORM&-colorrenderbuffer&-depthformat&TextureFormat_D16_UNORM&-depthrenderbuffer&-stencilformat&TextureFormat_NONE&

GLES2 2.0.25 http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf PDF page 96 section 4.1.4 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."

The code should have drawn the cube (and it should not be blinking, since the stencil test should always pass). Instead, the result is that the stencil test always fails.

Here is another set of flags that suffer from that problem: https://dl.dropboxusercontent.com/u/40949268/emcc/rendertargets/Cube_d.html?-colorformat&TextureFormat_NONE&-depthformat&TextureFormat_NONE&-stencilformat&TextureFormat_NONE&
In that case, no offscreen FBO is created, but rendering is performed to canvas, i.e. FBO 0, but the canvas default backbuffer was created without a stencil buffer.

This is the second case that does not work:

II. Clear an offscreen stencil attachment to a custom integer value with glClear:

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&

and another set of options that showcase the problem:

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

In these two cases, I am guessing that even though a stencil buffer is available, glClear()ing that stencil buffer to a custom value of 4 or 5 fails, and the stencil buffer always retains its initial value of zero, and the stencil test fails since 0 != 4. However, this is just an educated guess, I have no proof that it's the glClear(GL_STENCIL_BUFFER_BIT) that would be failing in this case.

I'm sorry that I don't have a 'native' WebGL testcase, but only this Emscripten application. Can you reproduce what I'm seeing by visiting the above links?

Comment 51

[Jukka Jylänki]

Thanks for the try build Benoit!

I had a go with that, and the result is the same as with the nightly I built and reported above: Cases 1,2,3,A and B work, but cases I, and II don't.

I'm ok with keeping this bug item marked as fixed, looks like it does work. Should I open new bugs for the above two cases I and II?

Comment 52

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

Jukka, many thanks for the testing and the narrowed-down testcases. Can you please report on the results of the conformance test,

   https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-depth-texture.html

If it's not green for you, then this bug needs to be reopened.

Comment 53

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

It's not convenient for me to reboot to windows often, so I haven't tried your testcases there yet. I will do it ASAP but meanwhile, results from the conformance test should be most effective at narrowing down remaining issues.

Comment 54

[Jukka Jylänki]

Yes, the conformance test does pass, the result of the run is reported in comment 46. Also the new try build does pass the conformance test.

Having a dummy color texture is not needed for the FBO to be framebuffer complete at least in the following specs:

In GLES2 context, a FBO is framebuffer complete even if GL_COLOR_ATTACHMENT0 is null (section 4.4.5, Framebuffer Completeness)

In GL3.0 context, a FBO is framebuffer complete even if all GL_COLOR_ATTACHMENTi are null, if glDrawBuffers(0, 0); has been called. (section 4.4.4, Framebuffer Completeness)

Comment 55

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

Yes, but we've seen drivers where depth-only FBOs weren't actually supported... so just to confirm, did the color attachment (my new tryserver build in comment 49, as opposed to what's currently on mozilla-central) make a difference?

Comment 56

[Jukka Jylänki]

Right, there could of course be some flaky configurations out there.

The try build did not make any difference compared to the nightly build I did after the patch landed to central. In both builds the conformance test passes, and in both builds, the cases 1,2,3, A and B above work, and I and II don't.

Comment 57

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

Comment on attachment 812648 [details] [diff] [review]
Throw in a color attachment

Thanks Jukka, that clarifies it --- I was confused. Cancelling this review, then.

So at this point I don't know at all why the remaining testcases are not working on Windows, and would have to do some real debugging there.

But I have something better: we don't actually need to initialize depth textures at all, if we can instead remember that they are uninitialized and use a dummy black 1x1 texture instead when needed, as we already do for the case of incomplete textures. I have a patch set almost ready for review doing that, will upload it very soon --- it will completely sidestep this problem and make ANGLE no different from any other platform as far as WebGL depth textures are concerned.

Comment 58

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

(In reply to Benoit Jacob [:bjacob] from comment #57)
> But I have something better: we don't actually need to initialize depth
> textures at all, if we can instead remember that they are uninitialized and
> use a dummy black 1x1 texture instead when needed, as we already do for the
> case of incomplete textures. I have a patch set almost ready for review
> doing that, will upload it very soon --- it will completely sidestep this
> problem and make ANGLE no different from any other platform as far as WebGL
> depth textures are concerned.

Filed bug 922810 about this, have a try build compiling at the moment, see bug 922810 comment 9. I'd be very interested in hearing if it fixes the issues you're experiencing.

Comment 59

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

Er, use the newer try builds from bug 922810 comment 11 instead (there was a bug in the previous one).

Comment 60

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

I tried your testcases (comment 50) with a linux debug build with bug 922810 patches applied. The testcases that you reported as working, work there. The testcases "I" that you reported as not working, work in that build too. The testcases "II" that you reported as not working, do NOT seem to work for me: the whole cube intermittently disappears, and in the last link, the edges aren't dashed. But, I am using Mesa 8.0 intel drivers, so that could be a driver bug on my side. I have a build of Mesa 9.2 llvmpipe too, but that seems to have very poor support for depth textures.

Comment 61

[Jukka Jylänki]

Thanks!

My guess about the cause of the problem with the testcase I above was probably correct, since I was able to handwrite a repro showing the problem of the testcase I. I filed a separate bug about that, see https://bugzilla.mozilla.org/show_bug.cgi?id=922875 , but by comment 60 looks like you already may have a fix, and that will be a quickly-terminated bug report. (hopefully! :)

In the second link of testcase II, the edges should look all solid, since that link uses an offscreen FBO that does not have a depth buffer (so the draw code will draw the solid lines everywhere), but the cube should blink periodically.

In the first link of testcase II, the back-edges should look dashed, and the cube should also blink periodically, since there is both a depth and a stencil buffer present in the offscreen FBO.

I do not yet know what causes the failure in testcase II, I'll try to generate a handwritten repro for that as well, and have a go at your try build.

Comment 62

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

(In reply to Jukka Jylänki from comment #61)
> Thanks!
> 
> My guess about the cause of the problem with the testcase I above was
> probably correct, since I was able to handwrite a repro showing the problem
> of the testcase I. I filed a separate bug about that, see
> https://bugzilla.mozilla.org/show_bug.cgi?id=922875 , but by comment 60
> looks like you already may have a fix, and that will be a quickly-terminated
> bug report. (hopefully! :)
> 
> In the second link of testcase II, the edges should look all solid, since
> that link uses an offscreen FBO that does not have a depth buffer (so the
> draw code will draw the solid lines everywhere), but the cube should blink
> periodically.
> 
> In the first link of testcase II, the back-edges should look dashed, and the
> cube should also blink periodically, since there is both a depth and a
> stencil buffer present in the offscreen FBO.

Ah OK, I see. That is exactly what I am seeing with my patches (try build in bug 922810 comment 11) on linux/mesa, and since these patches remove the big ANGLE_depth_texture-specific code path, I am optimistic that they should fix the problem also on Windows/ANGLE. I'll be very interested to hear if they actually do!