Add "sample locations" feature to GrProcessor

Add "sample locations" feature to GrProcessor

Adds a "sample locations" feature to GrProcessor. When enabled, this
allows a processor to know inside the shader where all the samples are
located. Also adds various infastructure to query, cache, and identify
multisample data.

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

Committed: https://skia.googlesource.com/skia/+/28f45b949acc746849100fbe112ee5280f0594c9

[Chris Dalton, 2016-02-22 18:03:58]

Description was changed from

==========
Add sample locations to the pipeline

Adds an optional sample locations flag to GrPipelineBuilder. When
enabled, this flag causes the fragment builder to include sample
locations and make them available to fragment processors. Adds various
infastructure to query, cache, and identify multisample data.

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

to

==========
Add sample locations to the pipeline

Adds an optional sample locations flag to GrPipelineBuilder. When
enabled, this flag causes the fragment builder to include sample
locations and make them available to fragment processors. Adds various
infastructure to query, cache, and identify multisample data.

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

[Chris Dalton, 2016-02-22 18:06:18]

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

[Chris Dalton, 2016-02-22 18:06:48]

cdalton@nvidia.com changed reviewers:
+ mjk@nvidia.com

[Chris Dalton, 2016-02-22 18:06:48]

Adding mark for the OpenGL side.

[Mark Kilgard, 2016-02-22 18:41:04]

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

[Mark Kilgard, 2016-02-22 18:41:04]

LGTM as far as OpenGL is concerned

minor comments below

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:417: }
should you use SkPoint::operator< instead if it exists?

perhaps there isn't one or having one doesn't make sense

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3808: (*pattern)[i].set(pos[0] - 0.5f, 0.5f - pos[1]);
so do samples in Skia are in a [-0.5, 0.5) range where OpenGL uses [0,1] so
(0.5,0.5) is at the pixel center?

This is a bit surprising

The comment probably should note not just the inverted y direction but also the
2D shift by half-pixel for OpenGL pixel space vs. Skia device space

or is this to make interpolation in shaders easier by having pixel center at
(0,0)?

[Chris Dalton, 2016-02-22 19:10:43]

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:417: }
On 2016/02/22 18:41:04, Mark Kilgard wrote:
> should you use SkPoint::operator< instead if it exists?
> 
> perhaps there isn't one or having one doesn't make sense

This particular test doesn't have any real geometric meaning. I don't think it
would make much sense in SkPoint. Its only purpose is to provide an arbitrary
ordering for std::map.

It's probably worth adding a comment though.

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3808: (*pattern)[i].set(pos[0] - 0.5f, 0.5f - pos[1]);
On 2016/02/22 18:41:04, Mark Kilgard wrote:
> so do samples in Skia are in a [-0.5, 0.5) range where OpenGL uses [0,1] so
> (0.5,0.5) is at the pixel center?
> 
> This is a bit surprising
> 
> The comment probably should note not just the inverted y direction but also
the
> 2D shift by half-pixel for OpenGL pixel space vs. Skia device space
> 
> or is this to make interpolation in shaders easier by having pixel center at
> (0,0)?

The 0.5, 0.5 offset was added by me just because it makes sense for
interpolation in shaders.

You bring up a good point. As far as the skia API is concerned, maybe it would
make more sense for 0,0 to be the top left corner? Brian? Josh? Thoughts? The
builders can easily account for this elsewhere if needed.

[bsalomon, 2016-02-22 20:28:26]

https://codereview.chromium.org/1717393002/diff/20001/include/gpu/GrRenderTar...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1717393002/diff/20001/include/gpu/GrRenderTar...
include/gpu/GrRenderTarget.h:203: * greater than the surface descriptor's sample
count.
How does that happen? Is it referring to the fact that the desc's sample count
request gets rounded up to a value supported by the GL impl?

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.h#newcode496
src/gpu/GrGpu.h:496: class SamplePattern : public SkTArray<SkPoint, true> {
It seems a little odd to me for this to be a point array instead of just contain
one. Does the public array interface really make sense for this?

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrPipeline.h
File src/gpu/GrPipeline.h (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrPipeline.h#ne...
src/gpu/GrPipeline.h:210: const GrRenderTargetPriv::MultisampleSpecs&
getMultisampleSpecs() const {
We anticipate render target being removed from pipeline and being a property of
GrDrawTarget (once the multi-draw buffer change is complete and each draw target
contains draws for only one render target). Wondering how this will evolve in
that world.

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrPipelineBuild...
File src/gpu/GrPipelineBuilder.h (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrPipelineBuild...
src/gpu/GrPipelineBuilder.h:297: kSampleLocations_Flag = 0x04,
Where/how will this get set?

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3808: (*pattern)[i].set(pos[0] - 0.5f, 0.5f - pos[1]);
On 2016/02/22 19:10:43, Chris Dalton wrote:
> On 2016/02/22 18:41:04, Mark Kilgard wrote:
> > so do samples in Skia are in a [-0.5, 0.5) range where OpenGL uses [0,1] so
> > (0.5,0.5) is at the pixel center?
> > 
> > This is a bit surprising
> > 
> > The comment probably should note not just the inverted y direction but also
> the
> > 2D shift by half-pixel for OpenGL pixel space vs. Skia device space
> > 
> > or is this to make interpolation in shaders easier by having pixel center at
> > (0,0)?
> 
> The 0.5, 0.5 offset was added by me just because it makes sense for
> interpolation in shaders.
> 
> You bring up a good point. As far as the skia API is concerned, maybe it would
> make more sense for 0,0 to be the top left corner? Brian? Josh? Thoughts? The
> builders can easily account for this elsewhere if needed.

Skia's device coordinates have integers at the borders between pixels, so I
think it makes sense for 0.5, 0.5 in sample coordinates to be at the pixel
center.

[Chris Dalton, 2016-02-22 21:19:11]

https://codereview.chromium.org/1717393002/diff/20001/include/gpu/GrRenderTar...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1717393002/diff/20001/include/gpu/GrRenderTar...
include/gpu/GrRenderTarget.h:203: * greater than the surface descriptor's sample
count.
On 2016/02/22 20:28:25, bsalomon wrote:
> How does that happen? Is it referring to the fact that the desc's sample count
> request gets rounded up to a value supported by the GL impl?

Yeah, it's in 4.4.2 of the 3.0 spec:

 If samples is zero, then RENDERBUFFER_SAMPLES is set to zero. Otherwise
 samples represents a request for a desired minimum number of samples. Since
 different implementations may support different sample counts for multisample
 rendering, the actual number of samples allocated for the renderbuffer image is
 implementation-dependent. However, the resulting value for RENDERBUFFER_-
 SAMPLES is guaranteed to be greater than or equal to samples and no more than
 the next larger sample count supported by the implementation.

This isn't something we can always determine at creation time either:
- Mixed sampled render target won't have a stencil attachment yet.
- EXT_raster_multisample allows for the sample pattern to be different, and
never specifies whether the implementation-dependent sample count has to match
(though I can't imagine it ever not matching.)

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.h#newcode496
src/gpu/GrGpu.h:496: class SamplePattern : public SkTArray<SkPoint, true> {
On 2016/02/22 20:28:25, bsalomon wrote:
> It seems a little odd to me for this to be a point array instead of just
contain
> one. Does the public array interface really make sense for this?

Yeah, that felt a little weird. The only reason this class even exists is to
overload the < operator. Maybe the better option would be to give std::map a
custom comparator. It would also eliminate the confusion around what the
SamplePattern < operator was for.

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrPipeline.h
File src/gpu/GrPipeline.h (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrPipeline.h#ne...
src/gpu/GrPipeline.h:210: const GrRenderTargetPriv::MultisampleSpecs&
getMultisampleSpecs() const {
On 2016/02/22 20:28:25, bsalomon wrote:
> We anticipate render target being removed from pipeline and being a property
of
> GrDrawTarget (once the multi-draw buffer change is complete and each draw
target
> contains draws for only one render target). Wondering how this will evolve in
> that world.

I imagine it would be straightforward, but I can only guess since I'm really in
the dark when it comes to future plans :-(

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrPipelineBuild...
File src/gpu/GrPipelineBuilder.h (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrPipelineBuild...
src/gpu/GrPipelineBuilder.h:297: kSampleLocations_Flag = 0x04,
On 2016/02/22 20:28:26, bsalomon wrote:
> Where/how will this get set?

I envision it being set by the code that sets up the pipeline for a specific
type of batch.

e.g.

 batch = shapeRendering->createBatch(..., &pipelineBuilder);

Then ShapeRendering (or whatever it ends up being named) modifies the
HWAntialias and SampleLocations flags as needed.

For example, maybe somebody wants to draw something to a non-MSAA texture but
the view matrix has persective (not supported by coverage shaders). If
createBatch() notices you have EXT_raster_multisample, it can enable HW
antialias and sample locations in the builder, then return a batch for mixed
samples.

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3808: (*pattern)[i].set(pos[0] - 0.5f, 0.5f - pos[1]);
On 2016/02/22 20:28:26, bsalomon wrote:
> On 2016/02/22 19:10:43, Chris Dalton wrote:
> > On 2016/02/22 18:41:04, Mark Kilgard wrote:
> > > so do samples in Skia are in a [-0.5, 0.5) range where OpenGL uses [0,1]
so
> > > (0.5,0.5) is at the pixel center?
> > > 
> > > This is a bit surprising
> > > 
> > > The comment probably should note not just the inverted y direction but
also
> > the
> > > 2D shift by half-pixel for OpenGL pixel space vs. Skia device space
> > > 
> > > or is this to make interpolation in shaders easier by having pixel center
at
> > > (0,0)?
> > 
> > The 0.5, 0.5 offset was added by me just because it makes sense for
> > interpolation in shaders.
> > 
> > You bring up a good point. As far as the skia API is concerned, maybe it
would
> > make more sense for 0,0 to be the top left corner? Brian? Josh? Thoughts?
The
> > builders can easily account for this elsewhere if needed.
> 
> Skia's device coordinates have integers at the borders between pixels, so I
> think it makes sense for 0.5, 0.5 in sample coordinates to be at the pixel
> center.

Agreed. Now that I think about it I also prefer 0.5, 0.5 for the center.

[Chris Dalton, 2016-02-22 22:17:55]

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:417: }
On 2016/02/22 19:10:43, Chris Dalton wrote:
> On 2016/02/22 18:41:04, Mark Kilgard wrote:
> > should you use SkPoint::operator< instead if it exists?
> > 
> > perhaps there isn't one or having one doesn't make sense
> 
> This particular test doesn't have any real geometric meaning. I don't think it
> would make much sense in SkPoint. Its only purpose is to provide an arbitrary
> ordering for std::map.
> 
> It's probably worth adding a comment though.

Done.

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/GrGpu.h#newcode496
src/gpu/GrGpu.h:496: class SamplePattern : public SkTArray<SkPoint, true> {
On 2016/02/22 21:19:11, Chris Dalton wrote:
> On 2016/02/22 20:28:25, bsalomon wrote:
> > It seems a little odd to me for this to be a point array instead of just
> contain
> > one. Does the public array interface really make sense for this?
> 
> Yeah, that felt a little weird. The only reason this class even exists is to
> overload the < operator. Maybe the better option would be to give std::map a
> custom comparator. It would also eliminate the confusion around what the
> SamplePattern < operator was for.

Done.

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3808: (*pattern)[i].set(pos[0] - 0.5f, 0.5f - pos[1]);
On 2016/02/22 21:19:11, Chris Dalton wrote:
> On 2016/02/22 20:28:26, bsalomon wrote:
> > On 2016/02/22 19:10:43, Chris Dalton wrote:
> > > On 2016/02/22 18:41:04, Mark Kilgard wrote:
> > > > so do samples in Skia are in a [-0.5, 0.5) range where OpenGL uses [0,1]
> so
> > > > (0.5,0.5) is at the pixel center?
> > > > 
> > > > This is a bit surprising
> > > > 
> > > > The comment probably should note not just the inverted y direction but
> also
> > > the
> > > > 2D shift by half-pixel for OpenGL pixel space vs. Skia device space
> > > > 
> > > > or is this to make interpolation in shaders easier by having pixel
center
> at
> > > > (0,0)?
> > > 
> > > The 0.5, 0.5 offset was added by me just because it makes sense for
> > > interpolation in shaders.
> > > 
> > > You bring up a good point. As far as the skia API is concerned, maybe it
> would
> > > make more sense for 0,0 to be the top left corner? Brian? Josh? Thoughts?
> The
> > > builders can easily account for this elsewhere if needed.
> > 
> > Skia's device coordinates have integers at the borders between pixels, so I
> > think it makes sense for 0.5, 0.5 in sample coordinates to be at the pixel
> > center.
> 
> Agreed. Now that I think about it I also prefer 0.5, 0.5 for the center.

Done.

[Chris Dalton, 2016-02-22 22:20:48]

Description was changed from

==========
Add sample locations to the pipeline

Adds an optional sample locations flag to GrPipelineBuilder. When
enabled, this flag causes the fragment builder to include sample
locations and make them available to fragment processors. Adds various
infastructure to query, cache, and identify multisample data.

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

to

==========
Add sample locations to the pipeline

Adds an optional sample locations flag to GrPipelineBuilder. When
enabled, this flag causes the fragment builder to include built-in
information about the sample locations for the processors. Also adds 
various infastructure to query, cache, and identify multisample data.

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

[Chris Dalton, 2016-02-23 01:51:01]

https://codereview.chromium.org/1717393002/diff/40001/src/gpu/glsl/GrGLSLFrag...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1717393002/diff/40001/src/gpu/glsl/GrGLSLFrag...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp:174:
SkASSERT(fProgramBuilder->pipeline().hasSampleLocations());
I guess maskSampleCoverage() does a test/SkDEBUGFAIL/return instead of just an
assert.

Is there a preference on how to handle this? I don't see much precedent in this
file for asserts.

[Chris Dalton, 2016-02-29 20:50:43]

Description was changed from

==========
Add sample locations to the pipeline

Adds an optional sample locations flag to GrPipelineBuilder. When
enabled, this flag causes the fragment builder to include built-in
information about the sample locations for the processors. Also adds 
various infastructure to query, cache, and identify multisample data.

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

to

==========
Add "sample locations" feature to GrProcessor

Adds a "sample locations" feature to GrProcessor. When enabled, this
allows a processor to know inside the shader where all the samples are
located. Also adds various infastructure to query, cache, and identify
multisample data.

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

[Chris Dalton, 2016-02-29 20:52:50]

Made the changes we discussed in the meeting.

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/glsl/GrGLSLFrag...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/glsl/GrGLSLFrag...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp:188:
SkASSERT(fProgramBuilder->canUseSampleLocations());
Is an assert here good enough? Or do we prefer an actual check,followed by
emitting "vec2(0)" if sample locations are not supported?

[bsalomon, 2016-03-01 14:28:55]

https://codereview.chromium.org/1717393002/diff/60001/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/1717393002/diff/60001/include/gpu/GrProcessor...
include/gpu/GrProcessor.h:87: kFragmentPosition_RequiredFeature  = 1,
can we make this 1 << 0? Or make them 0x1 and 0x2?

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:4150:
this->flushRenderTarget(static_cast<GrGLRenderTarget*>(rt),
&SkIRect::EmptyIRect());
Is this why we wind up caching this on the RT? Is there no way to query GL for
the effective sample count and locations given some info about the
renderbuffer/texture that we want to create (like its internal format and
requested sample count).

The dual caching on the RT and GPU objects is the part of this CL I find the
most confusing. I think I understand what's going on, but it seems unfortunately
convoluted. Also, unfortunate that we're going to make this same query for each
target even though I assume we'll get the same sample count/pattern for any
given format/sample count request.

Even if we can't query without first binding a render target are there any
invariants guaranteed by the spec? e.g. same sized internal format and same
requested sample count -> the same actual sample count and locations?

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:4163: (*pattern)[i].set(pos[0], 1 - pos[1]);
Skia conceptually places the origin at the top of its canvas but we can render
either upside-down or right-side up from OpenGL's point of view. E.g. when we
render into the default framebuffer we don't want to render upside down so the
client will specify that the origin is kBottomLeft_GrSurfaceOrigin and we will
flip in the vertex shader. You can see how this works in 
GrGLSLFragmentShaderBuilder::fragmentPosition() we may or may not have to flip
gl_FragCoord. Seems like that surface origin property is is relevant here.

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/glsl/GrGLSLFrag...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/glsl/GrGLSLFrag...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp:188:
SkASSERT(fProgramBuilder->canUseSampleLocations());
On 2016/02/29 20:52:50, Chris Dalton wrote:
> Is an assert here good enough? Or do we prefer an actual check,followed by
> emitting "vec2(0)" if sample locations are not supported?

An assert seems fine.

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/glsl/GrGLSLFrag...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.h (right):

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/glsl/GrGLSLFrag...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.h:70: enum Coordinates {
Why do we need both of these?

[Chris Dalton, 2016-03-01 20:37:47]

https://codereview.chromium.org/1717393002/diff/60001/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/1717393002/diff/60001/include/gpu/GrProcessor...
include/gpu/GrProcessor.h:87: kFragmentPosition_RequiredFeature  = 1,
On 2016/03/01 14:28:55, bsalomon wrote:
> can we make this 1 << 0? Or make them 0x1 and 0x2?

Sure

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:4150:
this->flushRenderTarget(static_cast<GrGLRenderTarget*>(rt),
&SkIRect::EmptyIRect());
On 2016/03/01 14:28:55, bsalomon wrote:
> Is this why we wind up caching this on the RT? Is there no way to query GL for
> the effective sample count and locations given some info about the
> renderbuffer/texture that we want to create (like its internal format and
> requested sample count).
> 
> The dual caching on the RT and GPU objects is the part of this CL I find the
> most confusing. I think I understand what's going on, but it seems
unfortunately
> convoluted. Also, unfortunate that we're going to make this same query for
each
> target even though I assume we'll get the same sample count/pattern for any
> given format/sample count request.
> 
> Even if we can't query without first binding a render target are there any
> invariants guaranteed by the spec? e.g. same sized internal format and same
> requested sample count -> the same actual sample count and locations?

EXT_raster_multisample says: 

  "The sample locations chosen are a function of only the
   parameters to RasterSamplesEXT and not of any other state."

(https://developer.nvidia.com/sites/default/files/akamai/opengl/specs/GL_EXT_r...)

This is the reason why raster multisample state is getting cached on the GPU. 

But unfortunately, the spec for glGetMultisamplefv is very poorly written and
gives no indication of what state may affect sample locations.

(https://www.khronos.org/registry/gles/specs/3.1/es_spec_3.1.pdf p. 294)

Come to think of it, caching sample locations on the render target is just
speculatively guessing what state may affect sample locations, but still
assuming other state will not (e.g. blend state, depth test, scissor, phase of
the moon...)

I can guarantee that on NVIDIA, sample count is the only state that will affect
the sample pattern. And I think this is all but implied by the fact that it's
valid to swap a stencil buffer to any other render target as long as the sample
counts match. (e.g. how could it make any sense to swap stencil buffers around
if the sample locations didn't match?
https://www.khronos.org/opengles/sdk/docs/man3/html/glCheckFramebufferStatus....)

How about this: why don't we just cache sample patters on the GPU based on
sample count and origin? Then in debug mode we can call getMultisamplefv every
time and verify the cached sample pattern is correct.

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:4163: (*pattern)[i].set(pos[0], 1 - pos[1]);
On 2016/03/01 14:28:55, bsalomon wrote:
> Skia conceptually places the origin at the top of its canvas but we can render
> either upside-down or right-side up from OpenGL's point of view. E.g. when we
> render into the default framebuffer we don't want to render upside down so the
> client will specify that the origin is kBottomLeft_GrSurfaceOrigin and we will
> flip in the vertex shader. You can see how this works in 
> GrGLSLFragmentShaderBuilder::fragmentPosition() we may or may not have to flip
> gl_FragCoord. Seems like that surface origin property is is relevant here.

Ah, good point. Device space is a Skia concept, so this should follow the Skia
conventions.

It looks like GrGLIRect::setRelativeTo also gives a good definition of how to
convert between window space and device space. I'll model that.

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/glsl/GrGLSLFrag...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.h (right):

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/glsl/GrGLSLFrag...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.h:70: enum Coordinates {
On 2016/03/01 14:28:55, bsalomon wrote:
> Why do we need both of these?

The short answer is that it makes life easier for the GrProcessor.
interpolateAtOffset requires window coordinates, whereas for any coord
transforms it's easier (less bug prone) to just stay in Skia device coordinates.
This is especially true considering the conversion between device space and
window space depends on the origin and therefore must also be reflected in the
program key.

Generally a shader won't use both, but in the event that it does, the generated
byte code ought to be about the same anyway whether or not there are two
different arrays.

e.g. "interpolateAtOffset(v, deviceSpaceOffset * vec2(1, -1))": that second
parameter would get constant folded anyway so the call would still compile into
just a call to interpolateAtOffset with a constant value.

Now, the *real* answer is that the NVIDIA compiler has a bug if you call
"interpolateAtOffset(v, offset * vec2(1, -1))". And the workaround is to use a
separate array where the constant folding has already happend :-(

[Chris Dalton, 2016-03-07 16:09:38]

Implemented caching of multisample info based on sample count and surface origin
alone.

https://codereview.chromium.org/1717393002/diff/60001/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/1717393002/diff/60001/include/gpu/GrProcessor...
include/gpu/GrProcessor.h:87: kFragmentPosition_RequiredFeature  = 1,
On 2016/03/01 20:37:47, Chris Dalton wrote:
> On 2016/03/01 14:28:55, bsalomon wrote:
> > can we make this 1 << 0? Or make them 0x1 and 0x2?
> 
> Sure

Done.

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:4163: (*pattern)[i].set(pos[0], 1 - pos[1]);
On 2016/03/01 20:37:47, Chris Dalton wrote:
> On 2016/03/01 14:28:55, bsalomon wrote:
> > Skia conceptually places the origin at the top of its canvas but we can
render
> > either upside-down or right-side up from OpenGL's point of view. E.g. when
we
> > render into the default framebuffer we don't want to render upside down so
the
> > client will specify that the origin is kBottomLeft_GrSurfaceOrigin and we
will
> > flip in the vertex shader. You can see how this works in 
> > GrGLSLFragmentShaderBuilder::fragmentPosition() we may or may not have to
flip
> > gl_FragCoord. Seems like that surface origin property is is relevant here.
> 
> Ah, good point. Device space is a Skia concept, so this should follow the Skia
> conventions.
> 
> It looks like GrGLIRect::setRelativeTo also gives a good definition of how to
> convert between window space and device space. I'll model that.

Done.

[bsalomon, 2016-03-07 19:10:06]

https://codereview.chromium.org/1717393002/diff/80001/include/gpu/GrCaps.h
File include/gpu/GrCaps.h (right):

https://codereview.chromium.org/1717393002/diff/80001/include/gpu/GrCaps.h#ne...
include/gpu/GrCaps.h:279: const MultisampleSpecs& getMultisampleSpecs(const
GrSurfaceDesc&,
I'm curious why this has to be on GrCaps (as opposed to just GrGLCaps)? It seems
like putting  
It seems like you could have:

  GrRT::getSpecs(stencil)
     calls virtual GrGpu::getSpecs(this, stencil)

  GrGLGpu::getSpecs() override {
     auto rtBinder = [<capture the rt>] { bind the rt };
     // Make a call on GrGLCaps() that binds the RT if necessary and implements
caching
  }

I was hoping that in other APIs there would be less stateful way of making this
query that didn't need to necessitate a functor for tracking what state changes
occurred in order to get the sample locations.

[Chris Dalton, 2016-03-07 19:36:44]

https://codereview.chromium.org/1717393002/diff/80001/include/gpu/GrCaps.h
File include/gpu/GrCaps.h (right):

https://codereview.chromium.org/1717393002/diff/80001/include/gpu/GrCaps.h#ne...
include/gpu/GrCaps.h:279: const MultisampleSpecs& getMultisampleSpecs(const
GrSurfaceDesc&,
On 2016/03/07 19:10:06, bsalomon wrote:
> I'm curious why this has to be on GrCaps (as opposed to just GrGLCaps)? It
seems
> like putting  
> It seems like you could have:
> 
>   GrRT::getSpecs(stencil)
>      calls virtual GrGpu::getSpecs(this, stencil)
> 
>   GrGLGpu::getSpecs() override {
>      auto rtBinder = [<capture the rt>] { bind the rt };
>      // Make a call on GrGLCaps() that binds the RT if necessary and
implements
> caching
>   }
> 
> I was hoping that in other APIs there would be less stateful way of making
this
> query that didn't need to necessitate a functor for tracking what state
changes
> occurred in order to get the sample locations.

There's nothing platform specific about caching sample patterns based on sample
count + origin, and for that reason I felt that the caching logic belongs in
GrCaps. (The same logic would apply to any backend.)

As far as the functor is concerned, it seems like if the caps class is what
initiates the query, it will need a functor no matter what. Unless, were you
thinking GrGLCaps should make the direct calls to glGetMultisamplefv?

[Chris Dalton, 2016-03-07 21:01:03]

Moves the multisample queries back into GrGpu

[Mark Kilgard, 2016-03-07 21:12:52]

I'm no expert on the Skia code, but LGTM

this is a good approach to properly maintaining the sample locations

https://codereview.chromium.org/1717393002/diff/100001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1717393002/diff/100001/src/gpu/GrGpu.cpp#newc...
src/gpu/GrGpu.cpp:467: #if !defined(SK_DEBUG)
nice approach

[bsalomon, 2016-03-07 21:18:44]

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

[bsalomon, 2016-03-07 21:18:45]

lgtm. Greg, can you do a quick scan in case this has any implications for your
changes?

Chris, can you stub out the virtual method on GrVkGpu?

[egdaniel, 2016-03-07 21:27:52]

On 2016/03/07 21:18:45, bsalomon wrote:
> lgtm. Greg, can you do a quick scan in case this has any implications for your
> changes?
> 
> Chris, can you stub out the virtual method on GrVkGpu?

This looks fine to me. My only requests would have been to update
GrVkProgramDesc (we really need to get these things shared) which you did, and
to stub the GrVkGpu function.

[Chris Dalton, 2016-03-07 21:30:19]

On 2016/03/07 21:27:52, egdaniel wrote:
> On 2016/03/07 21:18:45, bsalomon wrote:
> > lgtm. Greg, can you do a quick scan in case this has any implications for
your
> > changes?
> > 
> > Chris, can you stub out the virtual method on GrVkGpu?
> 
> This looks fine to me. My only requests would have been to update
> GrVkProgramDesc (we really need to get these things shared) which you did, and
> to stub the GrVkGpu function.

GrVkProgramDesc should already be up to date (did you see an issue there?)

Just added the GrVkGpu stubs.

[egdaniel, 2016-03-07 21:32:04]

On 2016/03/07 21:30:19, Chris Dalton wrote:
> On 2016/03/07 21:27:52, egdaniel wrote:
> > On 2016/03/07 21:18:45, bsalomon wrote:
> > > lgtm. Greg, can you do a quick scan in case this has any implications for
> your
> > > changes?
> > > 
> > > Chris, can you stub out the virtual method on GrVkGpu?
> > 
> > This looks fine to me. My only requests would have been to update
> > GrVkProgramDesc (we really need to get these things shared) which you did,
and
> > to stub the GrVkGpu function.
> 
> GrVkProgramDesc should already be up to date (did you see an issue there?)
> 
> Just added the GrVkGpu stubs.

yeah I meant that you already did that, which would have been my only request

[Chris Dalton, 2016-03-07 21:43:33]

On 2016/03/07 21:32:04, egdaniel wrote:
> On 2016/03/07 21:30:19, Chris Dalton wrote:
> > On 2016/03/07 21:27:52, egdaniel wrote:
> > > On 2016/03/07 21:18:45, bsalomon wrote:
> > > > lgtm. Greg, can you do a quick scan in case this has any implications
for
> > your
> > > > changes?
> > > > 
> > > > Chris, can you stub out the virtual method on GrVkGpu?
> > > 
> > > This looks fine to me. My only requests would have been to update
> > > GrVkProgramDesc (we really need to get these things shared) which you did,
> and
> > > to stub the GrVkGpu function.
> > 
> > GrVkProgramDesc should already be up to date (did you see an issue there?)
> > 
> > Just added the GrVkGpu stubs.
> 
> yeah I meant that you already did that, which would have been my only request

Oh right. Now that I re-read that I see what you meant :)

Thanks for the reviews!

[Chris Dalton, 2016-03-07 21:43:41]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-03-07 21:43:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1717393002/#ps120001
(title: "vk stubs")

[commit-bot: I haz the power, 2016-03-07 21:43:50]

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

[commit-bot: I haz the power, 2016-03-07 21:58:29]

Description was changed from

==========
Add "sample locations" feature to GrProcessor

Adds a "sample locations" feature to GrProcessor. When enabled, this
allows a processor to know inside the shader where all the samples are
located. Also adds various infastructure to query, cache, and identify
multisample data.

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

to

==========
Add "sample locations" feature to GrProcessor

Adds a "sample locations" feature to GrProcessor. When enabled, this
allows a processor to know inside the shader where all the samples are
located. Also adds various infastructure to query, cache, and identify
multisample data.

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/+/28f45b949acc746849100fbe112ee5280f0594c9
==========

[commit-bot: I haz the power, 2016-03-07 21:58:31]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/28f45b949acc746849100fbe112ee5280f0594c9