Revert "Implement caching of filled paths in the tessellated path renderer."
This reverts commit 468dfa72eb6694145487be17876804dfca3b7adb.
This CL caused signficant GM diffs.
Revert "Fix resource cache test."
This reverts commit b001c41ed37d2018d38ce787b412ed741585d75d.
Revert "Fix SkData leaks at GrResourceKey::setCustomData() call sites."
This reverts commit c369348aa596d7be05c9ce0ca5d349e5d1903789.
These CLs depend on the first one.
BUG=skia:
TBR=bsalomon@google.com
Review URL: https://codereview.chromium.org/1260363007
diff --git a/include/core/SkPathRef.h b/include/core/SkPathRef.h
index c09f6e8..e7cc31c 100644
--- a/include/core/SkPathRef.h
+++ b/include/core/SkPathRef.h
@@ -180,7 +180,19 @@
*/
static void Rewind(SkAutoTUnref<SkPathRef>* pathRef);
- virtual ~SkPathRef();
+ virtual ~SkPathRef() {
+ SkDEBUGCODE(this->validate();)
+ sk_free(fPoints);
+
+ SkDEBUGCODE(fPoints = NULL;)
+ SkDEBUGCODE(fVerbs = NULL;)
+ SkDEBUGCODE(fVerbCnt = 0x9999999;)
+ SkDEBUGCODE(fPointCnt = 0xAAAAAAA;)
+ SkDEBUGCODE(fPointCnt = 0xBBBBBBB;)
+ SkDEBUGCODE(fGenerationID = 0xEEEEEEEE;)
+ SkDEBUGCODE(fEditorsAttached = 0x7777777;)
+ }
+
int countPoints() const { SkDEBUGCODE(this->validate();) return fPointCnt; }
int countVerbs() const { SkDEBUGCODE(this->validate();) return fVerbCnt; }
int countWeights() const { SkDEBUGCODE(this->validate();) return fConicWeights.count(); }
@@ -239,13 +251,6 @@
*/
uint32_t genID() const;
- struct GenIDChangeListener {
- virtual ~GenIDChangeListener() {}
- virtual void onChange() = 0;
- };
-
- void addGenIDChangeListener(GenIDChangeListener* listener);
-
SkDEBUGCODE(void validate() const;)
private:
@@ -417,8 +422,6 @@
return fPoints;
}
- void callGenIDChangeListeners();
-
enum {
kMinSize = 256,
};
@@ -443,8 +446,6 @@
mutable uint32_t fGenerationID;
SkDEBUGCODE(int32_t fEditorsAttached;) // assert that only one editor in use at any time.
- SkTDArray<GenIDChangeListener*> fGenIDChangeListeners; // pointers are owned
-
friend class PathRefTest_Private;
typedef SkRefCnt INHERITED;
};
diff --git a/include/gpu/GrResourceKey.h b/include/gpu/GrResourceKey.h
index 0a97c20..a353dc2 100644
--- a/include/gpu/GrResourceKey.h
+++ b/include/gpu/GrResourceKey.h
@@ -10,7 +10,6 @@
#define GrResourceKey_DEFINED
#include "GrTypes.h"
-#include "SkData.h"
#include "SkOnce.h"
#include "SkTemplates.h"
@@ -238,7 +237,6 @@
GrUniqueKey& operator=(const GrUniqueKey& that) {
this->INHERITED::operator=(that);
- this->setCustomData(that.getCustomData());
return *this;
}
@@ -247,14 +245,6 @@
}
bool operator!=(const GrUniqueKey& that) const { return !(*this == that); }
- void setCustomData(const SkData* data) {
- SkSafeRef(data);
- fData.reset(data);
- }
- const SkData* getCustomData() const {
- return fData.get();
- }
-
class Builder : public INHERITED::Builder {
public:
Builder(GrUniqueKey* key, Domain domain, int data32Count)
@@ -278,9 +268,6 @@
return SkToInt((innerKey.dataSize() >> 2) + 1);
}
};
-
-private:
- SkAutoTUnref<const SkData> fData;
};
/**
diff --git a/src/core/SkPathPriv.h b/src/core/SkPathPriv.h
index 51a1a80..934c730 100644
--- a/src/core/SkPathPriv.h
+++ b/src/core/SkPathPriv.h
@@ -59,10 +59,6 @@
int count = path.countVerbs();
return count >= 1 && path.fPathRef->verbs()[~(count - 1)] == SkPath::Verb::kClose_Verb;
}
-
- static void AddGenIDChangeListener(const SkPath& path, SkPathRef::GenIDChangeListener* listener) {
- path.fPathRef->addGenIDChangeListener(listener);
- }
};
#endif
diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index 38b4e71..44838f9 100644
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -23,27 +23,12 @@
pathRef->reset(copy);
}
fPathRef = *pathRef;
- fPathRef->callGenIDChangeListeners();
fPathRef->fGenerationID = 0;
SkDEBUGCODE(sk_atomic_inc(&fPathRef->fEditorsAttached);)
}
//////////////////////////////////////////////////////////////////////////////
-SkPathRef::~SkPathRef() {
- this->callGenIDChangeListeners();
- SkDEBUGCODE(this->validate();)
- sk_free(fPoints);
-
- SkDEBUGCODE(fPoints = NULL;)
- SkDEBUGCODE(fVerbs = NULL;)
- SkDEBUGCODE(fVerbCnt = 0x9999999;)
- SkDEBUGCODE(fPointCnt = 0xAAAAAAA;)
- SkDEBUGCODE(fPointCnt = 0xBBBBBBB;)
- SkDEBUGCODE(fGenerationID = 0xEEEEEEEE;)
- SkDEBUGCODE(fEditorsAttached = 0x7777777;)
-}
-
// As a template argument, this must have external linkage.
SkPathRef* sk_create_empty_pathref() {
SkPathRef* empty = SkNEW(SkPathRef);
@@ -169,7 +154,6 @@
void SkPathRef::Rewind(SkAutoTUnref<SkPathRef>* pathRef) {
if ((*pathRef)->unique()) {
SkDEBUGCODE((*pathRef)->validate();)
- (*pathRef)->callGenIDChangeListeners();
(*pathRef)->fBoundsIsDirty = true; // this also invalidates fIsFinite
(*pathRef)->fVerbCnt = 0;
(*pathRef)->fPointCnt = 0;
@@ -231,6 +215,13 @@
SkASSERT(!genIDMatch);
return false;
}
+ // We've done the work to determine that these are equal. If either has a zero genID, copy
+ // the other's. If both are 0 then genID() will compute the next ID.
+ if (0 == fGenerationID) {
+ fGenerationID = ref.genID();
+ } else if (0 == ref.fGenerationID) {
+ ref.fGenerationID = this->genID();
+ }
return true;
}
@@ -278,6 +269,9 @@
memcpy(this->verbsMemWritable(), ref.verbsMemBegin(), ref.fVerbCnt * sizeof(uint8_t));
memcpy(this->fPoints, ref.fPoints, ref.fPointCnt * sizeof(SkPoint));
fConicWeights = ref.fConicWeights;
+ // We could call genID() here to force a real ID (instead of 0). However, if we're making
+ // a copy then presumably we intend to make a modification immediately afterwards.
+ fGenerationID = ref.fGenerationID;
fBoundsIsDirty = ref.fBoundsIsDirty;
if (!fBoundsIsDirty) {
fBounds = ref.fBounds;
@@ -442,24 +436,6 @@
return fGenerationID;
}
-void SkPathRef::addGenIDChangeListener(GenIDChangeListener* listener) {
- if (NULL == listener || this == empty.get()) {
- SkDELETE(listener);
- return;
- }
- *fGenIDChangeListeners.append() = listener;
-}
-
-// we need to be called *before* the genID gets changed or zerod
-void SkPathRef::callGenIDChangeListeners() {
- for (int i = 0; i < fGenIDChangeListeners.count(); i++) {
- fGenIDChangeListeners[i]->onChange();
- }
-
- // Listeners get at most one shot, so whether these triggered or not, blow them away.
- fGenIDChangeListeners.deleteAll();
-}
-
#ifdef SK_DEBUG
void SkPathRef::validate() const {
this->INHERITED::validate();
diff --git a/src/gpu/GrTessellatingPathRenderer.cpp b/src/gpu/GrTessellatingPathRenderer.cpp
index 62b8f0e..d0c7d3d 100644
--- a/src/gpu/GrTessellatingPathRenderer.cpp
+++ b/src/gpu/GrTessellatingPathRenderer.cpp
@@ -13,8 +13,6 @@
#include "GrDefaultGeoProcFactory.h"
#include "GrPathUtils.h"
#include "GrVertices.h"
-#include "GrResourceCache.h"
-#include "GrResourceProvider.h"
#include "SkChunkAlloc.h"
#include "SkGeometry.h"
@@ -540,13 +538,12 @@
// Stage 1: convert the input path to a set of linear contours (linked list of Vertices).
void path_to_contours(const SkPath& path, SkScalar tolerance, const SkRect& clipBounds,
- Vertex** contours, SkChunkAlloc& alloc, bool *isLinear) {
+ Vertex** contours, SkChunkAlloc& alloc) {
SkScalar toleranceSqd = tolerance * tolerance;
SkPoint pts[4];
bool done = false;
- *isLinear = true;
SkPath::Iter iter(path, false);
Vertex* prev = NULL;
Vertex* head = NULL;
@@ -574,7 +571,6 @@
toleranceSqd, prev, &head, pointsLeft, alloc);
quadPts += 2;
}
- *isLinear = false;
break;
}
case SkPath::kMove_Verb:
@@ -594,14 +590,12 @@
int pointsLeft = GrPathUtils::quadraticPointCount(pts, tolerance);
prev = generate_quadratic_points(pts[0], pts[1], pts[2], toleranceSqd, prev,
&head, pointsLeft, alloc);
- *isLinear = false;
break;
}
case SkPath::kCubic_Verb: {
int pointsLeft = GrPathUtils::cubicPointCount(pts, tolerance);
prev = generate_cubic_points(pts[0], pts[1], pts[2], pts[3],
toleranceSqd, prev, &head, pointsLeft, alloc);
- *isLinear = false;
break;
}
case SkPath::kClose_Verb:
@@ -1335,25 +1329,6 @@
return d;
}
-struct TessInfo {
- SkScalar fTolerance;
- int fCount;
-};
-
-bool cache_match(GrVertexBuffer* vertexBuffer, SkScalar tol, int* actualCount) {
- if (!vertexBuffer) {
- return false;
- }
- const SkData* data = vertexBuffer->getUniqueKey().getCustomData();
- SkASSERT(data);
- const TessInfo* info = static_cast<const TessInfo*>(data->data());
- if (info->fTolerance == 0 || info->fTolerance < 3.0f * tol) {
- *actualCount = info->fCount;
- return true;
- }
- return false;
-}
-
};
GrTessellatingPathRenderer::GrTessellatingPathRenderer() {
@@ -1367,22 +1342,6 @@
return GrPathRenderer::kNoSupport_StencilSupport;
}
-namespace {
-
-// When the SkPathRef genID changes, invalidate a corresponding GrResource described by key.
-class PathInvalidator : public SkPathRef::GenIDChangeListener {
-public:
- explicit PathInvalidator(const GrUniqueKey& key) : fMsg(key) {}
-private:
- GrUniqueKeyInvalidatedMessage fMsg;
-
- void onChange() override {
- SkMessageBus<GrUniqueKeyInvalidatedMessage>::Post(fMsg);
- }
-};
-
-} // namespace
-
bool GrTessellatingPathRenderer::onCanDrawPath(const CanDrawPathArgs& args) const {
// This path renderer can draw all fill styles, but does not do antialiasing. It can do convex
// and concave paths, but we'll leave the convex ones to simpler algorithms.
@@ -1418,9 +1377,7 @@
fPipelineInfo = init;
}
- int tessellate(GrUniqueKey* key,
- GrResourceProvider* resourceProvider,
- SkAutoTUnref<GrVertexBuffer>& vertexBuffer) {
+ void generateGeometry(GrBatchTarget* batchTarget, const GrPipeline* pipeline) override {
SkRect pathBounds = fPath.getBounds();
Comparator c;
if (pathBounds.width() > pathBounds.height()) {
@@ -1435,11 +1392,11 @@
int contourCnt;
int maxPts = GrPathUtils::worstCasePointCount(fPath, &contourCnt, tol);
if (maxPts <= 0) {
- return 0;
+ return;
}
if (maxPts > ((int)SK_MaxU16 + 1)) {
SkDebugf("Path not rendered, too many verts (%d)\n", maxPts);
- return 0;
+ return;
}
SkPath::FillType fillType = fPath.getFillType();
if (SkPath::IsInverseFillType(fillType)) {
@@ -1447,78 +1404,6 @@
}
LOG("got %d pts, %d contours\n", maxPts, contourCnt);
- SkAutoTDeleteArray<Vertex*> contours(SkNEW_ARRAY(Vertex *, contourCnt));
-
- // For the initial size of the chunk allocator, estimate based on the point count:
- // one vertex per point for the initial passes, plus two for the vertices in the
- // resulting Polys, since the same point may end up in two Polys. Assume minimal
- // connectivity of one Edge per Vertex (will grow for intersections).
- SkChunkAlloc alloc(maxPts * (3 * sizeof(Vertex) + sizeof(Edge)));
- bool isLinear;
- path_to_contours(fPath, tol, fClipBounds, contours.get(), alloc, &isLinear);
- Poly* polys;
- polys = contours_to_polys(contours.get(), contourCnt, c, alloc);
- int count = 0;
- for (Poly* poly = polys; poly; poly = poly->fNext) {
- if (apply_fill_type(fillType, poly->fWinding) && poly->fCount >= 3) {
- count += (poly->fCount - 2) * (WIREFRAME ? 6 : 3);
- }
- }
- if (0 == count) {
- return 0;
- }
-
- size_t size = count * sizeof(SkPoint);
- if (!vertexBuffer.get() || vertexBuffer->gpuMemorySize() < size) {
- vertexBuffer.reset(resourceProvider->createVertexBuffer(
- size, GrResourceProvider::kStatic_BufferUsage, 0));
- }
- if (!vertexBuffer.get()) {
- SkDebugf("Could not allocate vertices\n");
- return 0;
- }
- SkPoint* verts = static_cast<SkPoint*>(vertexBuffer->map());
- LOG("emitting %d verts\n", count);
- SkPoint* end = polys_to_triangles(polys, fillType, verts);
- vertexBuffer->unmap();
- int actualCount = static_cast<int>(end - verts);
- LOG("actual count: %d\n", actualCount);
- SkASSERT(actualCount <= count);
-
- if (!fPath.isVolatile()) {
- TessInfo info;
- info.fTolerance = isLinear ? 0 : tol;
- info.fCount = actualCount;
- SkAutoTUnref<SkData> data(SkData::NewWithCopy(&info, sizeof(info)));
- key->setCustomData(data.get());
- resourceProvider->assignUniqueKeyToResource(*key, vertexBuffer.get());
- SkPathPriv::AddGenIDChangeListener(fPath, SkNEW(PathInvalidator(*key)));
- }
- return actualCount;
- }
-
- void generateGeometry(GrBatchTarget* batchTarget, const GrPipeline* pipeline) override {
- // construct a cache key from the path's genID and the view matrix
- static const GrUniqueKey::Domain kDomain = GrUniqueKey::GenerateDomain();
- GrUniqueKey key;
- GrUniqueKey::Builder builder(&key, kDomain, 2);
- builder[0] = fPath.getGenerationID();
- builder[1] = fPath.getFillType();
- builder.finish();
- GrResourceProvider* rp = batchTarget->resourceProvider();
- SkAutoTUnref<GrVertexBuffer> vertexBuffer(rp->findAndRefTByUniqueKey<GrVertexBuffer>(key));
- int actualCount;
- SkScalar screenSpaceTol = GrPathUtils::kDefaultTolerance;
- SkScalar tol = GrPathUtils::scaleToleranceToSrc(
- screenSpaceTol, fViewMatrix, fPath.getBounds());
- if (!cache_match(vertexBuffer.get(), tol, &actualCount)) {
- actualCount = tessellate(&key, rp, vertexBuffer);
- }
-
- if (actualCount == 0) {
- return;
- }
-
SkAutoTUnref<const GrGeometryProcessor> gp;
{
using namespace GrDefaultGeoProcFactory;
@@ -1537,15 +1422,53 @@
gp.reset(GrDefaultGeoProcFactory::Create(color, coverage, localCoords,
fViewMatrix));
}
-
batchTarget->initDraw(gp, pipeline);
- SkASSERT(gp->getVertexStride() == sizeof(SkPoint));
+
+ SkAutoTDeleteArray<Vertex*> contours(SkNEW_ARRAY(Vertex *, contourCnt));
+
+ // For the initial size of the chunk allocator, estimate based on the point count:
+ // one vertex per point for the initial passes, plus two for the vertices in the
+ // resulting Polys, since the same point may end up in two Polys. Assume minimal
+ // connectivity of one Edge per Vertex (will grow for intersections).
+ SkChunkAlloc alloc(maxPts * (3 * sizeof(Vertex) + sizeof(Edge)));
+ path_to_contours(fPath, tol, fClipBounds, contours.get(), alloc);
+ Poly* polys;
+ polys = contours_to_polys(contours.get(), contourCnt, c, alloc);
+ int count = 0;
+ for (Poly* poly = polys; poly; poly = poly->fNext) {
+ if (apply_fill_type(fillType, poly->fWinding) && poly->fCount >= 3) {
+ count += (poly->fCount - 2) * (WIREFRAME ? 6 : 3);
+ }
+ }
+ if (0 == count) {
+ return;
+ }
+
+ size_t stride = gp->getVertexStride();
+ SkASSERT(stride == sizeof(SkPoint));
+ const GrVertexBuffer* vertexBuffer;
+ int firstVertex;
+ SkPoint* verts = static_cast<SkPoint*>(
+ batchTarget->makeVertSpace(stride, count, &vertexBuffer, &firstVertex));
+ if (!verts) {
+ SkDebugf("Could not allocate vertices\n");
+ return;
+ }
+
+ LOG("emitting %d verts\n", count);
+ SkPoint* end = polys_to_triangles(polys, fillType, verts);
+ int actualCount = static_cast<int>(end - verts);
+ LOG("actual count: %d\n", actualCount);
+ SkASSERT(actualCount <= count);
GrPrimitiveType primitiveType = WIREFRAME ? kLines_GrPrimitiveType
: kTriangles_GrPrimitiveType;
GrVertices vertices;
- vertices.init(primitiveType, vertexBuffer.get(), 0, actualCount);
+ vertices.init(primitiveType, vertexBuffer, firstVertex, actualCount);
batchTarget->draw(vertices);
+
+ batchTarget->putBackVertices((size_t)(count - actualCount), stride);
+ return;
}
bool onCombineIfPossible(GrBatch*) override {
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index b0bd667..8272692 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -3715,21 +3715,6 @@
"path.lineTo(3, 4);\n");
}
-namespace {
-
-class ChangeListener : public SkPathRef::GenIDChangeListener {
-public:
- ChangeListener(bool *changed) : fChanged(changed) { *fChanged = false; }
- virtual ~ChangeListener() {}
- void onChange() override {
- *fChanged = true;
- }
-private:
- bool* fChanged;
-};
-
-}
-
class PathTest_Private {
public:
static void TestPathTo(skiatest::Reporter* reporter) {
@@ -3749,50 +3734,6 @@
SkRect reverseExpected = {-4, -4, 8, 8};
REPORTER_ASSERT(reporter, p.getBounds() == reverseExpected);
}
-
- static void TestPathrefListeners(skiatest::Reporter* reporter) {
- SkPath p;
-
- bool changed = false;
- p.moveTo(0, 0);
-
- // Check that listener is notified on moveTo().
-
- SkPathPriv::AddGenIDChangeListener(p, SkNEW(ChangeListener(&changed)));
- REPORTER_ASSERT(reporter, !changed);
- p.moveTo(10, 0);
- REPORTER_ASSERT(reporter, changed);
-
- // Check that listener is notified on lineTo().
- SkPathPriv::AddGenIDChangeListener(p, SkNEW(ChangeListener(&changed)));
- REPORTER_ASSERT(reporter, !changed);
- p.lineTo(20, 0);
- REPORTER_ASSERT(reporter, changed);
-
- // Check that listener is notified on reset().
- SkPathPriv::AddGenIDChangeListener(p, SkNEW(ChangeListener(&changed)));
- REPORTER_ASSERT(reporter, !changed);
- p.reset();
- REPORTER_ASSERT(reporter, changed);
-
- p.moveTo(0, 0);
-
- // Check that listener is notified on rewind().
- SkPathPriv::AddGenIDChangeListener(p, SkNEW(ChangeListener(&changed)));
- REPORTER_ASSERT(reporter, !changed);
- p.rewind();
- REPORTER_ASSERT(reporter, changed);
-
- // Check that listener is notified when pathref is deleted.
- {
- SkPath q;
- q.moveTo(10, 10);
- SkPathPriv::AddGenIDChangeListener(q, SkNEW(ChangeListener(&changed)));
- REPORTER_ASSERT(reporter, !changed);
- }
- // q went out of scope.
- REPORTER_ASSERT(reporter, changed);
- }
};
DEF_TEST(Paths, reporter) {
@@ -3939,7 +3880,6 @@
test_contains(reporter);
PathTest_Private::TestPathTo(reporter);
PathRefTest_Private::TestPathRef(reporter);
- PathTest_Private::TestPathrefListeners(reporter);
test_dump(reporter);
test_path_crbug389050(reporter);
test_path_crbugskia2820(reporter);
diff --git a/tests/ResourceCacheTest.cpp b/tests/ResourceCacheTest.cpp
index 43f0870..b795b01 100644
--- a/tests/ResourceCacheTest.cpp
+++ b/tests/ResourceCacheTest.cpp
@@ -845,21 +845,6 @@
REPORTER_ASSERT(reporter, 0 == cache->getResourceCount());
REPORTER_ASSERT(reporter, 0 == cache->getResourceBytes());
REPORTER_ASSERT(reporter, 0 == TestResource::NumAlive());
-
- {
- GrUniqueKey key2;
- make_unique_key<0>(&key2, 0);
- SkAutoTUnref<TestResource> d(SkNEW_ARGS(TestResource, (context->getGpu())));
- int foo = 4132;
- SkAutoTUnref<SkData> data(SkData::NewWithCopy(&foo, sizeof(foo)));
- key2.setCustomData(data.get());
- d->resourcePriv().setUniqueKey(key2);
- }
-
- GrUniqueKey key3;
- make_unique_key<0>(&key3, 0);
- SkAutoTUnref<GrGpuResource> d2(cache->findAndRefUniqueResource(key3));
- REPORTER_ASSERT(reporter, *(int*) d2->getUniqueKey().getCustomData()->data() == 4132);
}
static void test_purge_invalidated(skiatest::Reporter* reporter) {
@@ -1238,21 +1223,6 @@
}
}
-static void test_custom_data(skiatest::Reporter* reporter) {
- GrUniqueKey key1, key2;
- make_unique_key<0>(&key1, 1);
- make_unique_key<0>(&key2, 2);
- int foo = 4132;
- SkAutoTUnref<SkData> data(SkData::NewWithCopy(&foo, sizeof(foo)));
- key1.setCustomData(data.get());
- REPORTER_ASSERT(reporter, *(int*) key1.getCustomData()->data() == 4132);
- REPORTER_ASSERT(reporter, key2.getCustomData() == nullptr);
-
- // Test that copying a key also takes a ref on its custom data.
- GrUniqueKey key3 = key1;
- REPORTER_ASSERT(reporter, *(int*) key3.getCustomData()->data() == 4132);
-}
-
////////////////////////////////////////////////////////////////////////////////
DEF_GPUTEST(ResourceCache, reporter, factory) {
for (int type = 0; type < GrContextFactory::kLastGLContextType; ++type) {
@@ -1292,7 +1262,6 @@
test_timestamp_wrap(reporter);
test_flush(reporter);
test_large_resource_count(reporter);
- test_custom_data(reporter);
}
#endif