Use noperspective interpolation for 2D draws

Use noperspective interpolation for 2D draws

Adds a mechanism to notify GrGLSLVaryingHandler that a shader will not
emit geometry in perspective. This gives it a chance to use the
noperspective keyword if it is supported.

Updates the existing processors to notify the varying handler when
there is no perspective.

Begins using the noperspective keyword in GrGLGpu internal shaders.

Web scenes with observable benefit (Pixel C, gpu config):

  tabl_nofolo.skp        4.62 -> 3.33 ms   28%
  desk_tigersvg.skp      26.5 -> 24 ms      9%
  desk_pokemonwiki.skp   16.1 -> 14.9 ms    7%
  tabl_deviantart.skp    3.97 -> 3.7 ms     7%
  desk_gws.skp           3.85 -> 3.65 ms    5%

Also adds new methods for creating varyings with flat interpolation.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1673093002

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

[Chris Dalton, 2016-02-07 17:33:45]

Description was changed from

==========
Use noperspective interpolation for 2D draws

Adds a mechanism to notify GrGLSLVaryingHandler that a shader will not
emit geometry in perspective. This gives it a chance to use the
noperspective keyword if it is supported.

Updates the existing processors to notify the varying handler when
there is no perspective.

Begins using the noperspective keyword in GrGLGpu internal shaders.

Web scenes with observable benefit (Pixel C, gpu config):

  tabl_nofolo.skp        4.62 -> 3.33 ms   28%
  desk_tigersvg.skp      26.5 -> 24 ms      9%
  desk_pokemonwiki.skp   16.1 -> 14.9 ms    7%
  tabl_deviantart.skp    3.97 -> 3.7 ms     7%
  desk_gws.skp           3.85 -> 3.65 ms    5%

Also adds new methods for creating varyings with flat interpolation.

BUG=skia:
==========

to

==========
Use noperspective interpolation for 2D draws

Adds a mechanism to notify GrGLSLVaryingHandler that a shader will not
emit geometry in perspective. This gives it a chance to use the
noperspective keyword if it is supported.

Updates the existing processors to notify the varying handler when
there is no perspective.

Begins using the noperspective keyword in GrGLGpu internal shaders.

Web scenes with observable benefit (Pixel C, gpu config):

  tabl_nofolo.skp        4.62 -> 3.33 ms   28%
  desk_tigersvg.skp      26.5 -> 24 ms      9%
  desk_pokemonwiki.skp   16.1 -> 14.9 ms    7%
  tabl_deviantart.skp    3.97 -> 3.7 ms     7%
  desk_gws.skp           3.85 -> 3.65 ms    5%

Also adds new methods for creating varyings with flat interpolation.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Dalton, 2016-02-07 23:13:38]

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

[egdaniel, 2016-02-10 14:23:44]

just some quick first pass thoughts

https://codereview.chromium.org/1673093002/diff/60001/src/gpu/glsl/GrGLSLVary...
File src/gpu/glsl/GrGLSLVarying.cpp (right):

https://codereview.chromium.org/1673093002/diff/60001/src/gpu/glsl/GrGLSLVary...
src/gpu/glsl/GrGLSLVarying.cpp:28:
fVaryingShaderVars[i].addModifier("noperspective");
I recently added a finalize method to GrGLSLVarying which gets called after all
varyings have been added but before emiting any decals into code. I think this
loop would make more sense there. This will allow the builders to call
setNoPersective at anytime.

if (fNoPersective) { loop and add modifier; }

https://codereview.chromium.org/1673093002/diff/60001/src/gpu/glsl/GrGLSLVary...
File src/gpu/glsl/GrGLSLVarying.h (right):

https://codereview.chromium.org/1673093002/diff/60001/src/gpu/glsl/GrGLSLVary...
src/gpu/glsl/GrGLSLVarying.h:108: void addFlatVarying(const char* name,
instead of adding a new call here, does it make more sense to have another bool
on addVarying (that defaults to false) for flat? Similarly with the
addFlatPassThrough (wouldn't need the writePassThrough in that case)

[Chris Dalton, 2016-02-10 15:29:30]

Quick responses inline

https://codereview.chromium.org/1673093002/diff/60001/src/gpu/glsl/GrGLSLVary...
File src/gpu/glsl/GrGLSLVarying.cpp (right):

https://codereview.chromium.org/1673093002/diff/60001/src/gpu/glsl/GrGLSLVary...
src/gpu/glsl/GrGLSLVarying.cpp:28:
fVaryingShaderVars[i].addModifier("noperspective");
On 2016/02/10 14:23:44, egdaniel wrote:
> I recently added a finalize method to GrGLSLVarying which gets called after
all
> varyings have been added but before emiting any decals into code. I think this
> loop would make more sense there. This will allow the builders to call
> setNoPersective at anytime.
> 
> if (fNoPersective) { loop and add modifier; }

Ok that sounds great. onFinalize does sound like a better place for the loop.
(Though this method can already be called at any time as-is)

https://codereview.chromium.org/1673093002/diff/60001/src/gpu/glsl/GrGLSLVary...
File src/gpu/glsl/GrGLSLVarying.h (right):

https://codereview.chromium.org/1673093002/diff/60001/src/gpu/glsl/GrGLSLVary...
src/gpu/glsl/GrGLSLVarying.h:108: void addFlatVarying(const char* name,
On 2016/02/10 14:23:44, egdaniel wrote:
> instead of adding a new call here, does it make more sense to have another
bool
> on addVarying (that defaults to false) for flat? Similarly with the
> addFlatPassThrough (wouldn't need the writePassThrough in that case)

I thought this over quite a bit myself and my preference kept coming back to two
separate methods, addVarying/addFlatVarying. Either way, it's not a terribly
strong preference.

What about a hybrid approach? Keep addFlatVarying but add a parameter to
addPassThroughAttribute for flat or not?

[Chris Dalton, 2016-02-11 19:15:31]

With the finalize() method I ended up being able to rework this into something I
like a lot better

[egdaniel, 2016-02-12 03:55:07]

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLSha...
File src/gpu/glsl/GrGLSLShaderVar.h (right):

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLSha...
src/gpu/glsl/GrGLSLShaderVar.h:158: fExtraModifiers.appendf("%s ", modifier);
to be safe should be have a null check around this?

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
File src/gpu/glsl/GrGLSLVarying.cpp (right):

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
src/gpu/glsl/GrGLSLVarying.cpp:46: SkASSERT(varying->vsVarying() ||
willUseGeoShader);
if we add this assert, shouldn't we also have SkASSERT(fsVarying() ||
willUseGeoShader) as well?

And if we do that just have one SkASSERT((vsVarying && fsVarying) ||
willUseGeoShader);

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
src/gpu/glsl/GrGLSLVarying.cpp:48: VaryingInfo& v = fVaryings.push_back();
I don't necessarily dislike this VaryingInfo thing, but what exactly is it
buying us? Seems like it causes use to need to do the logic of setting the
in/out char variables twice, both below and again in finalize.

I guess the one thing it adds is that we will either have only flat or only
nopersepective modifier. But would it really be bad if we ended up with both?

[Chris Dalton, 2016-02-12 16:25:28]

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLSha...
File src/gpu/glsl/GrGLSLShaderVar.h (right):

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLSha...
src/gpu/glsl/GrGLSLShaderVar.h:158: fExtraModifiers.appendf("%s ", modifier);
On 2016/02/12 03:55:07, egdaniel wrote:
> to be safe should be have a null check around this?

Done.

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
File src/gpu/glsl/GrGLSLVarying.cpp (right):

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
src/gpu/glsl/GrGLSLVarying.cpp:46: SkASSERT(varying->vsVarying() ||
willUseGeoShader);
On 2016/02/12 03:55:07, egdaniel wrote:
> if we add this assert, shouldn't we also have SkASSERT(fsVarying() ||
> willUseGeoShader) as well?
> 
> And if we do that just have one SkASSERT((vsVarying && fsVarying) ||
> willUseGeoShader);

My idea behind that assert was that it made sure the fsIn didn't end up with an
empty string. I rearranged things so that's not an issues anymore and just took
out the assert.

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
src/gpu/glsl/GrGLSLVarying.cpp:48: VaryingInfo& v = fVaryings.push_back();
On 2016/02/12 03:55:07, egdaniel wrote:
> I don't necessarily dislike this VaryingInfo thing, but what exactly is it
> buying us? Seems like it causes use to need to do the logic of setting the
> in/out char variables twice, both below and again in finalize.
> 
> I guess the one thing it adds is that we will either have only flat or only
> nopersepective modifier. But would it really be bad if we ended up with both?

Yeah, you get a compile error if you use both (which really makes sense -- if
you pick two interpolation modes, which one did you actually want?)

The idea of the VaryingInfo list is that you build up an internal structure of
what you want the varyings to be, then during onFinalize you compile that list
into ShaderVars based on information you might not have known while building the
original list.

Another option would be to add interpolation modifiers on the ShaderVar, and
during onFinalize make a pass that converts smooth -> noperspective. I don't
like that approach because interpolation modifiers only apply to varyings, so it
makes sense to handle them inside the varying class rather than at the ShaderVar
level.

[egdaniel, 2016-02-12 16:33:58]

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
File src/gpu/glsl/GrGLSLVarying.cpp (right):

https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
src/gpu/glsl/GrGLSLVarying.cpp:48: VaryingInfo& v = fVaryings.push_back();
On 2016/02/12 16:25:28, Chris Dalton wrote:
> On 2016/02/12 03:55:07, egdaniel wrote:
> > I don't necessarily dislike this VaryingInfo thing, but what exactly is it
> > buying us? Seems like it causes use to need to do the logic of setting the
> > in/out char variables twice, both below and again in finalize.
> > 
> > I guess the one thing it adds is that we will either have only flat or only
> > nopersepective modifier. But would it really be bad if we ended up with
both?
> 
> Yeah, you get a compile error if you use both (which really makes sense -- if
> you pick two interpolation modes, which one did you actually want?)
> 
> The idea of the VaryingInfo list is that you build up an internal structure of
> what you want the varyings to be, then during onFinalize you compile that list
> into ShaderVars based on information you might not have known while building
the
> original list.
> 
> Another option would be to add interpolation modifiers on the ShaderVar, and
> during onFinalize make a pass that converts smooth -> noperspective. I don't
> like that approach because interpolation modifiers only apply to varyings, so
it
> makes sense to handle them inside the varying class rather than at the
ShaderVar
> level.

Just throwing in an idea off your comment. Would it make most sense to have a
GrGLSLVaryingShaderVar which is subclassed off of GrGLSLShaderVar? Cause I
almost feel the ShaderVars already are doing the work of the VaryingInfo and
this would then give us the flexibility for different ShaderVar uses.

[Chris Dalton, 2016-02-12 17:14:21]

On 2016/02/12 16:33:58, egdaniel wrote:
>
https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
> File src/gpu/glsl/GrGLSLVarying.cpp (right):
> 
>
https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
> src/gpu/glsl/GrGLSLVarying.cpp:48: VaryingInfo& v = fVaryings.push_back();
> On 2016/02/12 16:25:28, Chris Dalton wrote:
> > On 2016/02/12 03:55:07, egdaniel wrote:
> > > I don't necessarily dislike this VaryingInfo thing, but what exactly is it
> > > buying us? Seems like it causes use to need to do the logic of setting the
> > > in/out char variables twice, both below and again in finalize.
> > > 
> > > I guess the one thing it adds is that we will either have only flat or
only
> > > nopersepective modifier. But would it really be bad if we ended up with
> both?
> > 
> > Yeah, you get a compile error if you use both (which really makes sense --
if
> > you pick two interpolation modes, which one did you actually want?)
> > 
> > The idea of the VaryingInfo list is that you build up an internal structure
of
> > what you want the varyings to be, then during onFinalize you compile that
list
> > into ShaderVars based on information you might not have known while building
> the
> > original list.
> > 
> > Another option would be to add interpolation modifiers on the ShaderVar, and
> > during onFinalize make a pass that converts smooth -> noperspective. I don't
> > like that approach because interpolation modifiers only apply to varyings,
so
> it
> > makes sense to handle them inside the varying class rather than at the
> ShaderVar
> > level.
> 
> Just throwing in an idea off your comment. Would it make most sense to have a
> GrGLSLVaryingShaderVar which is subclassed off of GrGLSLShaderVar? Cause I
> almost feel the ShaderVars already are doing the work of the VaryingInfo and
> this would then give us the flexibility for different ShaderVar uses.

We could probably do something to merge GrGLSLVarying and the GrGLSLShaderVars.
I don't think we could subclass very easily though. For example, what happens if
you call GrGLSLVaryingShaderVar.appendDecl()? Does it append a declaration with
the "in" modifier or the "out" modifier?

[egdaniel, 2016-02-12 18:46:53]

On 2016/02/12 17:14:21, Chris Dalton wrote:
> On 2016/02/12 16:33:58, egdaniel wrote:
> >
>
https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
> > File src/gpu/glsl/GrGLSLVarying.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1673093002/diff/100001/src/gpu/glsl/GrGLSLVar...
> > src/gpu/glsl/GrGLSLVarying.cpp:48: VaryingInfo& v = fVaryings.push_back();
> > On 2016/02/12 16:25:28, Chris Dalton wrote:
> > > On 2016/02/12 03:55:07, egdaniel wrote:
> > > > I don't necessarily dislike this VaryingInfo thing, but what exactly is
it
> > > > buying us? Seems like it causes use to need to do the logic of setting
the
> > > > in/out char variables twice, both below and again in finalize.
> > > > 
> > > > I guess the one thing it adds is that we will either have only flat or
> only
> > > > nopersepective modifier. But would it really be bad if we ended up with
> > both?
> > > 
> > > Yeah, you get a compile error if you use both (which really makes sense --
> if
> > > you pick two interpolation modes, which one did you actually want?)
> > > 
> > > The idea of the VaryingInfo list is that you build up an internal
structure
> of
> > > what you want the varyings to be, then during onFinalize you compile that
> list
> > > into ShaderVars based on information you might not have known while
building
> > the
> > > original list.
> > > 
> > > Another option would be to add interpolation modifiers on the ShaderVar,
and
> > > during onFinalize make a pass that converts smooth -> noperspective. I
don't
> > > like that approach because interpolation modifiers only apply to varyings,
> so
> > it
> > > makes sense to handle them inside the varying class rather than at the
> > ShaderVar
> > > level.
> > 
> > Just throwing in an idea off your comment. Would it make most sense to have
a
> > GrGLSLVaryingShaderVar which is subclassed off of GrGLSLShaderVar? Cause I
> > almost feel the ShaderVars already are doing the work of the VaryingInfo and
> > this would then give us the flexibility for different ShaderVar uses.
> 
> We could probably do something to merge GrGLSLVarying and the
GrGLSLShaderVars.
> I don't think we could subclass very easily though. For example, what happens
if
> you call GrGLSLVaryingShaderVar.appendDecl()? Does it append a declaration
with
> the "in" modifier or the "out" modifier?

I in the future we could do a big refactor of GrGLSLShaderVar and make nice
subclasses of it, but it would be a significant change to get it there. So I'm
happy just punting on that now.

[egdaniel, 2016-02-12 18:53:07]

just one more nit and we should be good to go

https://codereview.chromium.org/1673093002/diff/120001/src/gpu/glsl/GrGLSLVar...
File src/gpu/glsl/GrGLSLVarying.h (right):

https://codereview.chromium.org/1673093002/diff/120001/src/gpu/glsl/GrGLSLVar...
src/gpu/glsl/GrGLSLVarying.h:149: SkString         fName;
I think using fName is kind of confusing since it really just means fVsOut. I
say we just change the name back to that, but leave the changed logic/uses the
same.

[Chris Dalton, 2016-02-12 19:35:03]

https://codereview.chromium.org/1673093002/diff/120001/src/gpu/glsl/GrGLSLVar...
File src/gpu/glsl/GrGLSLVarying.h (right):

https://codereview.chromium.org/1673093002/diff/120001/src/gpu/glsl/GrGLSLVar...
src/gpu/glsl/GrGLSLVarying.h:149: SkString         fName;
On 2016/02/12 18:53:06, egdaniel wrote:
> I think using fName is kind of confusing since it really just means fVsOut. I
> say we just change the name back to that, but leave the changed logic/uses the
> same.

Done.

[egdaniel, 2016-02-12 19:39:25]

lgtm

[Chris Dalton, 2016-02-12 19:40:44]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-02-12 19:40:54]

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

[commit-bot: I haz the power, 2016-02-12 20:14:09]

Description was changed from

==========
Use noperspective interpolation for 2D draws

Adds a mechanism to notify GrGLSLVaryingHandler that a shader will not
emit geometry in perspective. This gives it a chance to use the
noperspective keyword if it is supported.

Updates the existing processors to notify the varying handler when
there is no perspective.

Begins using the noperspective keyword in GrGLGpu internal shaders.

Web scenes with observable benefit (Pixel C, gpu config):

  tabl_nofolo.skp        4.62 -> 3.33 ms   28%
  desk_tigersvg.skp      26.5 -> 24 ms      9%
  desk_pokemonwiki.skp   16.1 -> 14.9 ms    7%
  tabl_deviantart.skp    3.97 -> 3.7 ms     7%
  desk_gws.skp           3.85 -> 3.65 ms    5%

Also adds new methods for creating varyings with flat interpolation.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Use noperspective interpolation for 2D draws

Adds a mechanism to notify GrGLSLVaryingHandler that a shader will not
emit geometry in perspective. This gives it a chance to use the
noperspective keyword if it is supported.

Updates the existing processors to notify the varying handler when
there is no perspective.

Begins using the noperspective keyword in GrGLGpu internal shaders.

Web scenes with observable benefit (Pixel C, gpu config):

  tabl_nofolo.skp        4.62 -> 3.33 ms   28%
  desk_tigersvg.skp      26.5 -> 24 ms      9%
  desk_pokemonwiki.skp   16.1 -> 14.9 ms    7%
  tabl_deviantart.skp    3.97 -> 3.7 ms     7%
  desk_gws.skp           3.85 -> 3.65 ms    5%

Also adds new methods for creating varyings with flat interpolation.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/c08f196648463d44eb85e17c5815dbf8f709a42a
==========

[commit-bot: I haz the power, 2016-02-12 20:14:09]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/c08f196648463d44eb85e17c5815dbf8f709a42a