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"); }

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

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

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

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.

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.

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

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 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 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.

(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.

(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)

(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 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-.

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

Removed all trailing whitespace.

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.

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 on attachment 8358171 [details] [diff] [review] Remove trailing whitespace. Review of attachment 8358171 [details] [diff] [review]: ----------------------------------------------------------------- r++

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 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 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 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.

(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.

(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.

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 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)

Rebase onto Bug 962392.

Rebase onto Bug 962392.

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 on attachment 8364837 [details] [diff] [review] Remove trailing whitespace. Carry r=bjacob

Comment on attachment 8364839 [details] [diff] [review] Fix WebGL framebuffer completeness checks. Carry r=bjacob

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

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

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

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

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?

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

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

(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 :)

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

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 :/

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

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

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

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.

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?

[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:

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 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.

This time without whitespace changes.

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 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 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.

(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 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 on attachment 8386453 [details] [diff] [review] bug-948002-ff28-beta-rollup.patch wrong way - this is a minus, as per previous comment.

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...

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?

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.

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

(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

(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 ?

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.

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).