1030206 - webgl-1.0.3 conformance failture oes_texture_float test

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Walter Litwinczyk [:walter]]

Attachment: oes_texture_float_failure.png

This webgl conformance test fails: http://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/oes-texture-float.html

Comment 1

[Walter Litwinczyk [:walter]]

Attachment: oes_texture_float_patch

If I interpreted the spec correctly, the OES_texture_float extension should also enable the WEBGL_color_buffer_float extension:

From: http://www.khronos.org/registry/webgl/extensions/OES_texture_float/

> "Implementations supporting float rendering via this extension will implicitly enable the WEBGL_color_buffer_float extension and follow its requirements."

This fixes the issue because WebGLContext::ReadPixels() only checks for the color_buffer_*_float extensions when determining if GL_FLOAT is a valid type.

Comment 2

[Walter Litwinczyk [:walter]]

Attachment: Test passes with the patch (png)

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Walter Litwinczyk [:walter] from comment #1)
> Created attachment 8445968 [details] [diff] [review]
> oes_texture_float_patch
> 
> If I interpreted the spec correctly, the OES_texture_float extension should
> also enable the WEBGL_color_buffer_float extension:
> 
> From: http://www.khronos.org/registry/webgl/extensions/OES_texture_float/
> 
> > "Implementations supporting float rendering via this extension will implicitly enable the WEBGL_color_buffer_float extension and follow its requirements."
Yeah, this is phrased somewhat poorly, given that the next line says: "new implementations should not implicitly support float rendering".

Basically, if we have a choice, we should not implicitly enable WEBGL_color_buffer_float here.
> 
> This fixes the issue because WebGLContext::ReadPixels() only checks for the
> color_buffer_*_float extensions when determining if GL_FLOAT is a valid type.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8445968 [details] [diff] [review]
oes_texture_float_patch

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

::: content/canvas/src/WebGLExtensionTextureFloat.cpp
@@ +17,5 @@
> +    // implicitly enable the WEBGL_color_buffer_float extension and follow
> +    // its requirements.
> +    if (!context->IsExtensionEnabled(WebGLExtensionID::WEBGL_color_buffer_float)) {
> +        context->EnableExtension(WebGLExtensionID::WEBGL_color_buffer_float);
> +    }

Let's not do this.

The full paragraph from the spec:

Implementations supporting float rendering via this extension will implicitly enable the WEBGL_color_buffer_float extension and follow its requirements. This ensures correct behavior when a texture with pixel type FLOAT is attached to an FBO. Although this feature has historically been allowed, new implementations *should not* implicitly support float rendering and applications should be modified to explicitly enable WEBGL_color_buffer_float.

Comment 5

[Walter Litwinczyk [:walter]]

Attachment: oes_texture_float_patch

Disallow floating point attachments to FBOs unless WEBGL_color_buffer_float is enabled. I believe this applies to both texture and renderbuffer targets.

Comment 6

[Walter Litwinczyk [:walter]]

Attachment: Test passes (png)

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8446222 [details] [diff] [review]
oes_texture_float_patch

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +610,5 @@
> +        // Excerpt from http://www.khronos.org/registry/webgl/extensions/OES_texture_float/
> +        //
> +        // New implementations should not implicitly support float rendering and
> +        // applications should be modified to explicitly enable WEBGL_color_buffer_float.
> +        if (mColorAttachments[i].IsReadableFloat() &&

Why is this IsReadable, especially given we're about to Write to it?

Comment 8

[Walter Litwinczyk [:walter]]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Comment on attachment 8446222 [details] [diff] [review]
> oes_texture_float_patch
> 
> Review of attachment 8446222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLFramebuffer.cpp
> @@ +610,5 @@
> Why is this IsReadable, especially given we're about to Write to it?

Mostly because it checks for the floating point formats and I didn't want to make something that did the same thing with a different name. It would probably be better if it was renamed to something like "IsRenderableFloat()" ?

Also manually checking the Texture() and Renderbuffer() formats is a little messy.

Comment 9

[Walter Litwinczyk [:walter]]

Attachment: webgl_conformance_oes_texture

Since IsReadableFloat() is not used in many places (in fact, only 2, one being this patch). I renamed it to IsFloatType() to be more general. Nothing else changed.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8449001 [details] [diff] [review]
webgl_conformance_oes_texture

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +611,5 @@
> +        //
> +        // New implementations should not implicitly support float rendering and
> +        // applications should be modified to explicitly enable WEBGL_color_buffer_float.
> +        if (mColorAttachments[i].IsFloatType() &&
> +            !Context()->IsExtensionEnabled(WebGLExtensionID::WEBGL_color_buffer_float))

This depends on the float type, since we have separate extensions for float and half-float.

Comment 11

[Walter Litwinczyk [:walter]]

Attachment: webgl_conformance_oes_texture

Ah, yes you are correct. I've renamed and modified IsReadableFloat() to now take an enum of which float type to check for. I've split the incomplete check into two, one for half floats and one for full floats.

Comment 12

[Walter Litwinczyk [:walter]]

Attachment: webgl_conformance_oes_texture

Sorry, apparently forgot to qrefresh before exporting...

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8449067 [details] [diff] [review]
webgl_conformance_oes_texture

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +619,5 @@
> +        // applications should be modified to explicitly enable WEBGL_color_buffer_float.
> +        if (mColorAttachments[i].IsFloatType(Attachment::FULL_FLOAT) &&
> +            !Context()->IsExtensionEnabled(WebGLExtensionID::WEBGL_color_buffer_float))
> +        {
> +                hasIncomplete |= true;

bad indent

::: content/canvas/src/WebGLFramebuffer.h
@@ +53,5 @@
>  
>          bool HasAlpha() const;
> +
> +        // For IsFloatType()
> +        enum FloatType {

Use MFBT enum classes, as seen in GLContextTypes.h.

Comment 14

[Walter Litwinczyk [:walter]]

Attachment: webgl_conformance_oes_texture

r=jgilbert carried forward

Comment 15

[Walter Litwinczyk [:walter]]

Attachment: webgl_conformance_oes_texture

Try was failing in some cases when a color attachment was not defined. Added a check 'if (mColorAttachments[i].IsDefined())' before the floating point support.

Comment 16

[Walter Litwinczyk [:walter]]

Attachment: webgl_conformance_oes_texture_tests

Try tests are now outdated because they fail if floating point render targets are not supported, but if you look at the new WebGL 1.0.3 version of the test:

http://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/oes-texture-float.html

lines 204-214 of the source allows the implementation to not support a floating point render target. This aligns with the wording of the OES_texture_float extension, which says new implementations should not implicitly support it:

http://www.khronos.org/registry/webgl/extensions/OES_texture_float/

Also the try tests still fail because they were expecting failures for these tests, not sure what to do to fix that:

https://tbpl.mozilla.org/?tree=Try&rev=0092d02a6696

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8451959 [details] [diff] [review]
webgl_conformance_oes_texture_tests

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

::: content/canvas/test/webgl-conformance/conformance/extensions/oes-texture-float.html
@@ -157,5 @@
>      gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, texture, 0);
>      gl.bindTexture(gl.TEXTURE_2D, null);
> -    shouldBe("gl.checkFramebufferStatus(gl.FRAMEBUFFER)", "gl.FRAMEBUFFER_COMPLETE");
> -    // While strictly speaking it is probably legal for a WebGL implementation to support
> -    // floating-point textures but not as attachments to framebuffer objects, any such

This text is hilarious in retrospect, since this is exactly what many implementations do.

Comment 18

[Walter Litwinczyk [:walter]]

Attachment: webgl_conformance_oes_texture_tests

I talked with jgilbert last week about what to do with the failing android test (only fails on Android 2.3 opt) he said to mark the test as skipped for now. Otherwise there's no issues:

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

Comment 19

[u480271]

Comment on attachment 8457513 [details] [diff] [review]
webgl_conformance_oes_texture_tests

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

::: content/canvas/test/webgl-conformance/conformance/extensions/oes-texture-float.html
@@ +154,5 @@
>      var fbo = gl.createFramebuffer();
>      gl.bindFramebuffer(gl.FRAMEBUFFER, fbo);
>      gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, texture, 0);
>      gl.bindTexture(gl.TEXTURE_2D, null);
> +    if (gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE) {

Does this change bring us inline with 1.0.3?

Comment 20

[Walter Litwinczyk [:walter]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #19)
> Comment on attachment 8457513 [details] [diff] [review]
> webgl_conformance_oes_texture_tests
> 
> Review of attachment 8457513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> content/canvas/test/webgl-conformance/conformance/extensions/oes-texture-
> float.html
> @@ +154,5 @@
> >      var fbo = gl.createFramebuffer();
> >      gl.bindFramebuffer(gl.FRAMEBUFFER, fbo);
> >      gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, texture, 0);
> >      gl.bindTexture(gl.TEXTURE_2D, null);
> > +    if (gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE) {
> 
> Does this change bring us inline with 1.0.3?

Yeah, I stole it from the 1.0.3 test here: http://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/oes-texture-float.html

line ~209

Comment 21

[Walter Litwinczyk [:walter]]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d756bc2ba7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6471c4d6ffee

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6d756bc2ba7
https://hg.mozilla.org/mozilla-central/rev/6471c4d6ffee

Comment 24

[Luke Wagner [:luke]]

It seems this patch has broken one of Epic's upcoming demos, at least on Linux.  It's not yet public, but I think Jeff would have the link.  Email me if you need it.

Comment 25

[Walter Litwinczyk [:walter]]

This patch did introduce a behavioral change. The OES_texture_[half_]float extension no longer implicitly enables the WEBGL_color_buffer_[half_]float extension, could this be the issue?

Comment 26

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

If this exposed a bug in Epic's code, let's evangelize and tell them how to solve it.

Comment 27

[Luke Wagner [:luke]]

We can definitely get them to fix it if it is indeed their bug.  I think Jukka might know the answer to comment 25?

Comment 28

[Walter Litwinczyk [:walter]]

Unfortunately they probably won't be happy with this workaround (assuming the above is the issue), as the end user would have to explicitly enable the preference for it to work: Bug 1041301 Comment 2

Comment 29

[Luke Wagner [:luke]]

Do you mean the workaround is that the browser user would have to flip a pref (in about:config)?  If so, you're right, that's no workaround.  I still haven't tested outside Linux; will this change affect all platforms equally?

Comment 30

[Walter Litwinczyk [:walter]]

> Do you mean the workaround is that the browser user would have to flip a pref 
> (in about:config)? If so, you're right, that's no workaround. 
Yes, that's what would currently be required, because WEBGL_color_buffer_float is still a draft extension.

> I still haven't tested outside Linux; will this change affect all platforms equally?
Yeah, this change should affect everything equally. 

Milan has said in Bug 1041301 Comment 3 to back out this behavior change while we're thinking of a suitable fix. So once that's done it shouldn't block Epic (and others) anymore.

Comment 31

[Luke Wagner [:luke]]

Great, thanks!

Comment 32

[Jukka Jylänki]

In Emscripten, independent of which way this feature eventually goes, rebuilding with Emscripten compiler version 1.7.0 or newer (dated Oct 23, 2013) will make the application automatically enable both WEBGL_color_buffer_float and OES_texture_float extensions at GL context creation time, so Emscripten has had the logic for a long time that is safe for both old and new spec wording. I am quite sure that all newer demos that Epic builds are with a compiler version that is much newer than this, so they already have this change. Perhaps old existing demos on the web like Epic Citadel that were from long time ago might have been compiled with an older version.

The issue then like Walter mentions above is that even if the currently compiled demos have good code, the extension WEBGL_color_buffer_float is not there yet but requires enabling a pref, so effectively the Firefox patch making this behavioral change accidentally made floating point textures go back to an experimental feature, which is what breaks the demos here.

Comment 33

[Jukka Jylänki]

Oh, for reference, here's the original Emscripten commit that adjusted the getExtension() logic so that Emscripten became safe from this issue: https://github.com/kripken/emscripten/commit/12918ccc0a3f754f56349e82c680a9df405c4731

Comment 34

[Jukka Jylänki]

Hmm, testing on Windows on a laptop with Intel HD 3000, and another computer with GeForce GT 9600, in current Nightly, even if I enable webgl.enable-draft-extensions = true, I don't get WEBGL_color_buffer_float in the extensions list. Is that expected?

Comment 35

[Walter Litwinczyk [:walter]]

It very well could be the expected behavior. If you check the console output of Firefox (OS console, not web console) you should see something like:

> OpenGL version detected: 200
> OpenGL vendor: Google Inc.
> OpenGL renderer: ANGLE (NVIDIA GeForce GT 750M    Direct3D9Ex vs_3_0 ps_3_0)

And depending on what that is, it may have been determined that those features are not supported. For example the two features required for the WEBGL_color_buffer_float are renderbuffer_color_float and frag_color_float, which both require at least OpenGL 3.0 or some list of extensions.

Now it may be a bug because I would expect something higher than OpenGL 2.0 to be supported on my hardware. Jeff Gilbert or Dan Glastonbury would know more about this.

Comment 36

[Jukka Jylänki]

Both are D3D11 level hardware, and have the following GL flags:

Intel HD 3000, Win7:
GL_VERSION: 3.1.0 - Build 9.17.10.3517
GL_VENDOR: Intel
GL_RENDERER: Intel(R) HD Graphics 3000
GL_SHADING_LANGUAGE_VERSION: 1.40 - Intel Build 9.17.10.3517

nVidia GeForce 9600GT, Win8.1:
GL_VERSION: 3.3.0
GL_VENDOR: NVIDIA Corporation
GL_RENDERER: GeForce 9600 GT/PCIe/SSE2
GL_SHADING_LANGUAGE_VERSION: 3.30 NVIDIA via Cg compiler

Floating point render targets work on both hardware, both in native GL and in WebGL, but in current Nightly I am not able to see WEBGL_color_buffer_float on either systems even after setting webgl.enable-draft-extensions = true.

Comment 37

[u480271]

> It very well could be the expected behavior. If you check the console output
> of Firefox (OS console, not web console) you should see something like:
> 
> > OpenGL version detected: 200
> > OpenGL vendor: Google Inc.
> > OpenGL renderer: ANGLE (NVIDIA GeForce GT 750M    Direct3D9Ex vs_3_0 ps_3_0)

The problem is with ANGLE. ANGLE have decided that they can't easily support the requirements for WEBGL_color_buffer_float. Instead they provide silent support for render to FP through OES_texture_float extension.

Comment 38

[Jukka Jylänki]

Oh interesting.

So that means that if this piece of the OES_texture_float spec "Although this feature has historically been allowed, new implementations should not implicitly support float rendering and applications should be modified to explicitly enable WEBGL_color_buffer_float." is kept alive (Vlad was suggesting he'd favor removing that), then Firefox would have a bug due to the ANGLE limitation, since it would mean before implementing this spec change, Firefox was able to render to FP textures, but after implementing the change, it would not, since as it stands it's not supporting WEBGL_color_buffer_float on these hardware?

Comment 39

[u480271]

(In reply to Jukka Jylänki from comment #38)
> Oh interesting.
> 
> So that means that if this piece of the OES_texture_float spec "Although
> this feature has historically been allowed, new implementations should not
> implicitly support float rendering and applications should be modified to
> explicitly enable WEBGL_color_buffer_float." is kept alive (Vlad was
> suggesting he'd favor removing that), then Firefox would have a bug due to
> the ANGLE limitation, since it would mean before implementing this spec
> change, Firefox was able to render to FP textures, but after implementing
> the change, it would not, since as it stands it's not supporting
> WEBGL_color_buffer_float on these hardware?

Yeah, I think that's a good summary.