Andre Weissflog Reporter
	Description • 11 years ago

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

	Alon Zakai (:azakai)
	Comment 1 • 11 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 2 • 11 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 3 • 11 years ago

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

	Jukka Jylänki
	Comment 4 • 11 years ago

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.

	u480271 Assignee
	Comment 5 • 11 years ago

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.

	u480271 Assignee
	Comment 6 • 11 years ago

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

	u480271 Assignee
	Comment 7 • 11 years ago

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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 8 • 11 years ago

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?

	Benoit Jacob [:bjacob] (mostly away)
	Comment 9 • 11 years ago

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.

	u480271 Assignee
	Comment 10 • 11 years ago

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

	u480271 Assignee
	Comment 11 • 11 years ago

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

	Benoit Jacob [:bjacob] (mostly away)
	Comment 12 • 11 years ago

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

	Kelsey Gilbert [:jgilbert]
	Comment 13 • 11 years ago

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

	u480271 Assignee
	Comment 14 • 11 years ago

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

	u480271 Assignee
	Comment 15 • 11 years ago

Removed all trailing whitespace.

	u480271 Assignee
	Comment 16 • 11 years ago

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.

	u480271 Assignee
	Comment 17 • 11 years ago

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.

	Kelsey Gilbert [:jgilbert]
	Comment 19 • 11 years ago

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

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

r++

	Kelsey Gilbert [:jgilbert]
	Comment 20 • 11 years ago

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

	Kelsey Gilbert [:jgilbert]
	Comment 21 • 11 years ago

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

	Benoit Jacob [:bjacob] (mostly away)
	Comment 22 • 11 years ago

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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 23 • 11 years ago

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.

	u480271 Assignee
	Comment 24 • 10 years ago

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

	u480271 Assignee
	Comment 25 • 10 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 26 • 10 years ago

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

	Kelsey Gilbert [:jgilbert]
	Comment 27 • 10 years ago

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)

	u480271 Assignee
	Comment 28 • 10 years ago

Rebase onto Bug 962392.

	u480271 Assignee
	Comment 29 • 10 years ago

Rebase onto Bug 962392.

	u480271 Assignee
	Comment 30 • 10 years ago

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.

	u480271 Assignee
	Comment 31 • 10 years ago

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

Carry r=bjacob

	u480271 Assignee
	Comment 32 • 10 years ago

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

Carry r=bjacob

	u480271 Assignee
	Comment 33 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 10 years ago

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

	Andre Weissflog Reporter
	Comment 36 • 10 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 37 • 10 years ago

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

	u480271 Assignee
	Comment 38 • 10 years ago

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?

	Paul Silaghi, QA [:pauly]
	Comment 39 • 10 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 40 • 10 years ago

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

	Andre Weissflog Reporter
	Comment 42 • 10 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 43 • 10 years ago

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

	Andre Weissflog Reporter
	Comment 44 • 10 years ago

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

	Andre Weissflog Reporter
	Comment 45 • 10 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 46 • 10 years ago

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

	u480271 Assignee
	Comment 47 • 10 years ago

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

	Andre Weissflog Reporter
	Comment 48 • 10 years ago

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.

	Andre Weissflog Reporter
	Comment 49 • 10 years ago

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?

	u480271 Assignee
	Comment 50 • 10 years ago

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 51 • 10 years ago

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.

	u480271 Assignee
	Comment 52 • 10 years ago

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.

	u480271 Assignee
	Comment 53 • 10 years ago

This time without whitespace changes.

	u480271 Assignee
	Comment 54 • 10 years ago

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:

	Kelsey Gilbert [:jgilbert]
	Comment 55 • 10 years ago

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.

	Kelsey Gilbert [:jgilbert]
	Comment 56 • 10 years ago

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.

	u480271 Assignee
	Comment 57 • 10 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 58 • 10 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 59 • 10 years ago

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

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

	Andre Weissflog Reporter
	Comment 60 • 10 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 62 • 10 years ago

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?

	Andre Weissflog Reporter
	Comment 63 • 10 years ago

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.

	Andre Weissflog Reporter
	Comment 64 • 10 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 65 • 10 years ago

(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

	Paul Silaghi, QA [:pauly]
	Comment 66 • 10 years ago

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

	Andre Weissflog Reporter
	Comment 67 • 10 years ago

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.

	Ioana (away)
	Comment 68 • 10 years ago

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