Upgrade PixelStorei to ES3/WebGL2.

Upgrade PixelStorei to ES3/WebGL2.

BUG=295792, 429053
TEST=gpu_unittests, webgl2_conformance
R=kbr@chromium.org,bajones@chromium.org,piman@chromium.org

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

[Zhenyao Mo, 2015-11-24 23:38:51]

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

[Zhenyao Mo, 2015-11-24 23:40:23]

kbr, bajones: blink side
piman, bajones: command buffer side

Please take a look.

(Note that I won't be able to properly test the effects of these parameters
until I implement 3D texture uploading path, but this is blocking that, so I
request landing this without WebGL conformance test coverage, and I'll add the
coverage when I implement 3D texture uploading)

[commit-bot: I haz the power, 2015-11-24 23:41:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1474513003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1474513003/40001

[bajones, 2015-11-25 00:03:05]

https://codereview.chromium.org/1474513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1474513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:880:
resetUnpackParameters();
There are several similar bits of code in WebGLRenderingContextBase, and it
seems like with WebGL2 they'll want to reset the full set of unpack params as
well. I think reset/restoreUnpackParameters should be a virtual method defined
by WebGLRenderingContextBase that only resets the unpack alignment, and then is
overridden in WebGL2 with the logic you have here.

[Ken Russell (switch to Gerrit), 2015-11-25 00:59:41]

https://codereview.chromium.org/1474513003/diff/40001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1474513003/diff/40001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:10245: f.write("  if
(feature_info_->IsES3Enabled()) {\n");
This needs to be IsES3Capable() because we may switch between virtual contexts
that do and do not have ES3 enabled. See higher up in the autogenerated file
around the handling of GL_RASTERIZER_DISCARD.

https://codereview.chromium.org/1474513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1474513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:880:
resetUnpackParameters();
On 2015/11/25 00:03:05, bajones wrote:
> There are several similar bits of code in WebGLRenderingContextBase, and it
> seems like with WebGL2 they'll want to reset the full set of unpack params as
> well. I think reset/restoreUnpackParameters should be a virtual method defined
> by WebGLRenderingContextBase that only resets the unpack alignment, and then
is
> overridden in WebGL2 with the logic you have here.

Agree with this. It looks like WebGLRenderingContextBase will assume at least
that GL_ROW_WIDTH is reset to the default. Hopefully the already ported dEQP
tests will cover these code paths well.

[commit-bot: I haz the power, 2015-11-25 03:55:09]

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

[commit-bot: I haz the power, 2015-11-25 03:55:10]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2015-11-25 18:18:48]

Revised. Please take another look.

https://codereview.chromium.org/1474513003/diff/40001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1474513003/diff/40001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:10245: f.write("  if
(feature_info_->IsES3Enabled()) {\n");
On 2015/11/25 00:59:41, Ken Russell wrote:
> This needs to be IsES3Capable() because we may switch between virtual contexts
> that do and do not have ES3 enabled. See higher up in the autogenerated file
> around the handling of GL_RASTERIZER_DISCARD.

Done.

https://codereview.chromium.org/1474513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1474513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:880:
resetUnpackParameters();
On 2015/11/25 00:59:41, Ken Russell wrote:
> On 2015/11/25 00:03:05, bajones wrote:
> > There are several similar bits of code in WebGLRenderingContextBase, and it
> > seems like with WebGL2 they'll want to reset the full set of unpack params
as
> > well. I think reset/restoreUnpackParameters should be a virtual method
defined
> > by WebGLRenderingContextBase that only resets the unpack alignment, and then
> is
> > overridden in WebGL2 with the logic you have here.
> 
> Agree with this. It looks like WebGLRenderingContextBase will assume at least
> that GL_ROW_WIDTH is reset to the default. Hopefully the already ported dEQP
> tests will cover these code paths well.

Done.

[Zhenyao Mo, 2015-11-25 18:18:54]

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

[commit-bot: I haz the power, 2015-11-25 18:20:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1474513003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1474513003/60001

[commit-bot: I haz the power, 2015-11-25 21:13:54]

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

[commit-bot: I haz the power, 2015-11-25 21:13:55]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2015-11-25 21:16:51]

OK, try bots are happy.

PTAL.

[bajones, 2015-11-25 21:29:53]

LGTM, thanks for the revisions!

[Ken Russell (switch to Gerrit), 2015-11-25 21:34:56]

Very nice. LGTM

[piman, 2015-11-25 23:00:20]

not lgtm, I don't think this is what we want to do.

PixelStore configures the layout of 2D memory as it goes in and out of the API.
For example, how to interpret the |data| parameter to glTexImage2D, or
glReadPixels

If we blindly send them and apply them service-side, it means that:
- on the client side, when we copy things in and out of shared memory, we're not
respecting the PixelStore parameters, so we're not looking at the memory as the
client configured it
- on the service side, we tell GL to look at the data in shared memory in a
different way than the way it's actually laid out.

IOW, this would be a tool to let WebGL dereference outside of the array buffers
passed to the API (possibly causing memory corruption and js->native elevation),
and to let the client tell the server to dereference outside of the shared
memory (possibly causing memory corruption and renderer->GPU elevation).


I think instead, we need a lot more logic:
- on the client side, track all those values. When doing a transfer, size the
  shared memory based on assuming all these new values are 0 (except for the
  alignment, which is already handled, and makes sense to handle a priori). An
  argument could be made to also size using the real row_length, but
  that's a tradeoff between copying more data in one batch vs copying less data
  but 1 batch per line. Copy the data into shared memory by taking all those
  values into account to properly address into the user buffer.
- on the service side, keep all values at 0 (except alignment, and possibly
  row_length depending on the above).

Now, that's for when the data doesn't come from a buffer object. If the data
does come from a buffer object, then we do need to pass along the right values
service-side. So in reality, we still want to send those values over in the
command buffer, but they need to be tracked there, and must not be blindly sent
to GL, instead syncing them to the right values before each transfer.

[Zhenyao Mo, 2015-12-01 00:13:42]

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

[Zhenyao Mo, 2015-12-01 00:14:37]

piman: Please look again.

Note that I didn't fix the Computation of image size for texture uploading from
PIXEL_UNPACK_BUFFER yet. Will do that in the next CL.

[commit-bot: I haz the power, 2015-12-01 00:14:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1474513003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1474513003/100001

[piman, 2015-12-01 01:27:34]

https://codereview.chromium.org/1474513003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1474513003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8893: // ES3, including more
pack parameters.
For safety, because we're not doing range checking on the pixel pack buffer, I
think the right thing to do is to fail with INVALID_OPERATION if a pack buffer
is bound, until it's fully implemented.

https://codereview.chromium.org/1474513003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9083: // To simply the state
tracking, we don't go down the asyn path if
nit: asyn->async

[piman, 2015-12-01 01:30:52]

Also, the client-side pack values are not cached/handled for
GLES2Implementation::ReadPixels, like the unpack ones are for Tex*Image*

[commit-bot: I haz the power, 2015-12-01 03:58:31]

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

[commit-bot: I haz the power, 2015-12-01 03:58:32]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2015-12-01 22:11:53]

We also need to update WebGL ReadPixels array buffer size validation to
accommodate the ES3 pack parameters.  However, it seems endless loop, and I have
to stop somewhere for this CL.

piman: PTAL

https://codereview.chromium.org/1474513003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1474513003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8893: // ES3, including more
pack parameters.
On 2015/12/01 01:27:33, piman (slow to review) wrote:
> For safety, because we're not doing range checking on the pixel pack buffer, I
> think the right thing to do is to fail with INVALID_OPERATION if a pack buffer
> is bound, until it's fully implemented.

Done.

https://codereview.chromium.org/1474513003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9083: // To simply the state
tracking, we don't go down the asyn path if
On 2015/12/01 01:27:33, piman (slow to review) wrote:
> nit: asyn->async

Done.

[Zhenyao Mo, 2015-12-01 22:13:33]

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

[commit-bot: I haz the power, 2015-12-01 22:16:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1474513003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1474513003/120001

[Zhenyao Mo, 2015-12-01 22:26:43]

Patchset #8 (id:140001) has been deleted

[Zhenyao Mo, 2015-12-01 22:26:53]

Patchset #7 (id:120001) has been deleted

[Zhenyao Mo, 2015-12-01 22:26:58]

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

[commit-bot: I haz the power, 2015-12-01 22:27:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1474513003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1474513003/160001

[commit-bot: I haz the power, 2015-12-02 00:26:57]

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

[commit-bot: I haz the power, 2015-12-02 00:26:58]

Dry run: This issue passed the CQ dry run.

[piman, 2015-12-02 01:11:59]

LGTM, looking forward to follow ups.

[Zhenyao Mo, 2015-12-02 01:16:55]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2015-12-02 01:16:55]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/1474513003/#ps160001
(title: "client side")

[commit-bot: I haz the power, 2015-12-02 01:19:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1474513003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1474513003/160001

[commit-bot: I haz the power, 2015-12-02 02:16:42]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-12-02 02:17:27]

Description was changed from

==========
Upgrade PixelStorei to ES3/WebGL2.

BUG=295792,429053
TEST=gpu_unittests, webgl2_conformance
R=kbr@chromium.org,bajones@chromium.org,piman@chromium.org
==========

to

==========
Upgrade PixelStorei to ES3/WebGL2.

BUG=295792,429053
TEST=gpu_unittests, webgl2_conformance
R=kbr@chromium.org,bajones@chromium.org,piman@chromium.org

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

[commit-bot: I haz the power, 2015-12-02 02:17:28]

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

[Jamie Madill, 2015-12-02 13:50:08]

A revert of this CL (patchset #7 id:160001) has been created in
https://codereview.chromium.org/1494553002/ by jmadill@chromium.org.

The reason for reverting is: Failing the WebGL 2 CTS:

WebglConformance.conformance2_state_gl_get_calls:
[3560:1299:1202/051124:INFO:CONSOLE(11)]
"context.getParameter(context.UNPACK_SKIP_IMAGES) should be false (of type
boolean). Was 0 (of type number).", source:  (11)
[3560:1299:1202/051124:INFO:CONSOLE(11)] "FAIL
context.getParameter(context.UNPACK_SKIP_IMAGES) should be false (of type
boolean). Was 0 (of type number).", source:  (11)
[3560:1299:1202/051124:INFO:CONSOLE(11)]
"context.getParameter(context.UNPACK_SKIP_PIXELS) should be false (of type
boolean). Was 0 (of type number).", source:  (11)
[3560:1299:1202/051124:INFO:CONSOLE(11)] "FAIL
context.getParameter(context.UNPACK_SKIP_PIXELS) should be false (of type
boolean). Was 0 (of type number).", source:  (11)
[3560:1299:1202/051124:INFO:CONSOLE(11)]
"context.getParameter(context.UNPACK_SKIP_ROWS) should be false (of type
boolean). Was 0 (of type number).", source:  (11)
[3560:1299:1202/051124:INFO:CONSOLE(11)] "FAIL
context.getParameter(context.UNPACK_SKIP_ROWS) should be false (of type
boolean). Was 0 (of type number).", source:  (11)

http://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.10%20Release%2...
http://build.chromium.org/p/chromium.gpu.fyi/builders/Win8%20Debug%20%28NVIDI...
http://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28NV....

[Zhenyao Mo, 2015-12-02 17:29:40]

On 2015/12/02 13:50:08, Jamie Madill wrote:
> A revert of this CL (patchset #7 id:160001) has been created in
> https://codereview.chromium.org/1494553002/ by mailto:jmadill@chromium.org.
> 
> The reason for reverting is: Failing the WebGL 2 CTS:
> 
> WebglConformance.conformance2_state_gl_get_calls:
> [3560:1299:1202/051124:INFO:CONSOLE(11)]
> "context.getParameter(context.UNPACK_SKIP_IMAGES) should be false (of type
> boolean). Was 0 (of type number).", source:  (11)
> [3560:1299:1202/051124:INFO:CONSOLE(11)] "FAIL
> context.getParameter(context.UNPACK_SKIP_IMAGES) should be false (of type
> boolean). Was 0 (of type number).", source:  (11)
> [3560:1299:1202/051124:INFO:CONSOLE(11)]
> "context.getParameter(context.UNPACK_SKIP_PIXELS) should be false (of type
> boolean). Was 0 (of type number).", source:  (11)
> [3560:1299:1202/051124:INFO:CONSOLE(11)] "FAIL
> context.getParameter(context.UNPACK_SKIP_PIXELS) should be false (of type
> boolean). Was 0 (of type number).", source:  (11)
> [3560:1299:1202/051124:INFO:CONSOLE(11)]
> "context.getParameter(context.UNPACK_SKIP_ROWS) should be false (of type
> boolean). Was 0 (of type number).", source:  (11)
> [3560:1299:1202/051124:INFO:CONSOLE(11)] "FAIL
> context.getParameter(context.UNPACK_SKIP_ROWS) should be false (of type
> boolean). Was 0 (of type number).", source:  (11)
> 
>
http://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.10%20Release%2...
>
http://build.chromium.org/p/chromium.gpu.fyi/builders/Win8%20Debug%20%28NVIDI...
>
http://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28NV....

Jamie, my CL is correct and the test is wrong.  So I am going to reland and
suppress the failure.

(I informed capn not to revert, but forget you are constantly watching the
bots.)