Support EXT_color_buffer_half_float on ANGLE

RESOLVED FIXED in Firefox 39

Status

()

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

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

(Blocks: 1 bug)

38 Branch
mozilla39
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 wontfix, firefox39 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
Based on the code comment, we only don't support this because ANGLE doesn't allow ReadPixels from half-float sources with type FLOAT. (which is reasonable)

If ANGLE supports ReadPixels with HALF_FLOAT, we can just read that, then convert to FLOAT. Slow, but "don't use readback".
(Assignee)

Updated

3 years ago
Blocks: 1124187

Comment 1

3 years ago
Hmmm, I vaguely remember half-float render targets being "write-only" as there is no corresponding half-float typed array type on JS side.

Most common use case of half-float render targets is anyways just using them all the time on the GPU side: you write into them via shader and read into another shader, never using ReadPixels.

Not sure how that's dealt with in Chrome, if ReadPixels is simply not allowed / returning null, or if there is some slow emulation path via floats.

Both options would be valid solutions. Half-float render targets are very useful even with non-functioning / very slow ReadPixels.
(Assignee)

Comment 2

3 years ago
(In reply to AlteredQualia from comment #1)
> Hmmm, I vaguely remember half-float render targets being "write-only" as
> there is no corresponding half-float typed array type on JS side.
> 
> Most common use case of half-float render targets is anyways just using them
> all the time on the GPU side: you write into them via shader and read into
> another shader, never using ReadPixels.
> 
> Not sure how that's dealt with in Chrome, if ReadPixels is simply not
> allowed / returning null, or if there is some slow emulation path via floats.
> 
> Both options would be valid solutions. Half-float render targets are very
> useful even with non-functioning / very slow ReadPixels.

We use Uint16Arrays for half-floats in WebGL. Unfortunately, which you can probably ReadPixels with HALF_FLOAT type with the implementation-given aux format/types. (Read into a Uint16Array)

Unfortunately, the GLES extension specifies that it's only guaranteed to be readable with RGBA/FLOAT, so we sort of ought to support that. ANGLE does *not* allow this, so we just need an emulation path, which is fine. (In theory, we could GL_OOM or something, but that's not really good-spirited :))
(Assignee)

Comment 3

3 years ago
Created attachment 8567267 [details] [diff] [review]
0001-Repair-and-test-WEBGL_color_buffer_float.patch
Attachment #8567267 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

3 years ago
Created attachment 8567268 [details] [diff] [review]
0002-Fix-half-float-support.patch
Attachment #8567268 - Flags: review?(jmuizelaar)
(Assignee)

Comment 5

3 years ago
Created attachment 8567269 [details] [diff] [review]
0003-Improve-test.patch
Attachment #8567269 - Flags: review?(jmuizelaar)
Comment on attachment 8567267 [details] [diff] [review]
0001-Repair-and-test-WEBGL_color_buffer_float.patch

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

::: gfx/angle/src/libGLESv2/libGLESv2.cpp
@@ +295,4 @@
>  
>      if (context)
>      {
> +        //context->getState().setBlendColor(gl::clamp01(red), gl::clamp01(green), gl::clamp01(blue), gl::clamp01(alpha));

Do you want to keep the commented stuff here?
Attachment #8567267 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8567268 [details] [diff] [review]
0002-Fix-half-float-support.patch

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1868,5 @@
> +{
> +    size_t aligned = unaligned;
> +    while (aligned % alignment != 0)
> +        aligned++;
> +    return aligned;

how about:
tmp = unaligned + alignment - 1;
return tmp - (tmp % alignment);

@@ +1886,5 @@
> +        readType == LOCAL_GL_HALF_FLOAT &&
> +        destFormat == LOCAL_GL_RGBA &&
> +        destType == LOCAL_GL_FLOAT)
> +    {
> +        size_t readBytesPerPixel = 2 * 4;

sizeof(uint16_t)*4

@@ +1887,5 @@
> +        destFormat == LOCAL_GL_RGBA &&
> +        destType == LOCAL_GL_FLOAT)
> +    {
> +        size_t readBytesPerPixel = 2 * 4;
> +        size_t destBytesPerPixel = 4 * 4;

sizeof(float)*4

@@ +2175,5 @@
>          if (!subrect_data)
>              return ErrorOutOfMemory("readPixels: subrect_data");
>  
> +        //gl->fReadPixels(subrect_x, subrect_y, subrect_width, subrect_height,
> +        //                format, type, subrect_data.get());

Worth adding a comment here about why the commented out code is there
Attachment #8567268 - Flags: review?(jmuizelaar) → review+
Attachment #8567269 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 8

3 years ago
Comment on attachment 8567267 [details] [diff] [review]
0001-Repair-and-test-WEBGL_color_buffer_float.patch

This touches webidl, so we need DOM peer sign-off.
Attachment #8567267 - Flags: review?(khuey)
Attachment #8567267 - Flags: review?(khuey) → review+
(Assignee)

Comment 9

3 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/110d66c1fd6e

Comment 10

3 years ago
Will fixing this bug resolve all the conformance/extensions/oes-texture-half-float-with-image-data.html failures?

https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/oes-texture-half-float-with-image-data.html?webglVersion=1
https://hg.mozilla.org/mozilla-central/rev/110d66c1fd6e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 12

3 years ago
(In reply to Luke from comment #10)
> Will fixing this bug resolve all the
> conformance/extensions/oes-texture-half-float-with-image-data.html failures?
> 
> https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/oes-
> texture-half-float-with-image-data.html?webglVersion=1

I do not know. I only tested with the test file included (though not activated) in this patch.

Comment 13

3 years ago
Yes, not only are the real world demos working, but all of the related Conformance tests are passing now too. Nice work team!

Comment 14

3 years ago
This patch only fixed the oes-texture-half-float-with-image-data.html tests. The oes-texture-half-float-with-video.html are still failing. I opened up Bug 1137494 for that those.
Attachment #8567268 - Flags: review?(dglastonbury)
Attachment #8567269 - Flags: review?(dglastonbury)
Let's get a second round of reviews, and if we feel good about it, ask for the Aurora/38 and Beta/37 uplift.  Needinfo just as a heads up to Jeff.
Flags: needinfo?(jgilbert)

Comment 16

3 years ago
I can confirm my deferred rendering demos are now working in current Nightly. Yay! Thanks.

Performance though seems to be quite low - just about half of what Chrome Canary gets with DX11 ANGLE:

http://alteredqualia.com/xg/examples/deferred_skin.html (36 vs 60 fps)
http://alteredqualia.com/xg/examples/animation_physics_terrain.html (27 vs 48 fps)
http://alteredqualia.com/xg/examples/animation_physics_ammo.html (22-27 fps vs 44-60 fps)
(Assignee)

Comment 17

3 years ago
(In reply to AlteredQualia from comment #16)
> I can confirm my deferred rendering demos are now working in current
> Nightly. Yay! Thanks.
> 
> Performance though seems to be quite low - just about half of what Chrome
> Canary gets with DX11 ANGLE:
> 
> http://alteredqualia.com/xg/examples/deferred_skin.html (36 vs 60 fps)
> http://alteredqualia.com/xg/examples/animation_physics_terrain.html (27 vs
> 48 fps)
> http://alteredqualia.com/xg/examples/animation_physics_ammo.html (22-27 fps
> vs 44-60 fps)

This is likely e10s. With browser.tabs.remote.autostart.1 set to false, followed by a browser restart, you should see improved perf.
Flags: needinfo?(jgilbert)

Comment 18

3 years ago
@Jeff Thanks a lot for the suggestion. Turning off e10s radically improved performance: Nightly has now the same speed as Canary. Excellent. This is a historical milestone (at least on my computer).
Comment on attachment 8567268 [details] [diff] [review]
0002-Fix-half-float-support.patch

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1868,5 @@
> +{
> +    size_t aligned = unaligned;
> +    while (aligned % alignment != 0)
> +        aligned++;
> +    return aligned;

If alignment is going to be power of two, can't this become a masking operation?

::: dom/canvas/WebGLExtensionColorBufferHalfFloat.cpp
@@ +26,5 @@
>      gl::GLContext* gl = webgl->GL();
>  
>      // ANGLE doesn't support ReadPixels from a RGBA16F with RGBA/FLOAT.
> +    return gl->IsSupported(gl::GLFeature::renderbuffer_color_half_float) ||
> +           gl->IsANGLE();

Does the comment make sense? Extension is enabled if ANGLE, but it needs to be read as RGBA/HALF_FLOAT?!

::: dom/canvas/test/webgl-mochitest/test_webgl_color_buffer_float.html
@@ +312,2 @@
>      TestScreenColor(gl, fbType, 0, 1.5, 0.5, 1);
> +  } else

If one clause has {}s, should all clauses.

@@ +322,5 @@
>  
>    if (fbType == gl.UNSIGNED_BYTE)
>      TestScreenColor(gl, fbType, 0, 0.5, 1.0, 1);
> +  else if (fbType == FLOAT ||
> +           fbType == HALF_FLOAT_OES)

This could go all go on one line and be and still be fewer than 80 chars?
Attachment #8567268 - Flags: review?(dglastonbury) → review+
Attachment #8567269 - Flags: review?(dglastonbury) → review+
We're really tight on time for 37 at this point - we may have to settle for doing 38 uplift only.
Flags: needinfo?(jgilbert)
Jeff & Jeff, is this something we should release note?  It sort of feels like it is, but I wouldn't be able to word it, so let me know what you think.
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(jgilbert)
Flags: needinfo?(jgilbert)
jgilbert can answer this better than I.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 23

3 years ago
(In reply to Milan Sreckovic [:milan] from comment #21)
> Jeff & Jeff, is this something we should release note?  It sort of feels
> like it is, but I wouldn't be able to word it, so let me know what you think.

I'm not very familiar with how relnotes are phrased, but something like:
"Support rendering to float32 and float16 framebuffers in WebGL."
Flags: needinfo?(jgilbert)
(Assignee)

Comment 24

3 years ago
Comment on attachment 8567267 [details] [diff] [review]
0001-Repair-and-test-WEBGL_color_buffer_float.patch

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: No float16 rendering on Windows.
[Describe test coverage new/current, TreeHerder]: test included, on 39+.
[Risks and why]: No risk, simple patch exposing simple functionality.
[String/UUID change made/needed]: none
Attachment #8567267 - Flags: approval-mozilla-beta?
(Assignee)

Comment 25

3 years ago
Comment on attachment 8567268 [details] [diff] [review]
0002-Fix-half-float-support.patch

(see first patch)
Attachment #8567268 - Flags: approval-mozilla-beta?
(Assignee)

Comment 26

3 years ago
Comment on attachment 8567269 [details] [diff] [review]
0003-Improve-test.patch

(see first patch)
Attachment #8567269 - Flags: approval-mozilla-beta?
Comment on attachment 8567267 [details] [diff] [review]
0001-Repair-and-test-WEBGL_color_buffer_float.patch

I am sorry but this is too late for 38 too.
We only take stability patches now.
This will be part of 39.
Attachment #8567267 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox38: --- → wontfix
Attachment #8567268 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8567269 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(Assignee)

Comment 28

3 years ago
I will note that if the memory usage hit is acceptable, devs should be able to use float32s instead of float16s on Windows for WebGL.
You need to log in before you can comment on or make changes to this bug.