Add gl_SampleMask functionality to fragment builders

Add gl_SampleMask functionality to fragment builders

Adds methods for overriding and masking a fragment's sample mask.


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

Committed: https://skia.googlesource.com/skia/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094

Committed: https://skia.googlesource.com/skia/+/33ad701bc30387127c427fb1e38c781d5de33491

[Chris Dalton, 2016-02-11 22:07:41]

Description was changed from

==========
Add infastructure for gl_SampleMask

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

to

==========
Add infastructure for gl_SampleMask

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

[Chris Dalton, 2016-02-11 22:14:34]

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

[Chris Dalton, 2016-02-11 22:14:35]

Greg- I've been adding you on the glsl ones, am I correct in believing this is
your area? Is there anybody else I should add to the glsl changes?

[Chris Dalton, 2016-02-12 19:39:37]

Updated the API to be more consistent with the other methods on
GrGLSLFragmentBuilder

[bsalomon, 2016-02-12 19:59:26]

Overall looks good. I have some questions about the sample count that is used to
enable the feature in GLSL:

Is it possible for the builder to just know the render target's sample count? It
seems like a roundabout way for it to arrive from a GrProcessor. Is it really
even necessary? Would it hurt to just say it's an int and we'll cap sample
counts at 32? If not, then the sample count is being used to modify the
generated code and the chosen mask variable type would have to be incorporated
into the program key.

Also, just observing that because this is on the FS builder it will be available
to all FPs. I suppose that could be a good thing because clip effects could
adjust the mask rather than compute a floating point coverage value.

[Chris Dalton, 2016-02-12 20:22:16]

On 2016/02/12 19:59:26, bsalomon wrote:
> Overall looks good. I have some questions about the sample count that is used
to
> enable the feature in GLSL:
> 
> Is it possible for the builder to just know the render target's sample count?
It
> seems like a roundabout way for it to arrive from a GrProcessor. Is it really
> even necessary? Would it hurt to just say it's an int and we'll cap sample
> counts at 32? If not, then the sample count is being used to modify the
> generated code and the chosen mask variable type would have to be incorporated
> into the program key.
> 
> Also, just observing that because this is on the FS builder it will be
available
> to all FPs. I suppose that could be a good thing because clip effects could
> adjust the mask rather than compute a floating point coverage value.

Ok I just tested the perf effect of using highp vs lowp on the sample mask and
there is absolutely no difference. I'm happy to remove the sample count in this
change and always use highp instead.

However for the change that adds support for interpolation at sample locations,
the fragment shader and key will need to incorporate sample locations for a
particular render target. So we still need to come up with a way for the builder
to know rt specifics.

[Chris Dalton, 2016-02-12 20:43:03]

Removed the sample count. I like this version a lot better.

[bsalomon, 2016-02-12 20:58:17]

lgtm

https://codereview.chromium.org/1690963003/diff/80001/src/gpu/glsl/GrGLSL.h
File src/gpu/glsl/GrGLSL.h (right):

https://codereview.chromium.org/1690963003/diff/80001/src/gpu/glsl/GrGLSL.h#n...
src/gpu/glsl/GrGLSL.h:126: static inline GrSLPrecision
GrGLSLIntMinPrecisionForNBits(int bits) {
If we're not using this can we remove it?

[Chris Dalton, 2016-02-12 21:01:50]

https://codereview.chromium.org/1690963003/diff/80001/src/gpu/glsl/GrGLSL.h
File src/gpu/glsl/GrGLSL.h (right):

https://codereview.chromium.org/1690963003/diff/80001/src/gpu/glsl/GrGLSL.h#n...
src/gpu/glsl/GrGLSL.h:126: static inline GrSLPrecision
GrGLSLIntMinPrecisionForNBits(int bits) {
On 2016/02/12 20:58:17, bsalomon wrote:
> If we're not using this can we remove it?

Oops. Done.

[Chris Dalton, 2016-02-12 21:02:15]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-02-12 21:02:15]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1690963003/#ps100001
(title: "Remove GrGLSLIntMinPrecisionForNBits")

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

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

[commit-bot: I haz the power, 2016-02-12 21:24:28]

Description was changed from

==========
Add infastructure for gl_SampleMask

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

to

==========
Add infastructure for gl_SampleMask

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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094
==========

[commit-bot: I haz the power, 2016-02-12 21:24:29]

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

[dogben, 2016-02-12 23:20:35]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1693113002/ by benjaminwagner@google.com.

The reason for reverting is: Several bots failing with error 'invalid profile
"es" for version 320'..

[Chris Dalton, 2016-02-17 09:14:45]

Description was changed from

==========
Add infastructure for gl_SampleMask

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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094
==========

to

==========
Add infastructure for gl_SampleMask

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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094
==========

[Chris Dalton, 2016-02-17 09:39:12]

https://codereview.chromium.org/1690963003/diff/100001/src/gpu/glsl/GrGLSLFra...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1690963003/diff/100001/src/gpu/glsl/GrGLSLFra...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp:117: "override_coverage");
As I think about it more, I don't like this being here.

This is different than the other enables because enabling the other features
doesn't change the semantic meaning of the program. It just makes sure the
program will compile.

Here, however, calling enableFeature(kSampleMaskOverrideCoverage) causes the
draw to produce different results, and therefore the caller needs to account for
this in their program key.

I have a few alternative options in mind and would like to hear your feedback on
which you like better:

1) Just simplify it and let the GrPrimitiveProcessor add new outputs. It would
then add this redefinition of gl_SampleMask.

2) Add kShaderOverrideCoverage_Flag to GrPipeline(Builder)::Flags, then let the
framework handle putting this in the program key as well as redefining
gl_SampleMask.

3) Add features that can be enabled in GrProcessorKeyBuilder for things like
this that affect the program key.

I don't like #3.

We will also need to enable sample locations in the fragment shader, and for
that I think I like #2 best. However, it doesn't seem like as good of a fit for
sample_mask_override_coverage because this extension is pretty GLSL specific. So
maybe it doesn't belong in a "Gr" class.

#1 is simple, however it seems to violate the shader abstractions if we just let
anything define new outputs.

What do you think? Maybe a 4th option could be to add a new method to the
builder that redefines gl_SampleMask, and it is understood that the caller is
responsible for including this in their key?

[bsalomon, 2016-02-17 17:51:53]

On 2016/02/17 09:39:12, Chris Dalton wrote:
>
https://codereview.chromium.org/1690963003/diff/100001/src/gpu/glsl/GrGLSLFra...
> File src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp (right):
> 
>
https://codereview.chromium.org/1690963003/diff/100001/src/gpu/glsl/GrGLSLFra...
> src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp:117: "override_coverage");
> As I think about it more, I don't like this being here.
> 
> This is different than the other enables because enabling the other features
> doesn't change the semantic meaning of the program. It just makes sure the
> program will compile.
> 
> Here, however, calling enableFeature(kSampleMaskOverrideCoverage) causes the
> draw to produce different results, and therefore the caller needs to account
for
> this in their program key.
> 
> I have a few alternative options in mind and would like to hear your feedback
on
> which you like better:
> 
> 1) Just simplify it and let the GrPrimitiveProcessor add new outputs. It would
> then add this redefinition of gl_SampleMask.
> 
> 2) Add kShaderOverrideCoverage_Flag to GrPipeline(Builder)::Flags, then let
the
> framework handle putting this in the program key as well as redefining
> gl_SampleMask.
> 
> 3) Add features that can be enabled in GrProcessorKeyBuilder for things like
> this that affect the program key.
> 
> I don't like #3.
> 
> We will also need to enable sample locations in the fragment shader, and for
> that I think I like #2 best. However, it doesn't seem like as good of a fit
for
> sample_mask_override_coverage because this extension is pretty GLSL specific.
So
> maybe it doesn't belong in a "Gr" class.
> 
> #1 is simple, however it seems to violate the shader abstractions if we just
let
> anything define new outputs.
> 
> What do you think? Maybe a 4th option could be to add a new method to the
> builder that redefines gl_SampleMask, and it is understood that the caller is
> responsible for including this in their key?

It seems to me like the override should only ever be initiated by the
GrPrimProc. It allows the shader to add to the covarage mask and I can't think
of a use case for a GrFragProc to do that. Currently all coverage processors
reduce coverage, never add it. So perhaps we could have a FSBuilder query that
provides the variable name of the mask (and enables the feature on the first
call). The GrPP could somehow signal whether the mask, if it is requested (e.g.
bool GrPP::useSampleMaskOverride()), should be in override mode or not. Then
when the FSBuilder is first queried for the mask var it either enables it in
override mode or not.

[Chris Dalton, 2016-02-19 03:22:20]

Description was changed from

==========
Add infastructure for gl_SampleMask

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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094
==========

to

==========
Add sample mask functionality to GLSL fragment builders

Adds methods for overriding and masking a fragment's sample mask.


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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094
==========

[Chris Dalton, 2016-02-19 03:24:15]

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

[commit-bot: I haz the power, 2016-02-19 03:24:30]

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

[Chris Dalton, 2016-02-19 03:25:54]

Description was changed from

==========
Add sample mask functionality to GLSL fragment builders

Adds methods for overriding and masking a fragment's sample mask.


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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094
==========

to

==========
Add gl_SampleMask functionality to fragment builders

Adds methods for overriding and masking a fragment's sample mask.


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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094
==========

[Chris Dalton, 2016-02-19 03:42:25]

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

[commit-bot: I haz the power, 2016-02-19 03:42:35]

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

[commit-bot: I haz the power, 2016-02-19 04:18:55]

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

[commit-bot: I haz the power, 2016-02-19 04:18:56]

Dry run: This issue passed the CQ dry run.

[Chris Dalton, 2016-02-19 20:00:44]

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

[commit-bot: I haz the power, 2016-02-19 20:00:53]

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

[Chris Dalton, 2016-02-19 20:02:41]

Ok, this is ready for re-review.

- Added maskSampleCoverage and overrideSampleCoverage methods to the fragment
builders

- Fixed the 320 es bot failures

https://codereview.chromium.org/1690963003/diff/160001/src/gpu/gl/GrGLGLSL.cpp
File src/gpu/gl/GrGLGLSL.cpp (left):

https://codereview.chromium.org/1690963003/diff/160001/src/gpu/gl/GrGLGLSL.cp...
src/gpu/gl/GrGLGLSL.cpp:36: if (ver >= GR_GLSL_VER(3,1)) {
This is why the bots were failing before. These values need to be:

GR_GLSL_VER(3,20)
GR_GLSL_VER(3,10)

not

GR_GLSL_VER(3,2)
GR_GLSL_VER(3,1)

[Chris Dalton, 2016-02-19 20:02:45]

Ok, this is ready for re-review.

- Added maskSampleCoverage and overrideSampleCoverage methods to the fragment
builders

- Fixed the 320 es bot failures

[commit-bot: I haz the power, 2016-02-19 20:30:26]

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

[commit-bot: I haz the power, 2016-02-19 20:30:27]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-02-22 14:38:44]

On 2016/02/19 20:30:27, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

lgtm

[Chris Dalton, 2016-02-22 15:32:34]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-02-22 15:32:57]

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

[commit-bot: I haz the power, 2016-02-22 15:55:46]

Description was changed from

==========
Add gl_SampleMask functionality to fragment builders

Adds methods for overriding and masking a fragment's sample mask.


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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094
==========

to

==========
Add gl_SampleMask functionality to fragment builders

Adds methods for overriding and masking a fragment's sample mask.


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/+/533cefe5b9c7cec2592fc7ca00ee4cf69a26c094

Committed:
https://skia.googlesource.com/skia/+/33ad701bc30387127c427fb1e38c781d5de33491
==========

[commit-bot: I haz the power, 2016-02-22 15:55:47]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/33ad701bc30387127c427fb1e38c781d5de33491