Add EXT_color_buffer_half_float extension support in GPU commandbuffer service.

Add EXT_color_buffer_half_float extension support in GPU commandbuffer service.

BUG=258347

[Jun Jiang, 2013-07-09 10:28:19]

Add EXT_color_buffer_half_float extension support in GPU commandbuffer service. 
For the function GetTexType(GLenum type) added in gles_cmd_decoder.cc for
glReadPixels(), it is a temporary usage. It may be moved to ui/gl/ further or
another patch is needed to refine the validation and mapping of the type for
glReadPixels.

[junj, 2013-07-10 00:17:01]

On 2013/07/09 10:28:19, Jun Jiang wrote:
> Add EXT_color_buffer_half_float extension support in GPU commandbuffer
service. 
> For the function GetTexType(GLenum type) added in gles_cmd_decoder.cc for
> glReadPixels(), it is a temporary usage. It may be moved to ui/gl/ further or
> another patch is needed to refine the validation and mapping of the type for
> glReadPixels.

Add more GPU try bots to run EXT_color_buffer_half_float_unittest.cc.

[Ken Russell (switch to Gerrit), 2013-07-10 02:14:24]

https://codereview.chromium.org/18246005/diff/1/gpu/command_buffer/service/fe...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/18246005/diff/1/gpu/command_buffer/service/fe...
gpu/command_buffer/service/feature_info.cc:465:
validators_.read_pixel_type.AddValue(GL_FLOAT);
There's a general problem with this patch and the associated ANGLE one that it
combines EXT_color_buffer_half_float with rendering to and reading back from
FP32 targets. These must be split up. Another problem is that
http://www.khronos.org/registry/webgl/extensions/WEBGL_color_buffer_float/ is a
WebGL extension, but there's no underlying extension defined against OpenGL ES
2.0. EXT_color_buffer_float is defined against ES 3.0, so when ANGLE supports ES
3.0, it can just start supporting EXT_color_buffer_float.

I think the necessary path forward is:

 - Define an ANGLE_color_buffer_float extension (perhaps just in the ANGLE
repository; I don't see an advantage to pushing this up to Khronos at this
point, given that ES 3.0 is their focus)
 - Implement that in the current ANGLE
 - Expose two extensions here, EXT_color_buffer_half_float and
ANGLE_color_buffer_float
 - Later, in WebGLRenderingContext, expose WEBGL_color_buffer_float if the
underlying ANGLE_color_buffer_float extension is supported

What do you think?

[Jun Jiang, 2013-07-10 12:32:13]

On 2013/07/10 02:14:24, Ken Russell wrote:
>
https://codereview.chromium.org/18246005/diff/1/gpu/command_buffer/service/fe...
> File gpu/command_buffer/service/feature_info.cc (right):
> 
>
https://codereview.chromium.org/18246005/diff/1/gpu/command_buffer/service/fe...
> gpu/command_buffer/service/feature_info.cc:465:
> validators_.read_pixel_type.AddValue(GL_FLOAT);
> There's a general problem with this patch and the associated ANGLE one that it
> combines EXT_color_buffer_half_float with rendering to and reading back from
> FP32 targets. These must be split up. Another problem is that
> http://www.khronos.org/registry/webgl/extensions/WEBGL_color_buffer_float/ is
a
> WebGL extension, but there's no underlying extension defined against OpenGL ES
> 2.0. EXT_color_buffer_float is defined against ES 3.0, so when ANGLE supports
ES
> 3.0, it can just start supporting EXT_color_buffer_float.
> 
> I think the necessary path forward is:
> 
>  - Define an ANGLE_color_buffer_float extension (perhaps just in the ANGLE
> repository; I don't see an advantage to pushing this up to Khronos at this
> point, given that ES 3.0 is their focus)
>  - Implement that in the current ANGLE
>  - Expose two extensions here, EXT_color_buffer_half_float and
> ANGLE_color_buffer_float
>  - Later, in WebGLRenderingContext, expose WEBGL_color_buffer_float if the
> underlying ANGLE_color_buffer_float extension is supported
> 
> What do you think?

Hi, Kenneth. Thanks for your comments. 
You're right.
EXT_color_buffer_half_float(http://www.khronos.org/registry/gles/extensions/EXT/EXT_color_buffer_half_float.txt)
only allows reading back in GL_HALF_FLOAT_OES type, not FP32 type. WebGL
EXT_color_buffer_half_float extension
(http://www.khronos.org/registry/webgl/extensions/WEBGL_color_buffer_float/)
requires to read RGBA/FLOAT data from Framebuffer. I should not expect reading
back FP32 type is legal with only EXT_color_buffer_half_float supported. Instead
I should check if EXT_color_buffer_float or similar extension is supported.
Otherwise, a HALF_FLOAT to FLOAT type conversion is needed to read back
RGBA/FLOAT from framebuffer attachment which is of HALF_FLOAT_OES type.
And I will continue the remaining work:
1. Another patch for ANGLE_color_buffer_float extension should be added in
ANGLE.
2. The code for EXT_color_buffer_half_float should be refined to check the
existence of ANGLE_color_buffer_float when reading back FP32 type. If not, a
FP16 to FP32 conversion is needed.
3. expose ANGLE_color_buffer_float extension in GPU commandbuffer and a new
patch to support WEBGL_color_buffer_float is needed at Blink.

[Jun Jiang, 2013-07-11 15:59:57]

On 2013/07/10 12:32:13, Jun Jiang wrote:
> On 2013/07/10 02:14:24, Ken Russell wrote:
> >
>
https://codereview.chromium.org/18246005/diff/1/gpu/command_buffer/service/fe...
> > File gpu/command_buffer/service/feature_info.cc (right):
> > 
> >
>
https://codereview.chromium.org/18246005/diff/1/gpu/command_buffer/service/fe...
> > gpu/command_buffer/service/feature_info.cc:465:
> > validators_.read_pixel_type.AddValue(GL_FLOAT);
> > There's a general problem with this patch and the associated ANGLE one that
it
> > combines EXT_color_buffer_half_float with rendering to and reading back from
> > FP32 targets. These must be split up. Another problem is that
> > http://www.khronos.org/registry/webgl/extensions/WEBGL_color_buffer_float/
is
> a
> > WebGL extension, but there's no underlying extension defined against OpenGL
ES
> > 2.0. EXT_color_buffer_float is defined against ES 3.0, so when ANGLE
supports
> ES
> > 3.0, it can just start supporting EXT_color_buffer_float.
> > 
> > I think the necessary path forward is:
> > 
> >  - Define an ANGLE_color_buffer_float extension (perhaps just in the ANGLE
> > repository; I don't see an advantage to pushing this up to Khronos at this
> > point, given that ES 3.0 is their focus)
> >  - Implement that in the current ANGLE
> >  - Expose two extensions here, EXT_color_buffer_half_float and
> > ANGLE_color_buffer_float
> >  - Later, in WebGLRenderingContext, expose WEBGL_color_buffer_float if the
> > underlying ANGLE_color_buffer_float extension is supported
> > 
> > What do you think?
> 
> Hi, Kenneth. Thanks for your comments. 
> You're right.
>
EXT_color_buffer_half_float(http://www.khronos.org/registry/gles/extensions/EXT/EXT_color_buffer_half_float.txt)
> only allows reading back in GL_HALF_FLOAT_OES type, not FP32 type. WebGL
> EXT_color_buffer_half_float extension
> (http://www.khronos.org/registry/webgl/extensions/WEBGL_color_buffer_float/)
> requires to read RGBA/FLOAT data from Framebuffer. I should not expect reading
> back FP32 type is legal with only EXT_color_buffer_half_float supported.
Instead
> I should check if EXT_color_buffer_float or similar extension is supported.
> Otherwise, a HALF_FLOAT to FLOAT type conversion is needed to read back
> RGBA/FLOAT from framebuffer attachment which is of HALF_FLOAT_OES type.
> And I will continue the remaining work:
> 1. Another patch for ANGLE_color_buffer_float extension should be added in
> ANGLE.
> 2. The code for EXT_color_buffer_half_float should be refined to check the
> existence of ANGLE_color_buffer_float when reading back FP32 type. If not, a
> FP16 to FP32 conversion is needed.
> 3. expose ANGLE_color_buffer_float extension in GPU commandbuffer and a new
> patch to support WEBGL_color_buffer_float is needed at Blink.

I have re-checked the spec at
http://www.khronos.org/registry/gles/extensions/EXT/EXT_color_buffer_half_flo...
and it seems some of my above statement are wrong. Reading RGBA/FLOAT should be
always allowed when the attachment is RGBA16F_EXT or RGB16F_EXT. On the other
hand, RGBA/OES_HALF_FLOAT may not be allowed on the RGBA16F_EXT attachment. 
The spec mentions only two combination pairs of format/type is allowed. The
first is RGBA/UNSIGNED_BYTE for non-float attachments or RGBA/FLOAT for float
attachments. The second is implementation-chosen format and may be queried via
glGet with pname "IMPLEMENTATION_COLOR_READ_FORMAT" and
"IMPLEMENTATION_COLOR_READ_TYPE". So for RGBA16F_EXT attachment, the
implementation-chose type may be HALF_FLOAT_OES but not constantly.
The format and type check in GPU command buffer doesn't obey this rule as well
and Gregg has mention that it should be fixed before. I have made a patch to fix
the checking on format/type pairs for glReadPixels and did some basic test on
Linux and Win7. It works but needs more testing.
Moreover, I have tried to enable "GL_ANGLE_color_buffer_float" extension on win7
and it seems FP32 rendering was only valid for DX9 but not for DX11. On my win7
using DX11, only R32_FLOAT and R32G32_FLOAT can be rendered to when querying the
device via mDevice->CheckFormatSupport(). I will clean up the code for
"GL_ANGLE_color_buffer_float" and "GL_EXT_color_buffer_half_float" together and
send to ANGLE for review. Again, more testing is needed before the review.
It seems to me there are several patches here. We'd better to make it one by one
to make sure it is well tested.

[Jun Jiang, 2013-07-14 05:14:55]

Thanks for Kenneth's and zmo's comments and I have uploaded the related patch
with ANGLE for review at https://codereview.appspot.com/11238044/. I have
updated the patch at GPU commandbuffer service(specially, the test cases) in
this issue as well. Please take another look.

[Jun Jiang, 2013-07-15 03:23:09]

On 2013/07/14 05:14:55, Jun Jiang wrote:
> Thanks for Kenneth's and zmo's comments and I have uploaded the related patch
> with ANGLE for review at https://codereview.appspot.com/11238044/. I have
> updated the patch at GPU commandbuffer service(specially, the test cases) in
> this issue as well. Please take another look.

The compile failure for linux_gpu bot is not introduced by this patch. And it
seems the linux_gpu bot can not build successfully for a long time.

[Ken Russell (switch to Gerrit), 2013-07-15 20:29:16]

+jbauman as reviewer

Let's please make sure that https://codereview.appspot.com/11238044/ lands
before this one. It's important that the GL_ANGLE_color_buffer_float semantics
be defined before referencing the extension.

[Ken Russell (switch to Gerrit), 2013-07-15 21:01:30]

On 2013/07/11 15:59:57, Jun Jiang wrote:
> I have re-checked the spec at
>
http://www.khronos.org/registry/gles/extensions/EXT/EXT_color_buffer_half_flo...
> and it seems some of my above statement are wrong. Reading RGBA/FLOAT should
be
> always allowed when the attachment is RGBA16F_EXT or RGB16F_EXT. On the other
> hand, RGBA/OES_HALF_FLOAT may not be allowed on the RGBA16F_EXT attachment. 
> The spec mentions only two combination pairs of format/type is allowed. The
> first is RGBA/UNSIGNED_BYTE for non-float attachments or RGBA/FLOAT for float
> attachments. The second is implementation-chosen format and may be queried via
> glGet with pname "IMPLEMENTATION_COLOR_READ_FORMAT" and
> "IMPLEMENTATION_COLOR_READ_TYPE". So for RGBA16F_EXT attachment, the
> implementation-chose type may be HALF_FLOAT_OES but not constantly.
> The format and type check in GPU command buffer doesn't obey this rule as well
> and Gregg has mention that it should be fixed before. I have made a patch to
fix
> the checking on format/type pairs for glReadPixels and did some basic test on
> Linux and Win7. It works but needs more testing.

Thanks for re-checking this behavior.

Unfortunate that the ReadPixels format/type checking in the command buffer
service has been incorrect for this long for FP framebuffers. There's probably a
lot of WebGL code that does readbacks from the GPU from floating-point
framebuffers expecting that RGBA/UNSIGNED_BYTE readbacks work. That probably
implies that that will continue to have to work for some time -- and that the
IMPLEMENTATION_COLOR_READ_FORMAT/TYPE will have to be RGBA/UNSIGNED_BYTE for
this case.

Let's move forward very carefully and precisely with this and the other CLs.
Extensive tests are needed.

[Jun Jiang, 2013-07-16 01:24:20]

On 2013/07/15 21:01:30, Ken Russell wrote:
> On 2013/07/11 15:59:57, Jun Jiang wrote:
> > I have re-checked the spec at
> >
>
http://www.khronos.org/registry/gles/extensions/EXT/EXT_color_buffer_half_flo...
> > and it seems some of my above statement are wrong. Reading RGBA/FLOAT should
> be
> > always allowed when the attachment is RGBA16F_EXT or RGB16F_EXT. On the
other
> > hand, RGBA/OES_HALF_FLOAT may not be allowed on the RGBA16F_EXT attachment. 
> > The spec mentions only two combination pairs of format/type is allowed. The
> > first is RGBA/UNSIGNED_BYTE for non-float attachments or RGBA/FLOAT for
float
> > attachments. The second is implementation-chosen format and may be queried
via
> > glGet with pname "IMPLEMENTATION_COLOR_READ_FORMAT" and
> > "IMPLEMENTATION_COLOR_READ_TYPE". So for RGBA16F_EXT attachment, the
> > implementation-chose type may be HALF_FLOAT_OES but not constantly.
> > The format and type check in GPU command buffer doesn't obey this rule as
well
> > and Gregg has mention that it should be fixed before. I have made a patch to
> fix
> > the checking on format/type pairs for glReadPixels and did some basic test
on
> > Linux and Win7. It works but needs more testing.
> 
> Thanks for re-checking this behavior.
> 
> Unfortunate that the ReadPixels format/type checking in the command buffer
> service has been incorrect for this long for FP framebuffers. There's probably
a
> lot of WebGL code that does readbacks from the GPU from floating-point
> framebuffers expecting that RGBA/UNSIGNED_BYTE readbacks work. That probably
> implies that that will continue to have to work for some time -- and that the
> IMPLEMENTATION_COLOR_READ_FORMAT/TYPE will have to be RGBA/UNSIGNED_BYTE for
> this case.
> 
> Let's move forward very carefully and precisely with this and the other CLs.
> Extensive tests are needed.

Hi, Kenneth. Thanks for your comments. I have prepared another patch to handle
ReadPixles format/type checking and your suggestions is of great help to make it
for better compatibility.
I also noticed that some drivers report RGBA/UNSIGNED_BYTE as the
implementation-chosen format/type pair for floating-point attachments when
querying pname by "IMPLEMENTATION_COLOR_READ_FORMAT/TYPE", as is the case in my
Ubuntu 12.04 (Mesa 8.0.4/Intel HD4000). I am not sure whether the existing WebGL
code expecting RGBA/UNSIGNED_BYTE readbacks from FP attachments has some
knowledge on the underlying GL drivers implementation.
I will collect more information on this and upload the patch for review some
time.

[Ken Russell (switch to Gerrit), 2013-07-31 01:56:26]

I'm going to take the liberty of closing this issue to get it off my review
queue since we're not planning to do it right now.