webgl-1.0.3 conformance failture oes_texture_float test

RESOLVED FIXED in mozilla33

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: walter, Assigned: walter)

Tracking

unspecified
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

307.04 KB, image/png
Details
188.29 KB, image/png
Details
6.33 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
5.13 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
Created attachment 8445964 [details]
oes_texture_float_failure.png

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

Comment 1

4 years ago
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."

This fixes the issue because WebGLContext::ReadPixels() only checks for the color_buffer_*_float extensions when determining if GL_FLOAT is a valid type.
Attachment #8445968 - Flags: review?(jgilbert)
(Assignee)

Comment 2

4 years ago
Created attachment 8445969 [details]
Test passes with the patch (png)
(Assignee)

Updated

4 years ago
Attachment #8445969 - Attachment description: Test passes with the patch → Test passes with the patch (png)
(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 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.
Attachment #8445968 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 5

4 years ago
Created attachment 8446222 [details] [diff] [review]
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.
Attachment #8445968 - Attachment is obsolete: true
Attachment #8445969 - Attachment is obsolete: true
Attachment #8446222 - Flags: review?(jgilbert)
(Assignee)

Comment 6

4 years ago
Created attachment 8446224 [details]
Test passes (png)
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?
Attachment #8446222 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
Created attachment 8449001 [details] [diff] [review]
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.
Attachment #8446222 - Attachment is obsolete: true
Attachment #8449001 - Flags: review?(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.
Attachment #8449001 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 11

4 years ago
Created attachment 8449066 [details] [diff] [review]
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.
Attachment #8449001 - Attachment is obsolete: true
Attachment #8449066 - Flags: review?(jgilbert)
(Assignee)

Comment 12

4 years ago
Created attachment 8449067 [details] [diff] [review]
webgl_conformance_oes_texture

Sorry, apparently forgot to qrefresh before exporting...
Attachment #8449066 - Attachment is obsolete: true
Attachment #8449066 - Flags: review?(jgilbert)
Attachment #8449067 - Flags: review?(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.
Attachment #8449067 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 14

4 years ago
Created attachment 8449076 [details] [diff] [review]
webgl_conformance_oes_texture

r=jgilbert carried forward
Attachment #8449067 - Attachment is obsolete: true
Attachment #8449076 - Flags: review+
(Assignee)

Comment 15

4 years ago
Created attachment 8451952 [details] [diff] [review]
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.
Attachment #8449076 - Attachment is obsolete: true
Attachment #8451952 - Flags: review?(jgilbert)
(Assignee)

Comment 16

4 years ago
Created attachment 8451959 [details] [diff] [review]
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
Attachment #8451959 - Flags: review?(jgilbert)
Attachment #8451952 - Flags: review?(jgilbert) → review+
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.
Attachment #8451959 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 8457513 [details] [diff] [review]
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
Attachment #8451959 - Attachment is obsolete: true
Attachment #8457513 - Flags: review?(dglastonbury)
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?
Attachment #8457513 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 20

4 years ago
(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
(Assignee)

Comment 21

4 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=e639e506831f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6d756bc2ba7
https://hg.mozilla.org/mozilla-central/rev/6471c4d6ffee
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Comment 24

4 years ago
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.
(Assignee)

Comment 25

4 years ago
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?

Updated

4 years ago
Depends on: 1041301
If this exposed a bug in Epic's code, let's evangelize and tell them how to solve it.

Comment 27

4 years ago
We can definitely get them to fix it if it is indeed their bug.  I think Jukka might know the answer to comment 25?
Flags: needinfo?(jjylanki)
(Assignee)

Comment 28

4 years ago
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

4 years ago
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?
(Assignee)

Comment 30

4 years ago
> 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

4 years ago
Great, thanks!

Updated

4 years ago
Flags: needinfo?(jjylanki)

Comment 32

4 years ago
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

4 years ago
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

4 years ago
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?
(Assignee)

Comment 35

4 years ago
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.
Flags: needinfo?(jgilbert)
Flags: needinfo?(dglastonbury)
(Assignee)

Updated

4 years ago
Blocks: 1041830

Comment 36

4 years ago
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.
> 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.
Flags: needinfo?(dglastonbury)

Comment 38

4 years ago
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?
(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.
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.