Float formats are allowed for CopyTexImage with EXT_color_buffer_float

Float formats are allowed for CopyTexImage with EXT_color_buffer_float

BUG=612542
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[qiankun, 2016-11-02 05:51:32]

Description was changed from

==========
Float formats are allowed for CopyTexImage with EXT_color_buffer_float

BUG=612542
TEST=gpu_unittests
==========

to

==========
Float formats are allowed for CopyTexImage with EXT_color_buffer_float

BUG=612542
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[qiankun, 2016-11-02 10:30:04]

qiankun.miao@intel.com changed reviewers:
+ kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org

[qiankun, 2016-11-02 10:30:05]

PTAL.

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13699: GLenum read_format,
GLenum read_type) {
May I add an argument like "report_msg" to control report message or not and
move this function to public. ValidateCopyTexFormat can be used to check if
glCopyTexImage2D can be used for glCopyTextureCHROMIUM. If not, there will be
some duplicated code.

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/test_helper.cc:541: (gl_info.is_es3 &&
strstr(extensions, "GL_OES_texture_float") &&
GL_OES_texture_float is needed for this condition, see
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/feature_info....

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/test_helper.cc:594: if (status_rgba ==
GL_FRAMEBUFFER_COMPLETE) {
see discussion in https://codereview.chromium.org/2443553002/#msg7.
This is used to align with the actual executed code:
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/feature_info....

[Zhenyao Mo, 2016-11-02 18:26:08]

zmo@chromium.org changed reviewers:
+ kainino@chromium.org

[Zhenyao Mo, 2016-11-02 18:26:09]

lgtm

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13699: GLenum read_format,
GLenum read_type) {
On 2016/11/02 10:30:05, qiankun wrote:
> May I add an argument like "report_msg" to control report message or not and
> move this function to public. ValidateCopyTexFormat can be used to check if
> glCopyTexImage2D can be used for glCopyTextureCHROMIUM. If not, there will be
> some duplicated code.

Looks like this function always generates INVALID_OPERATION.  What about adding
a helper function that doesn't generate GL error, but return true/false with an
error message? Then another function to simply call the helper, and if false,
generate INVALID_OPERATION with the right message.

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
(right):

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:2702:
command_line.AppendSwitch(switches::kEnableUnsafeES3APIs);
Again, this will conflict with kainino's effort of removing this. Please
coordinate with him when landing the CL.

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:2769:
command_line.AppendSwitch(switches::kEnableUnsafeES3APIs);
Same here.

[Kai Ninomiya, 2016-11-02 18:47:34]

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
(right):

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:2702:
command_line.AppendSwitch(switches::kEnableUnsafeES3APIs);
On 2016/11/02 18:26:09, Zhenyao Mo wrote:
> Again, this will conflict with kainino's effort of removing this. Please
> coordinate with him when landing the CL.

qiankun, don't worry about using kEnableUnsafeES3APIs - it will be easy for me
to fix when I rebase.

[Ken Russell (switch to Gerrit), 2016-11-02 20:33:45]

I defer to zmo's review.

[Kai Ninomiya, 2016-11-03 02:11:31]

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
(right):

https://codereview.chromium.org/2474593002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:2702:
command_line.AppendSwitch(switches::kEnableUnsafeES3APIs);
On 2016/11/02 18:47:34, Kai Ninomiya wrote:
> On 2016/11/02 18:26:09, Zhenyao Mo wrote:
> > Again, this will conflict with kainino's effort of removing this. Please
> > coordinate with him when landing the CL.
> 
> qiankun, don't worry about using kEnableUnsafeES3APIs - it will be easy for me
> to fix when I rebase.

My changes removing it have landed:
https://codereview.chromium.org/2471853002/
https://codereview.chromium.org/2471913003/

[qiankun, 2016-11-03 09:58:11]

The CQ bit was checked by qiankun.miao@intel.com

[qiankun, 2016-11-03 09:58:11]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2474593002/#ps20001
(title: "rebase only")

[commit-bot: I haz the power, 2016-11-03 09:58:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 11:59:03]

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

[commit-bot: I haz the power, 2016-11-03 11:59:04]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[qiankun, 2016-11-03 15:21:37]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2016-11-03 15:22:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 15:27:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-03 15:41:00]

Description was changed from

==========
Float formats are allowed for CopyTexImage with EXT_color_buffer_float

BUG=612542
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Float formats are allowed for CopyTexImage with EXT_color_buffer_float

BUG=612542
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-11-03 15:41:01]

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