948002 - WebGL framebuffer completeness check fails with depth texture (regression)

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Andre Weissflog]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

Summary:

My webgl demos fail a framebuffer completeness check in FF Nightly on OSX only since switching to depth textures. Nightly on Windows works, stable Firefox (26.0) on OSX and Windows works as well, must be a regression.

Might be related to (https://bugzilla.mozilla.org/show_bug.cgi?id=927981)?

To  reproduce:

- start current Nightly build on OSX (tested with 28.0a1 (2013-12-09), OSX 10.9, Intel HD 4000)
- navigate to http://www.flohofwoe.net/demos/dragonsfftest_asmjs.html
- note how the demo fails and the text console says that the frame buffer completeness check has failed
- Firefox 26.0 (beta channel) on the same machine works!
- Firefox Nightly on Windows7 works as well
- Chrome / Chrome Canary works

I notices this when I rewrite the GL depth buffer initialization code (now using a 24/8 depth/stencil texture instead of a 16-bit depth render buffer), here's the new (C++) code which initializes and attaches the depth render texture (this is an emscripten demo), the code which initializes the color-attachment is omitted

...
// create the depth texture
glGenTextures(1, &glDepthRenderTexture);
glBindTexture(GL_TEXTURE_2D, glDepthRenderTexture);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);    // no mipmap filtering!
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
n_printf("GLTextureFactory: creating 24.8 depth stencil texture\n");
glTexImage2D(GL_TEXTURE_2D, 0, GL_DEPTH_STENCIL, width, height, 0, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8, 0);
GL_CHECK_ERROR();

// attach depth texture to frame buffer
n_assert(0 != glDepthRenderTexture);
glFramebufferTexture2D(GL_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT, GL_TEXTURE_2D, glDepthRenderTexture, 0);
GL_CHECK_ERROR();

// check frame buffer for completeness
if (n3glCheckFramebufferStatus(GL_FRAMEBUFFER) != GL_FRAMEBUFFER_COMPLETE)
{
    n_error("GLTextureFactory::CreateRenderTarget(): frame buffer completeness check failed!\n");
}

Comment 1

[Alon Zakai (:azakai)]

I see this problem on Linux as well, so not OS X specific.

Comment 2

[Paul Silaghi, QA [:pauly]]

Last good revision: b4143e04bea1 (2013-11-04)
First bad revision: 770de5942471 (2013-11-05)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b4143e04bea1&tochange=770de5942471

Comment 3

[Paul Silaghi, QA [:pauly]]

Last good revision: 0777f32920b9
First bad revision: 4f259397bfb9
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0777f32920b9&tochange=4f259397bfb9

Comment 4

[Jukka Jylänki]

I just now noticed a bug with depth-only FBOs containing a depth texture, see https://bugzilla.mozilla.org/show_bug.cgi?id=953221 , that is similar.

Comment 5

[u480271]

I have a fix for this. In the process, I discovered an error in the mochi tests that showed the depth texture extension conformance test as passing when it had a javascript error. Fixing that error shows that my changes to support sRGB broke the completeness logic. (By the change in WebGLTexture::ImageInfo to store internal format instead of format, texture format checking in framebuffer completeness broke)

I worked with :bjacob before the vacation to clean up the logic. Now that I'm back from vacation, I'm working to clean up patches for landing.

Comment 6

[u480271]

Attachment: Add functions getExtensionWithKnownPrefixes andgetSupportedExtensionWithKnownPrefixes to fix tests that fail to parse.

Add the missing JS helper functions that caused webgl-depth-texture.html to
error.

Comment 7

[u480271]

Attachment: Fix WebGL framebuffer completeness checks.

When changing WebGLTexture::ImageInfo to consistently store GL internal format
instead of format, code that checked for depth textures broke because general
depth component format type was being checked instead of the sized formats.

With :bjacob, we audited the locations of the checks and updated the code to
accept the internal formats by utilizing helper functions that check the
GLenum.

Comment 8

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

Comment on attachment 8356343 [details] [diff] [review]
Add functions getExtensionWithKnownPrefixes andgetSupportedExtensionWithKnownPrefixes to fix tests that fail to parse.

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

R- for the seemingly unrelated change in webgl-test-utils.js. The rest is non-blocking, though if you choose to ignore the comment on WebGLTexture.cpp (about getting the format directly from the internalformat rather than from our own mozilla WebGL enum), I would like to hear the rationale for that.

::: content/canvas/src/WebGLContext.h
@@ +439,5 @@
>      }
>      void TexParameteri(GLenum target, GLenum pname, GLint param) {
>          TexParameter_base(target, pname, &param, nullptr);
>      }
> +

There are a lot of whitespace changes in this file, which isn't otherwise touched. While the whitespace changes are good, we generally refrain from making such whitespace changes outside of code that we're actually touching, to avoid making hg annotate less practical. It's OK for this time though --- just warning you in case some day a reviewer gets serious about that.

::: content/canvas/src/WebGLContextGL.cpp
@@ +1201,5 @@
>      {
>          return ErrorInvalidOperation("generateMipmap: Level zero of texture is not defined.");
>      }
> +
> +    const WebGLTexture::ImageInfo& imageInfo = tex->ImageInfoAt(imageTarget, 0);

Move this line three lines down, right where it's used.

::: content/canvas/src/WebGLTexelConversions.cpp
@@ +10,5 @@
>  
> +namespace WebGLTexelConversions {
> +
> +WebGLTexelFormat
> +GetWebGLTexelFormat(GLenum format, GLenum type)

Important: the 'format' parameter here should be named 'internalformat' for clarity, since the whole point is to move away from making decisions like this based on the non-internal format.

::: content/canvas/src/WebGLTexture.cpp
@@ +429,5 @@
>                          mContext->mPixelStoreUnpackAlignment);
>      MOZ_ASSERT(checked_byteLength.isValid()); // should have been checked earlier
>      void *zeros = calloc(1, checked_byteLength.value());
>  
> +    GLenum format = WebGLTexelConversions::GLFormatForTexelFormat(texelformat);

I find it strange that we choose to determine the format from the mozilla WebGLTexelFormat, instead of determining it from the OpenGL internalformat, which we have here (imageInfo.mInternalFormat). This seems like an unnecessary round-trip outside of and back into the realm of GL enums:  GL internalformat --> mozilla WebGLTexelFormat --> GL format , no?  (I could be missing something).

::: content/canvas/test/webgl/conformance/resources/webgl-test-utils.js
@@ +642,1 @@
>                  getGLErrorAsString(gl, glError) + " : " + opt_msg);

I don't understand what the webgl-test-utils.js change is doing in this patch. It seems unrelated?

Comment 9

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

Comment on attachment 8356344 [details] [diff] [review]
Fix WebGL framebuffer completeness checks.

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

Something wrong seems to have happened: this patch only adds a 'using namespace' statement.

Comment 10

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #9)
> Comment on attachment 8356344 [details] [diff] [review]
> Fix WebGL framebuffer completeness checks.
> 
> Review of attachment 8356344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Something wrong seems to have happened: this patch only adds a 'using
> namespace' statement.

Yeah, something is very bad.

Comment 11

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> Comment on attachment 8356343 [details] [diff] [review]
> Add functions getExtensionWithKnownPrefixes
> andgetSupportedExtensionWithKnownPrefixes to fix tests that fail to parse.
> 
> Review of attachment 8356343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are a lot of whitespace changes in this file, which isn't otherwise
> touched. While the whitespace changes are good, we generally refrain from
> making such whitespace changes outside of code that we're actually touching,
> to avoid making hg annotate less practical. It's OK for this time though ---
> just warning you in case some day a reviewer gets serious about that.

I've split the trailing whitespace removal into a separate patch.


> I find it strange that we choose to determine the format from the mozilla
> WebGLTexelFormat, instead of determining it from the OpenGL internalformat,
> which we have here (imageInfo.mInternalFormat). This seems like an
> unnecessary round-trip outside of and back into the realm of GL enums:  GL
> internalformat --> mozilla WebGLTexelFormat --> GL format , no?  (I could be
> missing something).

I took the easy way out. It felt a little dirty to write.

I couldn't find a concise list of the internalformat->format mapping for GL texture types. Instead of building a 4000-line function to map all the formats, it appeared that WebGLTexelFormat enumerated the formats that WebGL cares about.

I can build a mapping of GL internalformat->format based upon the WebGLTexelFormat enum and add an assert if that function is called with an unknown GLenum. (That's the best solution IMHO)

Comment 12

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

(In reply to Dan Glastonbury :djg :kamidphish from comment #11)
> I couldn't find a concise list of the internalformat->format mapping for GL
> texture types. Instead of building a 4000-line function to map all the
> formats, it appeared that WebGLTexelFormat enumerated the formats that WebGL
> cares about.

OK, that's sensible. Thanks for the explanation.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8356344 [details] [diff] [review]
Fix WebGL framebuffer completeness checks.

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

In your other review, you said this patch was bad, so marking r-.

Comment 14

[u480271]

Attachment: Add missing functions for webgl depth texture test.

Add functions getExtensionWithKnownPrefixes and
getSupportedExtensionWithKnownPrefixes to fix tests that fail to parse.

Comment 15

[u480271]

Attachment: Remove trailing whitespace.

Removed all trailing whitespace.

Comment 16

[u480271]

Attachment: Fix WebGL framebuffer completeness checks.

When changing WebGLTexture::ImageInfo to consistently store GL internal format
instead of format, code that checked for depth textures broke because general
depth component format type was being checked instead of the sized formats.

With :bjacob, we audited the locations of the checks and updated the code to
accept the internal formats by utilizing helper functions that check the
GLenum.

Comment 17

[u480271]

I simplified the patches to reduce the number of changes and pulled out all the trailing whitespace changes into one patch.

I'm looking for feedback on IsValidFBOXXXFormat functions. I'm not certain if we need the different versions for texture vs renderbuffer. I want to know if it would be possible to collate and collapse some of these.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8358171 [details] [diff] [review]
Remove trailing whitespace.

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

r++

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8358170 [details] [diff] [review]
Add missing functions for webgl depth texture test.

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

::: content/canvas/test/webgl/conformance/resources/webgl-test-utils.js
@@ +771,5 @@
> +var browserPrefixes = [
> +  "",
> +  "MOZ_",
> +  "OP_",
> +  "WEBKIT_"

MS? Probably doesn't matter much until we upstream it. (unless this is from upstream?)

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8358173 [details] [diff] [review]
Fix WebGL framebuffer completeness checks.

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

One thing that makes this hard to review (and generally follow) is the overlap between valid webgl enums and valid gl/gles enums. Without notes saying which ones are which, it's hard to really be sure what the function's supposed to do. We used to have WebGLEnum as well as GLEnum, which would be useful to use here to disambiguate this.

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +136,5 @@
> +static inline bool
> +IsValidFBOTextureDepthStencilFormat(GLenum internalFormat) {
> +    return (
> +        internalFormat == LOCAL_GL_DEPTH_STENCIL ||
> +        internalFormat == LOCAL_GL_DEPTH24_STENCIL8);

Is this for WebGL or GL/GLES?

::: content/canvas/src/WebGLTexelConversions.h
@@ +97,5 @@
>  
> +inline GLenum
> +GLFormatForTexelFormat(WebGLTexelFormat format) {
> +    switch (format) {
> +        case WebGLTexelFormat::R8:          return LOCAL_GL_LUMINANCE;

I think this is an existing fault, but IIRC, R8 is different from L8/Y8. (R,0,0,1) vs (Y,Y,Y,1).

::: content/canvas/src/WebGLTexture.cpp
@@ +434,4 @@
>      mContext->UpdateWebGLErrorAndClearGLError();
>      mContext->gl->fTexImage2D(imageTarget, level, imageInfo.mInternalFormat,
>                                imageInfo.mWidth, imageInfo.mHeight,
> +                              0, format, imageInfo.mType,

Assert `format == imageInfo.mInternalFormat`, since this must be true for GLES2+.

Comment 22

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

Comment on attachment 8358170 [details] [diff] [review]
Add missing functions for webgl depth texture test.

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

::: content/canvas/test/webgl/conformance/resources/webgl-test-utils.js
@@ +771,5 @@
> +var browserPrefixes = [
> +  "",
> +  "MOZ_",
> +  "OP_",
> +  "WEBKIT_"

Conversely, MOZ_ might not be needed anymore.

Comment 23

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

Comment on attachment 8358173 [details] [diff] [review]
Fix WebGL framebuffer completeness checks.

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

Just check that this doesn't regress online 'trunk' (1.0.3) conformance tests.

Comment 24

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #22)
> Comment on attachment 8358170 [details] [diff] [review]
> Add missing functions for webgl depth texture test.
> 
> Review of attachment 8358170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/test/webgl/conformance/resources/webgl-test-utils.js
> @@ +771,5 @@
> > +var browserPrefixes = [
> > +  "",
> > +  "MOZ_",
> > +  "OP_",
> > +  "WEBKIT_"
> 
> Conversely, MOZ_ might not be needed anymore.

This code was lifted directly from the Khronos conformance test JS harness.

Comment 25

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #21)
> Comment on attachment 8358173 [details] [diff] [review]
> Fix WebGL framebuffer completeness checks.
> 
> Review of attachment 8358173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One thing that makes this hard to review (and generally follow) is the
> overlap between valid webgl enums and valid gl/gles enums. Without notes
> saying which ones are which, it's hard to really be sure what the function's
> supposed to do. We used to have WebGLEnum as well as GLEnum, which would be
> useful to use here to disambiguate this.
> 
> ::: content/canvas/src/WebGLFramebuffer.cpp
> @@ +136,5 @@
> > +static inline bool
> > +IsValidFBOTextureDepthStencilFormat(GLenum internalFormat) {
> > +    return (
> > +        internalFormat == LOCAL_GL_DEPTH_STENCIL ||
> > +        internalFormat == LOCAL_GL_DEPTH24_STENCIL8);
> 
> Is this for WebGL or GL/GLES?

This is the GL/GLES internal format because internalFormat comes from the information stored in imageInfo.InternalFormat.
 
> ::: content/canvas/src/WebGLTexelConversions.h
> @@ +97,5 @@
> >  
> > +inline GLenum
> > +GLFormatForTexelFormat(WebGLTexelFormat format) {
> > +    switch (format) {
> > +        case WebGLTexelFormat::R8:          return LOCAL_GL_LUMINANCE;
> 
> I think this is an existing fault, but IIRC, R8 is different from L8/Y8.
> (R,0,0,1) vs (Y,Y,Y,1).

I believe I copied the conversion. Do you want it changed to LOCAL_GL_RED?

 
> ::: content/canvas/src/WebGLTexture.cpp
> @@ +434,4 @@
> >      mContext->UpdateWebGLErrorAndClearGLError();
> >      mContext->gl->fTexImage2D(imageTarget, level, imageInfo.mInternalFormat,
> >                                imageInfo.mWidth, imageInfo.mHeight,
> > +                              0, format, imageInfo.mType,
> 
> Assert `format == imageInfo.mInternalFormat`, since this must be true for
> GLES2+.

Hold on, can we do that? mContext->gl could be desktop GL. imageInfo.mInternalFormat is the internal format of the texture in the HW which could be GL or GL ES.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

The actions in comment 17 lead to believe this was left nominated for 28 in error. If that's an incorrect deduction, please renominate.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8358173 [details] [diff] [review]
Fix WebGL framebuffer completeness checks.

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

I think we need one extra round to clear this up. Please add comments to this code to indicate whether we're talking about GLES2/GL or WebGL behaviors for internalFormat.

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +155,5 @@
> +}
> +
> +static inline bool
> +IsValidFBORenderbufferDepthStencilFormat(GLenum internalFormat) {
> +    return internalFormat == LOCAL_GL_DEPTH_STENCIL;

Renderbuffers take only sized formats. This should be DEPTH24_STENCIL8. (in GLES2)

Comment 28

[u480271]

Attachment: Remove trailing whitespace.

Rebase onto Bug 962392.

Comment 29

[u480271]

Attachment: Fix WebGL framebuffer completeness checks.

Rebase onto Bug 962392.

Comment 30

[u480271]

Attachment: Add extra comments of meaning of GLenum to IsValidFBOXXX calls.

Extra round of clean up asked for by Jeff. I tested his suggestion of changing
IsValidFBORenderbufferDepthStencilFormat to check the sized format, but this
fails. I switched to checking the internal formats that are used in
GLContext::RenderbufferStorage, since this will allow a DEPTH24_STENCIL8
renderbuffer for checking a valid depth renderbuffer backed by depth_stencil
*but* have it be invalid as depth_stencil attachment.

Comment 31

[u480271]

Comment on attachment 8364837 [details] [diff] [review]
Remove trailing whitespace.

Carry r=bjacob

Comment 32

[u480271]

Comment on attachment 8364839 [details] [diff] [review]
Fix WebGL framebuffer completeness checks.

Carry r=bjacob

Comment 33

[u480271]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/732e2ad23877
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfeec70c1b29
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b636fd22dd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/56af0a9846fd

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/732e2ad23877
https://hg.mozilla.org/mozilla-central/rev/cfeec70c1b29
https://hg.mozilla.org/mozilla-central/rev/2b636fd22dd7
https://hg.mozilla.org/mozilla-central/rev/56af0a9846fd

Comment 36

[Andre Weissflog]

Confirmed working in current Nightly 2014-01-29 on OSX 10.9.1 with IntelHD4000. Thanks :)

Comment 37

[Paul Silaghi, QA [:pauly]]

Still not working for me on Nightly 2014-01-29 on OSX 10.7.5, nVIDIA GeForce 9400

Comment 38

[u480271]

I don't have access to OSX 10.7.5, nVIDIA GeForce 9400. Is there some way for me to get logs for the failure?

Comment 39

[Paul Silaghi, QA [:pauly]]

Attachment: OSX 10.8.5, nVIDIA GeForce 9400.txt

30.0a1 (2014-02-23), OSX 10.8.5, nVIDIA GeForce 9400
Status: FAIL

Comment 40

[Paul Silaghi, QA [:pauly]]

Attachment: OSX 10.8.5, AMD Radeon HD 6630 M 256.txt

30.0a1 (2014-02-23), OSX 10.8.5, AMD Radeon HD 6630 M 256
Status: PASS, but the dragon is black

Comment 41

[Paul Silaghi, QA [:pauly]]

Attachment: OSX 10.8.5, AMD Radeon HD 6630 M 256

Comment 42

[Andre Weissflog]

(In reply to Paul Silaghi, QA [:pauly] from comment #40)
> Created attachment 8380539 [details]
> OSX 10.8.5, AMD Radeon HD 6630 M 256.txt
> 
> 30.0a1 (2014-02-23), OSX 10.8.5, AMD Radeon HD 6630 M 256
> Status: PASS, but the dragon is black

Hmm, the black dragon could be unrelated to the framebuffer check, or it could be a sign that sampling from the depth texture doesn't work (which would be related to this issue though). Could you maybe try Chrome on the same machine, and on Firefox 26.0 (since when I wrote the ticket, the test-demo looked ok on FF26), and check if the Dragon there is black too? If yes then it is very likely unrelated to the framebuffer completeness check code, and is either a bug in the engine with that specific GPU/driver.

I'll see if we have a Radeon HD Mac around here somewhere in the meantime :)

Comment 43

[Paul Silaghi, QA [:pauly]]

Works fine in Chrome, FF 26, FF 29.0a2(2014-02-24), OSX 10.8.5, AMD Radeon HD 6630 M 256

Comment 44

[Andre Weissflog]

Ok, I found a similar machine Radeon HD XXXX, OSX 10.9.1, same problem (black dragon), but it also happens in the regular demos (http://www.flohofwoe.net/demos/dragons_asmjs.html). The difference between the demos is that the ..._fftest... demo (which exposed the framebuffer completeness check problem) uses a depth-texture as Z-Buffer, and later samples the depth-texture to get the per-pixel-depth for lighting. 

But the "regular" demos don't use a Depth-texture, but instead encode the per-pixel-depth in a regular the RG portion of a regular RGBA render-target.

So: I think the problem is unrelated to the framebuffer completeness check or depth render textures, but it is still a new problem, since in the current public firefox both demos render correctly in both demo versions.

You can also check the other demos on http://www.flohofwoe.net/demos.html, they should have the same problem.

Unfortunately I can't simply seize the Radeon Mac for further tests :/

Comment 45

[Andre Weissflog]

PS: I tested on a AMD Radeon HD 6750M 1024 MB.

Comment 46

[Paul Silaghi, QA [:pauly]]

What about the GeForce problem ? It's not working at all

Comment 47

[u480271]

I've discovered the source of the black models in Andre's demos is because of a bug that I introduced that broke compressed texture support. It's been fixed in Bug 975824

Comment 48

[Andre Weissflog]

Nice! FYI I just updated the public demos with the depth-texture-renderer, so there's no difference now between the special fftest-demo and the regular public demos. I also enabled Vertex Array Objects, since this landed recently in emscripten, but this shouldn't affect this issue either.

Cheers,
-Floh.

Comment 49

[Andre Weissflog]

Hi, me again :) It looks like that parts of this bug have slipped into Firefox 28.0? I'm now getting a failed framebuffer completeness check in FF 28 on OSX. Nightly works fine though.

You can check here: http://www.flohofwoe.net/demos/dragons_asmjs.html, the original http://www.flohofwoe.net/demos/dragonsfftest_asmjs.html also doesn't run in FF28)

The JS console now shows this message if the FB completeness check fails:
"Warning: GLTextureFactory::CreateRenderTarget(): frame buffer completeness check failed!"

Demo continues after this, but nothing can be rendered.

Is there a chance that the fix is merged into FF28?

Comment 50

[u480271]

Attachment: bug-948002-ff28-beta-rollup.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 948002
User impact if declined: WebGL will framebuffer completeness with regress from FF27 with incorrect results. 
Testing completed (on m-c, etc.): mozilla-central, mozilla-aurora
Risk to taking this patch (and alternatives if risky): Low risk. This patch was fixed in FF29 and has been tested there and also in FF30.
String or IDL/UUID changes made by this patch:

Comment 51

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

Rel-drivers - I know we're at the stage where we're only uplifting the tracked issues for 28 beta - let us know if you think this deserves being tracked.  It is a regression with a simple fix.

Comment 52

[u480271]

Comment on attachment 8385987 [details] [diff] [review]
bug-948002-ff28-beta-rollup.patch

Jeff, can you review? You've reviewed before. This one is a roll up of the others so there are a lot of whitespace changes rolled in.

Comment 53

[u480271]

Attachment: bug-948002-ff28-beta-rollup.patch

This time without whitespace changes.

Comment 54

[u480271]

Comment on attachment 8386453 [details] [diff] [review]
bug-948002-ff28-beta-rollup.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 948002
User impact if declined: WebGL will framebuffer completeness with regress from FF27 with incorrect results. 
Testing completed (on m-c, etc.): mozilla-central, mozilla-aurora
Risk to taking this patch (and alternatives if risky): Low risk. This patch was fixed in FF29 and has been tested there and also in FF30.
String or IDL/UUID changes made by this patch:

Comment 55

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8386453 [details] [diff] [review]
bug-948002-ff28-beta-rollup.patch

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +4204,3 @@
>          }
> +
> +        MOZ_CRASH("Invalid WebGL texture format/type?");

Below in this function we use MOZ_ASSERT, and return WebGLTexelFormat::BadFormat instead of crashing release builds. Shouldn't we just do that?

::: content/canvas/src/WebGLContextUtils.cpp
@@ +29,5 @@
> +IsGLDepthFormat(GLenum internalFormat)
> +{
> +    return (internalFormat == LOCAL_GL_DEPTH_COMPONENT ||
> +            internalFormat == LOCAL_GL_DEPTH_COMPONENT16 ||
> +            internalFormat == LOCAL_GL_DEPTH_COMPONENT32);

LOCAL_GL_DEPTH_COMPONENT24 is another depth format. I think WebGL doesn't expose it, though.

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +130,5 @@
> +        internalFormat == LOCAL_GL_RGBA32F_ARB);
> +}
> +
> +static inline bool
> +IsValidFBOTextureDepthFormat(GLenum internalFormat)

Shouldn't this just use IsGLDepthFormat and IsGLDepthStencilFormat?

@@ +168,5 @@
> +
> +static inline bool
> +IsValidFBORenderbufferDepthStencilFormat(GLenum internalFormat)
> +{
> +    return internalFormat == LOCAL_GL_DEPTH_STENCIL;

DEPTH24_STENCIL8 is a valid RB format, DEPTH_STENCIL is not. RBs only accept sized types.

Comment 56

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8386453 [details] [diff] [review]
bug-948002-ff28-beta-rollup.patch

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +168,5 @@
> +
> +static inline bool
> +IsValidFBORenderbufferDepthStencilFormat(GLenum internalFormat)
> +{
> +    return internalFormat == LOCAL_GL_DEPTH_STENCIL;

Ugh, disregard. WebGL uses DEPTH_STENCIL instead of DEPTH24_STENCIL8 for the RB format.

Comment 57

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #55)
> Comment on attachment 8386453 [details] [diff] [review]
> bug-948002-ff28-beta-rollup.patch
> 
> Review of attachment 8386453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +4204,3 @@
> >          }
> > +
> > +        MOZ_CRASH("Invalid WebGL texture format/type?");
> 
> Below in this function we use MOZ_ASSERT, and return
> WebGLTexelFormat::BadFormat instead of crashing release builds. Shouldn't we
> just do that?

I was aiming to change as little as possible. In m-c the MOZ_CRASH is gone and has been replaced with MOZ_ASSERT and return false.

Comment 58

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8386453 [details] [diff] [review]
bug-948002-ff28-beta-rollup.patch

While this is a regression, the range seems like it's been around since FF25 and with today's beta being the last and restricted to tracked/critical security issues we'll have to hold off on this and wait for 29.

Comment 59

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8386453 [details] [diff] [review]
bug-948002-ff28-beta-rollup.patch

wrong way - this is a minus, as per previous comment.

Comment 60

[Andre Weissflog]

Hmm, I think this particular problem (framebuffer completeness check failed when attaching a depth texture) didn't exist until FF28. It worked fine in FF27. There's probably not a lot of WebGL demos affected though...

Comment 62

[Paul Silaghi, QA [:pauly]]

This still doesn't work on FF 29b2, 31.0a1 (2014-03-25) on OSX 10.8.5, nVIDIA GeForce 9400.
But works fine instead on FF 29b2, 31.0a1 (2014-03-25) on OSX 10.8.5, AMD Radeon HD 6630 M 256.
Thoughts?

Comment 63

[Andre Weissflog]

Can you post the output of the text output field and the Javascript console when running this demo on the GeForce 9400 machine? The 9400 problem might be unrelated to the framebuffer completeness bug since I never had the chance to run the demos on such a machine.

Thanks!
-Floh.

Comment 64

[Andre Weissflog]

Erm, I forgot the link to the demo sorry: http://www.flohofwoe.net/demos/dragons_asmjs.html

Comment 65

[Paul Silaghi, QA [:pauly]]

(In reply to Andre Weissflog from comment #63)
> Can you post the output of the text output field and the Javascript console
> when running this demo on the GeForce 9400 machine? 
See comment 39

Comment 66

[Paul Silaghi, QA [:pauly]]

(In reply to Andre Weissflog from comment #64)
> Erm, I forgot the link to the demo sorry:
> http://www.flohofwoe.net/demos/dragons_asmjs.html
Is this different than the link in comment 0 ?

Comment 67

[Andre Weissflog]

Ah here's the culprit:

"Warning: GLExtWrapper: no depth texture support detected!" dragonsfftest_asmjs.html:59
Error: WebGL: texImage2D: invalid format DEPTH_STENCIL: need WEBGL_depth_texture enabled dragonsfftest_asmjs.js:4797
uncaught exception: abort() at stackTrace@http://www.flohofwoe.net/demos/dragonsfftest_asmjs.js:933:1
abort@http://www.flohofwoe.net/demos/dragonsfftest_asmjs.js:8794:3

The 9400 doesn't seem to expose the depth_texture extensions (you can also search for this in the demos' text field output). Without the depth_texture extension the demos can't run, so this problem is unrelated to the framebuffer completeness check.

Comment 68

[Ioana (away)]

Marking as verified per the above comments.

This is also fixed for me on Ubuntu 13.04 (Intel HD 4000 graphics), Windows 7 (nVIDIA GeForce GT 610) and Mac OS X 10.9 (AMD Radeon HD 6630 M 256).