Relax multi-sampling for floating-point color renderbuffers

Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[jchen10, 2016-08-01 02:04:50]

Description was changed from

==========
Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
==========

to

==========
Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[jchen10, 2016-08-01 02:09:00]

Description was changed from

==========
Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[jchen10, 2016-08-01 02:09:00]

jie.a.chen@intel.com changed reviewers:
+ yunchao.he@intel.com

[yunchao, 2016-08-01 02:49:42]

Jie, could you correct the error below. I think it will make other tests fail.

https://codereview.chromium.org/2197893003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2197893003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:661:
floatType = true;
The fall-through code here will be executed for integer internalformat above. It
is not correct.

[jchen10, 2016-08-01 07:39:58]

On 2016/08/01 02:49:42, yunchao wrote:
> Jie, could you correct the error below. I think it will make other tests fail.
> 
>
https://codereview.chromium.org/2197893003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2197893003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:661:
> floatType = true;
> The fall-through code here will be executed for integer internalformat above.
It
> is not correct.

Thanks for review!
Please take a look once more!

[yunchao, 2016-08-01 08:11:05]

lgtm

[yunchao, 2016-08-01 08:12:55]

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

[yunchao, 2016-08-01 08:12:56]

@zmo and @kbr, PTAL. Thanks a lot!

[jchen10, 2016-08-01 08:13:48]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 08:13:57]

Dry run: 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-08-01 08:13:58]

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

[commit-bot: I haz the power, 2016-08-01 08:13:59]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[qiankun, 2016-08-02 02:47:55]

Looks good to me with little suggestion.

https://codereview.chromium.org/2197893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2197893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:636:
target, samples, internalformat, width, height);
Merge with line 635.

https://codereview.chromium.org/2197893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
(right):

https://codereview.chromium.org/2197893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:282: void
callRenderbufferStorage(GLenum target, GLsizei samples, GLenum internalformat,
GLsizei width, GLsizei height, const char* functionName);
FYI: callRenderbufferStorage -> renderbufferStorageHelper?

[jchen10, 2016-08-02 03:19:48]

On 2016/08/02 02:47:55, qiankun wrote:
> Looks good to me with little suggestion.
> 
>
https://codereview.chromium.org/2197893003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2197893003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:636:
> target, samples, internalformat, width, height);
> Merge with line 635.
> 
>
https://codereview.chromium.org/2197893003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
> (right):
> 
>
https://codereview.chromium.org/2197893003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h:282: void
> callRenderbufferStorage(GLenum target, GLsizei samples, GLenum internalformat,
> GLsizei width, GLsizei height, const char* functionName);
> FYI: callRenderbufferStorage -> renderbufferStorageHelper?

Fixed, thanks for review!

[Ken Russell (switch to Gerrit), 2016-08-03 00:58:50]

Thanks for relaxing this. LGTM.

Could you please ensure that the WebGL 2.0 conformance tests cover this
functionality? Either the ported dEQP tests, or
conformance2/extensions/ext-color-buffer-float.html, or both? Thanks.

https://codereview.chromium.org/2197893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2197893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:635:
LOG(ERROR) << "RenderbufferStorageMultisampleCHROMIUM: samples:" << samples <<
", format:" << internalformat;
Please remove debug logging.

[jchen10, 2016-08-03 01:23:49]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 01:24:07]

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

[yunchao, 2016-08-03 01:43:39]

On 2016/08/03 00:58:50, Ken Russell wrote:
> Thanks for relaxing this. LGTM.
> 
> Could you please ensure that the WebGL 2.0 conformance tests cover this
> functionality? Either the ported dEQP tests, or
> conformance2/extensions/ext-color-buffer-float.html, or both? Thanks.
> 
>
https://codereview.chromium.org/2197893003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2197893003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:635:
> LOG(ERROR) << "RenderbufferStorageMultisampleCHROMIUM: samples:" << samples <<
> ", format:" << internalformat;
> Please remove debug logging.

Ken, thanks for your review. We already have a test case in deqp to cover this
feature. I have a pr with some discussions, could you have a look at
https://github.com/KhronosGroup/WebGL/pull/1946? Seems that @zmo is vacation
these two days...

[commit-bot: I haz the power, 2016-08-03 02:05:08]

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

[commit-bot: I haz the power, 2016-08-03 02:05:09]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jchen10, 2016-08-03 05:05:17]

The CQ bit was unchecked by jie.a.chen@intel.com

[jchen10, 2016-08-03 05:05:48]

The CQ bit was checked by jie.a.chen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 05:05:58]

Dry run: 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-08-03 06:46:49]

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

[commit-bot: I haz the power, 2016-08-03 06:46:50]

Dry run: This issue passed the CQ dry run.

[jchen10, 2016-08-03 06:50:22]

The CQ bit was checked by jie.a.chen@intel.com

[jchen10, 2016-08-03 06:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
yunchao.he@intel.com, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2197893003/#ps60001
(title: "Remove debug logging")

[commit-bot: I haz the power, 2016-08-03 06:50:30]

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-08-03 06:53:56]

Description was changed from

==========
Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-08-03 06:53:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-03 06:55:52]

Description was changed from

==========
Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Relax multi-sampling for floating-point color renderbuffers

It's now optionally allowed to multi-sample the renderbuffers of float-point
color using Ext_color_buffer_float extension against WebGL2. This CL simply
relaxes it.

BUG=633022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-08-03 06:55:53]

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