command_buffer: Implement EXT_blend_func_extended

command_buffer: Implement EXT_blend_func_extended

Implement EXT_blend_func_extended for command buffer ES 2.0 and ES 3.0
contexts.

For the ES 2.0 context, the extension supports dual-source blending with
pre-defined gl_SecondaryFragColorEXT and gl_SecondaryFragDataEXT
variables.

Currently EXT_blend_func_extended is only exposed if the service context
supports program interface query. This means OpenGL context or OpenGL
ES 3.1 context. This is to simplify the unit test expectation conditions.
Theoretically also ES 2.0 and ES 3.0 service contexts could support
EXT_blend_func_extended, but probably there will never be such a driver.

BUG=506765
Committed: https://crrev.com/8cefb231216c2777281f37a30e3fb8e67513ee2b
Cr-Commit-Position: refs/heads/master@{#363175}

[Kimmo Kinnunen, 2015-08-28 08:38:33]

kkinnunen@nvidia.com changed reviewers:
+ piman@chromium.org, vmiura@chromium.org, zmo@google.com

[Mark Kilgard, 2015-08-28 19:53:23]

markkilgard@gmail.com changed reviewers:
+ markkilgard@gmail.com

[Mark Kilgard, 2015-08-28 19:53:25]

LGTM

good testing for all this

1) perhaps gl_bindings_skia_in_process.cc changes could be reverted?

2) arguably EXT_gpu_shader4 changes aren't practically needed (though
technically correct)

For the record, the EXT_blend_func_extended is now checked into the Khronos
subversion tree:

  
https://cvs.khronos.org/svn/repos/registry/trunk/gles/extensions/EXT/EXT_blen...

Should be public soon.

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_group.cc:152: max_dual_source_draw_buffers_ =
0;
Context: just to set expectations, the value of GL_MAX_DUAL_SOURCE_DRAW_BUFFERS
is 1 for all GPUs which I am aware (NVIDIA or otherwise).

In other words, this functionality isn't really orthogonal to multiple render
targets.

This kind of makes sense because source two colors for the blend function is a
different way of using multiple shader color outputs that writing to multiple
color buffers.

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:1080:
validators_.dst_blend_factor.AddValue(GL_SRC_ALPHA_SATURATE_EXT);
good, this is actually explicitly documented in the EXT_blend_func_extended
extension and makes blend functionality more consistent with ES 3.x and big
OpenGL

In practical terms, nobody is going to care that SRC_ALPHA_SATURATE is allowed
on a dest function, but it makes everything more othogonal, particularly the
source and destination error checks are identical

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

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9348: }
yes, good to add this link check here

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/memory_program_cache.cc (left):

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/memory_program_cache.cc:46: VARYING_MAP
agreed, never used later

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/memory_program_cache.cc (right):

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/memory_program_cache.cc:384: this));
ignoring the code reformatting clean-up, the substantive change is adding
"fragment_output_variables"

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

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:668: }
agree with all this

the point being non-ES 2.0 shading languages don't have the gl_*Frag* variables
for this.  But ES 2.0 GLSL didn't have the layout sophistication of modern GLSL
dialects.

Good bridging.

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager_unittest.cc:82: feature_info_ =
nullptr;
is there a style convention for nullptr vs. NULL in this file? 

this source file seems to mix the 2 conventions a lot

not for cleaning up in this change...

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager_unittest.cc:2376: 
good testing for the various variants allowed

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/shader_manager.cc:75: &output_variable_list_,
&name_map_);
ignoring style rewrite, difference is the addition of "output_variable_list"

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc (right):

https://codereview.chromium.org/1309743005/diff/60001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc:356: }
very nice testing

https://codereview.chromium.org/1309743005/diff/60001/ui/gl/gl_bindings_autog...
File ui/gl/gl_bindings_autogen_gl.cc (right):

https://codereview.chromium.org/1309743005/diff/60001/ui/gl/gl_bindings_autog...
ui/gl/gl_bindings_autogen_gl.cc:611: } else if (ext.b_GL_EXT_gpu_shader4 ||
ext.b_GL_EXT_blend_func_extended) {
technically, correct to check for EXT_gpu_shader4

but practical existence of a driver that was pre-OpenGL 3.0 supporting
EXT_gpu_shader4 is minimal; still, this is correct

https://codereview.chromium.org/1309743005/diff/60001/ui/gl/gl_bindings_autog...
File ui/gl/gl_bindings_autogen_gl.h (right):

https://codereview.chromium.org/1309743005/diff/60001/ui/gl/gl_bindings_autog...
ui/gl/gl_bindings_autogen_gl.h:1059: bool b_GL_EXT_gpu_shader4;
right, for glBindFragDataLocationEXT which EXT_gpu_shader4 adds

but also OpenGL 3.0 adds glBindFragDataLocation 

motivation for EXT_gpu_shader4 is low but looks correct

https://codereview.chromium.org/1309743005/diff/60001/ui/gl/gl_bindings_skia_...
File ui/gl/gl_bindings_skia_in_process.cc (left):

https://codereview.chromium.org/1309743005/diff/60001/ui/gl/gl_bindings_skia_...
ui/gl/gl_bindings_skia_in_process.cc:989: StubGLBindFragDataLocationIndexed;
Am I missing something?

Things are moved around in this file but I don't see any change of substance.

Could this file go unchanged?

[Kimmo Kinnunen, 2015-08-31 12:22:59]

https://chromiumcodereview.appspot.com/1309743005/diff/60001/gpu/command_buff...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/1309743005/diff/60001/gpu/command_buff...
gpu/command_buffer/service/program_manager_unittest.cc:82: feature_info_ =
nullptr;
On 2015/08/28 19:53:24, Mark Kilgard wrote:
> is there a style convention for nullptr vs. NULL in this file? 
> 
> this source file seems to mix the 2 conventions a lot
> 
> not for cleaning up in this change...

I think I've been instructed to use nullptr for the lines I change. It's not
always easy to notice the NULLs, sometimes the auto-formatter causes additional
NULLs to become "your changes". Well, those probably still should be changed,
aswell. Anyway, I've cleaned up this patch to use nullptrs.

https://chromiumcodereview.appspot.com/1309743005/diff/60001/gpu/command_buff...
File gpu/command_buffer/service/shader_manager.cc (right):

https://chromiumcodereview.appspot.com/1309743005/diff/60001/gpu/command_buff...
gpu/command_buffer/service/shader_manager.cc:75: &output_variable_list_,
&name_map_);
On 2015/08/28 19:53:25, Mark Kilgard wrote:
> ignoring style rewrite, difference is the addition of "output_variable_list"

Just a FYI: the style changes come by default from the tooling.

[Zhenyao Mo, 2015-08-31 21:12:29]

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

[Zhenyao Mo, 2015-08-31 21:12:31]

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/build_gles2_cmd_buffer.py:2825: 'unsafe': True,
Remove the 'unsafe'. It is only intended for ES3 functions that are not fully
validated in the command buffer service side before passing down to the driver.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/build_gles2_cmd_buffer.py:3982: 'unsafe': True,
Remove unsafe.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/build_gles2_cmd_buffer.py:3991: 'unsafe': True,
Remove unsafe.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
File gpu/command_buffer/service/context_group.cc (right):

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/context_group.cc:148:
GetIntegerv(GL_MAX_DUAL_SOURCE_DRAW_BUFFERS,
GL_MAX_DUAL_SOURCE_DRAW_BUFFERS_EXT, or on some machines it may fail to find the
enum def.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
File gpu/command_buffer/service/disk_cache_proto.proto (right):

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/disk_cache_proto.proto:35: repeated
ShaderAttributeProto output_variables = 5;
Have you verified what are the returned values for ShGetOutputVariables() in
ES2? Are they empty or gl_FragColor/gl_FragData?

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5659: if
(!unsafe_es3_apis_enabled())
No need for this as the extension can be applied to ES2 also.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5711: if
(!unsafe_es3_apis_enabled())
No need for this.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5724: (index == 1 && colorName
>= group_->max_dual_source_draw_buffers())) {
I think it should be index >= 1 instead of index == 1.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9407: 
nit: remove the empty line.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
File gpu/command_buffer/service/program_manager.cc (right):

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/program_manager.cc:646: DCHECK(output_var.location ==
-1);
nit: Use DCHECK_EQ and DCHECK_NE here and below.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/program_manager.cc:1874: return
glGetFragDataLocation(service_id_, original_name.c_str());
This is incorrect (based on my understanding that output variables are also
name-mapped during ANGLE's shader translation stage). Should use mapped_name to
call underlying driver.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/program_manager.cc:1884: return
glGetFragDataIndex(service_id_, original_name.c_str());
Same here, pass down mapped_name to the driver.

Please add a ES3 test case where the output variable names are mapped to catch
this bug.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
File gpu/command_buffer/service/shader_manager.cc (right):

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/service/shader_manager.cc:205: return output_var.mappedName
== original_name;
This looks incorrect.  It should be output_var.originalName == original_name.

Also, at least add a TODO that if this lookup ever becomes perf concern,
implement a hash_map for the data.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
File gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc (right):

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc:24: float
Weight(float dst[4], float src[4], float src1[4]) {
What's the point of passing in dst? It's never used.

https://chromiumcodereview.appspot.com/1309743005/diff/100001/gpu/command_buf...
gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc:345: for (int i
= 0; i < 2; ++i) {
Why run it twice?

[Mark Kilgard, 2015-09-03 23:12:03]

Spec now public in Khronos OpenGL ES registry:

https://www.khronos.org/registry/gles/
https://www.khronos.org/registry/gles/extensions/EXT/EXT_blend_func_extended.txt

[Mark Kilgard, 2015-09-15 21:17:30]

On 2015/09/03 23:12:03, Mark Kilgard wrote:
> Spec now public in Khronos OpenGL ES registry:
> 
> https://www.khronos.org/registry/gles/
>
https://www.khronos.org/registry/gles/extensions/EXT/EXT_blend_func_extended.txt

Windows and Linux drivers for NVIDIA GPUs supporting this extension (and
EXT_multisample_compatibility) are available from here:

  https://developer.nvidia.com/opengl-driver

The Windows driver version is 355.97; Linux is 355.00.09.

Create an OpenGL ES context (this driver provides OpenGL ES 3.2 contexts) will
support both EXT_blend_func_extended and EXT_multisample_compatibility.  You'll
need a Fermi-, Kepler-, or Maxwell-based GPU.

This is a developer beta driver but fully functional.  Public drivers will
support this functionality soon as well.

[Kimmo Kinnunen, 2015-09-24 13:16:28]

Thanks for the comments. A new version with consistent queries of output
variables from the driver, PTAL when you have time.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/bui...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:2825: 'unsafe': True,
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> Remove the 'unsafe'. It is only intended for ES3 functions that are not fully
> validated in the command buffer service side before passing down to the
driver.

Done.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3982: 'unsafe': True,
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> Remove unsafe.

Done.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:3991: 'unsafe': True,
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> Remove unsafe.

Done.

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

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.cc:148:
GetIntegerv(GL_MAX_DUAL_SOURCE_DRAW_BUFFERS,
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> GL_MAX_DUAL_SOURCE_DRAW_BUFFERS_EXT, or on some machines it may fail to find
the
> enum def.

Done.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/disk_cache_proto.proto (right):

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/disk_cache_proto.proto:35: repeated
ShaderAttributeProto output_variables = 5;
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> Have you verified what are the returned values for ShGetOutputVariables() in
> ES2? Are they empty or gl_FragColor/gl_FragData?

Yeah, I implemented it in ANGLE for purposes of this patch.

For vertex shaders, it's empty. I can fix it to be whatever you want, if we need
vertex outputs for consistency.

For fragment shaders, it might contain gl_FragColor, gl_FragData,
gl_SecondaryFragColor, gl_SecondaryFragData, gl_FragDepthEXT.

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

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5659: if
(!unsafe_es3_apis_enabled())
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> No need for this as the extension can be applied to ES2 also.

No, these functions are not specified in ext_bfe for ES2.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5711: if
(!unsafe_es3_apis_enabled())
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> No need for this.

Not specified for ES2.
For this usecase, ES2 version of the extension adds "gl_SecondaryFragColor" /
"gl_SecondaryFragData", which go to color index 1 of the blend equation.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5724: (index == 1 && colorName
>= group_->max_dual_source_draw_buffers())) {
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> I think it should be index >= 1 instead of index == 1.

Nope. In the extension, it says index must be 0 or 1.
" <index> may be zero or one to specify that the color
    be used as either the first or second color input to the blend
    equation, respectively, as described in Section 15.1.5 (Blending)."

My interpretation of the GL text: 

The idea of the extension is that there's 2 colors in blend equation: src and
src1.
The result color is the combination of these.

The api allows arbitrary number of src colors. However, it arbitrary blend
equation is not logical with the current blend equation, as adding colors would
not increase the expressiveness of the blend equation, as there's only so many
"slots" to define what source goes where.

So if, in the future bfe2 would invent new blend equation (more slots), more
source colors would be important. In this case, the binding functions wouldn't
necessarily need to be extended, as they would already support more indeices.

Just my guess why it's written the way it is..

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9407: 
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> nit: remove the empty line.

Done.

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

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:646: DCHECK(output_var.location ==
-1);
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> nit: Use DCHECK_EQ and DCHECK_NE here and below.

Done.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:1874: return
glGetFragDataLocation(service_id_, original_name.c_str());
On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> This is incorrect (based on my understanding that output variables are also
> name-mapped during ANGLE's shader translation stage). Should use mapped_name
to
> call underlying driver.

Done.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:1884: return
glGetFragDataIndex(service_id_, original_name.c_str());
On 2015/08/31 21:12:31, Zhenyao Mo wrote:
> Same here, pass down mapped_name to the driver.
> 
> Please add a ES3 test case where the output variable names are mapped to catch
> this bug.

Done.

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

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_manager.cc:205: return output_var.mappedName
== original_name;
On 2015/08/31 21:12:31, Zhenyao Mo wrote:
> This looks incorrect.  It should be output_var.originalName == original_name.

Done.

> Also, at least add a TODO that if this lookup ever becomes perf concern,
> implement a hash_map for the data.

The idea is to use list rather than map, since there is ever only few output
variables.
I added an comment. We can use a map, if need be.

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc (right):

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc:24: float
Weight(float dst[4], float src[4], float src1[4]) {
On 2015/08/31 21:12:31, Zhenyao Mo wrote:
> What's the point of passing in dst? It's never used.
Just for clarity. The weight function of GL blend equation has DST as its input.

Similar to below, where BlendEquationFuncAdd would not need all those terms, but
without those, it would be less evident what it tries to model.

I'll /*dst*/ it for clarity. Hope that's ok?

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc:345: for (int i
= 0; i < 2; ++i) {
On 2015/08/31 21:12:31, Zhenyao Mo wrote:
> Why run it twice?

To confirm the setters do not affect the getters until link. I'll add a comment.

[Zhenyao Mo, 2015-09-30 00:23:48]

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/disk_cache_proto.proto (right):

https://codereview.chromium.org/1309743005/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/disk_cache_proto.proto:35: repeated
ShaderAttributeProto output_variables = 5;
On 2015/09/24 13:16:27, Kimmo Kinnunen wrote:
> On 2015/08/31 21:12:30, Zhenyao Mo wrote:
> > Have you verified what are the returned values for ShGetOutputVariables() in
> > ES2? Are they empty or gl_FragColor/gl_FragData?
> 
> Yeah, I implemented it in ANGLE for purposes of this patch.
> 
> For vertex shaders, it's empty. I can fix it to be whatever you want, if we
need
> vertex outputs for consistency.
> 
> For fragment shaders, it might contain gl_FragColor, gl_FragData,
> gl_SecondaryFragColor, gl_SecondaryFragData, gl_FragDepthEXT.

Sounds good.

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

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5539: case
GL_MAX_DUAL_SOURCE_DRAW_BUFFERS:
Use _EXT instead.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5763: GLuint colorNumber =
static_cast<GLint>(c.colorNumber);
static_cast<GLuint>

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5789: if ((index == 0 &&
colorName >= group_->max_draw_buffers()) ||
You should validate index == 0 || index == 1) first.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5816: GLuint colorNumber =
static_cast<GLint>(c.colorNumber);
GLuint

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5817: GLuint index =
static_cast<GLint>(c.index);
GLuint

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9643: return
error::kGenericError;
We return kInvalidArguments in such cases.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:617: length = 0;
indentation wrong

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:809: const auto& binding =
key_value.second;
It's worth adding a comment here that glBindFragDataLocationIndexed(index=0) is
the same as glBindFragDataLocation().

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.h (right):

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.h:94: ProgramOutputInfo() : size(-1),
type(GL_NONE), colorName(-1), index(-1) {}
index is GLuint, so init it to -1 is confusing.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.h:95: bool IsValid() const { return
colorName != -1; }
I think you should declare colorName also as GLuint to be consistent with the gl
function signature, but use type != GL_NONE to indicate if the info is
initialized or not.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.h:300: GLuint index = 0);
Chromium coding style doesn't allow default arguments generally.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc (right):

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc:154: program_ =
0;
Should we return early here?

[Zhenyao Mo, 2015-09-30 00:26:16]

I have a suggestion.  With such a large CL, reviewing takes a lot of time.  So
when you respond to review comments and update with a new CL, it would be best
you do a clean rebase (without your code update) and upload it first with a
description "clean rebase", and then modify your CL on top of that.  Then it's
clear to reviewers where are the new changes.  Otherwise, like right now, I
basically have to almost re-read the entire patch set 9.

[Kimmo Kinnunen, 2015-10-08 13:18:12]

So I had to remove the usage of program interface query. This is due to the need
to support ES 3.0 glGetFragDataLocation(). 

ES 3.0 does not have program interface query, so glGetFragDataLocation
implementation must use the translator.

I was not very successful implementing the two different code paths bug-free.

For this reason, I ended up implementing EXT_bfe with the translator as a means
to list the output variables. I hope this is ok. 

I also fixed "[0]" array name problem here, similar to the problem in path
rendering texturing patch. I also added a test for it. I hope this is ok.

Sorry, I didn't manage to do the clean rebase, as some time passed between the
original clean rebase and then the finished implementation. I hope it's not too
inconvenient.

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

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5539: case
GL_MAX_DUAL_SOURCE_DRAW_BUFFERS:
On 2015/09/30 00:23:47, Zhenyao Mo wrote:
> Use _EXT instead.

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5763: GLuint colorNumber =
static_cast<GLint>(c.colorNumber);
On 2015/09/30 00:23:47, Zhenyao Mo wrote:
> static_cast<GLuint>

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5789: if ((index == 0 &&
colorName >= group_->max_draw_buffers()) ||
On 2015/09/30 00:23:47, Zhenyao Mo wrote:
> You should validate index == 0 || index == 1) first.

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5816: GLuint colorNumber =
static_cast<GLint>(c.colorNumber);
On 2015/09/30 00:23:47, Zhenyao Mo wrote:
> GLuint

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5817: GLuint index =
static_cast<GLint>(c.index);
On 2015/09/30 00:23:47, Zhenyao Mo wrote:
> GLuint

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9643: return
error::kGenericError;
On 2015/09/30 00:23:47, Zhenyao Mo wrote:
> We return kInvalidArguments in such cases.
Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:458: GLint index;
removed also this stale hunk.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:617: length = 0;
On 2015/09/30 00:23:48, Zhenyao Mo wrote:
> indentation wrong

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:664: // Query the fragment output
variables from the driver. We expose
I ended up removing this program interface query usage, since the translator is
needed anyway, see below.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:719: const std::string& name =
output_var.mappedName;
So the code would always still have the translator codepath. It was hard to
implement and maintain the code correctly for both code paths.
In the new patch, there's only this codepath. I hope it's ok.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:809: const auto& binding =
key_value.second;
On 2015/09/30 00:23:48, Zhenyao Mo wrote:
> It's worth adding a comment here that glBindFragDataLocationIndexed(index=0)
is
> the same as glBindFragDataLocation().

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.h (right):

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.h:94: ProgramOutputInfo() : size(-1),
type(GL_NONE), colorName(-1), index(-1) {}
On 2015/09/30 00:23:48, Zhenyao Mo wrote:
> index is GLuint, so init it to -1 is confusing.

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.h:95: bool IsValid() const { return
colorName != -1; }
On 2015/09/30 00:23:48, Zhenyao Mo wrote:
> I think you should declare colorName also as GLuint to be consistent with the
gl
> function signature, but use type != GL_NONE to indicate if the info is
> initialized or not.

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.h:300: GLuint index = 0);
On 2015/09/30 00:23:48, Zhenyao Mo wrote:
> Chromium coding style doesn't allow default arguments generally.

Done.

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc (right):

https://codereview.chromium.org/1309743005/diff/160001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_ext_blend_func_extended_unittest.cc:154: program_ =
0;
On 2015/09/30 00:23:48, Zhenyao Mo wrote:
> Should we return early here?

Done.

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

LGTM

[Kimmo Kinnunen, 2015-10-09 11:51:15]

piman@:
Could you review
third_party/mojo/src/mojo/public/c/gles2/gles2_call_visitor_chromium_extension_autogen.h
? Thanks!

[piman, 2015-10-09 17:10:58]

RS LGTM for _autogen file.

[Kimmo Kinnunen, 2015-11-03 11:58:38]

The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-03 11:59:27]

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

[commit-bot: I haz the power, 2015-11-03 12:56:27]

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

[commit-bot: I haz the power, 2015-11-03 12:56:28]

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

[Kimmo Kinnunen, 2015-11-03 13:37:13]

kkinnunen@nvidia.com changed reviewers:
+ jam@chromium.org

[Kimmo Kinnunen, 2015-11-03 13:37:14]

jam: would you have time to look at
third_party/mojo/src/mojo/public/c/gles2/gles2_call_visitor_chromium_extension_autogen.h?
Thanks.

zmo: I'm sorry, I had to update this in process of working towards merging this.
So the array handling was still broken. I had to change the looping in
Program::DetectProgramOutputLocationBindingConflicts and 
Program::ExecuteProgramOutputBindCalls.

[jam, 2015-11-03 17:10:22]

On 2015/11/03 13:37:14, Kimmo Kinnunen wrote:
> jam: would you have time to look at
>
third_party/mojo/src/mojo/public/c/gles2/gles2_call_visitor_chromium_extension_autogen.h?
> Thanks.
> 
> zmo: I'm sorry, I had to update this in process of working towards merging
this.
> So the array handling was still broken. I had to change the looping in
> Program::DetectProgramOutputLocationBindingConflicts and 
> Program::ExecuteProgramOutputBindCalls.

lgtm

[Kimmo Kinnunen, 2015-12-03 12:32:17]

On 2015/11/03 13:37:14, Kimmo Kinnunen wrote:
> zmo: I'm sorry, I had to update this in process of working towards merging
this.
> So the array handling was still broken. I had to change the looping in
> Program::DetectProgramOutputLocationBindingConflicts and 
> Program::ExecuteProgramOutputBindCalls.


zmo@: Ping, did you notice this one? I know you said you can not keep coming
back to this, but this was a bit of non-trivial change and I'd feel
uncomfortable merging the patch{ without fixing the looping...

[Zhenyao Mo, 2015-12-03 18:50:04]

On 2015/12/03 12:32:17, Kimmo Kinnunen wrote:
> On 2015/11/03 13:37:14, Kimmo Kinnunen wrote:
> > zmo: I'm sorry, I had to update this in process of working towards merging
> this.
> > So the array handling was still broken. I had to change the looping in
> > Program::DetectProgramOutputLocationBindingConflicts and 
> > Program::ExecuteProgramOutputBindCalls.
> 
> 
> zmo@: Ping, did you notice this one? I know you said you can not keep coming
> back to this, but this was a bit of non-trivial change and I'd feel
> uncomfortable merging the patch{ without fixing the looping...

Sorry I didn't realize you were waiting for me to double check.

The latest Program::DetectProgramOutputLocationBindingConflicts and
Program::ExecuteProgramOutputBindCalls LGTM.

[Kimmo Kinnunen, 2015-12-04 06:51:14]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-12-04 06:51:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com, piman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1309743005/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-04 06:51:44]

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

[commit-bot: I haz the power, 2015-12-04 06:53:37]

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

[commit-bot: I haz the power, 2015-12-04 06:53:38]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kimmo Kinnunen, 2015-12-04 07:30:00]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-12-04 07:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com, jam@chromium.org, piman@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1309743005/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-04 07:30:24]

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

[commit-bot: I haz the power, 2015-12-04 07:34:05]

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

[commit-bot: I haz the power, 2015-12-04 07:34:06]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kimmo Kinnunen, 2015-12-04 09:05:37]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-12-04 09:05:43]

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

[commit-bot: I haz the power, 2015-12-04 09:36:38]

Description was changed from

==========
command_buffer: Implement EXT_blend_func_extended

Implement EXT_blend_func_extended for command buffer ES 2.0 and ES 3.0
contexts.

For the ES 2.0 context, the extension supports dual-source blending with
pre-defined gl_SecondaryFragColorEXT and gl_SecondaryFragDataEXT
variables.

Currently EXT_blend_func_extended is only exposed if the service context
supports program interface query. This means OpenGL context or OpenGL
ES 3.1 context. This is to simplify the unit test expectation conditions.
Theoretically also ES 2.0 and ES 3.0 service contexts could support
EXT_blend_func_extended, but probably there will never be such a driver.


BUG=506765
==========

to

==========
command_buffer: Implement EXT_blend_func_extended

Implement EXT_blend_func_extended for command buffer ES 2.0 and ES 3.0
contexts.

For the ES 2.0 context, the extension supports dual-source blending with
pre-defined gl_SecondaryFragColorEXT and gl_SecondaryFragDataEXT
variables.

Currently EXT_blend_func_extended is only exposed if the service context
supports program interface query. This means OpenGL context or OpenGL
ES 3.1 context. This is to simplify the unit test expectation conditions.
Theoretically also ES 2.0 and ES 3.0 service contexts could support
EXT_blend_func_extended, but probably there will never be such a driver.


BUG=506765
==========

[commit-bot: I haz the power, 2015-12-04 09:36:40]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-12-04 09:37:22]

Description was changed from

==========
command_buffer: Implement EXT_blend_func_extended

Implement EXT_blend_func_extended for command buffer ES 2.0 and ES 3.0
contexts.

For the ES 2.0 context, the extension supports dual-source blending with
pre-defined gl_SecondaryFragColorEXT and gl_SecondaryFragDataEXT
variables.

Currently EXT_blend_func_extended is only exposed if the service context
supports program interface query. This means OpenGL context or OpenGL
ES 3.1 context. This is to simplify the unit test expectation conditions.
Theoretically also ES 2.0 and ES 3.0 service contexts could support
EXT_blend_func_extended, but probably there will never be such a driver.


BUG=506765
==========

to

==========
command_buffer: Implement EXT_blend_func_extended

Implement EXT_blend_func_extended for command buffer ES 2.0 and ES 3.0
contexts.

For the ES 2.0 context, the extension supports dual-source blending with
pre-defined gl_SecondaryFragColorEXT and gl_SecondaryFragDataEXT
variables.

Currently EXT_blend_func_extended is only exposed if the service context
supports program interface query. This means OpenGL context or OpenGL
ES 3.1 context. This is to simplify the unit test expectation conditions.
Theoretically also ES 2.0 and ES 3.0 service contexts could support
EXT_blend_func_extended, but probably there will never be such a driver.

BUG=506765

Committed: https://crrev.com/8cefb231216c2777281f37a30e3fb8e67513ee2b
Cr-Commit-Position: refs/heads/master@{#363175}
==========

[commit-bot: I haz the power, 2015-12-04 09:37:23]

Patchset 15 (id:??) landed as
https://crrev.com/8cefb231216c2777281f37a30e3fb8e67513ee2b
Cr-Commit-Position: refs/heads/master@{#363175}