Hook ANGLE_request_extension up to the passthrough cmd decoder.

Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2670213005
Cr-Commit-Position: refs/heads/master@{#457571}
Committed: https://chromium.googlesource.com/chromium/src/+/c7fc0726ec8cd4985cd6e416e50712b4aff1c8c7

[Geoff Lang, 2017-02-03 18:24:17]

Description was changed from

==========
Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
==========

to

==========
Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
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
==========

[Geoff Lang, 2017-02-03 18:25:50]

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

[commit-bot: I haz the power, 2017-02-03 18:26:30]

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

[Geoff Lang, 2017-02-03 20:10:40]

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

[commit-bot: I haz the power, 2017-02-03 20:11:17]

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, 2017-02-03 22:39:36]

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

[commit-bot: I haz the power, 2017-02-03 22:39:36]

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

[Geoff Lang, 2017-02-06 18:12:42]

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

[commit-bot: I haz the power, 2017-02-06 18:13:10]

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

[Geoff Lang, 2017-02-06 18:45:17]

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

[commit-bot: I haz the power, 2017-02-06 18:45:42]

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

[Geoff Lang, 2017-02-06 20:13:23]

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

[commit-bot: I haz the power, 2017-02-06 20:14:03]

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, 2017-02-06 22:05:27]

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

[commit-bot: I haz the power, 2017-02-06 22:05:28]

Dry run: This issue passed the CQ dry run.

[Geoff Lang, 2017-02-06 22:18:38]

Description was changed from

==========
Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
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

==========
Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
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
==========

[Geoff Lang, 2017-02-06 22:18:39]

geofflang@chromium.org changed reviewers:
+ jbauman@chromium.org, zmo@chromium.org

[Geoff Lang, 2017-02-06 22:19:06]

PTAL

[Zhenyao Mo, 2017-02-06 22:31:38]

Mostly look good with a few questions.

https://codereview.chromium.org/2670213005/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc (right):

https://codereview.chromium.org/2670213005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc:173: // be enabled
dynamically
I am wondering if this is necessary - WebGL has no plans to share resources in
the near future, so basically WebGL will be in the one context per group
situation.

https://codereview.chromium.org/2670213005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp (right):

https://codereview.chromium.org/2670213005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp:108: return true;
So when we actually try to enable this extension, this function may return
false. Will this cause problems?

[Geoff Lang, 2017-02-07 19:00:54]

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

[Geoff Lang, 2017-02-07 19:01:16]

https://codereview.chromium.org/2670213005/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc (right):

https://codereview.chromium.org/2670213005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc:173: // be enabled
dynamically
On 2017/02/06 22:31:38, Zhenyao Mo wrote:
> I am wondering if this is necessary - WebGL has no plans to share resources in
> the near future, so basically WebGL will be in the one context per group
> situation.

I wanted to be sure that the feature info doesn't get out of sync, I don't think
there is currently anything stopping a non-webgl context from calling
glRequestExtensionCHOMRIUM.  I could add an assert in DoRequestExtensionCHOMRIUM
that the context group only has one context or keep it as is, have a preference?

https://codereview.chromium.org/2670213005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp (right):

https://codereview.chromium.org/2670213005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp:108: return true;
On 2017/02/06 22:31:38, Zhenyao Mo wrote:
> So when we actually try to enable this extension, this function may return
> false. Will this cause problems?

The comment below says that we can only get here if GL_EXT_draw_buffers is
supported but it's not very clear that it can be supported but not enabled yet. 
I added a new method to Extensions3DUtil, isExtensionRequestable, to make it
more readable.

[commit-bot: I haz the power, 2017-02-07 19:01:28]

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, 2017-02-07 20:33:44]

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

[commit-bot: I haz the power, 2017-02-07 20:33:45]

Dry run: This issue passed the CQ dry run.

[Geoff Lang, 2017-02-15 21:43:31]

ping

[Zhenyao Mo, 2017-02-15 23:24:20]

Sorry it slipped out of my radar.

https://codereview.chromium.org/2670213005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp (right):

https://codereview.chromium.org/2670213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp:105: if
(extensionsUtil->isExtensionRequestable("GL_EXT_draw_buffers")) {
This is also true even if the extension is not enabled, i.e., the code after
this block will never be hit.

I am OK with deleting this satisfiesWebGLRequirements() and trust the drivers to
do the right thing. (If someone reports a bad driver, then let's add a
workaround to disable this extension inside command buffer).

[Geoff Lang, 2017-02-16 15:38:15]

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

[commit-bot: I haz the power, 2017-02-16 15:38:34]

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

[Geoff Lang, 2017-02-16 15:38:45]

https://codereview.chromium.org/2670213005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp (right):

https://codereview.chromium.org/2670213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp:105: if
(extensionsUtil->isExtensionRequestable("GL_EXT_draw_buffers")) {
On 2017/02/15 23:24:20, Zhenyao Mo wrote:
> This is also true even if the extension is not enabled, i.e., the code after
> this block will never be hit.
> 
> I am OK with deleting this satisfiesWebGLRequirements() and trust the drivers
to
> do the right thing. (If someone reports a bad driver, then let's add a
> workaround to disable this extension inside command buffer).

Removed this method.  I agree and prefer to trust the driver until we find
issues.

[commit-bot: I haz the power, 2017-02-16 17:22:39]

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

[commit-bot: I haz the power, 2017-02-16 17:22:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Zhenyao Mo, 2017-02-16 21:51:11]

On 2017/02/16 15:38:45, Geoff Lang wrote:
>
https://codereview.chromium.org/2670213005/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp (right):
> 
>
https://codereview.chromium.org/2670213005/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp:105: if
> (extensionsUtil->isExtensionRequestable("GL_EXT_draw_buffers")) {
> On 2017/02/15 23:24:20, Zhenyao Mo wrote:
> > This is also true even if the extension is not enabled, i.e., the code after
> > this block will never be hit.
> > 
> > I am OK with deleting this satisfiesWebGLRequirements() and trust the
drivers
> to
> > do the right thing. (If someone reports a bad driver, then let's add a
> > workaround to disable this extension inside command buffer).
> 
> Removed this method.  I agree and prefer to trust the driver until we find
> issues.

Sorry to mislead you on this, but it's not for driver bugs, it's an extra
requirement from WebGL extension.

See https://www.khronos.org/registry/webgl/extensions/WEBGL_draw_buffers/.

So I think what needs to happen is when we decide if this extension is available
in WebGL1, we already need to enable it in MANGLE.  Just as what's happening
right now with command buffer. Otherwise it won't work here.

[Geoff Lang, 2017-03-07 18:21:29]

Description was changed from

==========
Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
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

==========
Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[Geoff Lang, 2017-03-07 18:21:40]

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

[commit-bot: I haz the power, 2017-03-07 18:23:10]

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, 2017-03-07 18:37:36]

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

[commit-bot: I haz the power, 2017-03-07 18:37:37]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[Geoff Lang, 2017-03-07 18:44:32]

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

[commit-bot: I haz the power, 2017-03-07 18:45:40]

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, 2017-03-07 20:14:34]

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

[commit-bot: I haz the power, 2017-03-07 20:14:36]

Dry run: Try jobs failed on following builders:
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[Geoff Lang, 2017-03-07 22:26:22]

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

[commit-bot: I haz the power, 2017-03-07 22:26: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, 2017-03-08 02:58:44]

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

[commit-bot: I haz the power, 2017-03-08 02:58:45]

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

[Geoff Lang, 2017-03-08 18:35:43]

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

[commit-bot: I haz the power, 2017-03-08 18:36:31]

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, 2017-03-08 20:55:00]

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

[commit-bot: I haz the power, 2017-03-08 20:55:01]

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

[Geoff Lang, 2017-03-09 00:41:16]

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

[commit-bot: I haz the power, 2017-03-09 00:41:46]

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, 2017-03-09 02:30:12]

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

[commit-bot: I haz the power, 2017-03-09 02:30:14]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2017-03-09 18:03:18]

LGTM with one perf concern (could be future improvement, so feel free to land as
is)

https://codereview.chromium.org/2670213005/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2670213005/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2613:
context_->ReinitializeDynamicBindings();
This is potentially a perf concern for WebGL startup (they enable a bunch of
extensions at startup, each calling into this.

In theory, we can initialize all bindings unconditionally, and let validation
decide if a command is legitimate or not.

Could you at least add a TODO and maybe create a bug to look into this later?

[Geoff Lang, 2017-03-16 14:59:46]

https://codereview.chromium.org/2670213005/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2670213005/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2613:
context_->ReinitializeDynamicBindings();
On 2017/03/09 18:03:18, Zhenyao Mo wrote:
> This is potentially a perf concern for WebGL startup (they enable a bunch of
> extensions at startup, each calling into this.
> 
> In theory, we can initialize all bindings unconditionally, and let validation
> decide if a command is legitimate or not.
> 
> Could you at least add a TODO and maybe create a bug to look into this later?

Done, put a comment next to GLContext::ReinitializeDynamicBindings.

[Zhenyao Mo, 2017-03-16 16:04:06]

On 2017/03/16 14:59:46, Geoff Lang wrote:
>
https://codereview.chromium.org/2670213005/diff/220001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc
(right):
> 
>
https://codereview.chromium.org/2670213005/diff/220001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2613:
> context_->ReinitializeDynamicBindings();
> On 2017/03/09 18:03:18, Zhenyao Mo wrote:
> > This is potentially a perf concern for WebGL startup (they enable a bunch of
> > extensions at startup, each calling into this.
> > 
> > In theory, we can initialize all bindings unconditionally, and let
validation
> > decide if a command is legitimate or not.
> > 
> > Could you at least add a TODO and maybe create a bug to look into this
later?
> 
> Done, put a comment next to GLContext::ReinitializeDynamicBindings.

lgtm again

[jbauman, 2017-03-16 19:35:02]

lgtm

[Geoff Lang, 2017-03-16 19:44:20]

The CQ bit was checked by geofflang@chromium.org

[commit-bot: I haz the power, 2017-03-16 19:45:15]

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, 2017-03-16 21:48:27]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1489693460374090,
"parent_rev": "bfa14e6ef0b8adc0f56d899f3776fe4b960b8978", "commit_rev":
"c7fc0726ec8cd4985cd6e416e50712b4aff1c8c7"}

[commit-bot: I haz the power, 2017-03-16 21:49:08]

Description was changed from

==========
Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Hook ANGLE_request_extension up to the passthrough cmd decoder.

Have each decoder own its own FeatureInfo so it can be updated as extensions
are enabled.

BUG=682745
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2670213005
Cr-Commit-Position: refs/heads/master@{#457571}
Committed:
https://chromium.googlesource.com/chromium/src/+/c7fc0726ec8cd4985cd6e416e507...
==========

[commit-bot: I haz the power, 2017-03-16 21:49:10]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/c7fc0726ec8cd4985cd6e416e507...