Validate fbo color image format and fragment shader output variable type.

Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable corresponding to it.

This also fixes the issue where we fail to initialize uninitialized images before calling ClearBuffer*.

BUG=429053, 584059
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

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

[Zhenyao Mo, 2016-07-12 23:45:26]

Description was changed from

==========
Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable
corresponding to it.

BUG=429053
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
==========

to

==========
Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable
corresponding to it.

BUG=429053
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-07-12 23:48:08]

piman: This is not a complete CL (missing unit tests).

Could you take a look and let me know if you agree with the implementation?

If yes, then I'll add all the unit tests.

This change is for https://www.khronos.org/registry/webgl/specs/latest/2.0/#5.3

[Zhenyao Mo, 2016-07-12 23:48:45]

kbr, cwallez: FYI

[Zhenyao Mo, 2016-07-13 00:58:23]

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

[commit-bot: I haz the power, 2016-07-13 01:04:28]

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-13 01:57:06]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 01:57:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_optional_gpu_tests_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[Corentin Wallez, 2016-07-13 14:50:06]

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

[Corentin Wallez, 2016-07-13 14:50:07]

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

https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8070: adjusted_draw_buffers[ii]
= GL_NONE;
This looks like it will be doing an allocation for every draw call in WebGL2.
Can we avoid this cost, for example by lazy allocating adjusted_draw_buffers or
using dirty bits?

[Zhenyao Mo, 2016-07-13 15:39:10]

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

[commit-bot: I haz the power, 2016-07-13 15:39:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhenyao Mo, 2016-07-13 15:40:43]

On 2016/07/13 14:50:07, Corentin Wallez wrote:
>
https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:8070:
adjusted_draw_buffers[ii]
> = GL_NONE;
> This looks like it will be doing an allocation for every draw call in WebGL2.
> Can we avoid this cost, for example by lazy allocating adjusted_draw_buffers
or
> using dirty bits?

Thanks for the advice. I can track DrawBuffers at DrawBuffers call and when a
fbo is bound and an bound fbo is modified. So we don't have to handle this
filtering at draw call time.

[Zhenyao Mo, 2016-07-13 15:52:10]

On 2016/07/13 15:40:43, Zhenyao Mo wrote:
> On 2016/07/13 14:50:07, Corentin Wallez wrote:
> >
>
https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:8070:
> adjusted_draw_buffers[ii]
> > = GL_NONE;
> > This looks like it will be doing an allocation for every draw call in
WebGL2.
> > Can we avoid this cost, for example by lazy allocating adjusted_draw_buffers
> or
> > using dirty bits?
> 
> Thanks for the advice. I can track DrawBuffers at DrawBuffers call and when a
> fbo is bound and an bound fbo is modified. So we don't have to handle this
> filtering at draw call time.

That means I also need to track active program's shader output, which feels like
an overkill optimization.

I can keep adjusted_draw_buffers part of an fbo state. So we don't have to
allocate each draw call.

[Corentin Wallez, 2016-07-13 16:57:26]

On 2016/07/13 at 15:52:10, zmo wrote:
> On 2016/07/13 15:40:43, Zhenyao Mo wrote:
> > On 2016/07/13 14:50:07, Corentin Wallez wrote:
> > >
> >
https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:8070:
> > adjusted_draw_buffers[ii]
> > > = GL_NONE;
> > > This looks like it will be doing an allocation for every draw call in
WebGL2.
> > > Can we avoid this cost, for example by lazy allocating
adjusted_draw_buffers
> > or
> > > using dirty bits?
> > 
> > Thanks for the advice. I can track DrawBuffers at DrawBuffers call and when
a
> > fbo is bound and an bound fbo is modified. So we don't have to handle this
> > filtering at draw call time.
> 
> That means I also need to track active program's shader output, which feels
like an overkill optimization.
> 
> I can keep adjusted_draw_buffers part of an fbo state. So we don't have to
allocate each draw call.

Sounds good.

[commit-bot: I haz the power, 2016-07-13 17:09:36]

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

[commit-bot: I haz the power, 2016-07-13 17:09:37]

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

[Corentin Wallez, 2016-07-13 18:09:06]

On 2016/07/13 at 16:57:26, Corentin Wallez wrote:
> On 2016/07/13 at 15:52:10, zmo wrote:
> > On 2016/07/13 15:40:43, Zhenyao Mo wrote:
> > > On 2016/07/13 14:50:07, Corentin Wallez wrote:
> > > >
> > >
https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
> > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > > 
> > > >
> > >
https://codereview.chromium.org/2142353002/diff/1/gpu/command_buffer/service/...
> > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:8070:
> > > adjusted_draw_buffers[ii]
> > > > = GL_NONE;
> > > > This looks like it will be doing an allocation for every draw call in
WebGL2.
> > > > Can we avoid this cost, for example by lazy allocating
adjusted_draw_buffers
> > > or
> > > > using dirty bits?
> > > 
> > > Thanks for the advice. I can track DrawBuffers at DrawBuffers call and
when a
> > > fbo is bound and an bound fbo is modified. So we don't have to handle this
> > > filtering at draw call time.
> > 
> > That means I also need to track active program's shader output, which feels
like an overkill optimization.
> > 
> > I can keep adjusted_draw_buffers part of an fbo state. So we don't have to
allocate each draw call.
> 
> Sounds good.

I confirm that this patch fixes the draw-buffers failures on Linux NVIDIA.

[Zhenyao Mo, 2016-07-13 21:32:36]

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

[commit-bot: I haz the power, 2016-07-13 21:34:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Russell (switch to Gerrit), 2016-07-13 21:37:27]

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

[Ken Russell (switch to Gerrit), 2016-07-13 21:37:28]

This is awesome!

I'm not an OWNER but it LGTM overall. Good that we won't restore the DrawBuffers
setting unless we touched it.

[Zhenyao Mo, 2016-07-13 22:23:13]

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

[Zhenyao Mo, 2016-07-13 22:23:32]

piman: Please review.

[commit-bot: I haz the power, 2016-07-13 22:24:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhenyao Mo, 2016-07-13 22:24:30]

Description was changed from

==========
Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable
corresponding to it.

BUG=429053
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable
corresponding to it.

This also fixes the issue where we fail to initialize uninitialized images
before calling ClearBuffer*.

BUG=429053,584059
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-07-13 22:35:55]

https://codereview.chromium.org/2142353002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/2142353002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:525: for (size_t ii = 0; ii <
manager_->max_draw_buffers_; ++ii) {
This loop seems kind of expensive...
Truth is, we don't really handle more than 16 draw buffers in practice (since
validation disallows anything beyond GL_DRAW_BUFFER15), and there actually isn't
currently any GPU that handles more than 8 (according to
http://delphigl.de/glcapsviewer/gl_stats_caps_single.php?listreportsbycap=GL_...
)

I think we can make this significantly cheaper by coalescing the information
into bitfields, 2 bits per buffer (so 32 bits for the 16 buffers we support), so
that we can use bitwise logic operations to find whether the shader matches the
FBO:
- on the FBO side, one fbo_buffer_bound_mask for which the bit value is either
0x3 if the draw buffer is bound, 0x0 otherwise, and one fbo_buffer_type_mask
that contains the type (only relevant if the buffer is bound)
- on the shader side, similarly, shader_buffer_written_mask and
shader_buffer_type_mask

Then the logic for draw can be simplified to:
if (fbo_buffer_bound_mask & ~shader_buffer_written_mask)
  adjust = true;
if ((fbo_buffer_bound_mask & fbo_buffer_type_mask) != (fbo_buffer_bound_mask &
shader_buffer_type_mask))
  return false;

(and something equivalent for the clear case)

Then we only need a loop to generate the proper array for glDrawBuffers, in the
adjust case (which should be rare).

https://codereview.chromium.org/2142353002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:897: it->first >=
GL_COLOR_ATTACHMENT0 + manager_->max_draw_buffers_) {
this should be manager_->max_color_attachments_

https://codereview.chromium.org/2142353002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:900: size_t index = it->first
- GL_COLOR_ATTACHMENT0;
I think there's confusion about how color_attachment_base_types_ is indexed...
Here it's setup indexed by the color attachment index, but in
ValidateAndAdjustDrawBuffers,
it's looked up by draw buffer index.

I think it would make sense to keep the cache relative to the draw buffer index
- it would make the loop at draw time a bit cheaper. It means the loop in this
function needs to go over the draw buffers rather than the attachments. It means
that we have to run this again from SetDrawBuffers, but I think that's ok - I
prefer paying the cost at glDrawBuffers time rather than every draw call.

[piman, 2016-07-13 23:16:36]

https://codereview.chromium.org/2142353002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/2142353002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:900: size_t index = it->first
- GL_COLOR_ATTACHMENT0;
On 2016/07/13 22:35:55, piman wrote:
> I think there's confusion about how color_attachment_base_types_ is indexed...
> Here it's setup indexed by the color attachment index, but in
> ValidateAndAdjustDrawBuffers,
> it's looked up by draw buffer index.
> 
> I think it would make sense to keep the cache relative to the draw buffer
index
> - it would make the loop at draw time a bit cheaper. It means the loop in this
> function needs to go over the draw buffers rather than the attachments. It
means
> that we have to run this again from SetDrawBuffers, but I think that's ok - I
> prefer paying the cost at glDrawBuffers time rather than every draw call.

Ok, so I now understand, this works in ES because it has an extra constraint
that draw_buffers[i] can only be GL_COLOR_ATTACHMENTi or GL_NONE (as opposed to
desktop GL where no such constraint exist).

Still, looping over the draw buffers instead of the attachments is a bit
simpler, and lets us do some of the processing (the GL_NONE case) ahead of time.

[commit-bot: I haz the power, 2016-07-13 23:56:53]

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

[commit-bot: I haz the power, 2016-07-13 23:56:54]

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

[Zhenyao Mo, 2016-07-14 22:06:11]

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

[commit-bot: I haz the power, 2016-07-14 22:06:50]

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-14 22:06:51]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[Zhenyao Mo, 2016-07-14 22:10:17]

piman: Please review again.

I addressed your comments and also add comprehensive unit tests for these bit
mask functions.

[commit-bot: I haz the power, 2016-07-14 23:22:44]

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

[commit-bot: I haz the power, 2016-07-14 23:22:45]

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

[piman, 2016-07-14 23:41:14]

Thanks for the tests! A few comments, one functional typo, and some nits.

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:525: uint32_t mask = 0x33 <<
shift_bits;
Did you mean 0x3 instead of 0x33?

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:525: uint32_t mask = 0x33 <<
shift_bits;
nit: this |mask| hides the variable on l.515 - it would be nice to avoid.

Actually, if you do, you can combine the 2 ifs below, say:
uint32_t buffer_mask = 0x3 << shift_bits;
if ((mask & buffer_mask) == 0u)
  adjusted_draw_buffers_[ii] = GL_NONE;

(you want to set adjusted_draw_buffers_[ii] to GL_NONE if either the buffer
isn't present or not written to, and that's exactly if the corresponding bits in
the outer |mask| are 0).

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:540: // SHADER_VARIABLE_FLOAT
is 0x3.
nit: if you want to defend against this changing, you can use
0x55555555*SHADER_VARIABLE_FLOAT  (0x55555555 will broadcast the other value to
all slots).

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:869: ii < GL_COLOR_ATTACHMENT0
+ manager_->max_color_attachments_; ++ii) {
nit: how about: for (size_t index = 0; index < manager_->max_draw_buffers;
++index) {
  GLenum draw_buffer = draw_buffers_[index];
  if (draw_buffer == GL_NONE)
    continue;
  auto iter = attachments_.find(draw_buffer);
  [...]
}

It reads a little simpler, with no extraneous index math, and makes it clear
everything is indexed based on the draw buffer.

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:350: // "FragData0" and
"FragData1" returns 0.
I'm not sure why that is unexpected? Your usage of glBindFragDataLocationIndexed
sets the same location (draw buffer) 'loc' for both, and that's what's returned
by glGetFragDataLocation - glGetFragDataIndex would give a different result (0
and 1 respectively).
This is for dual-source blending. I'm not sure what the semantics should be for
this CL, if it is on - should the target be considered "written" if both sources
are written, or is only one enough?

[Zhenyao Mo, 2016-07-15 02:41:15]

Revised. Please take another look.

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:525: uint32_t mask = 0x33 <<
shift_bits;
On 2016/07/14 23:41:14, piman wrote:
> nit: this |mask| hides the variable on l.515 - it would be nice to avoid.
> 
> Actually, if you do, you can combine the 2 ifs below, say:
> uint32_t buffer_mask = 0x3 << shift_bits;
> if ((mask & buffer_mask) == 0u)
>   adjusted_draw_buffers_[ii] = GL_NONE;
> 
> (you want to set adjusted_draw_buffers_[ii] to GL_NONE if either the buffer
> isn't present or not written to, and that's exactly if the corresponding bits
in
> the outer |mask| are 0).

Done.

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:540: // SHADER_VARIABLE_FLOAT
is 0x3.
On 2016/07/14 23:41:14, piman wrote:
> nit: if you want to defend against this changing, you can use
> 0x55555555*SHADER_VARIABLE_FLOAT  (0x55555555 will broadcast the other value
to
> all slots).

Ha, nice trick.  Thanks.

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:869: ii < GL_COLOR_ATTACHMENT0
+ manager_->max_color_attachments_; ++ii) {
On 2016/07/14 23:41:14, piman wrote:
> nit: how about: for (size_t index = 0; index < manager_->max_draw_buffers;
> ++index) {
>   GLenum draw_buffer = draw_buffers_[index];
>   if (draw_buffer == GL_NONE)
>     continue;
>   auto iter = attachments_.find(draw_buffer);
>   [...]
> }
> 
> It reads a little simpler, with no extraneous index math, and makes it clear
> everything is indexed based on the draw buffer.

Done.

[Zhenyao Mo, 2016-07-15 02:42:50]

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

https://codereview.chromium.org/2142353002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6845: }
Have to take this out because it caused a lot of deqp tests to fail because they
allow undefined results.

Will do this and ClearBuffer similar situations in a followup CL.

[Zhenyao Mo, 2016-07-15 02:43:04]

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

[commit-bot: I haz the power, 2016-07-15 02:43:22]

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-15 02:43:25]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[piman, 2016-07-15 03:12:10]

lgtm

[commit-bot: I haz the power, 2016-07-15 03:40:55]

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

[commit-bot: I haz the power, 2016-07-15 03:40:56]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Zhenyao Mo, 2016-07-15 06:56:26]

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

[commit-bot: I haz the power, 2016-07-15 06:56:45]

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-15 06:56:48]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[Zhenyao Mo, 2016-07-15 07:02:05]

On 2016/07/15 03:12:10, piman wrote:
> lgtm

piman: Please take another look.  I had some logic error, so ended up calling
DrawBuffers every draw call.  Now it's fixed and added a unit test case to guard
it.

[commit-bot: I haz the power, 2016-07-15 08:34:25]

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

[commit-bot: I haz the power, 2016-07-15 08:34:26]

Dry run: This issue passed the CQ dry run.

[piman, 2016-07-15 18:57:25]

lgtm

[Zhenyao Mo, 2016-07-15 19:17:03]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2016-07-15 19:17:03]

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/2142353002/#ps120001
(title: "Fix")

[commit-bot: I haz the power, 2016-07-15 19:17:49]

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-15 19:17:52]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-15 19:23:35]

Description was changed from

==========
Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable
corresponding to it.

This also fixes the issue where we fail to initialize uninitialized images
before calling ClearBuffer*.

BUG=429053,584059
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable
corresponding to it.

This also fixes the issue where we fail to initialize uninitialized images
before calling ClearBuffer*.

BUG=429053,584059
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-07-15 19:23:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-15 19:23:46]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 19:26:52]

Description was changed from

==========
Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable
corresponding to it.

This also fixes the issue where we fail to initialize uninitialized images
before calling ClearBuffer*.

BUG=429053,584059
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Validate fbo color image format and fragment shader output variable type.

Also, set a draw buffer to NONE if there is no shader output variable
corresponding to it.

This also fixes the issue where we fail to initialize uninitialized images
before calling ClearBuffer*.

BUG=429053,584059
TEST=draw-buffers.html,gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-07-15 19:26:54]

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