Add more strict DCHECKs around context state.

Add more strict DCHECKs around context state.

Upon processing commands in the GLES2Decoder, assert that the context
is current.

Make virtual contexts' definition of "current" more strict. This
required moving a DCHECK inside the base GLContext class.

These aren't needed, but were useful during recent debugging to
confirm that the bug wasn't in these areas.

BUG=694359
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/2852353003
Cr-Commit-Position: refs/heads/master@{#469115}
Committed: https://chromium.googlesource.com/chromium/src/+/a21cb49e77bf727d692bbe8dd8eda85a9d089e93

[Ken Russell (switch to Gerrit), 2017-05-03 00:41:26]

Description was changed from

==========
Add more strict DCHECKs around context state.

Upon processing commands in the GLES2Decoder, assert that the context
is current.

Make virtual contexts' definition of "current" more strict. This
required moving a DCHECK inside the base GLContext class.

These aren't needed, but were useful during recent debugging to
confirm that the bug wasn't in these areas.

BUG=694359
==========

to

==========
Add more strict DCHECKs around context state.

Upon processing commands in the GLES2Decoder, assert that the context
is current.

Make virtual contexts' definition of "current" more strict. This
required moving a DCHECK inside the base GLContext class.

These aren't needed, but were useful during recent debugging to
confirm that the bug wasn't in these areas.

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

[Ken Russell (switch to Gerrit), 2017-05-03 00:44:11]

kbr@chromium.org changed reviewers:
+ jbauman@chromium.org

[Ken Russell (switch to Gerrit), 2017-05-03 00:44:12]

These aren't really needed, but I think they're useful. The change in
gl_context.cc to move the DCHECK lower is needed if the definition of
"IsCurrent" for virtual GL contexts is to be made more strict, but I'm not sure
it's a good idea. Please think this through. Thanks.

[Ken Russell (switch to Gerrit), 2017-05-03 17:51:09]

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

[Ken Russell (switch to Gerrit), 2017-05-03 17:51:10]

+zmo

Do you think these dchecks would be useful? If not, I'll close this CL.

[Zhenyao Mo, 2017-05-03 18:00:08]

On 2017/05/03 17:51:10, Ken Russell wrote:
> +zmo
> 
> Do you think these dchecks would be useful? If not, I'll close this CL.

I think they add readability. LGTM

[Ken Russell (switch to Gerrit), 2017-05-03 18:47:43]

On 2017/05/03 18:00:08, Zhenyao Mo wrote:
> On 2017/05/03 17:51:10, Ken Russell wrote:
> > +zmo
> > 
> > Do you think these dchecks would be useful? If not, I'll close this CL.
> 
> I think they add readability. LGTM

OK. Thanks Mo. Here we go.

[Ken Russell (switch to Gerrit), 2017-05-03 18:47:46]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2017-05-03 18:49:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2017-05-03 19:47:40]

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

[piman, 2017-05-03 19:47:41]

https://codereview.chromium.org/2852353003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gl_context_virtual.cc (right):

https://codereview.chromium.org/2852353003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gl_context_virtual.cc:59:
shared_context_->current_virtual_context_ == this;
One feature of this code is to avoid calling eglMakeCurrent (which is costly on
some Android devices in particular) when switching between on-screen and
offscreen contexts: since we use the same underlying GL context, if the main
framebuffer doesn't have to change (e.g. switching from on-screen to offscreen,
we don't use the main framebuffer, so whatever it is can stay).

I think this addition would defeat the purpose, because we would force
eglMakeCurrent, since IsCurrent would return false now in that case.

[commit-bot: I haz the power, 2017-05-03 21:00:32]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493837266639700, "parent_rev":
"054e8f0ade030ca9d679cdfafaacd6d2f2e97617", "commit_rev":
"a21cb49e77bf727d692bbe8dd8eda85a9d089e93"}

[commit-bot: I haz the power, 2017-05-03 21:00:48]

Description was changed from

==========
Add more strict DCHECKs around context state.

Upon processing commands in the GLES2Decoder, assert that the context
is current.

Make virtual contexts' definition of "current" more strict. This
required moving a DCHECK inside the base GLContext class.

These aren't needed, but were useful during recent debugging to
confirm that the bug wasn't in these areas.

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

==========
Add more strict DCHECKs around context state.

Upon processing commands in the GLES2Decoder, assert that the context
is current.

Make virtual contexts' definition of "current" more strict. This
required moving a DCHECK inside the base GLContext class.

These aren't needed, but were useful during recent debugging to
confirm that the bug wasn't in these areas.

BUG=694359
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/2852353003
Cr-Commit-Position: refs/heads/master@{#469115}
Committed:
https://chromium.googlesource.com/chromium/src/+/a21cb49e77bf727d692bbe8dd8ed...
==========

[commit-bot: I haz the power, 2017-05-03 21:00:50]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/a21cb49e77bf727d692bbe8dd8ed...

[Zhenyao Mo, 2017-05-03 22:21:34]

https://codereview.chromium.org/2852353003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gl_context_virtual.cc (right):

https://codereview.chromium.org/2852353003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gl_context_virtual.cc:59:
shared_context_->current_virtual_context_ == this;
On 2017/05/03 19:47:41, piman - OOO back 4-27 wrote:
> One feature of this code is to avoid calling eglMakeCurrent (which is costly
on
> some Android devices in particular) when switching between on-screen and
> offscreen contexts: since we use the same underlying GL context, if the main
> framebuffer doesn't have to change (e.g. switching from on-screen to
offscreen,
> we don't use the main framebuffer, so whatever it is can stay).
> 
> I think this addition would defeat the purpose, because we would force
> eglMakeCurrent, since IsCurrent would return false now in that case.

Thanks for pointing this out, although this design is confusing and could easily
be violated (like this CL) because the this function surely lets people think it
means the this virtual context is current.

Any suggestions on how we could clarify this and prevent future regression here?

[piman, 2017-05-03 22:43:40]

On 2017/05/03 22:21:34, Zhenyao Mo wrote:
>
https://codereview.chromium.org/2852353003/diff/1/gpu/command_buffer/service/...
> File gpu/command_buffer/service/gl_context_virtual.cc (right):
> 
>
https://codereview.chromium.org/2852353003/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/gl_context_virtual.cc:59:
> shared_context_->current_virtual_context_ == this;
> On 2017/05/03 19:47:41, piman - OOO back 4-27 wrote:
> > One feature of this code is to avoid calling eglMakeCurrent (which is costly
> on
> > some Android devices in particular) when switching between on-screen and
> > offscreen contexts: since we use the same underlying GL context, if the main
> > framebuffer doesn't have to change (e.g. switching from on-screen to
> offscreen,
> > we don't use the main framebuffer, so whatever it is can stay).
> > 
> > I think this addition would defeat the purpose, because we would force
> > eglMakeCurrent, since IsCurrent would return false now in that case.
> 
> Thanks for pointing this out, although this design is confusing and could
easily
> be violated (like this CL) because the this function surely lets people think
it
> means the this virtual context is current.
> 
> Any suggestions on how we could clarify this and prevent future regression
here?

Agreed it's very confusing.

To prevent regressions, it should be possible to write a unit test (should have
been done when the code was written in the first, agreed).

I think it could be done a better way either by adding a separate API (not sure
about naming... "NeedsMakeCurrentForSurface"?), or by moving the logic into each
of the platform contexts (maybe with a helper function to avoid duplication)

[Ken Russell (switch to Gerrit), 2017-05-03 23:04:37]

Description was changed from

==========
Add more strict DCHECKs around context state.

Upon processing commands in the GLES2Decoder, assert that the context
is current.

Make virtual contexts' definition of "current" more strict. This
required moving a DCHECK inside the base GLContext class.

These aren't needed, but were useful during recent debugging to
confirm that the bug wasn't in these areas.

BUG=694359
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/2852353003
Cr-Commit-Position: refs/heads/master@{#469115}
Committed:
https://chromium.googlesource.com/chromium/src/+/a21cb49e77bf727d692bbe8dd8ed...
==========

to

==========
Add more strict DCHECKs around context state.

Upon processing commands in the GLES2Decoder, assert that the context
is current.

Make virtual contexts' definition of "current" more strict. This
required moving a DCHECK inside the base GLContext class.

These aren't needed, but were useful during recent debugging to
confirm that the bug wasn't in these areas.

BUG=694359
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/2852353003
Cr-Commit-Position: refs/heads/master@{#469115}
Committed:
https://chromium.googlesource.com/chromium/src/+/a21cb49e77bf727d692bbe8dd8ed...
==========

[Ken Russell (switch to Gerrit), 2017-05-03 23:05:32]

On 2017/05/03 22:43:40, piman - OOO back 4-27 wrote:
> On 2017/05/03 22:21:34, Zhenyao Mo wrote:
> >
>
https://codereview.chromium.org/2852353003/diff/1/gpu/command_buffer/service/...
> > File gpu/command_buffer/service/gl_context_virtual.cc (right):
> > 
> >
>
https://codereview.chromium.org/2852353003/diff/1/gpu/command_buffer/service/...
> > gpu/command_buffer/service/gl_context_virtual.cc:59:
> > shared_context_->current_virtual_context_ == this;
> > On 2017/05/03 19:47:41, piman - OOO back 4-27 wrote:
> > > One feature of this code is to avoid calling eglMakeCurrent (which is
costly
> > on
> > > some Android devices in particular) when switching between on-screen and
> > > offscreen contexts: since we use the same underlying GL context, if the
main
> > > framebuffer doesn't have to change (e.g. switching from on-screen to
> > offscreen,
> > > we don't use the main framebuffer, so whatever it is can stay).
> > > 
> > > I think this addition would defeat the purpose, because we would force
> > > eglMakeCurrent, since IsCurrent would return false now in that case.
> > 
> > Thanks for pointing this out, although this design is confusing and could
> easily
> > be violated (like this CL) because the this function surely lets people
think
> it
> > means the this virtual context is current.
> > 
> > Any suggestions on how we could clarify this and prevent future regression
> here?
> 
> Agreed it's very confusing.
> 
> To prevent regressions, it should be possible to write a unit test (should
have
> been done when the code was written in the first, agreed).
> 
> I think it could be done a better way either by adding a separate API (not
sure
> about naming... "NeedsMakeCurrentForSurface"?), or by moving the logic into
each
> of the platform contexts (maybe with a helper function to avoid duplication)

This is pretty confusing -- upon first reading of
GLContext::MakeVirtuallyCurrent and GLContextVirtual::IsCurrent, it seems to me
that GLContext::MakeVirtuallyCurrent should be able to make the determination of
whether it needs to perform the MakeCurrent call, but now I see that it can't
because it doesn't have access to the GLContextVirtual's shared context.

I'm going to revert this and am leery about making further semantic changes
since we're collectively trying to move to using ANGLE on all platforms, which
has a more thorough implementation of virtual GL contexts.

[Ken Russell (switch to Gerrit), 2017-05-03 23:06:06]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2859963002/ by kbr@chromium.org.

The reason for reverting is: Breaks an optimization needed on some Android
devices for performance.
.