Fix ReadPixels from float fbo buffer in ES2/WebGL1.

Fix ReadPixels from float fbo buffer in ES2/WebGL1.

This CL change the default format/type from RGBA/UNSIGNED_BYTE to RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/f3640b929df962d140b9314275eaa9d91dea2ac8
Cr-Commit-Position: refs/heads/master@{#390499}

[Zhenyao Mo, 2016-04-27 23:50:48]

Description was changed from

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

The default format/type is set to RGBA/UNSIGNED_BYTE instead of
RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
==========

to

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

The default format/type is set to RGBA/UNSIGNED_BYTE instead of
RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-04-27 23:50:48]

Description was changed from

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

The default format/type is set to RGBA/UNSIGNED_BYTE instead of
RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

The default format/type is set to RGBA/UNSIGNED_BYTE instead of
RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-04-27 23:51:07]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 23:51:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1927593005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1927593005/1

[Zhenyao Mo, 2016-04-27 23:55:19]

piman: command_buffer and ui/gl
kbr, bajones: blink

PTAL

[piman, 2016-04-28 02:18:53]

lgtm

[commit-bot: I haz the power, 2016-04-28 03:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-28 03:00:36]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[Ken Russell (switch to Gerrit), 2016-04-28 13:31:35]

Thanks for fixing this Mo.

The CL description says:

"The default format/type is set to RGBA/UNSIGNED_BYTE instead of
RGBA/FLOAT."

Is this backward? The goal should be:

- For RGBA32F framebuffers, the default accepted readback format/type should be
RGBA/FLOAT, and the implementation color read format/type should be
RGBA/UNSIGNED_BYTE.

- For RGBA16F framebuffers, the default accepted readback format/type should be
RGBA/FLOAT, and the implementation color read format/type should be
RGBA/HALF_FLOAT_OES.

Do you agree?

What behavior does this CL implement?

[Zhenyao Mo, 2016-04-28 17:21:14]

On 2016/04/28 13:31:35, Ken Russell OOO till 5-2-2016 wrote:
> Thanks for fixing this Mo.
> 
> The CL description says:
> 
> "The default format/type is set to RGBA/UNSIGNED_BYTE instead of
> RGBA/FLOAT."
> 
> Is this backward? The goal should be:
> 
> - For RGBA32F framebuffers, the default accepted readback format/type should
be
> RGBA/FLOAT, and the implementation color read format/type should be
> RGBA/UNSIGNED_BYTE.
> 
> - For RGBA16F framebuffers, the default accepted readback format/type should
be
> RGBA/FLOAT, and the implementation color read format/type should be
> RGBA/HALF_FLOAT_OES.
> 
> Do you agree?
> 
> What behavior does this CL implement?

I don't think the implementation color read format/type you mentioned is
guaranteed to be supported on ES2/ES3.

So the current implementation always try to query the driver first (everywhere),
and if an error occurs, indicating such query isn't supported, then we fall back
to internal selection of the second pair.

For half float, our fallback is RGBA/HALF_FLOAT_OES,
For float, our fallback is RGBA/FLOAT (this is the same as the primary pair, I
can change it to RGBA/UNSIGNED_BYTE)

Does this sound acceptable?

[Zhenyao Mo, 2016-04-28 17:25:51]

Description was changed from

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

The default format/type is set to RGBA/UNSIGNED_BYTE instead of
RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

This CL change the default format/type from RGBA/UNSIGNED_BYTE to RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-04-28 17:56:14]

On 2016/04/28 17:21:14, Zhenyao Mo wrote:
> On 2016/04/28 13:31:35, Ken Russell OOO till 5-2-2016 wrote:
> > Thanks for fixing this Mo.
> > 
> > The CL description says:
> > 
> > "The default format/type is set to RGBA/UNSIGNED_BYTE instead of
> > RGBA/FLOAT."
> > 
> > Is this backward? The goal should be:
> > 
> > - For RGBA32F framebuffers, the default accepted readback format/type should
> be
> > RGBA/FLOAT, and the implementation color read format/type should be
> > RGBA/UNSIGNED_BYTE.
> > 
> > - For RGBA16F framebuffers, the default accepted readback format/type should
> be
> > RGBA/FLOAT, and the implementation color read format/type should be
> > RGBA/HALF_FLOAT_OES.
> > 
> > Do you agree?
> > 
> > What behavior does this CL implement?
> 
> I don't think the implementation color read format/type you mentioned is
> guaranteed to be supported on ES2/ES3.

That's possible. I'd just like to point out the default read format/type
behavior again:

For ES3 and WebGL 2.0, the behavior is defined by EXT_color_buffer_float, and
https://www.khronos.org/registry/gles/extensions/EXT/EXT_color_buffer_float.txt
says:
  "For floating-point rendering surfaces, the combination format RGBA and type
FLOAT is accepted."

For WebGL 1.0, the behavior's effectively defined by
https://www.khronos.org/registry/webgl/extensions/EXT_color_buffer_half_float/
and https://www.khronos.org/registry/webgl/extensions/WEBGL_color_buffer_float/
. These also say that RGBA/FLOAT is the default accepted format/type for
readbacks from floating-point frame buffers. (These extensions also state that
RGBA/UNSIGNED_BYTE can't be used, which I think has to be removed from these
extensions, since the implementation color read format and type should allow
this.)

> So the current implementation always try to query the driver first
(everywhere),
> and if an error occurs, indicating such query isn't supported, then we fall
back
> to internal selection of the second pair.
> 
> For half float, our fallback is RGBA/HALF_FLOAT_OES,
> For float, our fallback is RGBA/FLOAT (this is the same as the primary pair, I
> can change it to RGBA/UNSIGNED_BYTE)
> 
> Does this sound acceptable?

Yes. I've been told this is what Firefox does; RGBA/UNSIGNED_BYTE are used as
the implementation color read format and type for FP32 framebuffers. I also
think that that combination has to keep working on desktop to avoid breaking web
sites.

The conformance tests will need to be rolled forward in order for this CL to
pass the newly strengthened oes-texture-half-float.html test.

This CL LGTM if this is the implemented behavior.

[Zhenyao Mo, 2016-04-28 19:32:40]

The conformance test is already rolled in. With this CL, it begins to pass on
everywhere but Windows - ANGLE hasn't been updated to allow float type
ReadPixels.

[Zhenyao Mo, 2016-04-28 19:32:55]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2016-04-28 19:32:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1927593005/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-04-28 19:33:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1927593005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1927593005/20001

[commit-bot: I haz the power, 2016-04-28 22:04:52]

Description was changed from

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

This CL change the default format/type from RGBA/UNSIGNED_BYTE to RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

This CL change the default format/type from RGBA/UNSIGNED_BYTE to RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-04-28 22:04:54]

Committed patchset #2 (id:20001)

[pasko, 2016-04-29 17:26:16]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1929203003/ by pasko@chromium.org.

The reason for reverting is: Reason: crbug.com/607936

(GLReadbackTest.ReadPixelsFloat fails on "Lollipop Tablet Tester").

[commit-bot: I haz the power, 2016-04-30 17:21:50]

Patchset 2 (id:??) landed as
https://crrev.com/f3640b929df962d140b9314275eaa9d91dea2ac8
Cr-Commit-Position: refs/heads/master@{#390499}

[commit-bot: I haz the power, 2016-04-30 17:57:50]

Description was changed from

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

This CL change the default format/type from RGBA/UNSIGNED_BYTE to RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix ReadPixels from float fbo buffer in ES2/WebGL1.

This CL change the default format/type from RGBA/UNSIGNED_BYTE to RGBA/FLOAT.

Also, simplify querying IMPLMENTATION_READ_COLOR_FORMAT/TYPE by
always calling down to the driver (even if the two enums are not
supported) and fall back to internal decision if a GL error is
generated.

BUG=607283
TEST=webgl_conformance,webgl2_conformance
R=piman@chromium.org,kbr@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/f3640b929df962d140b9314275eaa9d91dea2ac8
Cr-Commit-Position: refs/heads/master@{#390499}
==========