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

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.

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

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?

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

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.

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

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

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

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

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.

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.

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)

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

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

We're really tight on time for 37 at this point - we may have to settle for doing 38 uplift only.

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.

jgilbert can answer this better than I.

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

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

Comment on attachment 8567268 [details] [diff] [review] 0002-Fix-half-float-support.patch (see first patch)

Comment on attachment 8567269 [details] [diff] [review] 0003-Improve-test.patch (see first patch)

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.

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.