Add layout qualifiers to GrGLShaderBuilder

Add layout qualifiers to GrGLShaderBuilder

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/e4017d8ca709bc0b4fc0f95c26eb592a8d5c597b

[Chris Dalton, 2015-04-29 06:01:56]

cdalton@nvidia.com changed reviewers:
+ egdaniel@google.com

[egdaniel, 2015-04-29 13:00:10]

egdaniel@google.com changed reviewers:
+ jvanverth@google.com

[egdaniel, 2015-04-29 13:00:10]

Add Jim since he has been the one who most recently has looked at our shader
building code

[jvanverth1, 2015-04-29 14:00:13]

jvanverth@google.com changed reviewers:
+ bsalomon@google.com

[jvanverth1, 2015-04-29 14:01:43]

Maybe I'm missing something, but I'm not sure how this generates what we'd want.
It looks like you'd end up with

layout (<param0>, <param1>, ...) out;

where <param0>, etc. could be anything. I'm guessing the params are supposed to
be something like "location=0" but there's nothing that limits it to that. I
also don't see where that maps to a particular output variable.

It seems like it'd be better to add an optional "location" param to
GrGLGPBuilder::addVarying(). But maybe I don't understand the use case. 

Adding Brian, too.

[Chris Dalton, 2015-04-29 14:47:17]

On 2015/04/29 14:01:43, jvanverth1 wrote:
> Maybe I'm missing something, but I'm not sure how this generates what we'd
want.
> It looks like you'd end up with
> 
> layout (<param0>, <param1>, ...) out;
> 
> where <param0>, etc. could be anything. I'm guessing the params are supposed
to
> be something like "location=0" but there's nothing that limits it to that. I
> also don't see where that maps to a particular output variable.
> 
> It seems like it'd be better to add an optional "location" param to
> GrGLGPBuilder::addVarying(). But maybe I don't understand the use case. 
> 
> Adding Brian, too.

Jim, that actually is the usecase in mind for this change. layout() can be used
on a generic interface qualifier like "out" or "uniform" instead of an
individual variable, kind of like "precision lowp float;" vs "lowp float foo;".
See section 4.4, Layout Qualifiers:

https://www.opengl.org/registry/doc/GLSLangSpec.4.40.pdf#page=65&zoom=auto,11...

It's also like the "static" keyword in C++ in that it keeps getting reused and
abused for things that really aren't intuitive for the word "layout". In my
case, the KHR_blend_equation_advanced extension
(https://www.opengl.org/registry/specs/KHR/blend_equation_advanced.txt) adds new
layout qualifiers valid only for the interface qualifier out, and requires them
to be included in shaders that will be used with advanced blending equations.
So, for example, you would have to add the following to your shader at global
scope:

layout(blend_support_all_equations) out;

I consciously chose to not limit the params for layout() since there are a lot
of them (see the table in the spec), and extensions can always pop up defining
new ones.

[egdaniel, 2015-04-29 18:57:28]

On 2015/04/29 14:47:17, Chris Dalton wrote:
> On 2015/04/29 14:01:43, jvanverth1 wrote:
> > Maybe I'm missing something, but I'm not sure how this generates what we'd
> want.
> > It looks like you'd end up with
> > 
> > layout (<param0>, <param1>, ...) out;
> > 
> > where <param0>, etc. could be anything. I'm guessing the params are supposed
> to
> > be something like "location=0" but there's nothing that limits it to that. I
> > also don't see where that maps to a particular output variable.
> > 
> > It seems like it'd be better to add an optional "location" param to
> > GrGLGPBuilder::addVarying(). But maybe I don't understand the use case. 
> > 
> > Adding Brian, too.
> 
> Jim, that actually is the usecase in mind for this change. layout() can be
used
> on a generic interface qualifier like "out" or "uniform" instead of an
> individual variable, kind of like "precision lowp float;" vs "lowp float
foo;".
> See section 4.4, Layout Qualifiers:
> 
>
https://www.opengl.org/registry/doc/GLSLangSpec.4.40.pdf#page=65&zoom=auto,11...
> 
> It's also like the "static" keyword in C++ in that it keeps getting reused and
> abused for things that really aren't intuitive for the word "layout". In my
> case, the KHR_blend_equation_advanced extension
> (https://www.opengl.org/registry/specs/KHR/blend_equation_advanced.txt) adds
new
> layout qualifiers valid only for the interface qualifier out, and requires
them
> to be included in shaders that will be used with advanced blending equations.
> So, for example, you would have to add the following to your shader at global
> scope:
> 
> layout(blend_support_all_equations) out;
> 
> I consciously chose to not limit the params for layout() since there are a lot
> of them (see the table in the spec), and extensions can always pop up defining
> new ones.

So this comment applies in parts both to this CL and the follow up advanced
blend cl.

Okay so I'm not a big fan of being able to pass in an arbitrary string into the
public layout method. Second (and more related to your follow up cl) without any
data it is hard to say that on all platforms we should always use the "all"
blend layout value. I could imagine this could be slow if it adds some crazy
code onto the shader itself. Finally, XP's should not be aware of the specific
layout qualifiers. Instead they should be sending in their equation enum and
letting the builder figure out what to do.

So I propose moving addLayoutQualifier to be a private method. Then add a public
method on to the GrGLXPFragmentProcessor (just landed) that takes an equation
and GLSLCaps. This function can look at the caps to decide whether or not to use
all or the specific equation qualifier, get the appropriate string from the
equation, and call the private layoutQualifier function. Also if the caps does
return to use individual equations instead of all, the equation will needed to
be added to the XPs shader key.

[Chris Dalton, 2015-04-30 12:07:28]

On 2015/04/29 18:57:28, egdaniel wrote:
> On 2015/04/29 14:47:17, Chris Dalton wrote:
> > On 2015/04/29 14:01:43, jvanverth1 wrote:
> > > Maybe I'm missing something, but I'm not sure how this generates what we'd
> > want.
> > > It looks like you'd end up with
> > > 
> > > layout (<param0>, <param1>, ...) out;
> > > 
> > > where <param0>, etc. could be anything. I'm guessing the params are
supposed
> > to
> > > be something like "location=0" but there's nothing that limits it to that.
I
> > > also don't see where that maps to a particular output variable.
> > > 
> > > It seems like it'd be better to add an optional "location" param to
> > > GrGLGPBuilder::addVarying(). But maybe I don't understand the use case. 
> > > 
> > > Adding Brian, too.
> > 
> > Jim, that actually is the usecase in mind for this change. layout() can be
> used
> > on a generic interface qualifier like "out" or "uniform" instead of an
> > individual variable, kind of like "precision lowp float;" vs "lowp float
> foo;".
> > See section 4.4, Layout Qualifiers:
> > 
> >
>
https://www.opengl.org/registry/doc/GLSLangSpec.4.40.pdf#page=65&zoom=auto,11...
> > 
> > It's also like the "static" keyword in C++ in that it keeps getting reused
and
> > abused for things that really aren't intuitive for the word "layout". In my
> > case, the KHR_blend_equation_advanced extension
> > (https://www.opengl.org/registry/specs/KHR/blend_equation_advanced.txt) adds
> new
> > layout qualifiers valid only for the interface qualifier out, and requires
> them
> > to be included in shaders that will be used with advanced blending
equations.
> > So, for example, you would have to add the following to your shader at
global
> > scope:
> > 
> > layout(blend_support_all_equations) out;
> > 
> > I consciously chose to not limit the params for layout() since there are a
lot
> > of them (see the table in the spec), and extensions can always pop up
defining
> > new ones.
> 
> So this comment applies in parts both to this CL and the follow up advanced
> blend cl.
> 
> Okay so I'm not a big fan of being able to pass in an arbitrary string into
the
> public layout method. Second (and more related to your follow up cl) without
any
> data it is hard to say that on all platforms we should always use the "all"
> blend layout value. I could imagine this could be slow if it adds some crazy
> code onto the shader itself. Finally, XP's should not be aware of the specific
> layout qualifiers. Instead they should be sending in their equation enum and
> letting the builder figure out what to do.
> 
> So I propose moving addLayoutQualifier to be a private method. Then add a
public
> method on to the GrGLXPFragmentProcessor (just landed) that takes an equation
> and GLSLCaps. This function can look at the caps to decide whether or not to
use
> all or the specific equation qualifier, get the appropriate string from the
> equation, and call the private layoutQualifier function. Also if the caps does
> return to use individual equations instead of all, the equation will needed to
> be added to the XPs shader key.

Agreed. If the extension exposes individual qualifiers for each blend mode, we
ought to use them.

Updated to make addLayoutQualifier protected and instead have an
enableBlendEquation method on the GrGLXPFragmentProcessor. It now also detects
the NV version of the extension and does not add any qualifiers to the shader in
that case. Do you guys have a test system with Intel graphics that supports this
extension? If not the shader qualifiers aren't going to have any test
coverage...

[egdaniel, 2015-05-06 17:26:28]

lgtm

[Chris Dalton, 2015-05-06 18:39:39]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-05-06 18:39:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1114813002/100001

[commit-bot: I haz the power, 2015-05-06 18:49:02]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/e4017d8ca709bc0b4fc0f95c26eb592a8d5c597b