current program can be null in ES2/ES3 contexts.

current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
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/14ef40159115ca5059907835a34bcd1667af7631
Cr-Commit-Position: refs/heads/master@{#407659}

[Zhenyao Mo, 2016-07-23 16:04:56]

Description was changed from

==========
current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
==========

to

==========
current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
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
==========

[Zhenyao Mo, 2016-07-23 16:10:40]

piman: PTAL

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

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9140: return true;
This is actually the crash fix.  Everything else is cleanup.

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9159: for (size_t ii = 0; ii <
shader_attrib_active_mask.size(); ++ii) {
This is a minor optimization. We can just query the entire masks altogether.

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/program_manager.cc (left):

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.cc:377:
vertex_input_type_written_mask_.resize(packed_size);
We don't need to resize every link because they never change size.  So move it
to Program constructor.  This probably isn't a perf gain at all, but
conceptually I feel this is cleaner.

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/program_manager.h (right):

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.h:586: std::vector<uint32_t>
vertex_input_active_mask_;
I think this is a better name, since we don't write to the attribs in vertex
shader like the fragment output variables.

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

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/vertex_attrib_manager.cc:163: GLuint shift_bits =
(index % 16) * 2;
We only need to do this if the glEnableDisable call actually change the state. 
This is to fit with the rest of the code - less confusing.

[Zhenyao Mo, 2016-07-23 16:11:02]

Description was changed from

==========
current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
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

==========
current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
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
==========

[Zhenyao Mo, 2016-07-23 16:13:02]

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

[commit-bot: I haz the power, 2016-07-23 16:13:09]

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, 2016-07-23 16:58:48]

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

[commit-bot: I haz the power, 2016-07-23 16:58:49]

Dry run: This issue passed the CQ dry run.

[yunchao, 2016-07-24 04:52:12]

yunchao.he@intel.com changed reviewers:
+ yunchao.he@intel.com

[yunchao, 2016-07-24 04:52:13]

I am not qualified to review your patch, Zhenyao. But the code LGTM.

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

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9140: return true;
On 2016/07/23 16:10:40, Zhenyao Mo wrote:
> This is actually the crash fix.  Everything else is cleanup.

I see...

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/program_manager.cc (left):

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.cc:377:
vertex_input_type_written_mask_.resize(packed_size);
On 2016/07/23 16:10:40, Zhenyao Mo wrote:
> We don't need to resize every link because they never change size.  So move it
> to Program constructor.  This probably isn't a perf gain at all, but
> conceptually I feel this is cleaner.

Acknowledged.

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/program_manager.h (right):

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.h:586: std::vector<uint32_t>
vertex_input_active_mask_;
On 2016/07/23 16:10:40, Zhenyao Mo wrote:
> I think this is a better name, since we don't write to the attribs in vertex
> shader like the fragment output variables.

Agreed!

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

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/vertex_attrib_manager.cc:163: GLuint shift_bits =
(index % 16) * 2;
On 2016/07/23 16:10:40, Zhenyao Mo wrote:
> We only need to do this if the glEnableDisable call actually change the state.

> This is to fit with the rest of the code - less confusing.

Acknowledged.

[piman, 2016-07-25 20:04:04]

LGTM + couple nits.

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

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.cc:314: packed_size +=
(manager_->max_vertex_attribs() % 16 == 0) ? 0 : 1;
nit: uint32_t packed_size = (manager_->max_vertex_attribs() + 15) / 16
Avoids an if test, extra modulo, and is standard pattern to round up.

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

https://codereview.chromium.org/2174173002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/vertex_attrib_manager.cc:131: packed_size +=
(max_vertex_attribs % 16 == 0) ? 0 : 1;
nit: ditto, uint32_t packed_size = (max_vertex_attribs + 15) / 16;

[Zhenyao Mo, 2016-07-25 23:52:36]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2016-07-25 23:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
yunchao.he@intel.com, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2174173002/#ps20001
(title: "update per piman review")

[commit-bot: I haz the power, 2016-07-25 23:53:34]

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-26 00:50:40]

Description was changed from

==========
current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
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

==========
current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
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-07-26 00:50:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-26 00:52:56]

Description was changed from

==========
current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
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

==========
current program can be null in ES2/ES3 contexts.

They are only required to be non null in WebGL.

This also did some clean up and a minor optimization.

BUG=630802
TEST=fuzzer case in the bug
R=piman@chromium.org
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/14ef40159115ca5059907835a34bcd1667af7631
Cr-Commit-Position: refs/heads/master@{#407659}
==========

[commit-bot: I haz the power, 2016-07-26 00:52:57]

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

[Mark P, 2016-07-26 04:43:05]

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

The reason for reverting is: Caused webkit test failure on WebKit Win7 (dbg)
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

unexpected_failures:
printing/webgl-oversized-printing.html
virtual/threaded/printing/webgl-oversized-printing.html

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...

---
---
E:\b\rr\tmpymgexl\w\layout-test-results\printing/webgl-oversized-printing-expected.txt
+++
E:\b\rr\tmpymgexl\w\layout-test-results\printing/webgl-oversized-printing-actual.txt
@@ -1,5 +1,5 @@
-PASS successfullyParsed is true
+CONSOLE ERROR: line 13: Uncaught TypeError: Cannot read property 'clearColor'
of null
+FAIL successfullyParsed should be true. Was false.
 
 TEST COMPLETE
-PASS Printed without crashing.
---

---
---
E:\b\rr\tmpymgexl\w\layout-test-results\virtual/threaded/printing/webgl-oversized-printing-expected.txt
+++
E:\b\rr\tmpymgexl\w\layout-test-results\virtual/threaded/printing/webgl-oversized-printing-actual.txt
@@ -1,5 +1,5 @@
-PASS successfullyParsed is true
+CONSOLE ERROR: line 13: Uncaught TypeError: Cannot read property 'clearColor'
of null
+FAIL successfullyParsed should be true. Was false.
 
 TEST COMPLETE
-PASS Printed without crashing.
---
.