From 0c492cfe1713d6895d1d513e754d938ff0faa5e5 Mon Sep 17 00:00:00 2001 From: Mike Reed Date: Thu, 16 Mar 2017 16:44:25 +0000 Subject: [PATCH] Revert[3] "store vertices arrays inline with object""" This reverts commit 7d9f9e30204ee8a380443b868e4cc281319a2051. Reason for revert: speculative revert to try to fix google3 Original change's description: > Revert[2] "store vertices arrays inline with object"" > > This reverts commit 9e62df6ecd1000860ad19ab9425579dfb7002ba0. > > Reason for revert: behavior in reader32 fixed > > Fix is here: https://skia-review.googlesource.com/c/9729/ > > Original change's description: > > Revert "store vertices arrays inline with object" > > > > This reverts commit eaaebb19a17d213355e7a70e0cfabe4ba61929d4. > > > > Reason for revert: may call SkReader32::read(null, 0) -- reader needs to handle this > > > > Original change's description: > > > store vertices arrays inline with object > > > > > > Also unify some of naming (esp. around texCoords) > > > > > > BUG=skia:6366 > > > > > > Change-Id: I5a6793f029cccf0cd0a2c1d180b259ce4eab526f > > > Reviewed-on: https://skia-review.googlesource.com/9705 > > > Commit-Queue: Mike Reed > > > Reviewed-by: Brian Salomon > > > > > > > TBR=bsalomon@google.com,reed@google.com,reviews@skia.org > > NOPRESUBMIT=true > > NOTREECHECKS=true > > NOTRY=true > > BUG=skia:6366 > > > > Change-Id: Ie421654bcd74d74f8be6676291e3d6e16e2a7a16 > > Reviewed-on: https://skia-review.googlesource.com/9727 > > Reviewed-by: Mike Reed > > Commit-Queue: Mike Reed > > > > TBR=bsalomon@google.com,reviews@skia.org,reed@google.com > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=skia:6366 > > Change-Id: I1f12108fff8f551d66455cfadd6d5dd9412e9aa8 > Reviewed-on: https://skia-review.googlesource.com/9760 > Reviewed-by: Mike Reed > Commit-Queue: Mike Reed > TBR=bsalomon@google.com,reviews@skia.org,reed@google.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=skia:6366 Change-Id: Ie23130a07fbecd5664e37291bc167008a6b496bc Reviewed-on: https://skia-review.googlesource.com/9806 Reviewed-by: Mike Reed Commit-Queue: Mike Reed --- gm/vertices.cpp | 3 +- include/core/SkVertices.h | 97 ++++++-------- src/core/SkVertices.cpp | 267 ++++++++++++++++++++------------------- src/gpu/ops/GrDrawVerticesOp.cpp | 2 +- src/gpu/ops/GrDrawVerticesOp.h | 2 +- src/utils/SkShadowUtils.cpp | 4 +- tests/VerticesTest.cpp | 4 +- 7 files changed, 182 insertions(+), 197 deletions(-) diff --git a/gm/vertices.cpp b/gm/vertices.cpp index 0d79827..864ac3d 100644 --- a/gm/vertices.cpp +++ b/gm/vertices.cpp @@ -214,8 +214,7 @@ static void draw_batching(SkCanvas* canvas, bool useObject) { // Triangle fans can't batch so we convert to regular triangles, static constexpr int kNumTris = kMeshIndexCnt - 2; SkVertices::Builder builder(SkCanvas::kTriangles_VertexMode, kMeshVertexCnt, 3 * kNumTris, - SkVertices::kHasColors_BuilderFlag | - SkVertices::kHasTexCoords_BuilderFlag); + SkVertices::kHasColors_Flag | SkVertices::kHasTexs_Flag); SkPoint* pts = builder.positions(); SkPoint* texs = builder.texCoords(); diff --git a/include/core/SkVertices.h b/include/core/SkVertices.h index 8d6f211..af4e3bc 100644 --- a/include/core/SkVertices.h +++ b/include/core/SkVertices.h @@ -16,10 +16,14 @@ #include "SkRefCnt.h" /** - * An immutable set of vertex data that can be used with SkCanvas::drawVertices. + * An immutable set of vertex data that can be used with SkCanvas::drawVertices. Clients are + * encouraged to provide a bounds on the vertex positions if they can compute one more cheaply than + * looping over the positions. */ class SkVertices : public SkNVRefCnt { public: + ~SkVertices() { sk_free((void*)fPositions); } + /** * Create a vertices by copying the specified arrays. texs and colors may be nullptr, * and indices is ignored if indexCount == 0. @@ -38,93 +42,72 @@ public: return MakeCopy(mode, vertexCount, positions, texs, colors, 0, nullptr); } - struct Sizes; - - enum BuilderFlags { - kHasTexCoords_BuilderFlag = 1 << 0, - kHasColors_BuilderFlag = 1 << 1, + enum Flags { + kHasTexs_Flag = 1 << 0, + kHasColors_Flag = 1 << 1, }; class Builder { public: Builder(SkCanvas::VertexMode mode, int vertexCount, int indexCount, uint32_t flags); + ~Builder(); - bool isValid() const { return fVertices != nullptr; } + bool isValid() const { return fPositions != nullptr; } - // if the builder is invalid, these will return 0 - int vertexCount() const; - int indexCount() const; - SkPoint* positions(); - SkPoint* texCoords(); // returns null if there are no texCoords - SkColor* colors(); // returns null if there are no colors - uint16_t* indices(); // returns null if there are no indices + int vertexCount() const { return fVertexCnt; } + int indexCount() const { return fIndexCnt; } + SkPoint* positions() { return fPositions; } + SkPoint* texCoords() { return fTexs; } + SkColor* colors() { return fColors; } + uint16_t* indices() { return fIndices; } - // Detach the built vertices object. After the first call, this will always return null. sk_sp detach(); private: - Builder(SkCanvas::VertexMode mode, int vertexCount, int indexCount, const Sizes&); - - void init(SkCanvas::VertexMode mode, int vertexCount, int indexCount, const Sizes&); - - // holds a partially complete object. only completed in detach() - sk_sp fVertices; - - friend class SkVertices; + SkPoint* fPositions; // owner of storage, use sk_free + SkPoint* fTexs; + SkColor* fColors; + uint16_t* fIndices; + int fVertexCnt; + int fIndexCnt; + SkCanvas::VertexMode fMode; }; - uint32_t uniqueID() const { return fUniqueID; } SkCanvas::VertexMode mode() const { return fMode; } - const SkRect& bounds() const { return fBounds; } - - bool hasColors() const { return SkToBool(this->colors()); } - bool hasTexCoords() const { return SkToBool(this->texCoords()); } - bool hasIndices() const { return SkToBool(this->indices()); } + uint32_t uniqueID() const { return fUniqueID; } int vertexCount() const { return fVertexCnt; } + bool hasColors() const { return SkToBool(fColors); } + bool hasTexCoords() const { return SkToBool(fTexs); } const SkPoint* positions() const { return fPositions; } const SkPoint* texCoords() const { return fTexs; } const SkColor* colors() const { return fColors; } + bool isIndexed() const { return SkToBool(fIndexCnt); } int indexCount() const { return fIndexCnt; } const uint16_t* indices() const { return fIndices; } - // returns approximate byte size of the vertices object - size_t approximateSize() const; + size_t size() const { + return fVertexCnt * (sizeof(SkPoint) * (this->hasTexCoords() ? 2 : 1) + sizeof(SkColor)) + + fIndexCnt * sizeof(uint16_t); + } - /** - * Recreate a vertices from a buffer previously created by calling encode(). - * Returns null if the data is corrupt or the length is incorrect for the contents. - */ - static sk_sp Decode(const void* buffer, size_t length); + const SkRect& bounds() const { return fBounds; } - /** - * Pack the vertices object into a byte buffer. This can be used to recreate the vertices - * by calling Decode() with the buffer. - */ + static sk_sp Decode(const void*, size_t); sk_sp encode() const; private: SkVertices() {} - static sk_sp Alloc(int vCount, int iCount, uint32_t builderFlags, - size_t* arraySize); - - // we store this first, to pair with the refcnt in our base-class, so we don't have an - // unnecessary pad between it and the (possibly 8-byte aligned) ptrs. + const SkPoint* fPositions; // owner of storage, use sk_free + const SkPoint* fTexs; + const SkColor* fColors; + const uint16_t* fIndices; + SkRect fBounds; uint32_t fUniqueID; - - // these point inside our allocation, so none of these can be "freed" - SkPoint* fPositions; - SkPoint* fTexs; - SkColor* fColors; - uint16_t* fIndices; - - SkRect fBounds; // computed to be the union of the fPositions[] - int fVertexCnt; - int fIndexCnt; - + int fVertexCnt; + int fIndexCnt; SkCanvas::VertexMode fMode; - // below here is where the actual array data is stored. }; #endif diff --git a/src/core/SkVertices.cpp b/src/core/SkVertices.cpp index 1980b7e..936d70d 100644 --- a/src/core/SkVertices.cpp +++ b/src/core/SkVertices.cpp @@ -20,199 +20,202 @@ static int32_t next_id() { return id; } -struct SkVertices::Sizes { - Sizes(int vertexCount, int indexCount, bool hasTexs, bool hasColors) { - int64_t vSize = (int64_t)vertexCount * sizeof(SkPoint); - int64_t tSize = hasTexs ? (int64_t)vertexCount * sizeof(SkPoint) : 0; - int64_t cSize = hasColors ? (int64_t)vertexCount * sizeof(SkColor) : 0; - int64_t iSize = (int64_t)indexCount * sizeof(uint16_t); - - int64_t total = sizeof(SkVertices) + vSize + tSize + cSize + iSize; - if (!sk_64_isS32(total)) { - sk_bzero(this, sizeof(*this)); - } else { - fTotal = SkToSizeT(total); - fVSize = SkToSizeT(vSize); - fTSize = SkToSizeT(tSize); - fCSize = SkToSizeT(cSize); - fISize = SkToSizeT(iSize); - fArrays = fTotal - sizeof(SkVertices); // just the sum of the arrays - } - } - - bool isValid() const { return fTotal != 0; } - - size_t fTotal; // size of entire SkVertices allocation (obj + arrays) - size_t fArrays; // size of all the arrays (V + T + C + I) - size_t fVSize; - size_t fTSize; - size_t fCSize; - size_t fISize; -}; +static size_t compute_arrays_size(int vertexCount, int indexCount, uint32_t builderFlags) { + if (vertexCount < 0 || indexCount < 0) { + return 0; // signal error + } -SkVertices::Builder::Builder(SkCanvas::VertexMode mode, int vertexCount, int indexCount, - uint32_t builderFlags) { - bool hasTexs = SkToBool(builderFlags & SkVertices::kHasTexCoords_BuilderFlag); - bool hasColors = SkToBool(builderFlags & SkVertices::kHasColors_BuilderFlag); - this->init(mode, vertexCount, indexCount, - SkVertices::Sizes(vertexCount, indexCount, hasTexs, hasColors)); + uint64_t size = vertexCount * sizeof(SkPoint); + if (builderFlags & SkVertices::kHasTexs_Flag) { + size += vertexCount * sizeof(SkPoint); + } + if (builderFlags & SkVertices::kHasColors_Flag) { + size += vertexCount * sizeof(SkColor); + } + size += indexCount * sizeof(uint16_t); + if (!sk_64_isS32(size)) { + return 0; // signal error + } + return (size_t)size; } SkVertices::Builder::Builder(SkCanvas::VertexMode mode, int vertexCount, int indexCount, - const SkVertices::Sizes& sizes) { - this->init(mode, vertexCount, indexCount, sizes); -} - -void SkVertices::Builder::init(SkCanvas::VertexMode mode, int vertexCount, int indexCount, - const SkVertices::Sizes& sizes) { - if (!sizes.isValid()) { - return; // fVertices will already be null + uint32_t flags) { + fPositions = nullptr; // signal that we have nothing to cleanup + fColors = nullptr; + fTexs = nullptr; + fIndices = nullptr; + fVertexCnt = 0; + fIndexCnt = 0; + + size_t size = compute_arrays_size(vertexCount, indexCount, flags); + if (0 == size) { + return; } - void* storage = ::operator new (sizes.fTotal); - fVertices.reset(new (storage) SkVertices); - - // need to point past the object to store the arrays - char* ptr = (char*)fVertices.get() + sizeof(SkVertices); + char* ptr = (char*)sk_malloc_throw(sk_64_asS32(size)); - fVertices->fPositions = (SkPoint*)ptr; ptr += sizes.fVSize; - fVertices->fTexs = sizes.fTSize ? (SkPoint*)ptr : nullptr; ptr += sizes.fTSize; - fVertices->fColors = sizes.fCSize ? (SkColor*)ptr : nullptr; ptr += sizes.fCSize; - fVertices->fIndices = sizes.fISize ? (uint16_t*)ptr : nullptr; - fVertices->fVertexCnt = vertexCount; - fVertices->fIndexCnt = indexCount; - fVertices->fMode = mode; - // We defer assigning fBounds and fUniqueID until detach() is called -} + fMode = mode; + fVertexCnt = vertexCount; + fIndexCnt = indexCount; + fPositions = (SkPoint*)ptr; // owner + ptr += vertexCount * sizeof(SkPoint); -sk_sp SkVertices::Builder::detach() { - if (fVertices) { - fVertices->fBounds.set(fVertices->fPositions, fVertices->fVertexCnt); - fVertices->fUniqueID = next_id(); - return std::move(fVertices); // this will null fVertices after the return + if (flags & kHasTexs_Flag) { + fTexs = (SkPoint*)ptr; + ptr += vertexCount * sizeof(SkPoint); + } + if (flags & kHasColors_Flag) { + fColors = (SkColor*)ptr; + ptr += vertexCount * sizeof(SkColor); + } + if (indexCount) { + fIndices = (uint16_t*)ptr; } - return nullptr; -} - -int SkVertices::Builder::vertexCount() const { - return fVertices ? fVertices->vertexCount() : 0; } -int SkVertices::Builder::indexCount() const { - return fVertices ? fVertices->indexCount() : 0; +SkVertices::Builder::~Builder() { + sk_free(fPositions); } -SkPoint* SkVertices::Builder::positions() { - return fVertices ? const_cast(fVertices->positions()) : nullptr; -} +sk_sp SkVertices::Builder::detach() { + if (!fPositions) { + return nullptr; + } -SkPoint* SkVertices::Builder::texCoords() { - return fVertices ? const_cast(fVertices->texCoords()) : nullptr; -} + SkVertices* obj = new SkVertices; + obj->fPositions = fPositions; // owner of storage, use sk_free + obj->fTexs = fTexs; + obj->fColors = fColors; + obj->fIndices = fIndices; + obj->fBounds.set(fPositions, fVertexCnt); + obj->fUniqueID = next_id(); + obj->fVertexCnt = fVertexCnt; + obj->fIndexCnt = fIndexCnt; + obj->fMode = fMode; -SkColor* SkVertices::Builder::colors() { - return fVertices ? const_cast(fVertices->colors()) : nullptr; -} + fPositions = nullptr; // so we don't free the memory, now that obj owns it -uint16_t* SkVertices::Builder::indices() { - return fVertices ? const_cast(fVertices->indices()) : nullptr; + return sk_sp(obj); } -/////////////////////////////////////////////////////////////////////////////////////////////////// - sk_sp SkVertices::MakeCopy(SkCanvas::VertexMode mode, int vertexCount, const SkPoint pos[], const SkPoint texs[], const SkColor colors[], int indexCount, const uint16_t indices[]) { - Sizes sizes(vertexCount, indexCount, texs != nullptr, colors != nullptr); - if (!sizes.isValid()) { + uint32_t flags = 0; + if (texs) { + flags |= kHasTexs_Flag; + } + if (colors) { + flags |= kHasColors_Flag; + } + Builder builder(mode, vertexCount, indexCount, flags); + if (!builder.isValid()) { return nullptr; } - Builder builder(mode, vertexCount, indexCount, sizes); - SkASSERT(builder.isValid()); - - sk_careful_memcpy(builder.positions(), pos, sizes.fVSize); - sk_careful_memcpy(builder.texCoords(), texs, sizes.fTSize); - sk_careful_memcpy(builder.colors(), colors, sizes.fCSize); - sk_careful_memcpy(builder.indices(), indices, sizes.fISize); - + memcpy(builder.positions(), pos, vertexCount * sizeof(SkPoint)); + if (texs) { + memcpy(builder.texCoords(), texs, vertexCount * sizeof(SkPoint)); + } + if (colors) { + memcpy(builder.colors(), colors, vertexCount * sizeof(SkColor)); + } + if (indices) { + memcpy(builder.indices(), indices, indexCount * sizeof(uint16_t)); + } return builder.detach(); } -size_t SkVertices::approximateSize() const { - Sizes sizes(fVertexCnt, fIndexCnt, this->hasTexCoords(), this->hasColors()); - SkASSERT(sizes.isValid()); - return sizeof(SkVertices) + sizes.fArrays; -} - /////////////////////////////////////////////////////////////////////////////////////////////////// -// storage = packed | vertex_count | index_count | pos[] | texs[] | colors[] | indices[] -// = header + arrays +// storage = flags | vertex_count | index_count | pos[] | texs[] | colors[] | indices[] #define kMode_Mask 0x0FF #define kHasTexs_Mask 0x100 #define kHasColors_Mask 0x200 -#define kHeaderSize (3 * sizeof(uint32_t)) sk_sp SkVertices::encode() const { - // packed has room for addtional flags in the future (e.g. versioning) - uint32_t packed = static_cast(fMode); - SkASSERT((packed & ~kMode_Mask) == 0); // our mode fits in the mask bits - if (this->hasTexCoords()) { - packed |= kHasTexs_Mask; + uint32_t flags = static_cast(fMode); + SkASSERT((flags & ~kMode_Mask) == 0); + if (fTexs) { + flags |= kHasTexs_Mask; } - if (this->hasColors()) { - packed |= kHasColors_Mask; + if (fColors) { + flags |= kHasColors_Mask; } - Sizes sizes(fVertexCnt, fIndexCnt, this->hasTexCoords(), this->hasColors()); - SkASSERT(sizes.isValid()); - const size_t size = kHeaderSize + sizes.fArrays; + size_t size = sizeof(uint32_t) * 3; // flags | verts_count | indices_count + size += fVertexCnt * sizeof(SkPoint); + if (fTexs) { + size += fVertexCnt * sizeof(SkPoint); + } + if (fColors) { + size += fVertexCnt * sizeof(SkColor); + } + size += fIndexCnt * sizeof(uint16_t); sk_sp data = SkData::MakeUninitialized(size); SkWriter32 writer(data->writable_data(), data->size()); - writer.write32(packed); + writer.write32(flags); writer.write32(fVertexCnt); writer.write32(fIndexCnt); - writer.write(fPositions, sizes.fVSize); - writer.write(fTexs, sizes.fTSize); - writer.write(fColors, sizes.fCSize); - writer.write(fIndices, sizes.fISize); + writer.write(fPositions, fVertexCnt * sizeof(SkPoint)); + if (fTexs) { + writer.write(fTexs, fVertexCnt * sizeof(SkPoint)); + } + if (fColors) { + writer.write(fColors, fVertexCnt * sizeof(SkColor)); + } + writer.write(fIndices, fIndexCnt * sizeof(uint16_t)); return data; } sk_sp SkVertices::Decode(const void* data, size_t length) { - if (length < kHeaderSize) { - return nullptr; + if (length < 3 * sizeof(uint32_t)) { + return nullptr; // buffer too small } SkReader32 reader(data, length); - const uint32_t packed = reader.readInt(); - const int vertexCount = reader.readInt(); - const int indexCount = reader.readInt(); + uint32_t storageFlags = reader.readInt(); + SkCanvas::VertexMode mode = static_cast(storageFlags & kMode_Mask); + int vertexCount = reader.readInt(); + int indexCount = reader.readInt(); + uint32_t builderFlags = 0; + if (storageFlags & kHasTexs_Mask) { + builderFlags |= SkVertices::kHasTexs_Flag; + } + if (storageFlags & kHasColors_Mask) { + builderFlags |= SkVertices::kHasColors_Flag; + } - const SkCanvas::VertexMode mode = static_cast(packed & kMode_Mask); - const bool hasTexs = SkToBool(packed & kHasTexs_Mask); - const bool hasColors = SkToBool(packed & kHasColors_Mask); - Sizes sizes(vertexCount, indexCount, hasTexs, hasColors); - if (!sizes.isValid()) { + size_t size = compute_arrays_size(vertexCount, indexCount, builderFlags); + if (0 == size) { return nullptr; } - if (kHeaderSize + sizes.fArrays != length) { + + length -= 3 * sizeof(uint32_t); // already read the header + if (length < size) { // buffer too small return nullptr; } - Builder builder(mode, vertexCount, indexCount, sizes); + Builder builder(mode, vertexCount, indexCount, builderFlags); + if (!builder.isValid()) { + return nullptr; + } - reader.read(builder.positions(), sizes.fVSize); - reader.read(builder.texCoords(), sizes.fTSize); - reader.read(builder.colors(), sizes.fCSize); - reader.read(builder.indices(), sizes.fISize); + reader.read(builder.positions(), vertexCount * sizeof(SkPoint)); + if (builderFlags & SkVertices::kHasTexs_Flag) { + reader.read(builder.texCoords(), vertexCount * sizeof(SkPoint)); + } + if (builderFlags & SkVertices::kHasColors_Flag) { + reader.read(builder.colors(), vertexCount * sizeof(SkColor)); + } + reader.read(builder.indices(), indexCount * sizeof(uint16_t)); return builder.detach(); } diff --git a/src/gpu/ops/GrDrawVerticesOp.cpp b/src/gpu/ops/GrDrawVerticesOp.cpp index aa13cae..5d88378 100644 --- a/src/gpu/ops/GrDrawVerticesOp.cpp +++ b/src/gpu/ops/GrDrawVerticesOp.cpp @@ -257,7 +257,7 @@ bool GrDrawVerticesOp::onCombineIfPossible(GrOp* t, const GrCaps& caps) { return false; } - if (fMeshes[0].fVertices->hasIndices() != that->fMeshes[0].fVertices->hasIndices()) { + if (fMeshes[0].fVertices->isIndexed() != that->fMeshes[0].fVertices->isIndexed()) { return false; } diff --git a/src/gpu/ops/GrDrawVerticesOp.h b/src/gpu/ops/GrDrawVerticesOp.h index eddf201..c837734 100644 --- a/src/gpu/ops/GrDrawVerticesOp.h +++ b/src/gpu/ops/GrDrawVerticesOp.h @@ -97,7 +97,7 @@ private: bool isIndexed() const { // Consistency enforced in onCombineIfPossible. - return fMeshes[0].fVertices->hasIndices(); + return fMeshes[0].fVertices->isIndexed(); } bool requiresPerVertexColors() const { diff --git a/src/utils/SkShadowUtils.cpp b/src/utils/SkShadowUtils.cpp index 402dca5..eb015aa 100644 --- a/src/utils/SkShadowUtils.cpp +++ b/src/utils/SkShadowUtils.cpp @@ -232,12 +232,12 @@ private: i = fCount++; } else { i = gRandom.nextULessThan(MAX_ENTRIES); - fSize -= fEntries[i].fVertices->approximateSize(); + fSize -= fEntries[i].fVertices->size(); } fEntries[i].fFactory = factory; fEntries[i].fVertices = vertices; fEntries[i].fMatrix = matrix; - fSize += vertices->approximateSize(); + fSize += vertices->size(); return vertices; } diff --git a/tests/VerticesTest.cpp b/tests/VerticesTest.cpp index 9621d80..8cf5514 100644 --- a/tests/VerticesTest.cpp +++ b/tests/VerticesTest.cpp @@ -53,8 +53,8 @@ DEF_TEST(Vertices, reporter) { int vCount = 4; int iCount = 6; - const uint32_t texFlags[] = { 0, SkVertices::kHasTexCoords_BuilderFlag }; - const uint32_t colFlags[] = { 0, SkVertices::kHasColors_BuilderFlag }; + const uint32_t texFlags[] = { 0, SkVertices::kHasTexs_Flag }; + const uint32_t colFlags[] = { 0, SkVertices::kHasColors_Flag }; for (auto texF : texFlags) { for (auto colF : colFlags) { uint32_t flags = texF | colF; -- 2.7.4