Remove VirtualGLApi

Remove VirtualGLApi

At this point, VirtualGLApi only hooks the extension string query, and for not a very good reason, since it ends up serving the extension string of the underlying context.
OTOH it adds a bunch of complexity to require the switch between RealGLApi and the VirtualGLApi, so instead fold the logic into GLContext.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

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

[piman, 2016-07-08 02:33:53]

Description was changed from

==========
Remove VirtualGLApi

BUG=
==========

to

==========
Remove VirtualGLApi

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-07-08 03:40:43]

Patchset #1 (id:1) has been deleted

[piman, 2016-07-08 04:06:41]

Description was changed from

==========
Remove VirtualGLApi

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove VirtualGLApi

At this point, VirtualGLApi only hooks the extension string query, and for not a
very good reason, since it ends up serving the extension string of the
underlying context.
OTOH it adds a bunch of complexity to require the switch between RealGLApi and
the VirtualGLApi, so instead fold the logic into GLContext.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-07-08 04:06:46]

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

[commit-bot: I haz the power, 2016-07-08 04:07:23]

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

[piman, 2016-07-08 04:09:05]

piman@chromium.org changed reviewers:
+ sievers@chromium.org

[piman, 2016-07-08 04:09:06]

Maybe I'm missing something important, but I ran tests locally, and ran chrome
with --use-virtualized-gl-contexts, and everything seems ok.

[commit-bot: I haz the power, 2016-07-08 06:09:20]

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

[commit-bot: I haz the power, 2016-07-08 06:09:20]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[no sievers, 2016-07-08 17:52:19]

lgtm. good catch.

We used to override the extension because we had to at least say that we don't
support queries, which we do now though with the virtualization we have for
that.

And as far as having a VirtualGLApi is concerned, the original idea was to
override a bunch of GL functions to restore the state lazily on a per call
basis. But that's not going to happen, and we now have at least a bit of
smartness in RestoreState() to avoid redundant GL calls.

https://codereview.chromium.org/2132913002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gl_context_virtual.cc (left):

https://codereview.chromium.org/2132913002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gl_context_virtual.cc:33: if
(!shared_context_->MakeCurrent(compatible_surface)) {
So this would potentially defer catching an incompatible surface until we really
have to make it current later, and probably only matter for onscreen.

I guess that's fine since it's more of a static assumption that we don't have
virtual context groups with incompatible surface configs.

[piman, 2016-07-08 18:30:59]

https://codereview.chromium.org/2132913002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gl_context_virtual.cc (left):

https://codereview.chromium.org/2132913002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gl_context_virtual.cc:33: if
(!shared_context_->MakeCurrent(compatible_surface)) {
On 2016/07/08 17:52:19, sievers wrote:
> So this would potentially defer catching an incompatible surface until we
really
> have to make it current later, and probably only matter for onscreen.
> 
> I guess that's fine since it's more of a static assumption that we don't have
> virtual context groups with incompatible surface configs.

MakeVirtuallyCurrent below will effectively call MakeCurrent(shared_context_,
compatible_surface) - unless that combination is already current, so I think the
mismatch will be caught anyway.

It used to be that SetupForVirtualization needed the context to be current, but
since it's gone, I think this is not needed any more.

[no sievers, 2016-07-08 18:34:56]

https://codereview.chromium.org/2132913002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gl_context_virtual.cc (left):

https://codereview.chromium.org/2132913002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gl_context_virtual.cc:33: if
(!shared_context_->MakeCurrent(compatible_surface)) {
On 2016/07/08 18:30:58, piman wrote:
> On 2016/07/08 17:52:19, sievers wrote:
> > So this would potentially defer catching an incompatible surface until we
> really
> > have to make it current later, and probably only matter for onscreen.
> > 
> > I guess that's fine since it's more of a static assumption that we don't
have
> > virtual context groups with incompatible surface configs.
> 
> MakeVirtuallyCurrent below will effectively call MakeCurrent(shared_context_,
> compatible_surface) - unless that combination is already current, so I think
the
> mismatch will be caught anyway.
> 
> It used to be that SetupForVirtualization needed the context to be current,
but
> since it's gone, I think this is not needed any more.

Ah you're right, since this here was using the same permissive
GLContext*Virtual*::IsCurrent() check. Sounds good and definitely much simpler.

[piman, 2016-07-08 20:16:35]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2016-07-08 20:16:51]

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-07-08 21:23:30]

Description was changed from

==========
Remove VirtualGLApi

At this point, VirtualGLApi only hooks the extension string query, and for not a
very good reason, since it ends up serving the extension string of the
underlying context.
OTOH it adds a bunch of complexity to require the switch between RealGLApi and
the VirtualGLApi, so instead fold the logic into GLContext.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove VirtualGLApi

At this point, VirtualGLApi only hooks the extension string query, and for not a
very good reason, since it ends up serving the extension string of the
underlying context.
OTOH it adds a bunch of complexity to require the switch between RealGLApi and
the VirtualGLApi, so instead fold the logic into GLContext.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-07-08 21:23:31]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2016-07-08 21:23:45]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 21:26:16]

Description was changed from

==========
Remove VirtualGLApi

At this point, VirtualGLApi only hooks the extension string query, and for not a
very good reason, since it ends up serving the extension string of the
underlying context.
OTOH it adds a bunch of complexity to require the switch between RealGLApi and
the VirtualGLApi, so instead fold the logic into GLContext.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove VirtualGLApi

At this point, VirtualGLApi only hooks the extension string query, and for not a
very good reason, since it ends up serving the extension string of the
underlying context.
OTOH it adds a bunch of complexity to require the switch between RealGLApi and
the VirtualGLApi, so instead fold the logic into GLContext.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-07-08 21:26:17]

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