Get ReadPixels to work with INT and UNSIGNED_INT format.

Get ReadPixels to work with INT and UNSIGNED_INT format.

In order to get it to work and test it, glClearBuffer*() has to be strengthened.

The remaining issue for ReadPixles is to return reasonable implementation
specific format/type. Currently it always return RGBA and UNSIGNED_BYTE,
which won't work for INT and UNSIGNED_INT framebuffer.

With this CL, read-pixels-from-fbo-test.html finally work (minus the
implementation specific format/type).

BUG=429053
TEST=conformance2/reading/read-pixels-from-fbo-test.html
R=sievers@chromium.org

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

[Zhenyao Mo, 2015-08-24 22:42:47]

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

[Zhenyao Mo, 2015-08-24 22:42:47]

sievers: PTAL
others: FYI (feel free to review)

[Zhenyao Mo, 2015-08-24 22:42:51]

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

[commit-bot: I haz the power, 2015-08-24 22:43:07]

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

[commit-bot: I haz the power, 2015-08-24 23:14:13]

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

[commit-bot: I haz the power, 2015-08-24 23:14:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Russell (switch to Gerrit), 2015-08-24 23:16:13]

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

[Ken Russell (switch to Gerrit), 2015-08-24 23:16:13]

Fantastic. Great work persevering with this. Non-owner LGTM.

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

https://codereview.chromium.org/1309333003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5778: // To avoid undefined
results, return without calling the gl function.
Here and throughout, I guess we're silently failing this call in the command
buffer? Do you think it's worth raising an error, as is defined in the WebGL
spec?

https://codereview.chromium.org/1309333003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6032:
framebuffer->ClearIntegerBuffers();
Is it worth tracking more state to know whether there are any integer buffers
attached to the current framebuffer (to save the iteration in the common case)?

[Zhenyao Mo, 2015-08-24 23:39:25]

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

https://codereview.chromium.org/1309333003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5778: // To avoid undefined
results, return without calling the gl function.
On 2015/08/24 23:16:13, Ken Russell wrote:
> Here and throughout, I guess we're silently failing this call in the command
> buffer? Do you think it's worth raising an error, as is defined in the WebGL
> spec?

No, we are silently passing through, no error generated.  According to ES3 spec,
no error should be generated, and the results are undefined.  So here we just do
nothing.  Generating an error is WebGL 2 specific and we should do it on the
blink side.

https://codereview.chromium.org/1309333003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6032:
framebuffer->ClearIntegerBuffers();
On 2015/08/24 23:16:13, Ken Russell wrote:
> Is it worth tracking more state to know whether there are any integer buffers
> attached to the current framebuffer (to save the iteration in the common
case)?

Good point. I'll revise accordingly.

[Zhenyao Mo, 2015-08-25 02:17:28]

Now the gpu_unittests should pass.

sievers: PTAL

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

https://codereview.chromium.org/1309333003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6032:
framebuffer->ClearIntegerBuffers();
On 2015/08/24 23:39:25, Zhenyao Mo wrote:
> On 2015/08/24 23:16:13, Ken Russell wrote:
> > Is it worth tracking more state to know whether there are any integer
buffers
> > attached to the current framebuffer (to save the iteration in the common
> case)?
> 
> Good point. I'll revise accordingly.

Actually it is no trivia code, so I'll do it in a followup.  Will add a TODO
here as reminder.

[Zhenyao Mo, 2015-08-25 02:17:32]

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

[commit-bot: I haz the power, 2015-08-25 02:17:45]

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

[Ken Russell (switch to Gerrit), 2015-08-25 02:18:39]

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

https://codereview.chromium.org/1309333003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6032:
framebuffer->ClearIntegerBuffers();
On 2015/08/25 02:17:28, Zhenyao Mo wrote:
> On 2015/08/24 23:39:25, Zhenyao Mo wrote:
> > On 2015/08/24 23:16:13, Ken Russell wrote:
> > > Is it worth tracking more state to know whether there are any integer
> buffers
> > > attached to the current framebuffer (to save the iteration in the common
> > case)?
> > 
> > Good point. I'll revise accordingly.
> 
> Actually it is no trivia code, so I'll do it in a followup.  Will add a TODO
> here as reminder.

Sounds fine. If it adds more complexity than it's worth, don't worry about it.

[commit-bot: I haz the power, 2015-08-25 03:37:42]

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

[commit-bot: I haz the power, 2015-08-25 03:37:43]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2015-08-26 20:32:46]

On 2015/08/25 03:37:43, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Ping

[no sievers, 2015-08-26 21:44:20]

lgtm

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

https://codereview.chromium.org/1309333003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5798: if
(!GLES2Util::IsSignedIntegerFormat(internal_format)) {
Is 'unsigned int' here really part of the undefined cases for which no
conversion is clearly defined? Similarly, 'signed' in the function below.

https://codereview.chromium.org/1309333003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5927: if
(!BoundFramebufferHasDepthAttachment() &&
'&& -> ||' ?

[Zhenyao Mo, 2015-08-26 23:09:55]

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

https://codereview.chromium.org/1309333003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5798: if
(!GLES2Util::IsSignedIntegerFormat(internal_format)) {
On 2015/08/26 21:44:20, sievers wrote:
> Is 'unsigned int' here really part of the undefined cases for which no
> conversion is clearly defined? Similarly, 'signed' in the function below.

For example, how do you convert a negative value to a unsigned value? Or how do
you convert a MAX_UINT to a signed? There are multiple ways to do that
(clamping, static_cast, etc), and the spec doesn't specify one, so that's
"undefined" in my eyes.

https://codereview.chromium.org/1309333003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5927: if
(!BoundFramebufferHasDepthAttachment() &&
On 2015/08/26 21:44:20, sievers wrote:
> '&& -> ||' ?

We still need to call the clear() if one of the depth/stencil exists.

[Zhenyao Mo, 2015-08-26 23:10:02]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2015-08-26 23:10:02]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1309333003/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-08-26 23:10:46]

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

[commit-bot: I haz the power, 2015-08-27 00:18:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-27 00:19:43]

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