command_buffer: Add support for creating non-WebGL ES 3 contexts

command_buffer: Add support for creating non-WebGL ES 3 contexts

Add support for creating non-WebGL ES 3 contexts.

Replaces "web gl version" in context creation attributes with
"context type".

Adds an example use-case, a GL test that uses ES 3.0 context.

Makes it possible to use ES 3.0 functions exposed by command buffer.
Previously the ES 3.0 functions were removed by the linker.

Needed, because up-coming features might have ES 3.0 related code,
and that code needs to be tested. EXT_blend_func_extended is an
example of such a feature.

BUG=506765
Committed: https://crrev.com/631033e40be24ce9c4eecf227f4295ae8c015d0e
Cr-Commit-Position: refs/heads/master@{#350319}

[Kimmo Kinnunen, 2015-08-28 08:12:22]

kkinnunen@nvidia.com changed reviewers:
+ piman@chromium.org, zmo@google.com

[Kimmo Kinnunen, 2015-08-28 08:37:23]

kkinnunen@nvidia.com changed reviewers:
+ vmiura@chromium.org

[Zhenyao Mo, 2015-08-28 17:16:24]

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

[Zhenyao Mo, 2015-08-28 17:16:25]

Thanks for upgrading to ES3 Context support.

Mostly looks good with a few minor suggestions.

https://codereview.chromium.org/1325433003/diff/40001/gpu/blink/webgraphicsco...
File gpu/blink/webgraphicscontext3d_impl.cc (right):

https://codereview.chromium.org/1325433003/diff/40001/gpu/blink/webgraphicsco...
gpu/blink/webgraphicscontext3d_impl.cc:1248: default:
nit: move default to the last.

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils.h (right):

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.h:216: enum ContextType {
Can we move this outside ContextCreationAttribHelper, so we can reference it as
gpu::gles2::ContextType, and use it everywhere instead of multiple enum
definition?

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_group.h:47: enum ContextType {
Can we not redefine the ContextType? And just use the one defined in the
gles2_cmd_util.h?

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2658: default:
nit: can you move default to the last?

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3305: default:
nit: move default to the last.

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:234: default:
nit: move default to the last.

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_manager.h (right):

https://codereview.chromium.org/1325433003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_manager.h:48: enum ContextType {
Can we not redefine this, but instead, use the one defined in gles2_cmd_util.h?

[Mark Kilgard, 2015-08-28 21:33:15]

markkilgard@gmail.com changed reviewers:
+ markkilgard@gmail.com

[Mark Kilgard, 2015-08-28 21:33:16]

lgtm

[Kimmo Kinnunen, 2015-08-31 12:23:13]

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/blink/webgra...
File gpu/blink/webgraphicscontext3d_impl.cc (right):

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/blink/webgra...
gpu/blink/webgraphicscontext3d_impl.cc:1248: default:
On 2015/08/28 17:16:24, Zhenyao Mo wrote:
> nit: move default to the last.

Done...

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/command_buff...
File gpu/command_buffer/common/gles2_cmd_utils.h (right):

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/command_buff...
gpu/command_buffer/common/gles2_cmd_utils.h:216: enum ContextType {
On 2015/08/28 17:16:25, Zhenyao Mo wrote:
> Can we move this outside ContextCreationAttribHelper, so we can reference it
as
> gpu::gles2::ContextType, and use it everywhere instead of multiple enum
> definition?

Done.

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/command_buff...
File gpu/command_buffer/service/context_group.h (right):

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/command_buff...
gpu/command_buffer/service/context_group.h:47: enum ContextType {
On 2015/08/28 17:16:25, Zhenyao Mo wrote:
> Can we not redefine the ContextType? And just use the one defined in the
> gles2_cmd_util.h?

Sure. (To me, they appeared as being in different level of layering, since they
have different export defines)

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/command_buff...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/command_buff...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3305: default:
On 2015/08/28 17:16:25, Zhenyao Mo wrote:
> nit: move default to the last.

Done.

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/command_buff...
File gpu/command_buffer/tests/gl_manager.h (right):

https://chromiumcodereview.appspot.com/1325433003/diff/40001/gpu/command_buff...
gpu/command_buffer/tests/gl_manager.h:48: enum ContextType {
On 2015/08/28 17:16:25, Zhenyao Mo wrote:
> Can we not redefine this, but instead, use the one defined in
gles2_cmd_util.h?

Done.

[Zhenyao Mo, 2015-08-31 17:51:01]

LGTM

[Kimmo Kinnunen, 2015-09-01 18:00:51]

kkinnunen@nvidia.com changed reviewers:
+ sievers@google.com

[Kimmo Kinnunen, 2015-09-01 18:00:52]

sievers@, piman@: would it be possible to get a review on the .gyp, .gn and gl
bindings files?

[piman, 2015-09-01 18:05:56]

lgtm

[Kimmo Kinnunen, 2015-09-04 06:38:59]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-09-04 06:39:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com, piman@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1325433003/#ps120001
(title: "rebase without the dependency")

[commit-bot: I haz the power, 2015-09-04 06:39:18]

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

[Kimmo Kinnunen, 2015-09-04 06:41:33]

The CQ bit was unchecked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-09-04 10:20:04]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-09-04 10:20:31]

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

[commit-bot: I haz the power, 2015-09-04 10:42:35]

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

[commit-bot: I haz the power, 2015-09-04 10:42:36]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kimmo Kinnunen, 2015-09-05 10:10:46]

zmo@ would thist be ok with this change

https://codereview.chromium.org/1325433003/diff/140001/gpu/blink/webgraphicsc...
File gpu/blink/webgraphicscontext3d_impl.cc (right):

https://codereview.chromium.org/1325433003/diff/140001/gpu/blink/webgraphicsc...
gpu/blink/webgraphicscontext3d_impl.cc:1255: case 0:
So this case for "webGLVersion" needed to be handled.

[Zhenyao Mo, 2015-09-07 21:21:29]

On 2015/09/05 10:10:46, Kimmo Kinnunen wrote:
> zmo@ would thist be ok with this change
> 
>
https://codereview.chromium.org/1325433003/diff/140001/gpu/blink/webgraphicsc...
> File gpu/blink/webgraphicscontext3d_impl.cc (right):
> 
>
https://codereview.chromium.org/1325433003/diff/140001/gpu/blink/webgraphicsc...
> gpu/blink/webgraphicscontext3d_impl.cc:1255: case 0:
> So this case for "webGLVersion" needed to be handled.

So in what cases this is needed?  I thought coming from blink WebGL side, WebGL
version is either 1 or 2, nothing else?

[Kimmo Kinnunen, 2015-09-21 07:37:09]

On 2015/09/07 21:21:29, Zhenyao Mo wrote:
> On 2015/09/05 10:10:46, Kimmo Kinnunen wrote:
> > zmo@ would thist be ok with this change
> > 
> >
>
https://codereview.chromium.org/1325433003/diff/140001/gpu/blink/webgraphicsc...
> > File gpu/blink/webgraphicscontext3d_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1325433003/diff/140001/gpu/blink/webgraphicsc...
> > gpu/blink/webgraphicscontext3d_impl.cc:1255: case 0:
> > So this case for "webGLVersion" needed to be handled.
> 
> So in what cases this is needed?  I thought coming from blink WebGL side,
WebGL
> version is either 1 or 2, nothing else?

I think compositor uses it. See synchronous_compositor_factory_impl.cc:45,
GetDefaultAttribs(). I think this might be a sw layering violation due to mostly
cut-and-paste. These should be easy to fix, mostly by not using
WebGraphicsContext3D::Attributes at the wrong level in the sw stack. You can
find some use in the places where a context is created, like test base classes
and such.

However, in the blink side, using webglVersion == 0 seems to be intention. The
WebGraphicsContext3D::Attributes::webglVersion initializes to zero by default,
and it is used in quite some places. (Not that I think it would be an intuitive
use of the variable...)

I could, in theory, replace the unneeded WebGraphicsContext3D::Attributes usage
with ContextCreationAttribHelper usage. However, this wouldn't fix blink side,
so the zero case needs to be handled anyway. Thus I don't know if it makes sense
for me to start refactoring the compositor and tests?

[Zhenyao Mo, 2015-09-21 17:02:28]

I see. LGTM then.

[Kimmo Kinnunen, 2015-09-22 05:51:51]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-09-22 05:51:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com, piman@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1325433003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-22 05:51:56]

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

[commit-bot: I haz the power, 2015-09-22 06:12:13]

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

[commit-bot: I haz the power, 2015-09-22 06:12:14]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Kimmo Kinnunen, 2015-09-22 06:27:57]

kkinnunen@nvidia.com changed reviewers:
+ dtrainor@chromium.org

[Kimmo Kinnunen, 2015-09-22 06:27:58]

dtrainor@: could you look at the blimp_context_provider.cc change?

[Kimmo Kinnunen, 2015-09-22 08:20:30]

kkinnunen@nvidia.com changed reviewers:
+ sky@chromium.org

[Kimmo Kinnunen, 2015-09-22 08:20:31]

sky@: could you look at
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc? Thanks!

[David Trainor- moved to gerrit, 2015-09-22 14:56:13]

blimp_context_provider lgtm thanks!

[sky, 2015-09-22 16:42:17]

LGTM

[Kimmo Kinnunen, 2015-09-23 05:29:04]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-09-23 05:29:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com, piman@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1325433003/#ps200001
(title: "missing hunk")

[commit-bot: I haz the power, 2015-09-23 05:30:18]

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

[commit-bot: I haz the power, 2015-09-23 06:39:11]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-09-23 06:39:56]

Patchset 11 (id:??) landed as
https://crrev.com/631033e40be24ce9c4eecf227f4295ae8c015d0e
Cr-Commit-Position: refs/heads/master@{#350319}