953221 - Rendering to a depth-only FBO does not work.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jukka Jylänki]

1. Visit https://dl.dropboxusercontent.com/u/40949268/emcc/ShadowMap/ShadowMap.html

Observed: Shadow rendering is broken, and the scene is dark without lighting.

Expected: The scene renders with a top-down light, with the spheres casting shadows down on the ground plane and each other. Works in Chrome.

It looks like Firefox (wrongfully) complains that FBOs that only have a depth attachment would not be framebuffer complete. According to GLES2 spec, depth-only FBOs are ok and should be framebuffer complete.

Comment 2

[u480271]

When testing 948002 fixed this, there appears to be more problems.

Comment 3

[Kelsey Gilbert [:jgilbert]]

I believe this is allowed by the spec, though. The spec only guarantees the following:

  The following combinations of framebuffer object attachments, when all of the attachments
  are framebuffer attachment complete, non-zero, and have the same width and height, must
  result in the framebuffer being framebuffer complete:

    COLOR_ATTACHMENT0 = RGBA/UNSIGNED_BYTE texture
    COLOR_ATTACHMENT0 = RGBA/UNSIGNED_BYTE texture + DEPTH_ATTACHMENT = DEPTH_COMPONENT16 renderbuffer
    COLOR_ATTACHMENT0 = RGBA/UNSIGNED_BYTE texture + DEPTH_STENCIL_ATTACHMENT = DEPTH_STENCIL renderbuffer 

We could potentially add workarounds for this, but we'd need to allocate and manage a shadow color buffer of the same size as the depth buffer. I don't think we should do this, as an app is strictly better at knowing the optimal lifetime for this fake color buffer.

Comment 4

[u480271]

Attachment: Minor erroneous complete check fails

Mostly related to lack of color attachment.

Comment 5

[u480271]

Attachment: Disable glDrawBuffer/glReadBuffer when no color attachment

This is mainly for OSX where the OGL version requires this so that the framebuffer complete checks pass.

Comment 6

[u480271]

Jukka,
  Along with the patches from Bug 948002, I have gotten support for this to the point where the shadow map rendering work on page load. Unfortunately pressing any of the keys to change the behaviour results in incorrect rendering. I'm still investigating this.

Do you have the source used to produce the example?

Comment 7

[u480271]

After much more investigation with apitrace I am suspecting the error I'm seeing arises from emscipten/asmjs. At this point, I'm going to get review the current patches and land them. Below is a summary of the error I'm seeing.

=====================================================================================

When the page loads I see the following code to setup a depth-only FBO for shadow map rendering:

// Setup 512x512 shadow map - gl texture name is 5
BindTexture(GL_TEXTURE_2D, 5)
TexImage2D(GL_TEXTURE_2D, 0, GL_DEPTH_COMPONENT, 512, 512, 0, GL_DEPTH_COMPONENT, GL_UNSIGNED_INT, 00000000)
BindTexture(GL_TEXTURE_2D, 5) // [Redundant]

// Setup depth-only FBO
BindFramebuffer(GL_FRAMEBUFFER, 1596E718)
FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, 00000000, 0)
FramebufferTexture2D(GL_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT, GL_TEXTURE_2D, 00000000, 0)
FramebufferTexture2D(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT, GL_TEXTURE_2D, 00000000, 0)
FramebufferTexture2D(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_TEXTURE_2D, 15EBF200, 0)

At this point, FBO attachments look like:
    GL_COLOR_ATTACHMENT0: None
    GL_DEPTH_ATTACHMENT: T GL_TEXTURE_2D 15EBF200 0 - Complete
    GL_STENCIL_ATTACHMENT: None
    GL_DEPTH_STENCIL_ATTACHMENT: None

// Render shadow map
Clear(0x00004500)
// ... DrawArrays/DrawElements

// Render main scene
BindFramebuffer(GL_FRAMEBUFFER, 00000000)
Clear(0x00004500)
ActiveTexture(GL_TEXTURE1)
BindTexture(GL_TEXTURE_2D, 5)
// ... DrawArrays/DrawElements

If I press any of the keys to control the rendering, shadows disappear. (eg. Press up arrow)

// Delete current 512x512, depth texture
ActiveTexture(GL_TEXTURE1)
BindTexture(GL_TEXTURE_2D, 0)
ActiveTexture(GL_TEXTURE0)

// Setup 512x512 shadow map - gl texture name is 8
BindTexture(GL_TEXTURE_2D, 8)
TexImage2D(GL_TEXTURE_2D, 0, GL_DEPTH_COMPONENT, 1024, 1024, 0, GL_DEPTH_COMPONENT, GL_UNSIGNED_INT, 00000000)
BindTexture(GL_TEXTURE_2D, 8) // [Redundant]

// Setup depth-only FBO
BindFramebuffer(GL_FRAMEBUFFER, 1596E718)
FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, 00000000, 0)
FramebufferTexture2D(GL_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT, GL_TEXTURE_2D, 00000000, 0)
FramebufferTexture2D(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT, GL_TEXTURE_2D, 00000000, 0)
FramebufferTexture2D(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_TEXTURE_2D, 0C5C8820, 0)

FBO attachments look like:
    GL_COLOR_ATTACHMENT0: None
    GL_DEPTH_ATTACHMENT: T GL_TEXTURE_2D 0C5C8820 0 - Complete
    GL_STENCIL_ATTACHMENT: None
    GL_DEPTH_STENCIL_ATTACHMENT: None

// Render shadow map
Clear(0x00004500)
// ... DrawArrays/DrawElements

// Render main scene
BindFramebuffer(GL_FRAMEBUFFER, 00000000)
Clear(0x00004500)
// !!! Missing binding of texture 8 to GL_TEXTURE1 !!!
// ... DrawArrays/DrawElements

Comment 8

[u480271]

Asking for review from Benoit, because Jeff is going to be out-of-office tomorrow.

Comment 9

[Jukka Jylänki]

Thanks Dan for going all the way to track this!

Indeed there was a bug in the application code - a GL state caching layer mechanism did not account for deleting a texture to invalidate the cache. I have uploaded a fixed build here: https://dl.dropboxusercontent.com/u/40949268/emcc/bugs/ShadowMap2/ShadowMap.html

Comment 10

[Jukka Jylänki]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> I believe this is allowed by the spec, though. The spec only guarantees the
> following:

In GLES2 2.0.25, depth-only FBOs are considered framebuffer complete (section 4.4.5). In desktop GL 3.0, depth-only FBOs are also safe, but additionally require that one nulls the appropriate buffer draw and read targets with glDrawBuffer(GL_NONE); and glReadBuffer(GL_NONE); and/or glDrawBuffers(...) before checking for completeness. (section 4.4.4). I did not find what desktop GL 2 says, or check the extensions related to this, but I'd hope that they have the same guarantee.

Dan's patch looks good, and that's what I do on desktop GL as well.

Comment 11

[u480271]

(In reply to Jukka Jylänki from comment #9)
> Thanks Dan for going all the way to track this!

The curious thing is the previous version worked when I tried it in Google Chrome.

Comment 12

[u480271]

I've tested the version at https://dl.dropboxusercontent.com/u/40949268/emcc/bugs/ShadowMap2/ShadowMap.html with patches from Bug 948002 & Bug 953221 and shadow maps appear to work correctly.

Comment 13

[Jukka Jylänki]

(In reply to Dan Glastonbury :djg :kamidphish from comment #11)
> (In reply to Jukka Jylänki from comment #9)
> > Thanks Dan for going all the way to track this!
> 
> The curious thing is the previous version worked when I tried it in Google
> Chrome.

Oh yes, running both in Chrome and native worked ok. That was just a lucky accident, and Firefox happened to uncover a bug in the program. The difference that these had is that Chrome and native seem to give location ordinals to shader uniforms in the order they are listed in the shader program, but Firefox seems to use a std::map or similar in implementation, and assign the location ordinals in alphabetical order. By coincidence, the layout in Chrome and native happened to hide the program bug with its state caches.

Comment 14

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

Comment on attachment 8360787 [details] [diff] [review]
Minor erroneous complete check fails

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

Important: this patch adds 1 trailing whitespace character to every new otherwise empty line in WebGLUtils.cpp and in WebGLUtils.h. Please fix!

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +511,5 @@
>          }
>      }
>  
>      // ensure INVALID_FRAMEBUFFER_OPERATION in zero-size case
> +// This is erroneous and already checked by HasIncompleteAttachment() above

In that case, do remove it :-)

::: content/canvas/src/WebGLUtils.cpp
@@ +10,5 @@
> + 
> +const char*
> +GLenumToStr(GLenum e) {
> +    switch (e) {
> +    case LOCAL_GL_TRIANGLES:                    return "GL_TRIANGLES";

I know I'm contradicting what I have often been saying about hiding control flow in macros, but, _this_ is a decent use case for macros, as it is otherwise very tedious to verify the correcness of this function.

#define HANDLE_GL_ENUM(x) case LOCAL_##x: return #x;

 ...

#undef HANDLE_GL_ENUM

Comment 15

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

Comment on attachment 8360788 [details] [diff] [review]
Disable glDrawBuffer/glReadBuffer when no color attachment

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

a question:

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +624,5 @@
>              ColorAttachment(i).FinalizeAttachment(LOCAL_GL_COLOR_ATTACHMENT0 + i);
>      }
>  
> +    //    if (!mContext->IsExtensionEnabled(WebGLContext::WEBGL_draw_buffers)) {
> +        GLenum colorBufferSource = 

Trailing \w

@@ +627,5 @@
> +    //    if (!mContext->IsExtensionEnabled(WebGLContext::WEBGL_draw_buffers)) {
> +        GLenum colorBufferSource = 
> +            ColorAttachment(0).IsDefined() ? LOCAL_GL_COLOR_ATTACHMENT0 : LOCAL_GL_NONE;
> +        mContext->gl->fDrawBuffer(colorBufferSource);
> +        mContext->gl->fReadBuffer(colorBufferSource);

Don't you need, then, to undo that when reverting to the default framebuffer (gl.bindFramebuffer(null)) ?

Comment 16

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #14)

Benoit, I only used WebGLUtils for a debugging aid. Should I keep it in the patch, or remove it?

Comment 17

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #16)
> (In reply to Benoit Jacob [:bjacob] from comment #14)
> 
> Benoit, I only used WebGLUtils for a debugging aid. Should I keep it in the
> patch, or remove it?

I actually would love to have this enum-to-string function stay in.

Comment 18

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

(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> (In reply to Dan Glastonbury :djg :kamidphish from comment #16)
> > (In reply to Benoit Jacob [:bjacob] from comment #14)
> > 
> > Benoit, I only used WebGLUtils for a debugging aid. Should I keep it in the
> > patch, or remove it?
> 
> I actually would love to have this enum-to-string function stay in.

Note that since it's not WebGL specific, it might be better lodged in gfx/gl, but up to you --- I don't have a strong opinion either way.

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #18)
> (In reply to Jeff Gilbert [:jgilbert] from comment #17)
> > (In reply to Dan Glastonbury :djg :kamidphish from comment #16)
> > > (In reply to Benoit Jacob [:bjacob] from comment #14)
> > > 
> > > Benoit, I only used WebGLUtils for a debugging aid. Should I keep it in the
> > > patch, or remove it?
> > 
> > I actually would love to have this enum-to-string function stay in.
> 
> Note that since it's not WebGL specific, it might be better lodged in
> gfx/gl, but up to you --- I don't have a strong opinion either way.

Yep. gfx/gl/GLDebugUtils.h?

Comment 20

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> (In reply to Benoit Jacob [:bjacob] from comment #18)

> Yep. gfx/gl/GLDebugUtils.h?

Done

Comment 21

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #15)
> Don't you need, then, to undo that when reverting to the default framebuffer
> (gl.bindFramebuffer(null)) ?

That's a good question. It currently works in the shadow map test case, which isn't a good reason.

Looking through the code, I see the ClearScreen() is calling glDrawBuffers, which might be enabling the draw buffer.

Comment 22

[u480271]

Attachment: Minor erroneous complete check fails

Removed #if 0 code in response to bjacob review comment. Carry r=bjacob.

Comment 23

[u480271]

Attachment: Disable glDrawBuffer/glReadBuffer when no color attachment

This is mainly for OSX where the OGL version requires this so that the
framebuffer complete checks pass. Added code to enable GL_DRAW_BUFFER and
GL_READ_BUFFER when switching back to default framebuffer.

Comment 24

[u480271]

Attachment: GLenum to C string helper

Split out from webgl into general gfx/gl. Replaced case stmts with macro.

Comment 25

[u480271]

Attachment: GLenum to C string helper

Include GLTypes.h for GLenum definition.

Comment 26

[u480271]

Attachment: GLenum to C string helper

Comment 27

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

Comment on attachment 8363467 [details] [diff] [review]
Disable glDrawBuffer/glReadBuffer when no color attachment

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +612,5 @@
>          if (ColorAttachment(i).IsDefined())
>              ColorAttachment(i).FinalizeAttachment(LOCAL_GL_COLOR_ATTACHMENT0 + i);
>      }
>  
> +    //    if (!mContext->IsExtensionEnabled(WebGLContext::WEBGL_draw_buffers)) {

Don't leave disabled code in a comment, remove it.

@@ +613,5 @@
>              ColorAttachment(i).FinalizeAttachment(LOCAL_GL_COLOR_ATTACHMENT0 + i);
>      }
>  
> +    //    if (!mContext->IsExtensionEnabled(WebGLContext::WEBGL_draw_buffers)) {
> +        GLenum colorBufferSource = 

Trailing \w.

Comment 28

[Jukka Jylänki]

Hey Dan, have you tested the ShadowMap2 sample on OSX after your patch? On my Macbook Pro, I don't get shadows either with or without a dummy color buffer.

Comment 29

[Kelsey Gilbert [:jgilbert]]

(In reply to Jukka Jylänki from comment #28)
> Hey Dan, have you tested the ShadowMap2 sample on OSX after your patch? On
> my Macbook Pro, I don't get shadows either with or without a dummy color
> buffer.

This could very well be an driver implementation choice, but we should check on it.
Perhaps it would be a good idea to throw together a quick mochitest that tests a bunch of this stuff.

Comment 30

[u480271]

(In reply to Jukka Jylänki from comment #28)
> Hey Dan, have you tested the ShadowMap2 sample on OSX after your patch? On
> my Macbook Pro, I don't get shadows either with or without a dummy color
> buffer.

I tested yesterday and I get shadows. This patch builds upon 948002. Did you apply the patches from that bug first?

Comment 31

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #27)
> Comment on attachment 8363467 [details] [diff] [review]
> Disable glDrawBuffer/glReadBuffer when no color attachment
> 
> Review of attachment 8363467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLFramebuffer.cpp
> @@ +612,5 @@
> >          if (ColorAttachment(i).IsDefined())
> >              ColorAttachment(i).FinalizeAttachment(LOCAL_GL_COLOR_ATTACHMENT0 + i);
> >      }
> >  
> > +    //    if (!mContext->IsExtensionEnabled(WebGLContext::WEBGL_draw_buffers)) {
> 
> Don't leave disabled code in a comment, remove it.
> 
> @@ +613,5 @@
> >              ColorAttachment(i).FinalizeAttachment(LOCAL_GL_COLOR_ATTACHMENT0 + i);
> >      }
> >  
> > +    //    if (!mContext->IsExtensionEnabled(WebGLContext::WEBGL_draw_buffers)) {
> > +        GLenum colorBufferSource = 
> 
> Trailing \w.

Umm. Yeah. That was silly of me.

Comment 32

[u480271]

Attachment: Disable glDrawBuffer/glReadBuffer when no color attachment

Remove commented code and trailing whitespace.

Comment 33

[u480271]

Comment on attachment 8364053 [details] [diff] [review]
Disable glDrawBuffer/glReadBuffer when no color attachment

Carry r=bjacob

Comment 34

[u480271]

I was looking into a crash I was getting with Angle and it turns of fDrawBuffer/fReadBuffer is NULL for GLES. D'oh!

In the process of investigating, I was reminded that I read that GL_DRAW_BUFFER/GL_READ_BUFFER state is per *FBO*.

Page 8, http://developer.amd.com/wordpress/media/2012/10/FramebufferObjects.pdf says:

"Note that this is a per-FBO state, so if you’re using separate FBOs for each render-to-texture setup, you can make these calls once at setup and forget it. If you’re using a global FBO and altering attachments you need to restore the draw-buffer and read-buffer states for regular color rendering."

This is why "it just works for me". Following this advice, I'm going to remove the code I added to reset the DRAW_BUFFER/READ_BUFFER state.

Comment 35

[u480271]

Attachment: Split out DRAW_BUFFER/READ_BUFFER state setup.

Cleaned up the code:
 * Added a bunch of error and precondition checking.
 * Removed unneeded "restoration" code when switching to default
   framebuffer.

Comment 36

[u480271]

Attachment: Split out DRAW_BUFFER/READ_BUFFER state setup.

Split out DRAW_BUFFER/READ_BUFFER state setup.

Cleaned up the code:
 * Added a bunch of error and precondition checking.
 * Removed unneeded "restoration" code when switching to default
   framebuffer.

Comment 37

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

Ah, sorry for not catching that. Jeff G would have caught it for sure.

Comment 38

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

Comment on attachment 8364190 [details] [diff] [review]
Split out DRAW_BUFFER/READ_BUFFER state setup.

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +622,5 @@
> +    // Note that this test is not performed if OpenGL 4.2 or ARB_ES2_compatibility is
> +    // available.
> +    if (aGL->IsGLES2() ||
> +        aGL->IsSupported(GLFeature::ES2_compatibility) ||
> +        aGL->IsAtLeast(ContextProfile::OpenGL, 420))

This sounds like it could be a new GLContextFeature. Jeff?

Comment 39

[u480271]

Attachment: Minor erroneous complete check fails

Rebase onto Bug 962392.

Comment 40

[u480271]

Attachment: Disable glDrawBuffer/glReadBuffer when no color attachment

Rebase onto Bug 962392

Comment 41

[u480271]

Attachment: Split out DRAW_BUFFER/READ_BUFFER state setup.

Rebase onto Bug 962392

Comment 42

[u480271]

Comment on attachment 8364840 [details] [diff] [review]
Minor erroneous complete check fails

Carry r=bjacob

Comment 43

[u480271]

Comment on attachment 8364841 [details] [diff] [review]
Disable glDrawBuffer/glReadBuffer when no color attachment

Carry r=bjacob

Comment 44

[u480271]

Comment on attachment 8364843 [details] [diff] [review]
Split out DRAW_BUFFER/READ_BUFFER state setup.

Carry r=bjacob

Comment 45

[u480271]

The patches build upon 948002, so need to be checked in with or after 948002 is landed.

https://tbpl.mozilla.org/?tree=Try&rev=254e945e1d8d

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/722a19e76517
https://hg.mozilla.org/integration/mozilla-inbound/rev/235034a27f3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c788a177d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef07214e11ff

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/722a19e76517
https://hg.mozilla.org/mozilla-central/rev/235034a27f3c
https://hg.mozilla.org/mozilla-central/rev/59c788a177d2
https://hg.mozilla.org/mozilla-central/rev/ef07214e11ff