Implement EXT_draw_buffers WebGL extention support in command buffer.

Implement EXT_draw_buffers WebGL extention support in command buffer.

A few things:
1) I only hooked up with desktop GL backend, i.e., GL_ARB_draw_buffers. Will add GLES support later.
2) Probably we need to do something to enable this for Pepper3D also? If yes, will do so in a seperate CL.
3) I didn't add the wordarounds for Mac/Linux driver bugs. Will do so in a seperate CL.

BUG=180397
TEST=gpu_unittests, webgl conformance tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188129

[Zhenyao Mo, 2013-03-07 01:06:58]

gman: please review.
jamesr: cc owner.
kbr, apatrick: FYI.

[greggman, 2013-03-08 21:48:28]

looks good mostly

needs unit tests for DrawBuffersEXT in gles2_cmd_decoder_unittest.cc

* Test that calling DrawBuffersEXT gets call correctly with valid args
* Test that calling DrawBuffersEXT when 0 is bound to the framebuffer translates
GL_BACK to GL_COLOR_ATTACHMENT0 if the backbuffer is really an FBO
* Test that calling DrawBuffersEXT with out of range values generates the
correct errors.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
gpu/command_buffer/service/feature_info.cc:617: if
(extensions.Contains("GL_ARB_draw_buffers")) {
Don't you also want 
  || extensions.Contains("GL_EXT_draw_buffers") 

as I think that's what ANGLE will return?

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:411: CHECK(index >= 0 && index
< kMaxDrawBuffers);
Do these need to be CHECKs? We don't do CHECKS for this kind of anywhere else.
CHECKs leave code in on release. It should be impossible to call this with a bad
value. DCHECK?

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:416: CHECK(n <=
kMaxDrawBuffers);
Same as above

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.h (right):

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.h:170: static const GLsizei
kMaxDrawBuffers = 16;
We don't do this for textures, why do it for draw buffers?

In other words, texture units are limited to glGetInteger(GL_MAX_TEXTURE_UNITS)
not GL_TEXTURE31 which is way lower than many current GPUs.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2884: 
It seems like you need some code here to work around that mac driver issue where
setting GL_COLOR_ATTACHMENTX to a framebuffer that doesn't have
GL_COLOR_ATTACHMENTX makes the framebuffer incomplete

Like you did for WebGL you need to call glDrawBufferEXT with GL_NONE for each
missing attachment no?

Ideally you'd only do this once so if attachments have not changed and
glDrawBufferEXT has not been called since for this framebuffer you don't need to
do it again.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4041: case GL_DRAW_BUFFER0_ARB:
If we're making this forward compatible it seems like rather than this being a
case it should be under default as in

   default:
     if (pname >= GL_DRAW_BUFFER0_ARB && 
         pname < GL_DRAW_BUFFER0_ARB + max_draw_buffers) {
       ...
       return true;
     }

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10002: if (framebuffer) {
framebuffer will be NULL if the current target is the backbuffer IIRC. You'll
need to do some other work here for the case where it's NULL. I'm guessing you
need to check that count/bufs is correct for the backbuffer. But then you need
to actually use GL_COLOR_ATTACHMENT0, GL_NONE etc since when it's not actually
the backbuffer.

[Zhenyao Mo, 2013-03-13 01:22:01]

Revised.  Please review again.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
gpu/command_buffer/service/feature_info.cc:617: if
(extensions.Contains("GL_ARB_draw_buffers")) {
On 2013/03/08 21:48:28, greggman wrote:
> Don't you also want 
>   || extensions.Contains("GL_EXT_draw_buffers") 
> 
> as I think that's what ANGLE will return?

I want to do it separately and explicitly when ANGLE EXT_draw_buffer is
implemented and tested.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:411: CHECK(index >= 0 && index
< kMaxDrawBuffers);
On 2013/03/08 21:48:28, greggman wrote:
> Do these need to be CHECKs? We don't do CHECKS for this kind of anywhere else.
> CHECKs leave code in on release. It should be impossible to call this with a
bad
> value. DCHECK?

Done.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:416: CHECK(n <=
kMaxDrawBuffers);
On 2013/03/08 21:48:28, greggman wrote:
> Same as above

Done.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.h (right):

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.h:170: static const GLsizei
kMaxDrawBuffers = 16;
On 2013/03/08 21:48:28, greggman wrote:
> We don't do this for textures, why do it for draw buffers?
> 
> In other words, texture units are limited to
glGetInteger(GL_MAX_TEXTURE_UNITS)
> not GL_TEXTURE31 which is way lower than many current GPUs.

Done.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2884: 
On 2013/03/08 21:48:28, greggman wrote:
> It seems like you need some code here to work around that mac driver issue
where
> setting GL_COLOR_ATTACHMENTX to a framebuffer that doesn't have
> GL_COLOR_ATTACHMENTX makes the framebuffer incomplete
> 
> Like you did for WebGL you need to call glDrawBufferEXT with GL_NONE for each
> missing attachment no?
> 
> Ideally you'd only do this once so if attachments have not changed and
> glDrawBufferEXT has not been called since for this framebuffer you don't need
to
> do it again.

Plan to do the workaround in a separate CL. Easy to review and track in the
future.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4041: case GL_DRAW_BUFFER0_ARB:
On 2013/03/08 21:48:28, greggman wrote:
> If we're making this forward compatible it seems like rather than this being a
> case it should be under default as in
> 
>    default:
>      if (pname >= GL_DRAW_BUFFER0_ARB && 
>          pname < GL_DRAW_BUFFER0_ARB + max_draw_buffers) {
>        ...
>        return true;
>      }

Done.

https://codereview.chromium.org/12545014/diff/45/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10002: if (framebuffer) {
On 2013/03/08 21:48:28, greggman wrote:
> framebuffer will be NULL if the current target is the backbuffer IIRC. You'll
> need to do some other work here for the case where it's NULL. I'm guessing you
> need to check that count/bufs is correct for the backbuffer. But then you need
> to actually use GL_COLOR_ATTACHMENT0, GL_NONE etc since when it's not actually
> the backbuffer.

Done.

[Zhenyao Mo, 2013-03-13 23:17:45]

Gregg, please have a look.  I think the bots are green again (see mac_asan)
after I fixed the memory bug.

[greggman, 2013-03-14 00:51:09]

just a couple of minor things then LGTM

https://codereview.chromium.org/12545014/diff/82003/gpu/command_buffer/common...
File gpu/command_buffer/common/gles2_cmd_utils.cc (right):

https://codereview.chromium.org/12545014/diff/82003/gpu/command_buffer/common...
gpu/command_buffer/common/gles2_cmd_utils.cc:627: case GL_COLOR_ATTACHMENT0:
This is going to have to change. Either you have to pass in
max_color_attachments or the places that call this need to check color
attachment separately or something.

https://codereview.chromium.org/12545014/diff/82003/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12545014/diff/82003/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10191: if (bufs[i] == GL_BACK)
It's this correct. I thought the only valid values for DrawBuffers on fbo 0 are

 bufs[0] == GL_NONE || bufs[0] == GL_BACK

and all other bufs have to be GL_NONE

no?

[Zhenyao Mo, 2013-03-14 01:49:34]

jamesr: cc/ owner review.

https://codereview.chromium.org/12545014/diff/82003/gpu/command_buffer/common...
File gpu/command_buffer/common/gles2_cmd_utils.cc (right):

https://codereview.chromium.org/12545014/diff/82003/gpu/command_buffer/common...
gpu/command_buffer/common/gles2_cmd_utils.cc:627: case GL_COLOR_ATTACHMENT0:
On 2013/03/14 00:51:10, greggman wrote:
> This is going to have to change. Either you have to pass in
> max_color_attachments or the places that call this need to check color
> attachment separately or something.

Done.

https://codereview.chromium.org/12545014/diff/82003/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12545014/diff/82003/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10191: if (bufs[i] == GL_BACK)
On 2013/03/14 00:51:10, greggman wrote:
> It's this correct. I thought the only valid values for DrawBuffers on fbo 0
are
> 
>  bufs[0] == GL_NONE || bufs[0] == GL_BACK
> 
> and all other bufs have to be GL_NONE
> 
> no? 

Done.

[jamesr, 2013-03-14 18:00:47]

cc lgtm

[Zhenyao Mo, 2013-03-14 18:04:52]

Committed patchset #8 manually as r188129 (presubmit successful).