Refactor context creation parameters into a struct.

Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
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/df7fff2d416e731d3a968cf6bd16bb6f5f60eaee
Cr-Commit-Position: refs/heads/master@{#431412}

[Geoff Lang, 2016-11-07 18:20:15]

Description was changed from

==========
Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
==========

to

==========
Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
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, 2016-11-07 18:23:45]

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

[commit-bot: I haz the power, 2016-11-07 18:24:33]

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

[Geoff Lang, 2016-11-07 19:26:57]

geofflang@chromium.org changed reviewers:
+ kylechar@chromium.org, piman@chromium.org, posciak@chromium.org,
rjkroege@chromium.org, torne@chromium.org

[Geoff Lang, 2016-11-07 19:26:58]

PTAL, a fairly simple change but touches lots of files.  

Also looking for thoughts about requesting gl::PreferIntegratedGpu being
implicit now because it is the default option.  I could add some constructors to
GLContextAttribs that make it explicit again.

[rjkroege, 2016-11-07 19:44:26]

*ozone* lgtm

[commit-bot: I haz the power, 2016-11-07 19:54:23]

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

[commit-bot: I haz the power, 2016-11-07 19:54:24]

Dry run: This issue passed the CQ dry run.

[piman, 2016-11-07 21:01:01]

The default value for GLContextAttribs is just fine, it's consistent with
ContextCreationAttribHelper which has the same default.

https://codereview.chromium.org/2480373002/diff/1/android_webview/browser/sco...
File android_webview/browser/scoped_app_gl_state_restore.cc (right):

https://codereview.chromium.org/2480373002/diff/1/android_webview/browser/sco...
android_webview/browser/scoped_app_gl_state_restore.cc:34:
GetAppContextAttribs())) {}
nit: the default GLContextAttribs would be perfectly fine here: Android doesn't
have dual-GPU support (i.e. the preference is ignored), and besides we'd
probably prefer the integrated anyway.

https://codereview.chromium.org/2480373002/diff/1/android_webview/browser/tes...
File android_webview/browser/test/fake_window.cc (right):

https://codereview.chromium.org/2480373002/diff/1/android_webview/browser/tes...
android_webview/browser/test/fake_window.cc:193: context_attribs.gpu_preference
= gl::PreferDiscreteGpu;
nit: ditto.

https://codereview.chromium.org/2480373002/diff/1/gpu/command_buffer/tests/gl...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/2480373002/diff/1/gpu/command_buffer/tests/gl...
gpu/command_buffer/tests/gl_manager.cc:407: context_attribs.gpu_preference =
gl::PreferDiscreteGpu;
nit: ditto, on Android, we can use the default.

https://codereview.chromium.org/2480373002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2480373002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator_unittest.cc:73:
context_attribs.gpu_preference = gl::PreferDiscreteGpu;
nit: ditto, default is fine.

https://codereview.chromium.org/2480373002/diff/1/ui/gl/gl_context_cgl.cc
File ui/gl/gl_context_cgl.cc (right):

https://codereview.chromium.org/2480373002/diff/1/ui/gl/gl_context_cgl.cc#new...
ui/gl/gl_context_cgl.cc:94: const GLContextAttribs& attribs) {
Here and other context types: I think we should fail Initialize() if we request
features that are not available (!bind_generates_resources,
webgl_compatibility_context). Otherwise it could lead to confusion and bugs.

[Geoff Lang, 2016-11-07 21:36:21]

https://codereview.chromium.org/2480373002/diff/1/android_webview/browser/sco...
File android_webview/browser/scoped_app_gl_state_restore.cc (right):

https://codereview.chromium.org/2480373002/diff/1/android_webview/browser/sco...
android_webview/browser/scoped_app_gl_state_restore.cc:34:
GetAppContextAttribs())) {}
On 2016/11/07 21:01:01, piman wrote:
> nit: the default GLContextAttribs would be perfectly fine here: Android
doesn't
> have dual-GPU support (i.e. the preference is ignored), and besides we'd
> probably prefer the integrated anyway.

Done.

https://codereview.chromium.org/2480373002/diff/1/android_webview/browser/tes...
File android_webview/browser/test/fake_window.cc (right):

https://codereview.chromium.org/2480373002/diff/1/android_webview/browser/tes...
android_webview/browser/test/fake_window.cc:193: context_attribs.gpu_preference
= gl::PreferDiscreteGpu;
On 2016/11/07 21:01:01, piman wrote:
> nit: ditto.

Done.

https://codereview.chromium.org/2480373002/diff/1/gpu/command_buffer/tests/gl...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/2480373002/diff/1/gpu/command_buffer/tests/gl...
gpu/command_buffer/tests/gl_manager.cc:407: context_attribs.gpu_preference =
gl::PreferDiscreteGpu;
On 2016/11/07 21:01:01, piman wrote:
> nit: ditto, on Android, we can use the default.

Done.

https://codereview.chromium.org/2480373002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2480373002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator_unittest.cc:73:
context_attribs.gpu_preference = gl::PreferDiscreteGpu;
On 2016/11/07 21:01:01, piman wrote:
> nit: ditto, default is fine.

Done.

https://codereview.chromium.org/2480373002/diff/1/ui/gl/gl_context_cgl.cc
File ui/gl/gl_context_cgl.cc (right):

https://codereview.chromium.org/2480373002/diff/1/ui/gl/gl_context_cgl.cc#new...
ui/gl/gl_context_cgl.cc:94: const GLContextAttribs& attribs) {
On 2016/11/07 21:01:01, piman wrote:
> Here and other context types: I think we should fail Initialize() if we
request
> features that are not available (!bind_generates_resources,
> webgl_compatibility_context). Otherwise it could lead to confusion and bugs.

Done, I used a DCHECK but can change it to a failure if needed.

[Geoff Lang, 2016-11-07 21:36:26]

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

[commit-bot: I haz the power, 2016-11-07 21:36:52]

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

[piman, 2016-11-07 21:47:17]

lgtm, thanks!

[commit-bot: I haz the power, 2016-11-07 22:57:12]

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

[commit-bot: I haz the power, 2016-11-07 22:57:13]

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

[Torne, 2016-11-08 14:34:29]

https://codereview.chromium.org/2480373002/diff/20001/android_webview/browser...
File android_webview/browser/scoped_app_gl_state_restore.cc (right):

https://codereview.chromium.org/2480373002/diff/20001/android_webview/browser...
android_webview/browser/scoped_app_gl_state_restore.cc:28:
gl::GLContextAttribs())) {}
If I'm reading correctly the default is PreferIntegratedGpu, but we were passing
PreferDiscreteGpu before. Is this an intentional change?

https://codereview.chromium.org/2480373002/diff/20001/android_webview/browser...
File android_webview/browser/test/fake_window.cc (right):

https://codereview.chromium.org/2480373002/diff/20001/android_webview/browser...
android_webview/browser/test/fake_window.cc:193: gl::GLContextAttribs());
Same here

[Geoff Lang, 2016-11-08 14:44:00]

https://codereview.chromium.org/2480373002/diff/20001/android_webview/browser...
File android_webview/browser/scoped_app_gl_state_restore.cc (right):

https://codereview.chromium.org/2480373002/diff/20001/android_webview/browser...
android_webview/browser/scoped_app_gl_state_restore.cc:28:
gl::GLContextAttribs())) {}
On 2016/11/08 14:34:29, Torne wrote:
> If I'm reading correctly the default is PreferIntegratedGpu, but we were
passing
> PreferDiscreteGpu before. Is this an intentional change?

Yes, piman suggested it because Android doesn't have dual-gpu support.

[Geoff Lang, 2016-11-08 14:44:01]

https://codereview.chromium.org/2480373002/diff/20001/android_webview/browser...
File android_webview/browser/scoped_app_gl_state_restore.cc (right):

https://codereview.chromium.org/2480373002/diff/20001/android_webview/browser...
android_webview/browser/scoped_app_gl_state_restore.cc:28:
gl::GLContextAttribs())) {}
On 2016/11/08 14:34:29, Torne wrote:
> If I'm reading correctly the default is PreferIntegratedGpu, but we were
passing
> PreferDiscreteGpu before. Is this an intentional change?

Yes, piman suggested it because Android doesn't have dual-gpu support.

[Torne, 2016-11-08 14:44:51]

Ah, I didn't see that earlier comment. I guess that makes sense. android_webview
LGTM

[kylechar, 2016-11-08 14:54:58]

You've already got lgtms from everything I'm familiar with but lgtm too.

[Geoff Lang, 2016-11-10 21:33:56]

Description was changed from

==========
Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
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

==========
Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
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, 2016-11-10 21:33:56]

geofflang@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2016-11-10 21:37:18]

media/ lgtm

[Geoff Lang, 2016-11-10 21:37:48]

The CQ bit was checked by geofflang@chromium.org

[Geoff Lang, 2016-11-10 21:37:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/2480373002/#ps20001
(title: "address piman's comments")

[commit-bot: I haz the power, 2016-11-10 21:38:25]

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-11 00:34:32]

Description was changed from

==========
Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
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

==========
Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
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
==========

[commit-bot: I haz the power, 2016-11-11 00:34:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-11 00:39:07]

Description was changed from

==========
Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
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

==========
Refactor context creation parameters into a struct.

This allows for easily adding new parameters to GL context creation, added
parameters for creating contexts with
EGL_CHROMIUM_create_context_bind_generates_resource and
EGL_ANGLE_create_context_webgl_compatibility.

Small related fix: disallow create-on-bind for vertex arrays and transform
feedbacks in the passthrough command buffer, these objects do not allow
create-on-bind by the ES 3.0 spec but the semantics were mistakenly added.

BUG=angleproject:1518
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/df7fff2d416e731d3a968cf6bd16bb6f5f60eaee
Cr-Commit-Position: refs/heads/master@{#431412}
==========

[commit-bot: I haz the power, 2016-11-11 00:39:08]

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

[Tom (Use chromium acct), 2016-11-11 01:26:13]

thomasanderson@google.com changed reviewers:
+ thomasanderson@google.com

[Tom (Use chromium acct), 2016-11-11 01:26:14]

Hi geofflang@

I'm suspecting this CL is causing a build failure on the Chromium Ozone FYI bot.

https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux/builds/681/s...

It's not necessary to revert this CL, but can you PTAL at the failure?

[Pawel Osciak, 2016-11-11 05:56:32]

media/gpu lgtm

[Geoff Lang, 2016-11-11 15:19:23]

On 2016/11/11 01:26:14, Tom Anderson wrote:
> Hi geofflang@
> 
> I'm suspecting this CL is causing a build failure on the Chromium Ozone FYI
bot.
> 
>
https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux/builds/681/s...
> 
> It's not necessary to revert this CL, but can you PTAL at the failure?

Created a follow-up CL here: https://codereview.chromium.org/2492223002/

[Alexander Alekseev, 2016-11-12 08:37:01]

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

The reason for reverting is: Speculative revert because of
https://crbug.com/664719.