Implement support for mixed sampled render targets

Implement support for mixed sampled render targets

Adds a new FBO type kStencil_MSFBOType that is selected whenever
NV_framebuffer_mixed_samples extension is available.  In this new
FBO type a non-msaa color buffer is created with a multisampled
stencil buffer attachment.

Replaces numSamples() with separate numColorSamples and numStencilSamples
methods in RenderTarget.

In mixed samples mode non-MSAA codepaths are used to draw simple shapes,
while NVPR-rendered paths and text are rendered with a multisampled
stencil.

BUG=skia:3177
Committed: https://skia.googlesource.com/skia/+/dded69693dd3779f081326cde24c3954505b129d

[Chris Dalton, 2015-03-12 07:43:34]

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

[Chris Dalton, 2015-03-12 07:43:35]

Great! I love how small this is getting.

Where's the magic that causes paths to be drawn with NVPR when the stencil
buffer is antialiased? From reading the code it looks to me like only text is
using mixed_samples:

 * GrContext::internalDrawPath calls
pipelineBuilder->getRenderTarget()->isMultisampled()
   * returns false since they aren't asking for stencil explicitly
 * Therefore, calls through to GrPathRendererChain::getPathRenderer() with
kColorAntiAlias_DrawType
   * kColorAntiAlias_DrawType makes antiAlias be true
 * Calls GrStencilAndCoverPathRenderer::canDrawPath with antiAlias = true
 * GrStencilAndCoverPathRenderer::canDrawPath returns false because antiAlias is
true
   * antiAlias must be false becaue nvpr relies on hardware MSAA

Can you verify whether this is the case? I think it will be ok to fix this by
modifying GrStencilAndCoverPathRenderer::canDrawPath something like this:

    return !stroke.isHairlineStyle() &&
           (!antiAlias || // doesn't do per-path AA, relies on the target having
MSAA
           
pipelineBuilder->getRenderTarget()->isMultisampled(kStencil_GrSampleConfig)) && 
           pipelineBuilder->getStencil().isDisabled();

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget.h
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget....
include/gpu/GrRenderTarget.h:35: */
Comment may need some work.

@param requestedConfig
@return true if the surface is multisampled in all of the buffers specified by
the sample config, false if any are non-MSAA

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget....
include/gpu/GrRenderTarget.h:41: * @return the number of samples-per-pixel or
zero if non-MSAA.
Comment:

@return the number of samples-per-pixel or zero if any of the specified buffers
is non-MSAA.

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrTypes.h#newco...
include/gpu/GrTypes.h:435: };
So I've been thinking:

rt.numSamples(kUnified_GrSampleConfig)  isn't clear
rt.isMultisampled(kStencil_GrSampleConfig)  is a confusing way to ask if it's
multisampled anywhere

What if we take this enum out of GrTypes.h, and just put it inside of
GrRenderTarget? Maybe make two enums, one for numSamples and one for
isMultisampled. Something where you can ask about whether different stages of
the pipeline have multisample. e.g:

numSamples(StencilAndColor)
numSamples(Stencil)

isMultisampled(Anythere)
isMultisampled(Everywhere)
isMultisampled(Color)
isMultisampled(Stencil)
isMultisampled(Raster)

Also, we would have to replace 'GrSampleConfig fSampleConfig' from
GrBackendRenderTargetDesc with a bool.

This is mostly just a brain dump so I'm curious to hear what you think.

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrTypes.h#newco...
include/gpu/GrTypes.h:578: */
Any reason why this is at the end rather than right after fSampleCnt?

Also, I may reword the comment a bit. Maybe something more along these lines:

If fSampleCnt is > 1, which buffers are multisampled (color and stencil, or just
stencil?). The default is both. Note that mixing multisampled and aliased
buffers is only possible if NV_framebuffer_mixed_samples is available.

https://codereview.chromium.org/1001503002/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/1/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:221: if (fGpu->caps()->pathRenderingSupport() &&
renderTarget->isMultisampled(kStencil_GrSampleConfig)) {
 >100 columns

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

https://codereview.chromium.org/1001503002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:354: GL_CALL(CoverageModulation(GR_GL_RGBA));
Let's verify there isn't a performance penalty to leaving this always enabled.
If there is, we will want to only enable as needed (at the same time we
enable/disable MULTISAMPLE).

Also, I don't think this belongs under PathRendering. I'd probably put it under
kBlend_GrGLBackendState, or _maybe_ kMSAAEnable_GrGLBackendState.

https://codereview.chromium.org/1001503002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2112: #define RT_HAS_MSAA
(rt->isMultisampled(kStencil_GrSampleConfig) || isLineDraw)
This feels awkward to check the stencil. Ideally there would be way to ask "Do
you have multisampling anywhere?"

Also don't forget to fix the line above in the #if 0

[Chris Dalton, 2015-03-12 08:04:52]

Also:

Where is the magic that enables MULTISAMPLE when drawing with nvpr, and disables
it otherwise?

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

https://codereview.chromium.org/1001503002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:354: GL_CALL(CoverageModulation(GR_GL_RGBA));
One more thing:

The coverage modulation we use may depend on the blending mode. Premultiplied
alpha would use GL_RGBA, whereas not-premultiplied alpha would need GL_ALPHA.
Have you thought this through and figured that GL_RGBA is always the correct
mode for skia? If not, Skia has already solved this problem for their own
coverage modulation so we chould just copy how they do it.

[Chris Dalton, 2015-03-13 03:11:32]

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget.h
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget....
include/gpu/GrRenderTarget.h:113: GrSampleConfig sampleConfig =
kUnified_GrSampleConfig)
I don't think sampleConfig needs a default value.

[vbuzinov, 2015-03-13 13:37:23]

Some of the comments not addressed yet.

New version: squash with 3rd commit, flushHWAAState receives a GrSampleConfig
that is set to Stencil by DrawPath/DrawPaths (to request the multisampling and
coverage modulation).  

This seems to turn multisampling on/off appropriately for NVPR.

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget.h
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget....
include/gpu/GrRenderTarget.h:35: */
On 2015/03/12 07:43:35, Chris Dalton wrote:
> Comment may need some work.
> 
> @param requestedConfig
> @return true if the surface is multisampled in all of the buffers specified by
> the sample config, false if any are non-MSAA

Done.

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget....
include/gpu/GrRenderTarget.h:41: * @return the number of samples-per-pixel or
zero if non-MSAA.
On 2015/03/12 07:43:35, Chris Dalton wrote:
> Comment:
> 
> @return the number of samples-per-pixel or zero if any of the specified
buffers
> is non-MSAA.

Done.

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrRenderTarget....
include/gpu/GrRenderTarget.h:113: GrSampleConfig sampleConfig =
kUnified_GrSampleConfig)
On 2015/03/13 03:11:32, Chris Dalton wrote:
> I don't think sampleConfig needs a default value.

Done.

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/1001503002/diff/1/include/gpu/GrTypes.h#newco...
include/gpu/GrTypes.h:578: */
On 2015/03/12 07:43:35, Chris Dalton wrote:
> Any reason why this is at the end rather than right after fSampleCnt?
> 
> Also, I may reword the comment a bit. Maybe something more along these lines:
> 
> If fSampleCnt is > 1, which buffers are multisampled (color and stencil, or
just
> stencil?). The default is both. Note that mixing multisampled and aliased
> buffers is only possible if NV_framebuffer_mixed_samples is available.

Done.

https://codereview.chromium.org/1001503002/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/1/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:221: if (fGpu->caps()->pathRenderingSupport() &&
renderTarget->isMultisampled(kStencil_GrSampleConfig)) {
On 2015/03/12 07:43:35, Chris Dalton wrote:
>  >100 columns

Done.

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

https://codereview.chromium.org/1001503002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:354: GL_CALL(CoverageModulation(GR_GL_RGBA));
On 2015/03/12 07:43:35, Chris Dalton wrote:
> Let's verify there isn't a performance penalty to leaving this always enabled.
> If there is, we will want to only enable as needed (at the same time we
> enable/disable MULTISAMPLE).
> 
> Also, I don't think this belongs under PathRendering. I'd probably put it
under
> kBlend_GrGLBackendState, or _maybe_ kMSAAEnable_GrGLBackendState.

1-Done. 2-Not done

[Chris Dalton, 2015-03-14 02:51:30]

Seva, I've uploaded a new patch that I think will help with the HWAA state:

https://codereview.chromium.org/1002013006/

It makes it so pipeline->isHWAntialiasState() is always false when
GL_MULTISAMPLE needs to be disabled for nvfms. That way we don't need to pass
that extra stencilConfig parameter to flushHWAAState().

Then, we turn on GL_MULTISAMPLE for nvpr by adding this line to
GrStencilAndCoverTextContext::init() after fPipelineBuilder.setFromPaint:

  fPipelineBuilder.enableState(GrPipelineBuilder::kHWAntialias_StateBit);

And this line to GrStencilAndCoverPathRenderer::onDrawPath:

  pipelineBuilder->enableState(GrPipelineBuilder::kHWAntialias_StateBit);



Also, the more I think about it, we really do need a new enum for
isMultisampled. isMultisampled(kStencil_SampleConfig) can feel like it's
suggesting the multisampling is ONLY in the stencil buffer..

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

https://codereview.chromium.org/1001503002/diff/20001/include/gpu/GrRenderTar...
include/gpu/GrRenderTarget.h:34: * @param requestedConfig
Oops, sorry. I didn't finish my previous comment and then forgot to get back to
it. We should also document the requestedConfig parameter in the comment, not
just list it. But since we may reconsider these enum names it can probably wait.

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

https://codereview.chromium.org/1001503002/diff/20001/src/gpu/GrPipeline.cpp#...
src/gpu/GrPipeline.cpp:44:
localColorPOI.setNeedsCoverageModulation(isMixedSampledTarget);
No need to force blending unless we're actually using the mixed samples: (after
https://codereview.chromium.org/1002013006/)

 localColorPOI.setNeedsCoverageModulation(pipelineBuilder.isHWAntialias() &&
isMixedSampledTarget);

(Or we may want to move the block that sets fFlags to before this line, then
say:

  localColorPOI.setNeedsCoverageModulation(this->isHWAntialiasState() &&
isMixedSampledTarget);

)

https://codereview.chromium.org/1001503002/diff/20001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/1001503002/diff/20001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverPathRenderer.cpp:164:
pipelineBuilder->stencil()->setDisabled();
After https://codereview.chromium.org/1002013006/, add:

  pipelineBuilder->enableState(kHWAntialias_StateBit);

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

https://codereview.chromium.org/1001503002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2113: if (kGL_GrGLStandard == this->glStandard()) {
GL_MULTISAMPLE doesn't traditionally exist in ES 2.0+. We need to modify this if
statement:

  if (kGL_GrGLStandard == this->glStandard() ||
this->glCaps().fbMixedSamplesSupport()) {

https://codereview.chromium.org/1001503002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2127: if (rt->isMultisampled(kStencil_GrSampleConfig)) {
I think this would want to say: (after applying
https://codereview.chromium.org/1002013006/)

  if (useHWAA && kStencil_GrSampleConfig == rt->sampleConfig())

https://codereview.chromium.org/1001503002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2128: GL_CALL(CoverageModulation(GR_GL_RGBA));
We should add a state variable for fCoverageModulation alongside fMSAAEnabled,
to remove redundant calls into the driver. We can set it to "unkown" during
reset context.

[Chris Dalton, 2015-03-16 07:30:49]

https://codereview.chromium.org/1001503002/diff/20001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/1001503002/diff/20001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverPathRenderer.cpp:164:
pipelineBuilder->stencil()->setDisabled();
On 2015/03/14 02:51:30, Chris Dalton wrote:
> After https://codereview.chromium.org/1002013006/, add:
> 
>   pipelineBuilder->enableState(kHWAntialias_StateBit);

Oops, actually this needs to be in an if, so intentionally aliased rendering
stays aliased.

  if (antiAlias) {
     
SkASSERT(pipelineBuilder->getRenderTarget()->isMultisampled(kStencil_GrSampleConfig));
      pipelineBuilder->enableState(kHWAntialias_StateBit);
  }

[Chris Dalton, 2015-03-16 19:31:24]

Almost there -- this is coming together very nicely.

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverPathRenderer.cpp:106:
pipelineBuilder->getRenderTarget()->isMultisampled(kStencil_GrSampleConfig));
We can take out this assert from here since we do it below.

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:361:
fPipelineBuilder.enableState(GrPipelineBuilder::kHWAntialias_StateBit);
I forgot before that we technically should only enable MSAA if the paint wants
it.

  fPaint.setState(GrPipelineBuilder::kHWAntialias_StateBit,
fPaint.isAntiAlias());

But on second thought, maybe we should modify GrPipelineBuilder::setFromPaint
instead? That way we wouldn't have to turn around and enable kHWAntialias right
after setFromPaint disables it.

  setFromPaint(const GrPaint&, bool rtIsMultisampled, const GrClip&)
  setFromPaint(const GrPaint&, GrRenderTarget* rt, const GrClip&, GrSampleConfig
= kUnified)

I'm not convinced whether it would be better or worse to modify setFromPaint
something like the above. What do you think?

[Chris Dalton, 2015-03-17 00:39:35]

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

https://codereview.chromium.org/1001503002/diff/40001/include/gpu/GrRenderTar...
include/gpu/GrRenderTarget.h:32: // GrRenderTarget
I've thought about it more, and I recommend creating this new enum:

 enum PipelineStage {
     kColorBuffer_PipelineStage,
     kStencilBuffer_PipelineStage,
     kEverywhere_PipelineStage = kColorBuffer_PipelineStage,
     kAnywhere_PipelineStage = kStencil_PipelineStage,
     kRaster_PipelineStage = kStencil_PipelineStage
 };

Then, isMultisampled and numSamples accept a PipelineStage enum instead.

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverPathRenderer.cpp:64:
pipelineBuilder->getStencil().isDisabled();
Here we can punt and only use nvfms if coverage modulation will work. I'm going
to break it up just because it's getting more complicated..

 if (stroke.isHairlineStyle()) {
     return false;
 }
 if (!pipelineBuilder->getStencil().isDisabled()) {
     return false;
 }
 if (antiAlias) {
     return
pipelineBuilder->getRenderTarget()->isMultisampled(kStencil_GrSampleConfig)) &&
            pipelineBuilder->canTweakAlphaForCoverage();
 } else {
     return true;
 }

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/GrStencilAndCov...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/GrStencilAndCov...
src/gpu/GrStencilAndCoverTextContext.cpp:64: return rec.getFormat() !=
SkMask::kARGB32_Format;
Add some sort of check about:

 if (paint.isAntiAlias() && kStencil_GrSampleConfig == rt->sampleConfig()) {
     // Somehow call through to can_tweak_alpha_for_coverage in
GrPorterDuffXferProcessor.cpp
 }

I added https://codereview.chromium.org/1010113004/ , which should gives an an
rt variable in this funciton.

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/effects/GrPorte...
File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right):

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/effects/GrPorte...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:20: static bool
can_tweak_alpha_for_coverage(GrBlendCoeff dstCoeff) {
This looks like the magic function that tells us if coverage modulation will
work :)

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

https://codereview.chromium.org/1001503002/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2123: GL_CALL(CoverageModulation(GR_GL_RGBA));
I verified with Brian that Skia always writes premultiplied alpha. So this is
correct to always just do GL_RGBA.

[Chris Dalton, 2015-03-19 12:07:41]

Looking good. Sorry I didn't send this sooner.

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

https://codereview.chromium.org/1001503002/diff/80001/include/gpu/GrRenderTar...
include/gpu/GrRenderTarget.h:38: */
Maybe pipeline stage would be better to say than buffer. For example, raster is
a pipeline stage that could be multisampled, but not a buffer...

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

https://codereview.chromium.org/1001503002/diff/80001/include/gpu/GrTypes.h#n...
include/gpu/GrTypes.h:442: */
I think this enum belongs inside the GrRenderTarget class.

https://codereview.chromium.org/1001503002/diff/80001/include/gpu/GrTypes.h#n...
include/gpu/GrTypes.h:448: kRaster_GrPipelineStage =
kStencilBuffer_GrPipelineStage
We maybe don't have to include kRaster for now. Sorry about that. It has some
extra considerations if a stencil buffer isn't attached yet...

[vbuzinov, 2015-03-19 13:04:05]

Moved PipelineStage to GrRenderTarget, adjusted comments

[Chris Dalton, 2015-03-20 07:01:45]

Sorry, one more set of nit picks, mostly about comments. Once these are
addressed the patch looks good to me!

https://codereview.chromium.org/1001503002/diff/100001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/100001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:37: * the two.  Used by the
NV_framebuffer_mixed_samples implementation.
This comment should probably talk about the pipeline stage instead of the
buffer. I might reword it something like this:

"On some hardware it is possible for a render target to have multisampling only
in certain stages of the graphics pipeline. This enum allows the caller to query
about specific stages in isMultisampled() and numSamples()."

https://codereview.chromium.org/1001503002/diff/100001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:41: kStencilBuffer_PipelineStage,
These equalities seem like they need a comment.

 // We only support two sample configs at the moment, unified and stencil. So we
can query multisample info for everywhere/anywhere by checking the correct
buffer.

https://codereview.chromium.org/1001503002/diff/100001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:50: *         specified by the sample config, false
if any are non-MSAA
"true if the surface is multisampled in the specified pipeline stage(s), false
otherwise"

https://codereview.chromium.org/1001503002/diff/100001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:61: *         buffers is non-MSAA.
"the number of samples-per-pixel, or zero if the render target is not
multisampled in the specified pipeline stage(s)"

https://codereview.chromium.org/1001503002/diff/100001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:64: kEverywhere_PipelineStage) const {
Now I'm nit-picking, but I'm wondering if this function could be more clear if
it were implemented in this way?

  return this->isMultisampled(pipelineStage) ? fDesc.fSampleCnt : 0;

Then isMultisampled would obviously be the one to check fSampleConfig instead:

 if (kStencil_GrSampleConfig == fSampleConfig &&
     kColorBuffer_PipelineStage == pipelineStage) {
     return false;
 }
 return 0 != fDesc.fSampleCnt;

Thoughs? This way seems more natural to me at the moment...

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

https://codereview.chromium.org/1001503002/diff/100001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:222:
renderTarget->isMultisampled(GrRenderTarget::kAnywhere_PipelineStage)) {
The nvpr requirement is for a mulitsample stencil buffer, so we should check
kStencilBuffer_PipelineStage here.

https://codereview.chromium.org/1001503002/diff/100001/src/gpu/GrPipelineBuil...
File src/gpu/GrPipelineBuilder.h (right):

https://codereview.chromium.org/1001503002/diff/100001/src/gpu/GrPipelineBuil...
src/gpu/GrPipelineBuilder.h:49: GrRenderTarget::PipelineStage pipelineStage =
I think it's a little confusing to pass a parameter named 'pipelineStage' here.
It's not entirely obvious what the purpose of the parameter is just from reading
the code. I'm tryig to brainstorm for an approach I like better but am having a
hard time. Do you like any of these?

 * Give PipelineStage parameter a really descriptive name that says "pipeline
stages that need to be multisampled in order to use HWAA"
  ** What I don't like about this approach is that GrPipelineBuilder decides
whether to use HWAA, when really the calling code is who knows whether we should
use it. In the case of text, we shouldn't have to check
isMultiSampled(StencilBuffer) because we already know it's multisampled.
 * Instead of PipelineStage, add a second setFromPaint() overload with a new
parameter named 'bool useHWAA'. The original version calls this overload with
useHWAA='rt->isMultisampled() && paint.isAntiAlias()'
  ** What I don't like about this approach is that setFromPaint() seems to be
the one that should check paint.isAntiAlias()
 * instead of 'bool useHWAA', add 'bool canUseHWAA' with default value of
'rt->isMultisampled()' and setFromPaint uses 'paint.isAntiAlias() && canUseHWAA'
  ** What I don't like about this approach is that GrStencilAndCoverTextContext
has to send a bool 'true' value, which is confusing to read.
 * Instead of PipelineStage parameter, add PipelineStage member variable to
GrPipelineBuilder. kHWAntialias_StateBit becomes kAntialias_StateBit (whether or
not paint is antialiased) and isHWAntialias() returns '0 != (fFlagBits &
kAntialias_StateBit) &&
fRenderTarget->isMultisampled(fStagesThatHaveToBeMultisampledInOrderToUseHWAA)'
 * ...?

Maybe we could discuss this offline.

https://codereview.chromium.org/1001503002/diff/100001/src/gpu/GrStencilAndCo...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/1001503002/diff/100001/src/gpu/GrStencilAndCo...
src/gpu/GrStencilAndCoverPathRenderer.cpp:175:
GrRenderTarget::kAnywhere_PipelineStage));
Here we should also check GrRenderTarget::kStencilBuffer_PipelineStage, since
that's nvpr's requirement.

[Chris Dalton, 2015-03-23 19:15:37]

lgtm after one final comment

https://codereview.chromium.org/1001503002/diff/140001/src/gpu/GrProcOptInfo.h
File src/gpu/GrProcOptInfo.h (right):

https://codereview.chromium.org/1001503002/diff/140001/src/gpu/GrProcOptInfo....
src/gpu/GrProcOptInfo.h:60: fNeedsCoverageModulation = true;
Oops, this looks like a bug we've glossed over in the past. Should say:

  fNeedsCoverageModulation = modulate;

[vbuzinov, 2015-03-24 06:25:31]

Oops.  How did I miss that one!  Thanks Chris.

[vbuzinov, 2015-03-24 06:34:47]

vbuzinov@nvidia.com changed reviewers:
+ bsalomon@google.com

[vbuzinov, 2015-03-24 06:34:47]

Adding Brian - could you take a look?

The patch depends on https://codereview.chromium.org/993363002/ and
https://codereview.chromium.org/1010113004/

[Mark Kilgard, 2015-03-24 14:05:08]

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

[Mark Kilgard, 2015-03-24 14:05:09]

LGTM

https://codereview.chromium.org/1001503002/diff/140001/src/gpu/GrPipeline.cpp
File src/gpu/GrPipeline.cpp (right):

https://codereview.chromium.org/1001503002/diff/140001/src/gpu/GrPipeline.cpp...
src/gpu/GrPipeline.cpp:55: optFlags =
xferProcessor->getOptimizations(localColorPOI,
For Google:  enabling/disabling GL_BLEND when using
GL_ONE,GL_ONE_MINUS_SRC_ALPHA or GL_SRC_ALPHA,GL_ONE_MINUS_SRC_ALPHA blending is
probably just costing you CPU drivers doing driver validation if the alpha from
shader is guaranteed 1.0

NVIDIA GPUs for the last 5+ years should be able to render "just as fast" for
these blend modes when source alpha==1.0 as having blending disabled.  The GPU
recognizes the blend optimization automatically and avoids read-modify-writes
during alpha==1.0 blending.  But calling glEnable(GL_BLEND)/glDisable(GL_BLEND)
is going to cause driver validations.

Please consider having a mode so K1 and other GPUs can simply not excessively
enable/disable blending.

[Chris Dalton, 2015-03-26 05:11:39]

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrStencilAndCo...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrStencilAndCo...
src/gpu/GrStencilAndCoverPathRenderer.cpp:71:
pipelineBuilder->canTweakAlphaForCoverage();
Here too -- may need to check if it will do in-shader blending.

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrStencilAndCo...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrStencilAndCo...
src/gpu/GrStencilAndCoverTextContext.cpp:67:
!paint.getXPFactory()->canTweakAlphaForCoverage()) {
We may need to check if it will do in-shader blending as well. Maybe something
like this:

if (skPaint.isAntiAlias() &&
    kStencil_GrSampleConfig == rt->sampleConfig() &&
    (!paint.getXPFactory()->canTweakAlphaForCoverage() ||
     paint.getXPFactory()->willNeedDstCopy(this->getGpu()->caps(),
some_conservative_GrProcOptInfos...))) {

Is there an easy way to know if an xfer processor will potentially do in-shader
blending?

Since coverage gets modulated into the color *after* the fragment shader
(stencil test), in-shader blending won't work right now. (Though we could
theoretically support in-shader blending if we used ARB_blend_func_extended to
do an additional coverage modulation after the shader...)

[bsalomon, 2015-03-31 16:37:32]

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

[bsalomon, 2015-03-31 16:37:34]

WRT Mark's comment about blend disabling: I'll file a bug about this. I think
we'll have to do it selectively because some HW/drivers aren't so smart. :)

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:39: enum PipelineStage {
I'm not sure about this name... 

enum BufferType {
kColor_BufferType,
kStencil_BufferType
};

?

Also, should it be a bitfield?

uint32_t buffersWithMSAA() const { return fMSAABufferBitfield; }

/** Param must only have one flag bit set */
int sampleCount(BufferType buffer) const {
   if (this->buffersWithMSAA() & buffer) {
       return fDesc.fSampleCnt;
   }
   return 0;
}

something like that?

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrTypes.h#...
include/gpu/GrTypes.h:569: GrSampleConfig                  fSampleConfig;
Maybe just have a flag in fFlags? kStencilMSAAOnly_GrSurfaceFlag?

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrStencilAndCo...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrStencilAndCo...
src/gpu/GrStencilAndCoverTextContext.cpp:67:
!paint.getXPFactory()->canTweakAlphaForCoverage()) {
On 2015/03/26 05:11:39, Chris Dalton wrote:
> We may need to check if it will do in-shader blending as well. Maybe something
> like this:
> 
> if (skPaint.isAntiAlias() &&
>     kStencil_GrSampleConfig == rt->sampleConfig() &&
>     (!paint.getXPFactory()->canTweakAlphaForCoverage() ||
>      paint.getXPFactory()->willNeedDstCopy(this->getGpu()->caps(),
> some_conservative_GrProcOptInfos...))) {
> 
> Is there an easy way to know if an xfer processor will potentially do
in-shader
> blending?
> 
> Since coverage gets modulated into the color *after* the fragment shader
> (stencil test), in-shader blending won't work right now. (Though we could
> theoretically support in-shader blending if we used ARB_blend_func_extended to
> do an additional coverage modulation after the shader...)

I think your concern is correct. Adding egdaniel@ for commenting on how to know
if in shader blending will occur. Greg, do we always know at this point?

I think you're right that this could always work with ARB_blend_func_extended.
Set output0 = <fragment shader computed coverage> and output1 = <fragment shader
blended color>. output0 gets further modulated by COVERAGE_MODULATION. Set the
blend coeffs to GL_SRC1_COLOR SRC1 and GL_ONE_MINUS_SRC1_COLOR.

Moreover, we could force xp's that don't allow tweak-alpha-for-coverage into
doing frag shader blending in order to be able to always use
mixedsample/COVERAGE_MODULATION.

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/effects/GrPort...
File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/effects/GrPort...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:406:
!colorPOI.needsCoverageModulation()) {
I know needCoverageModulation mirrors the extension language but in this context
I find it confusing. We use "coverage" to mean any fractional pixel coverage no
matter how it is implemented (stencil, fragment shader calculation) and we
always use it to "modulate" somewhere in the pipeline (glBlend or in the frag
shader).

Could we use something a little more descriptive?

applyStencilCoverageModulation? modulateByStencilCoverageFraction?

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLCaps.h
File src/gpu/gl/GrGLCaps.h (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLCaps.h#...
src/gpu/gl/GrGLCaps.h:71: kStencil_MSFBOType,
Maybe we should call this something a little more descriptive.
kMixedSamples_MSFBOType?

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

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2105: fCoverageModulationEnabled = kYes_TriState;
For cases where we couldn't tweak alpha for coverage what happens here? is
useHWAA false?

[Chris Dalton, 2015-03-31 19:47:30]

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:39: enum PipelineStage {
I'm going to try replying inline. Hopefully it works ok.

On 2015/03/31 16:37:33, bsalomon wrote:
> I'm not sure about this name... 
> 
> enum BufferType {
> kColor_BufferType,
> kStencil_BufferType
> };
> 
> ?

One thing I should clarify is that this enum by design was not related to the
internal data structure of the RT. It was just intended as a tool for client
code to query about the multisampling.

The motivation was that, while internally our msaa was defined by GrSampleConfig
and a sample count, it was cleaner for the client code to make queries that
looked like the actual information it wanted to know:

  rt->isMultisampled(ColorBuffer);
  rt->isMultisampled(Anywhere);
  rt->numSamples(StencilBuffer);
  rt->isMultisampled(Rasterizer);  // Rasterizer for the future

I recommended the name "PipelineStage" for Seva because in the future we will
want to query about msaa in more than just buffers (e.g. the rasterizer), but I
think I've changed my mind. It would probably be best to limit it to buffers for
now like you suggest. We can worry about the other pipeline stages when we need
to.

>
> Also, should it be a bitfield?
>
>
> uint32_t buffersWithMSAA() const { return fMSAABufferBitfield; }
>

For the internal data structure, we intentionally tried to avoid something that
could allow for configurations that aren't supported (e.g. msaa in color but not
stencil), which is why we went the GrSampleConfig route.

A bitfield is a good idea though. Going off your suggestion, what about
something along these lines?

 enum BufferBit {
     kColor_BufferBit = 1,
     kStencil_BufferBit = (1 << 1)
 };
 typedef uint32_t BufferBits;

 enum SampleConfig {
     kUnified_SampleConfig = kColor_BufferBit + kStencil_BufferBit,
     kStencil_SampleConfig = kStencil_BufferBit
 };

 // Constructor accepts a SampleConfing...

 SampleConfig fSampleConfig; // Only Unified or Stencil configs are allowed.

 enum {
     kAny_BufferBits = 0,
     kEvery_BufferBit = kColor_BufferBit + kSample_BufferBit
 };

 /* Returns true if we are multisampled in all the buffers, blah blah blah */
 bool isMultisampled(BufferBits buffers) {
     if ((fSampleConfig & buffers) != buffers) {
         return false;
     }
     return 0 != fDesc.fSampleCnt;
 }

 // numSamples stays the same as in the patch

Obviously not polished, but this way we could get a cool bitfield while still
enforcing only two legal sample configs in the data structure (and more
importantly, the constructor.)

>
> /** Param must only have one flag bit set */
> int sampleCount(BufferType buffer) const {
>    if (this->buffersWithMSAA() & buffer) {
>        return fDesc.fSampleCnt;
>    }
>    return 0;
> }
> 
> something like that?
> 
>

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLCaps.h
File src/gpu/gl/GrGLCaps.h (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLCaps.h#...
src/gpu/gl/GrGLCaps.h:71: kStencil_MSFBOType,
On 2015/03/31 16:37:33, bsalomon wrote:
> Maybe we should call this something a little more descriptive.
> kMixedSamples_MSFBOType?

I really like the name "MixedSamples" too. I'd like to see that to become our
default catch-phrase for things that deal with this rendering mode.

My only concern is that NV_framebuffer_mixed_samples allows for multiple color
samples as well (e.g 16:4, 4:2), whereas we are forcing the color samples to 1.
Are you guys ok with "MixedSamples" being understood within skia to mean N:1
rendering? (I am if you are)

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

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2105: fCoverageModulationEnabled = kYes_TriState;
On 2015/03/31 16:37:33, bsalomon wrote:
> For cases where we couldn't tweak alpha for coverage what happens here? is
> useHWAA false?

Yes that's right. useHWAA always ends up false when
rt->isMultisampled(Everywhere) is false, unless somebody who knows about mixed
samples explicity enables it. On rt's without mixed samples (i.e. everywhere ==
anywhere), the assert makes sure of that:

  SkASSERT(!useHWAA ||
rt->isMultisampled(GrRenderTarget::kAnywhere_PipelineStage));

So when canTweakAlphaForCoverage() is not possible, we don't enable hwaa and
instead fall back on the default Gr draw code.

[Chris Dalton, 2015-03-31 22:24:57]

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:39: enum PipelineStage {
Sorry to pollute your inboxes :) I've thought about it more and I think I really
like this hybrid bitfield approach!

 enum BufferBits {
     kAny_BufferBits = 0,
     kColor_BufferBit = 1,
     kStencil_BufferBit = (1 << 1),
     kAll_BufferBits = kColor_BufferBit + kStencil_BufferBit
 };

 /* Enorce only two legal configs, blah blah blah */
 enum SampleConfig {
     kUnified_SampleConfig = kColor_BufferBit + kStencil_BufferBit,
     kStencil_SampleConfig = kStencil_BufferBit
 };

 /*Returns true if fSampleCnt > 0 and all the buffers are msaa, blah blah blah*/
 bool isMultisampled(BufferBits buffers = kAll_BufferBits) {
     if (buffers != (fSampleConfig & buffers)) {
         // One of the buffers is not in our msaa config.
         return false;
     }
     return 0 != fDesc.fSampleCnt;
 }

 ...

 SampleConfig fSampleConfig;
};

GR_MAKE_BITFIELD_OPS(GrRenderTarget::BufferBits)

Seva, Brian, thoughts?

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrTypes.h#...
include/gpu/GrTypes.h:569: GrSampleConfig                  fSampleConfig;
On 2015/03/31 16:37:33, bsalomon wrote:
> Maybe just have a flag in fFlags? kStencilMSAAOnly_GrSurfaceFlag?

I don't see an fFlags field for GrBackendRenderTargetDesc, should we add one,
along with a GrBackendRenderTargetFlags enum? I really like that, and it also
lets us move GrSampleConfig out of this file and into GrRenderTarget.

[bsalomon, 2015-04-01 13:15:36]

I'm wondering if this should be a property of the render target at all as
opposed to something that is set on the pipeline object. If it is part of the
GrRenderTarget it may have adverse effects WRT the resource cache since it would
have to be part of the key even though it isn't an intrinsic property of the
texture backing the RT. What if it was specified on somewhere on the pipeline
object, maybe indirectly via gp or xp, and we attached a msaa sb the first time
the RT is used in this fashion.

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:223: GrStencilBuffer* sb =
renderTarget->renderTargetPriv().attachStencilBuffer();
I'm not sure that this is necessary anymore. We should just attach a stencil
buffer when it is first used. Here is a place where it seems a bit awkward that
the msaa-ness of the stencil is property of the render target, since at this
point the rt might not even have a stencil buffer yet.

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

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:939: GrTexture* GrGLGpu::onCreateTexture(const
GrSurfaceDesc& origDesc, bool budgeted,
Do there need to be any changes in here? Does the current CL only work with
wrapped textures and not those created internally?

[Chris Dalton, 2015-04-01 20:34:17]

> it would have to be part of the key even though it isn't an intrinsic property
> of the texture backing the RT.

What would have to be part of the key if msaa stencil is a property of the
render target? Sorry, I'm probably not as familiar with the keys as I should be.
Also, if we put mixed samples further down the pipeline, does that mean it's not
an MSFBOType anymore?

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:223: GrStencilBuffer* sb =
renderTarget->renderTargetPriv().attachStencilBuffer();
On 2015/04/01 13:15:35, bsalomon wrote:
> I'm not sure that this is necessary anymore. We should just attach a stencil
> buffer when it is first used. Here is a place where it seems a bit awkward
that
> the msaa-ness of the stencil is property of the render target, since at this
> point the rt might not even have a stencil buffer yet.

So I don't think it has to be awkward. While it is true that the OpenGL fbo
might not have a stencil buffer attached yet, that is just an implementation
detail. For our purposes, the RT object *does* have a stencil, and that stencil
buffer *is* multisampled, and if we ever try to use it, it will be there.
(That's my understanding.. correct me if I'm wrong.) I feel that is definitely a
property of our RT object though. Just because at the implementation level the
stencil buffer is allocated lazily, I don't think it changes the intrinsic
properties of the abstract RT object.

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

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:939: GrTexture* GrGLGpu::onCreateTexture(const
GrSurfaceDesc& origDesc, bool budgeted,
On 2015/04/01 13:15:36, bsalomon wrote:
> Do there need to be any changes in here? Does the current CL only work with
> wrapped textures and not those created internally?

No changes are needed, everything just works out.

caps.msFBOType is kStencil_MSFBOType (or, kMixedSamples_MSFBOType), so it does
the right thing when we call createRenderTargetObjects.

[vbuzinov, 2015-04-02 13:10:37]

Rebase + kMixedSamples_MSFBOType and renamed the xp's flag to
applyStencilCoverageModulation.

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/160001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:39: enum PipelineStage {
On 2015/03/31 22:24:57, Chris Dalton wrote:
> Sorry to pollute your inboxes :) I've thought about it more and I think I
really
> like this hybrid bitfield approach!
> 
>  enum BufferBits {
>      kAny_BufferBits = 0,
>      kColor_BufferBit = 1,
>      kStencil_BufferBit = (1 << 1),
>      kAll_BufferBits = kColor_BufferBit + kStencil_BufferBit
>  };
> 
>  /* Enorce only two legal configs, blah blah blah */
>  enum SampleConfig {
>      kUnified_SampleConfig = kColor_BufferBit + kStencil_BufferBit,
>      kStencil_SampleConfig = kStencil_BufferBit
>  };
> 
>  /*Returns true if fSampleCnt > 0 and all the buffers are msaa, blah blah
blah*/
>  bool isMultisampled(BufferBits buffers = kAll_BufferBits) {
>      if (buffers != (fSampleConfig & buffers)) {
>          // One of the buffers is not in our msaa config.
>          return false;
>      }
>      return 0 != fDesc.fSampleCnt;
>  }
> 
>  ...
> 
>  SampleConfig fSampleConfig;
> };
> 
> GR_MAKE_BITFIELD_OPS(GrRenderTarget::BufferBits)
> 
> Seva, Brian, thoughts?

This bitfield implementation is certainly easier to read and understand, so I am
all for it.

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/effects/GrPort...
File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/effects/GrPort...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:406:
!colorPOI.needsCoverageModulation()) {
On 2015/03/31 16:37:33, bsalomon wrote:
> I know needCoverageModulation mirrors the extension language but in this
context
> I find it confusing. We use "coverage" to mean any fractional pixel coverage
no
> matter how it is implemented (stencil, fragment shader calculation) and we
> always use it to "modulate" somewhere in the pipeline (glBlend or in the frag
> shader).
> 
> Could we use something a little more descriptive?
> 
> applyStencilCoverageModulation? modulateByStencilCoverageFraction?
> 

Done.

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLCaps.h
File src/gpu/gl/GrGLCaps.h (right):

https://codereview.chromium.org/1001503002/diff/160001/src/gpu/gl/GrGLCaps.h#...
src/gpu/gl/GrGLCaps.h:71: kStencil_MSFBOType,
On 2015/03/31 16:37:33, bsalomon wrote:
> Maybe we should call this something a little more descriptive.
> kMixedSamples_MSFBOType?

Done.

[Chris Dalton, 2015-04-07 23:48:24]

Looking good!

We still need to resolve the issue where an Xfer processor might use in-shader
blending with a mixed samples target. We may be able to determine that
information ahead of time, but I'm wondering if another approach might be to
make all the Xfer processors just do the right thing if
hasMixedSamplesModulation is true...

https://codereview.chromium.org/1001503002/diff/200001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/200001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:60: * @return true if the surface is multisampled
in the specified buffer(s),
Nit picking, but this is maybe ambiguous. How about:

"@return true if the surface is multisampled in every specified buffer"

https://codereview.chromium.org/1001503002/diff/200001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:74: *         is not multisampled in the specified
buffer(s)
Here too:

"@return the number of samples-per-pixel, or zero if any of the specified
buffers is not multisampled."

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:233:
renderTarget->isMultisampled(GrRenderTarget::BufferBits::kStencil_BufferBit)) {
We shouldn't need a scope resultion here ("BufferBits::").
"GrRenderTarget::kStencil_BufferBit" should be fine. There are a couple other
places that use this as well.

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

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:440: GrRenderTarget::kStencil_SampleConfig :
GrRenderTarget::kUnified_SampleConfig;
Looks like we broke 100 columns...

[Mark Kilgard, 2015-04-08 03:20:09]

LGTM with a few comments (see below)

great to see this come together

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:999: "Stencil",
Stencil doesn't seem sufficiently descriptive

would "MixedSamples" be a more descriptive string here?

that's more consistent with kMixedSamples_MSFBOType

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

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2112: GL_CALL(Enable(GR_GL_MULTISAMPLE));
warning: enabling/disabling GL_MULTISAMPLE is not officially supported in OpenGL
ES 2.x/3.x

NVIDIA has an accidental allowance (technically a bug) where the Tegra K1 & X1
ES drivers (and desktop ES contexts too) allow GL_MULTISAMPLE to be
enabled/disabled.

We are working to standardize an extension to be called
EXT_multisample_compatibility that will, when advertised, allow GL_MULTISAMPLE
(and GL_SAMPLE_ALPHA_TO_ONE) to be enabled/disabled.

Intel is a likely partner for the EXT.  I should send the specification to
Google's Skia team for their consideration too.  Any OpenGL 1.3 capable hardware
(though not ES-only hardware) should be able to support
EXT_multisample_compatibility.  It has many advantages for ensuring aliased
behavior and is useful with NV_framebuffer_mixed_samples in particular in
certain cases.

If the proposed EXT_multisample_compatibility extension is not advertised and
some NV-only extension such as NV_blend_equation_advanced (K1 or X1) or
NV_framebuffer_mixed_samples (K1 only) is supported, you can try to call
glEnable(GL_MULTISAMPLE).  If there is no GL_INVALID_ENUM error, assume
EXT_multisample_compatibility is de facto supported.

Note: the default GL_MULTISAMPLE state is enabled (and ES acts that way, just
with no way to do the disable).

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2124: if (kYes_TriState != fCoverageModulationEnabled) {
...technically this mode isn't an enable though Skia may use it that way

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.h#n...
src/gpu/gl/GrGLGpu.h:451: TriState fCoverageModulationEnabled;
is fCoverageModulationMode a better name for this?

technically NV_framebuffer_mixed_sample's isn't really an "enable" but rather a
mode.

While GL_RGBA is most typically used, GL_NONE (initial value), GL_ALPHA, and
GL_RGB are also supported.

I expect Skia's use of pre-multiplied alpha everywhere means GL_RGBA is the
interesting mode for Skia so if GL_NONE and GL_RGBA are the only options used,
that is kind of like an enable.

[Chris Dalton, 2015-04-08 04:55:26]

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:785: if
(ctxInfo.hasExtension("GL_NV_framebuffer_mixed_samples")) {
We may consider only using MixedSamples if the context also has nvpr, since
right now that's the only thing that actually uses it.

[vbuzinov, 2015-04-08 12:05:30]

Uploaded a new version plus please see some comments inline with the previous
version.

https://codereview.chromium.org/1001503002/diff/200001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/200001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:60: * @return true if the surface is multisampled
in the specified buffer(s),
On 2015/04/07 23:48:23, Chris Dalton wrote:
> Nit picking, but this is maybe ambiguous. How about:
> 
> "@return true if the surface is multisampled in every specified buffer"

Done.

https://codereview.chromium.org/1001503002/diff/200001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:74: *         is not multisampled in the specified
buffer(s)
On 2015/04/07 23:48:23, Chris Dalton wrote:
> Here too:
> 
> "@return the number of samples-per-pixel, or zero if any of the specified
> buffers is not multisampled."

Done.

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/GrContext.cpp#...
src/gpu/GrContext.cpp:233:
renderTarget->isMultisampled(GrRenderTarget::BufferBits::kStencil_BufferBit)) {
On 2015/04/07 23:48:23, Chris Dalton wrote:
> We shouldn't need a scope resultion here ("BufferBits::").
> "GrRenderTarget::kStencil_BufferBit" should be fine. There are a couple other
> places that use this as well.

Done.

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:785: if
(ctxInfo.hasExtension("GL_NV_framebuffer_mixed_samples")) {
On 2015/04/08 04:55:26, Chris Dalton wrote:
> We may consider only using MixedSamples if the context also has nvpr, since
> right now that's the only thing that actually uses it.

Done.  Should the caps property (fFBMixedSamplesSupport) be set to false in such
a case too?  So that GrGLGpu doesn't call glCoverageModulation for nothing?

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:999: "Stencil",
On 2015/04/08 03:20:09, Mark Kilgard wrote:
> Stencil doesn't seem sufficiently descriptive
> 
> would "MixedSamples" be a more descriptive string here?
> 
> that's more consistent with kMixedSamples_MSFBOType
> 

Done.

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

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:440: GrRenderTarget::kStencil_SampleConfig :
GrRenderTarget::kUnified_SampleConfig;
On 2015/04/07 23:48:23, Chris Dalton wrote:
> Looks like we broke 100 columns...

Done.

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2112: GL_CALL(Enable(GR_GL_MULTISAMPLE));
On 2015/04/08 03:20:09, Mark Kilgard wrote:
> warning: enabling/disabling GL_MULTISAMPLE is not officially supported in
OpenGL
> ES 2.x/3.x
> 
> NVIDIA has an accidental allowance (technically a bug) where the Tegra K1 & X1
> ES drivers (and desktop ES contexts too) allow GL_MULTISAMPLE to be
> enabled/disabled.
> 
> We are working to standardize an extension to be called
> EXT_multisample_compatibility that will, when advertised, allow GL_MULTISAMPLE
> (and GL_SAMPLE_ALPHA_TO_ONE) to be enabled/disabled.
> 
> Intel is a likely partner for the EXT.  I should send the specification to
> Google's Skia team for their consideration too.  Any OpenGL 1.3 capable
hardware
> (though not ES-only hardware) should be able to support
> EXT_multisample_compatibility.  It has many advantages for ensuring aliased
> behavior and is useful with NV_framebuffer_mixed_samples in particular in
> certain cases.
> 
> If the proposed EXT_multisample_compatibility extension is not advertised and
> some NV-only extension such as NV_blend_equation_advanced (K1 or X1) or
> NV_framebuffer_mixed_samples (K1 only) is supported, you can try to call
> glEnable(GL_MULTISAMPLE).  If there is no GL_INVALID_ENUM error, assume
> EXT_multisample_compatibility is de facto supported.
> 
> Note: the default GL_MULTISAMPLE state is enabled (and ES acts that way, just
> with no way to do the disable).  

Doesn't look like there was an explicit check for whether the context is GLES
here before our modification.  Of course, only with Nvfms multisampling will
actually be toggled.  We can add the ES and a caps check here when this
extension is available, but right now we actually depend on this "bug" to render
things properly...

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1001503002/diff/200001/src/gpu/gl/GrGLGpu.h#n...
src/gpu/gl/GrGLGpu.h:451: TriState fCoverageModulationEnabled;
On 2015/04/08 03:20:09, Mark Kilgard wrote:
> is fCoverageModulationMode a better name for this?
> 
> technically NV_framebuffer_mixed_sample's isn't really an "enable" but rather
a
> mode.
> 
> While GL_RGBA is most typically used, GL_NONE (initial value), GL_ALPHA, and
> GL_RGB are also supported.
> 
> I expect Skia's use of pre-multiplied alpha everywhere means GL_RGBA is the
> interesting mode for Skia so if GL_NONE and GL_RGBA are the only options used,
> that is kind of like an enable.

Since we only use GL_RGBA at this point I thought it would be easier to reuse
skia's TriState enum for it.  So it is either modulation, no modulation or
unknown.  Changing this to unknown or some value wouldn't be hard, though.

Come to think of it, Brian earlier mentioned that "coverage modulation" might
sound ambiguous with regards to skia's own notion of coverage.  Should I rename
the var to fMixedSamplesCoverageModulationEnabled or do you guys think that
would be unnecessary -- coverage doesn't come up in GrGLGpu otherwise?

[Mark Kilgard, 2015-04-09 22:09:30]

lgtm

[vbuzinov, 2015-04-10 12:29:47]

Patchset #14 (id:260001) has been deleted

[vbuzinov, 2015-04-17 09:29:44]

Patchset #16 (id:320001) has been deleted

[Chris Dalton, 2015-04-17 22:05:12]

https://codereview.chromium.org/1001503002/diff/340001/src/gpu/GrStencilAndCo...
File src/gpu/GrStencilAndCoverPathRenderer.cpp (right):

https://codereview.chromium.org/1001503002/diff/340001/src/gpu/GrStencilAndCo...
src/gpu/GrStencilAndCoverPathRenderer.cpp:186:
pipelineBuilder->enableState(GrPipelineBuilder::kHWAntialias_StateBit);
Sorry, I think I may have misled you about where to put this. We need to enable
kHWAntialias_StateBit *before* this function calls target->drawPath(). Can you
move this 'if' block to the top of the function?

[bsalomon, 2015-05-11 18:29:21]

So thinking about this more, I don't see a good reason to gate this on a overall
refactoring the render targets. Landing it won't make that task significantly
more complicated.

It seems like there are complexities in GrRenderTarget.h around sample count
queries that could be removed if we just made all the callers of numSamples and
isMultisampled be explicit about whether they are asking about color or stencil
samples. Would something like "bool colorSampleCount()" and "bool
stencilSampleCount()" work?  Maybe a helper like "isNonmixedMultisampled()" or
something to that effect? These might not be exactly the right queries, but my
general feedback is I'd rather update the call sites for msaa queries than try
to retrofit the new queries with default args that make the old code work.

[Chris Dalton, 2015-06-09 17:39:52]

Just a few comments. Looking good! I think the numColorSamples/numStencilSamples
part is a positive change. It's also nice to be able to just always use it now
and not worry about the blend mode.

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/GrStencilAndCo...
File src/gpu/GrStencilAndCoverTextContext.cpp (right):

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/GrStencilAndCo...
src/gpu/GrStencilAndCoverTextContext.cpp:376:
SkASSERT(fRenderTarget->isStencilBufferMultisampled());
This is really minor, but it might be more puristic to assert something like
this:

  SkASSERT(fRenderTarget->isStencilBufferMultisampled() ||
!fPaint.isAntiAlias());

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:705: fMSFBOType = kMixedSamples_MSFBOType;
So when I made the mixed samples support depend on dual source blending it broke
this part of your patch. Here we ought to only select kMixedSamples_MSFBOType if
the shader caps have mixed samples:

 fShaderCaps->mixedSamplesSupport()

There is one minor problem though where the shader caps don't get initialized
until after this function. I'll try and open some discussion today about how we
can handle this.

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:718: fMSFBOType = kMixedSamples_MSFBOType;
Ditto

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLCaps.h
File src/gpu/gl/GrGLCaps.h (left):

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLCaps.h#...
src/gpu/gl/GrGLCaps.h:352: bool fFBMixedSamplesSupport : 1;
Oops, thanks for catching that. I shouldn't have missed it in
https://codereview.chromium.org/1151793002

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

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:376: fCoverageModulationEnabled = kUnknown_TriState;
At this point we can just call:

 glCoverageModulationNV(GL_RGBA);

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:508: idDesc.fSampleConfig = (wrapDesc.fFlags &
kStencilMSAAOnly_GrBackendRenderTargetFlags) ?
Here we ought to ignore the multisample stencil buffer if we don't have full
mixed samples support. (e.g. somebody gives us a multisample stencil but we
don't have dual source blending, so we opt to not ever use it.)

 ((wrapDesc.fFlags & kStencilMSAAOnly_GrBackendRenderTargetFlags) &&
  this->caps()->shaderCaps()->mixedSamplesSupport())

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2104: this->glCaps().shaderCaps()->mixedSamplesSupport())
{
I would recommend rolling this into the multisampleDisableSupport cap, where
that cap is always true when we have mixed samples.

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1001503002/diff/480001/src/gpu/gl/GrGLGpu.h#n...
src/gpu/gl/GrGLGpu.h:483: TriState fCoverageModulationEnabled;
After discussions with the hardware guys, we can just leave coverage modulation
always enabled rather than tracking its state. It has no effect when not using
mixed samples.

[Chris Dalton, 2015-06-09 17:40:48]

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

[vbuzinov, 2015-06-10 11:23:27]

Addressed Chris's comments, currently there is still a problem with
uninitialized fShaderCaps.

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:703: } else if (fShaderCaps->mixedSamplesSupport() &&
fShaderCaps->pathRenderingSupport()) {
Currently crashes here due to uninitialized fShaderCaps

[Chris Dalton, 2015-06-10 19:52:39]

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLCaps.h
File src/gpu/gl/GrGLCaps.h (right):

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLCaps.h#...
src/gpu/gl/GrGLCaps.h:232: return fMultisampleDisableSupport ||
fShaderCaps->mixedSamplesSupport();
I guess I was kind of hoping we could roll the mixed samples check into the
fMultisampleDisableSupport bit itself. Then we don't need to do any extra logic
when checking it.

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

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:376: GL_CALL(CoverageModulation(GR_GL_RGBA));
We need to wrap this in a check for mixedSamplesSupport().

A comment about what it does and why it's ok to call it here seems fitting as
well.

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:511: GrRenderTarget::kUnified_SampleConfig;
If we block the mixed samples due to it not being supported, then we need to set
the sample count to zero as well. Otherwise it will render as if both the color
and stencil buffers are multisampled.

[bsalomon, 2015-06-10 19:57:07]

https://codereview.chromium.org/1001503002/diff/500001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/500001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:49: * @return true if the surface is multisampled
in all buffers,
Can we come up with a more clear name for this?

isUnifiedMultisampled()?

https://codereview.chromium.org/1001503002/diff/500001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/1001503002/diff/500001/include/gpu/GrTypes.h#...
include/gpu/GrTypes.h:535: enum GrBackendRenderTargetFlags {
I'm wondering if it is necessary to extend wrapped render targets to support
mixed samples.

Wrapped RTs are generally less useful than wrapped textures because they can
never be as a source for a draw. It really only exists for wrapping FBO 0. Our
clients typically don't use this but instead have Skia create a texture to
render to or they provide Skia a texture ID and we create the FBO and stencil
buffers to pair with it.

Is it possible for FBO 0 to be mixed?

If there is a client for this, could we have separate stencil/color sample
counts in the descriptor and simply fail if the combo is illegal?

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:703: } else if (fShaderCaps->mixedSamplesSupport() &&
fShaderCaps->pathRenderingSupport()) {
On 2015/06/10 11:23:27, vbuzinov wrote:
> Currently crashes here due to uninitialized fShaderCaps

Is it possible to just make the shader caps be initialized first?


At a higher level, is it wrong to enable kMixedSamples_MSFBOType without path
rendering?

[Chris Dalton, 2015-06-10 20:55:57]

https://codereview.chromium.org/1001503002/diff/500001/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/1001503002/diff/500001/include/gpu/GrTypes.h#...
include/gpu/GrTypes.h:535: enum GrBackendRenderTargetFlags {
On 2015/06/10 19:57:06, bsalomon wrote:
> I'm wondering if it is necessary to extend wrapped render targets to support
> mixed samples.
> 
> Wrapped RTs are generally less useful than wrapped textures because they can
> never be as a source for a draw. It really only exists for wrapping FBO 0. Our
> clients typically don't use this but instead have Skia create a texture to
> render to or they provide Skia a texture ID and we create the FBO and stencil
> buffers to pair with it.
> 
> Is it possible for FBO 0 to be mixed?
> 
> If there is a client for this, could we have separate stencil/color sample
> counts in the descriptor and simply fail if the combo is illegal?
> 
> 

Mixed samples is not supported for FBO 0 so we can just take this out. It just
won't be possible to wrap a mixed render target, which is fine.

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:703: } else if (fShaderCaps->mixedSamplesSupport() &&
fShaderCaps->pathRenderingSupport()) {
On 2015/06/10 19:57:07, bsalomon wrote:
> On 2015/06/10 11:23:27, vbuzinov wrote:
> > Currently crashes here due to uninitialized fShaderCaps
> 
> Is it possible to just make the shader caps be initialized first?
> 
> 
> At a higher level, is it wrong to enable kMixedSamples_MSFBOType without path
> rendering?

It's not wrong, just not very useful without path rendering since it's the only
thing that even knows how to use mixed samples at the moment.

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

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:511: GrRenderTarget::kUnified_SampleConfig;
On 2015/06/10 19:52:39, Chris Dalton wrote:
> If we block the mixed samples due to it not being supported, then we need to
set
> the sample count to zero as well. Otherwise it will render as if both the
color
> and stencil buffers are multisampled.

Looks like we're taking out kStencilMSAAOnly_GrBackendRenderTargetFlags so we
can ignore that last comment. Mixed samples just won't be supported with backend
RTs.

[Chris Dalton, 2015-06-11 00:02:29]

https://codereview.chromium.org/1001503002/diff/500001/include/gpu/GrRenderTa...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1001503002/diff/500001/include/gpu/GrRenderTa...
include/gpu/GrRenderTarget.h:91: return fSampleConfig;
Maybe change this to:

  bool hasMixedSamples() const {
      SkASSERT(kStencil_SampleConfig != fSampleConfig ||
this->isStencilBufferMultisampled());
      return kStencil_SampleConfig == fSampleConfig;
  }

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

https://codereview.chromium.org/1001503002/diff/500001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1619: case GrGLCaps::kInvalidate_InvalidateFBType:
Don't forget the workaround.

  if (renderTarget->hasMixedSamples() &&
      kNVIDIA_GrGLDriver == this->glContext().driver()) {
      break; // NVIDIA will crash calling glInvalidateFramebuffer with mixed
samples.
  }

[vbuzinov, 2015-06-11 12:30:57]

Addressed the comments, see also the comment below regarding
glInvalidateFramebuffer workaround.  Looks like we can still get a hang
invalidating a non-mixed-sampled FBO on a scene that also has mixed-sampled
FBOs.

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

https://codereview.chromium.org/1001503002/diff/540001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1622: this->glCaps().shaderCaps()->mixedSamplesSupport())
{
The nature of glInvalidateFramebuffer bug seems to be more convoluted.  Looks
like we are getting a hang when invalidating a non-mixed-sampled FBO on
amazon.skp that is used to compute the blur effect and has a mixed-sampled FBO
as a source.  Using a broader condition here until it gets resolved.

[bsalomon, 2015-06-11 13:33:14]

small request inline, otherwise lgtm

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

https://codereview.chromium.org/1001503002/diff/540001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1622: this->glCaps().shaderCaps()->mixedSamplesSupport())
{
On 2015/06/11 12:30:57, vbuzinov wrote:
> The nature of glInvalidateFramebuffer bug seems to be more convoluted.  Looks
> like we are getting a hang when invalidating a non-mixed-sampled FBO on
> amazon.skp that is used to compute the blur effect and has a mixed-sampled FBO
> as a source.  Using a broader condition here until it gets resolved.

Can we just disable support for framebuffer invalidation when setting up the
caps (until the issue is resolved)? That way if we were to add another code path
that uses the functionality it'd inherit the workaround.

[Chris Dalton, 2015-06-11 23:34:38]

Two small requests from me too

https://codereview.chromium.org/1001503002/diff/540001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1001503002/diff/540001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:240: if (kGL_GrGLStandard == standard ||
fShaderCaps->mixedSamplesSupport()) {
This won't quite work. See below how mixed samples support isn't initialized
until after this line. (In fact, it checks fMultisampleDisableSupport when
deciding if we can use mixed samples :) My bad for suggesting we do it this way.

I think we should take out the mixedSamplesSupport check entirely. The reason
being, ES is already locked out from using mixed samples because it dosen't have
dual source blending. NVIDIA is actively working on extensions that allow both
disabling multisample and dual source blend on ES, so we can wait until those
specs are published.

In other words, leave this line as:

 if (kGL_GrGLStandard == standard) {

In the future, once the specs are finished, we can change the ES branch to:

 fMultisampleDisableSupport =
ctxInfo.hasExtension("GL_EXT_multisample_compatibility");

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

https://codereview.chromium.org/1001503002/diff/540001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:379: // Enable coverage modulation suitable for
premultiplied alpha colors.
// This state has no effect when not rendering to a mixed sampled target.

[vbuzinov, 2015-06-12 06:00:01]

Added disabling of fDiscardRenderTargetSupport for NV + fms and addressed
Chris's comments.

[Chris Dalton, 2015-06-12 14:31:17]

Patch 27 lgtm

[bsalomon, 2015-06-12 14:35:03]

On 2015/06/12 14:31:17, Chris Dalton wrote:
> Patch 27 lgtm

lgtm too

[bsalomon, 2015-06-12 14:39:50]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-06-12 14:39:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com
Link to the patchset: https://codereview.chromium.org/1001503002/#ps560001
(title: "Disable fDiscardRenderTargetSupport for NV with fms")

[commit-bot: I haz the power, 2015-06-12 14:40:05]

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

[commit-bot: I haz the power, 2015-06-12 14:41:47]

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

[commit-bot: I haz the power, 2015-06-12 14:41:49]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[vbuzinov, 2015-06-12 15:38:51]

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

[vbuzinov, 2015-06-12 15:38:51]

The patchset sent to the CQ was uploaded after l-g-t-m from cdalton@nvidia.com,
bsalomon@google.com, markkilgard@gmail.com
Link to the patchset: https://codereview.chromium.org/1001503002/#ps580001
(title: "Fix build error related to isMultisamped renaming")

[commit-bot: I haz the power, 2015-06-12 15:39:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001503002/580001

[commit-bot: I haz the power, 2015-06-12 15:55:28]

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

[commit-bot: I haz the power, 2015-06-12 15:55:30]

Dry run: This issue passed the CQ dry run.

[vbuzinov, 2015-06-12 15:58:57]

The CQ bit was checked by vbuzinov@nvidia.com

[commit-bot: I haz the power, 2015-06-12 15:59:07]

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

[commit-bot: I haz the power, 2015-06-12 15:59:50]

Committed patchset #28 (id:580001) as
https://skia.googlesource.com/skia/+/dded69693dd3779f081326cde24c3954505b129d