Restore samplers' state across virtualized GL contexts.

Restore samplers' state across virtualized GL contexts.

Tested with new WebGL conformance test being added in
https://github.com/KhronosGroup/WebGL/pull/2383 .

BUG=713127
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/2837213003
Cr-Commit-Position: refs/heads/master@{#467520}
Committed: https://chromium.googlesource.com/chromium/src/+/69c721ec714b0650dec773173ef4be5a25347a49

[Ken Russell (switch to Gerrit), 2017-04-25 22:25:15]

Description was changed from

==========
Restore samplers' state across virtualized GL contexts.

Tested with new WebGL conformance test being added in
https://github.com/KhronosGroup/WebGL/pull/2383 .

BUG=713127
==========

to

==========
Restore samplers' state across virtualized GL contexts.

Tested with new WebGL conformance test being added in
https://github.com/KhronosGroup/WebGL/pull/2383 .

BUG=713127
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-04-25 22:34:01]

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

[Ken Russell (switch to Gerrit), 2017-04-25 22:34:01]

John: please review. Thanks.

Others: FYI.

Note: it didn't seem feasible to test this with a unit test. The WebGL
conformance test I'm in the process of adding is tiny, and reproduces the bug
100% of the time.

[jbauman, 2017-04-25 22:47:07]

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

https://codereview.chromium.org/2837213003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_state.cc:296: if (cur_id != prev_id) {
"if (!prev_state || cur_id != prev_id) {"

I think in theory a null prev_state means the previous state is unknown and
everything has to be restored. That's what RestoreTextureUnitBindings is doing,
anyway.

[Ken Russell (switch to Gerrit), 2017-04-26 01:02:25]

Please feel free to push me to write more/better tests for this. I feel bad that
I'm not adding some as part of this, but think that an entirely new unittest
which uses context_state and a mock GL is probably needed.

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

https://codereview.chromium.org/2837213003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_state.cc:296: if (cur_id != prev_id) {
On 2017/04/25 22:47:07, jbauman wrote:
> "if (!prev_state || cur_id != prev_id) {"
> 
> I think in theory a null prev_state means the previous state is unknown and
> everything has to be restored. That's what RestoreTextureUnitBindings is
doing,
> anyway.

Thank you for catching this. Done in current patch set.

[Ken Russell (switch to Gerrit), 2017-04-26 01:08:41]

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

[commit-bot: I haz the power, 2017-04-26 01:09:19]

Dry run: CQ is trying da patch.

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

[jbauman, 2017-04-26 01:22:45]

https://codereview.chromium.org/2837213003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_state.cc
(right):

https://codereview.chromium.org/2837213003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_state.cc:142:
TEST_P(GLES2DecoderRestoreStateTest, NullPreviousState) {
Could you modify these these tests to init the decoder with
CONTEXT_TYPE_OPENGLES3? I think the mock should catch any new calls that are
made.

[commit-bot: I haz the power, 2017-04-26 03:15:43]

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

[commit-bot: I haz the power, 2017-04-26 03:15:44]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-04-26 21:36:35]

Unit tests added. Please re-review. Thanks.

https://codereview.chromium.org/2837213003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_state.cc
(right):

https://codereview.chromium.org/2837213003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_state.cc:142:
TEST_P(GLES2DecoderRestoreStateTest, NullPreviousState) {
On 2017/04/26 01:22:45, jbauman wrote:
> Could you modify these these tests to init the decoder with
> CONTEXT_TYPE_OPENGLES3? I think the mock should catch any new calls that are
> made.

Thanks for the suggestion. Rather than do this, I've added three new tests to
this file which explicitly test the restoration of samplers.

[Ken Russell (switch to Gerrit), 2017-04-26 21:36:54]

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

[commit-bot: I haz the power, 2017-04-26 21:37:55]

Dry run: CQ is trying da patch.

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

[jbauman, 2017-04-26 21:40:09]

lgtm, thanks for adding the tests!

[Ken Russell (switch to Gerrit), 2017-04-26 21:48:36]

On 2017/04/26 21:40:09, jbauman wrote:
> lgtm, thanks for adding the tests!

Thanks for pushing me to add them!

[commit-bot: I haz the power, 2017-04-26 23:34:59]

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

[commit-bot: I haz the power, 2017-04-26 23:35:00]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-04-26 23:45:42]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2017-04-26 23:46:33]

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-04-26 23:58:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493250342518400,
"parent_rev": "250b06310437b1bddd49874ffc83048ecb26f74e", "commit_rev":
"69c721ec714b0650dec773173ef4be5a25347a49"}

[commit-bot: I haz the power, 2017-04-26 23:59:01]

Description was changed from

==========
Restore samplers' state across virtualized GL contexts.

Tested with new WebGL conformance test being added in
https://github.com/KhronosGroup/WebGL/pull/2383 .

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

==========
Restore samplers' state across virtualized GL contexts.

Tested with new WebGL conformance test being added in
https://github.com/KhronosGroup/WebGL/pull/2383 .

BUG=713127
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/2837213003
Cr-Commit-Position: refs/heads/master@{#467520}
Committed:
https://chromium.googlesource.com/chromium/src/+/69c721ec714b0650dec773173ef4...
==========

[commit-bot: I haz the power, 2017-04-26 23:59:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/69c721ec714b0650dec773173ef4...