Skip to content

Commit

Permalink
Revert "Add MSAA and non-aa modes to GrFillRRect Op"
Browse files Browse the repository at this point in the history 
This reverts commit 16a8e99.

Reason for revert: Amd/Radeon bugs and quality concerns

Original change's description:
> Add MSAA and non-aa modes to GrFillRRect Op
> 
> Adds a non-aa mode and an MSAA mode that uses the sample mask. Also
> adds a new cap to decide whether we prefer this new sample mask Op for
> large round rects, or whether it's faster to just continue drawing
> them as paths like before.
> 
> Bug: skia:
> Change-Id: Iea7d9a78766f67c196a02cb833c84a0ac3f1bbac
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/202921
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Chris Dalton <csmartdalton@google.com>

TBR=bsalomon@google.com,brianosman@google.com,csmartdalton@google.com,ethannicholas@google.com

Change-Id: If2c3293bdd8c19307e243d83c44fd328446fded7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/203961
Reviewed-by: Chris Dalton <csmartdalton@google.com>
Commit-Queue: Chris Dalton <csmartdalton@google.com>
  • Loading branch information
@csmartdalton86
csmartdalton86 authored and Skia Commit-Bot committed Mar 27, 2019
1 parent 2b7cca4 commit bf341ae
Show file tree
Hide file tree
Showing 10 changed files with 171 additions and 540 deletions.
2 changes: 1 addition & 1 deletion gm/samplelocations.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class SampleLocationsTestProcessor::Impl : public GrGLSLGeometryProcessor {
f->sampleOffsets(), coord.fsIn());
f->codeAppendf( "if (all(lessThanEqual(abs(samplecoord), float2(1)))) {");
f->maskOffMultisampleCoverage(
"~(1 << i)", GrGLSLFPFragmentBuilder::ScopeFlags::kInsideLoop);
"~(1 << i)", GrGLSLFragmentShaderBuilder::Scope::kInsideLoopOrBranch);
f->codeAppendf( "}");
f->codeAppendf("}");
}
Expand Down
4 changes: 0 additions & 4 deletions src/gpu/GrCaps.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ GrCaps::GrCaps(const GrContextOptions& options) {

fPreferVRAMUseOverFlushes = true;

fPreferTrianglesOverSampleMask = false;

// Default to true, allow older versions of OpenGL to disable explicitly
fClampToBorderSupport = true;

Expand Down Expand Up @@ -212,8 +210,6 @@ void GrCaps::dumpJSON(SkJSONWriter* writer) const {
writer->appendBool("Blacklist Coverage Counting Path Renderer [workaround]",
fBlacklistCoverageCounting);
writer->appendBool("Prefer VRAM Use over flushes [workaround]", fPreferVRAMUseOverFlushes);
writer->appendBool("Prefer more triangles over sample mask [MSAA only]",
fPreferTrianglesOverSampleMask);
writer->appendBool("Avoid stencil buffers [workaround]", fAvoidStencilBuffers);

if (this->advancedBlendEquationSupport()) {
Expand Down
5 changes: 0 additions & 5 deletions src/gpu/GrCaps.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ class GrCaps : public SkRefCnt {

bool preferVRAMUseOverFlushes() const { return fPreferVRAMUseOverFlushes; }

bool preferTrianglesOverSampleMask() const { return fPreferTrianglesOverSampleMask; }

bool blacklistCoverageCounting() const { return fBlacklistCoverageCounting; }

bool avoidStencilBuffers() const { return fAvoidStencilBuffers; }
Expand Down Expand Up @@ -360,9 +358,6 @@ class GrCaps : public SkRefCnt {
// ANGLE performance workaround
bool fPreferVRAMUseOverFlushes : 1;

// On some platforms it's better to make more triangles than to use the sample mask (MSAA only).
bool fPreferTrianglesOverSampleMask : 1;

// TODO: this may need to be an enum to support different fence types
bool fFenceSyncSupport : 1;

Expand Down
61 changes: 29 additions & 32 deletions src/gpu/GrRenderTargetContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1225,22 +1225,21 @@ void GrRenderTargetContext::drawRRect(const GrClip& origClip,
AutoCheckFlush acf(this->drawingManager());

GrAAType aaType = this->chooseAAType(aa);
if (GrAAType::kCoverage == aaType) {
std::unique_ptr<GrDrawOp> op;
if (style.isSimpleFill()) {
op = GrFillRRectOp::Make(fContext, viewMatrix, rrect, *this->caps(), std::move(paint));
}
if (!op) {
assert_alive(paint);
op = GrOvalOpFactory::MakeRRectOp(fContext, std::move(paint), viewMatrix, rrect, stroke,
this->caps()->shaderCaps());
}

std::unique_ptr<GrDrawOp> op;
if (style.isSimpleFill()) {
assert_alive(paint);
op = GrFillRRectOp::Make(
fContext, aaType, viewMatrix, rrect, *this->caps(), std::move(paint));
}
if (!op && GrAAType::kCoverage == aaType) {
assert_alive(paint);
op = GrOvalOpFactory::MakeRRectOp(
fContext, std::move(paint), viewMatrix, rrect, stroke, this->caps()->shaderCaps());

}
if (op) {
this->addDrawOp(*clip, std::move(op));
return;
if (op) {
this->addDrawOp(*clip, std::move(op));
return;
}
}

assert_alive(paint);
Expand Down Expand Up @@ -1631,31 +1630,29 @@ void GrRenderTargetContext::drawOval(const GrClip& clip,
AutoCheckFlush acf(this->drawingManager());

GrAAType aaType = this->chooseAAType(aa);

std::unique_ptr<GrDrawOp> op;
if (style.isSimpleFill()) {
if (GrAAType::kCoverage == aaType) {
std::unique_ptr<GrDrawOp> op;
// GrFillRRectOp has special geometry and a fragment-shader branch to conditionally evaluate
// the arc equation. This same special geometry and fragment branch also turn out to be a
// substantial optimization for drawing ovals (namely, by not evaluating the arc equation
// inside the oval's inner diamond). Given these optimizations, it's a clear win to draw
// ovals the exact same way we do round rects.
//
// However, we still don't draw true circles as round rects in coverage mode, because it can
// cause perf regressions on some platforms as compared to the dedicated circle Op.
if (GrAAType::kCoverage != aaType || oval.height() != oval.width()) {
// However, we still don't draw true circles as round rects, because it can cause perf
// regressions on some platforms as compared to the dedicated circle Op.
if (style.isSimpleFill() && oval.height() != oval.width()) {
op = GrFillRRectOp::Make(
fContext, viewMatrix, SkRRect::MakeOval(oval), *this->caps(), std::move(paint));
}
if (!op) {
assert_alive(paint);
op = GrFillRRectOp::Make(fContext, aaType, viewMatrix, SkRRect::MakeOval(oval),
*this->caps(), std::move(paint));
op = GrOvalOpFactory::MakeOvalOp(fContext, std::move(paint), viewMatrix, oval, style,
this->caps()->shaderCaps());
}
if (op) {
this->addDrawOp(clip, std::move(op));
return;
}
}
if (!op && GrAAType::kCoverage == aaType) {
assert_alive(paint);
op = GrOvalOpFactory::MakeOvalOp(fContext, std::move(paint), viewMatrix, oval, style,
this->caps()->shaderCaps());
}
if (op) {
this->addDrawOp(clip, std::move(op));
return;
}

assert_alive(paint);
Expand Down
6 changes: 0 additions & 6 deletions src/gpu/gl/GrGLCaps.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -526,12 +526,6 @@ void GrGLCaps::init(const GrContextOptions& contextOptions,
fPreferVRAMUseOverFlushes = !isANGLE;
#endif

if (kARM_GrGLVendor == ctxInfo.vendor()) {
// ARM seems to do better with larger quantities of fine triangles, as opposed to using the
// sample mask. (At least in our current round rect op.)
fPreferTrianglesOverSampleMask = true;
}

if (kChromium_GrGLDriver == ctxInfo.driver()) {
fMustClearUploadedBufferData = true;
}
Expand Down
63 changes: 10 additions & 53 deletions src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ const char* GrGLSLFragmentShaderBuilder::sampleOffsets() {
return "_sampleOffsets";
}

void GrGLSLFragmentShaderBuilder::maskOffMultisampleCoverage(
const char* mask, ScopeFlags scopeFlags) {
void GrGLSLFragmentShaderBuilder::maskOffMultisampleCoverage(const char* mask, Scope scope) {
const GrShaderCaps& shaderCaps = *fProgramBuilder->shaderCaps();
if (!shaderCaps.sampleVariablesSupport()) {
SkDEBUGFAIL("Attempted to mask sample coverage without support.");
Expand All @@ -102,58 +101,16 @@ void GrGLSLFragmentShaderBuilder::maskOffMultisampleCoverage(
this->addFeature(1 << kSampleVariables_GLSLPrivateFeature, extension);
}

if (!fHasModifiedSampleMask) {
fHasModifiedSampleMask = true;
if (ScopeFlags::kTopLevel != scopeFlags) {
this->codePrependf("gl_SampleMask[0] = ~0;");
}
if (!(ScopeFlags::kInsideLoop & scopeFlags)) {
this->codeAppendf("gl_SampleMask[0] = (%s);", mask);
return;
}
if (!fHasInitializedSampleMask && Scope::kTopLevel == scope) {
this->codeAppendf("gl_SampleMask[0] = (%s);", mask);
fHasInitializedSampleMask = true;
return;
}

this->codeAppendf("gl_SampleMask[0] &= (%s);", mask);
}

void GrGLSLFragmentShaderBuilder::applyFnToMultisampleMask(
const char* fn, const char* grad, ScopeFlags scopeFlags) {
SkASSERT(CustomFeatures::kSampleLocations & fProgramBuilder->header().processorFeatures());
SkDEBUGCODE(fUsedProcessorFeaturesThisStage_DebugOnly |= CustomFeatures::kSampleLocations);
SkDEBUGCODE(fUsedProcessorFeaturesAllStages_DebugOnly |= CustomFeatures::kSampleLocations);

int sampleCnt = fProgramBuilder->effectiveSampleCnt();
SkASSERT(sampleCnt > 1);

this->codeAppendf("{");

if (!grad) {
SkASSERT(fProgramBuilder->shaderCaps()->shaderDerivativeSupport());
// In order to use HW derivatives, our neighbors within the same primitive must also be
// executing the same code. A per-pixel branch makes this pre-condition impossible to
// fulfill.
SkASSERT(!(ScopeFlags::kInsidePerPixelBranch & scopeFlags));
this->codeAppendf("float2 grad = float2(dFdx(fn), dFdy(fn));");
this->codeAppendf("float fnwidth = fwidth(fn);");
grad = "grad";
} else {
this->codeAppendf("float fnwidth = abs(%s.x) + abs(%s.y);", grad, grad);
if (!fHasInitializedSampleMask) {
this->codePrependf("gl_SampleMask[0] = ~0;");
fHasInitializedSampleMask = true;
}

this->codeAppendf("int mask = 0;");
this->codeAppendf("if (%s*2 < fnwidth) {", fn); // Are ANY samples inside the implicit fn?
this->codeAppendf( "if (%s*-2 >= fnwidth) {", fn); // Are ALL samples inside the implicit?
this->codeAppendf( "mask = ~0;");
this->codeAppendf( "} else for (int i = 0; i < %i; ++i) {", sampleCnt);
this->codeAppendf( "float fnsample = dot(%s, _sampleOffsets[i]) + %s;", grad, fn);
this->codeAppendf( "if (fnsample < 0) {");
this->codeAppendf( "mask |= (1 << i);");
this->codeAppendf( "}");
this->codeAppendf( "}");
this->codeAppendf("}");
this->maskOffMultisampleCoverage("mask", scopeFlags);

this->codeAppendf("}");
this->codeAppendf("gl_SampleMask[0] &= (%s);", mask);
}

const char* GrGLSLFragmentShaderBuilder::dstColor() {
Expand Down Expand Up @@ -260,10 +217,10 @@ void GrGLSLFragmentShaderBuilder::onFinalize() {
== fUsedProcessorFeaturesAllStages_DebugOnly);

if (CustomFeatures::kSampleLocations & fProgramBuilder->header().processorFeatures()) {
this->definitions().append("const float2 _sampleOffsets[] = float2[](");
const GrPipeline& pipeline = fProgramBuilder->pipeline();
const SkTArray<SkPoint>& sampleLocations =
fProgramBuilder->renderTarget()->renderTargetPriv().getSampleLocations(pipeline);
this->definitions().append("const float2 _sampleOffsets[] = float2[](");
for (int i = 0; i < sampleLocations.count(); ++i) {
SkPoint offset = sampleLocations[i] - SkPoint::Make(.5f, .5f);
if (kBottomLeft_GrSurfaceOrigin == this->getSurfaceOrigin()) {
Expand Down
35 changes: 6 additions & 29 deletions src/gpu/glsl/GrGLSLFragmentShaderBuilder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,9 @@ class GrGLSLFPFragmentBuilder : virtual public GrGLSLFragmentBuilder {
*/
virtual const char* sampleOffsets() = 0;

enum class ScopeFlags {
// Every fragment will always execute this code, and will do it exactly once.
kTopLevel = 0,
// Either all fragments in a given primitive, or none, will execute this code.
kInsidePerPrimitiveBranch = (1 << 0),
// Any given fragment may or may not execute this code.
kInsidePerPixelBranch = (1 << 1),
// This code will be executed more than once.
kInsideLoop = (1 << 2)
enum class Scope : bool {
kTopLevel,
kInsideLoopOrBranch
};

/**
Expand All @@ -74,21 +68,7 @@ class GrGLSLFPFragmentBuilder : virtual public GrGLSLFragmentBuilder {
*
* Requires MSAA and GLSL support for sample variables.
*/
virtual void maskOffMultisampleCoverage(const char* mask, ScopeFlags) = 0;

/**
* Turns off coverage at each sample where the implicit function fn > 0.
*
* The provided "fn" value represents the implicit function at pixel center. We then approximate
* the implicit at each sample by riding the gradient, "grad", linearly from pixel center to
* each sample location.
*
* If "grad" is null, we approximate the gradient using HW derivatives.
*
* Requires MSAA and GLSL support for sample variables. Also requires HW derivatives if not
* providing a gradient.
*/
virtual void applyFnToMultisampleMask(const char* fn, const char* grad, ScopeFlags) = 0;
virtual void maskOffMultisampleCoverage(const char* mask, Scope) = 0;

/**
* Fragment procs with child procs should call these functions before/after calling emitCode
Expand All @@ -102,8 +82,6 @@ class GrGLSLFPFragmentBuilder : virtual public GrGLSLFragmentBuilder {
virtual void forceHighPrecision() = 0;
};

GR_MAKE_BITFIELD_CLASS_OPS(GrGLSLFPFragmentBuilder::ScopeFlags);

/*
* This class is used by Xfer processors to build their fragment code.
*/
Expand Down Expand Up @@ -141,8 +119,7 @@ class GrGLSLFragmentShaderBuilder : public GrGLSLFPFragmentBuilder, public GrGLS

// GrGLSLFPFragmentBuilder interface.
const char* sampleOffsets() override;
void maskOffMultisampleCoverage(const char* mask, ScopeFlags) override;
void applyFnToMultisampleMask(const char* fn, const char* grad, ScopeFlags) override;
void maskOffMultisampleCoverage(const char* mask, Scope) override;
const SkString& getMangleString() const override { return fMangleString; }
void onBeforeChildProcEmitCode() override;
void onAfterChildProcEmitCode() override;
Expand Down Expand Up @@ -210,7 +187,7 @@ class GrGLSLFragmentShaderBuilder : public GrGLSLFPFragmentBuilder, public GrGLS
bool fHasCustomColorOutput = false;
int fCustomColorOutputIndex = -1;
bool fHasSecondaryOutput = false;
bool fHasModifiedSampleMask = false;
bool fHasInitializedSampleMask = false;
bool fForceHighPrecision = false;

friend class GrGLSLProgramBuilder;
Expand Down
Loading

0 comments on commit bf341ae

Please sign in to comment.