Skip to content

Commit

Permalink
Remove pointer sidecar data in PaintParamsKey.
Browse files Browse the repository at this point in the history 
Pointer data is obliterated when we assemble draw passes, so this
feature didn't work for its intended purpose.

Change-Id: I85331aa7b6212c55fe16081937465c2f38e5a103
Bug: skia:13428
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/552718
Reviewed-by: Michael Ludwig <[email protected]>
Commit-Queue: Michael Ludwig <[email protected]>
Auto-Submit: John Stiles <[email protected]>
  • Loading branch information
@johnstiles-google
johnstiles-google authored and SkCQ committed Jun 24, 2022
1 parent b5ea901 commit 76f32c9
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 153 deletions.
1 change: 0 additions & 1 deletion src/core/SkKeyHelpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,6 @@ void RuntimeShaderBlock::BeginBlock(const SkKeyContext& keyContext,
builder->beginBlock(kCodeSnippetID);
builder->addBytes(sizeof(hash), reinterpret_cast<const uint8_t*>(&hash));
builder->addBytes(sizeof(uniformSize), reinterpret_cast<const uint8_t*>(&uniformSize));
builder->addPointer(shaderData.fEffect.get());
#endif // SK_GRAPHITE_ENABLED
break;
}
Expand Down
39 changes: 6 additions & 33 deletions src/core/SkPaintParamsKey.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ SkPaintParamsKeyBuilder::SkPaintParamsKeyBuilder(const SkShaderCodeDictionary* d
void SkPaintParamsKeyBuilder::checkReset() {
SkASSERT(!this->isLocked());
SkASSERT(this->sizeInBytes() == 0);
SkASSERT(this->numPointers() == 0);
SkASSERT(fIsValid);
SkASSERT(fStack.empty());
#ifdef SK_GRAPHITE_ENABLED
Expand Down Expand Up @@ -156,10 +155,9 @@ void SkPaintParamsKeyBuilder::checkExpectations(DataPayloadType actualType, uint

static int field_size(DataPayloadType type) {
switch (type) {
case DataPayloadType::kByte:
case DataPayloadType::kPointerIndex: return 1;
case DataPayloadType::kInt: return 4;
case DataPayloadType::kFloat4: return 16;
case DataPayloadType::kByte: return 1;
case DataPayloadType::kInt: return 4;
case DataPayloadType::kFloat4: return 16;
}
SkUNREACHABLE;
}
Expand Down Expand Up @@ -195,14 +193,6 @@ void SkPaintParamsKeyBuilder::add(int numColors, const SkColor4f* colors) {
this->addToKey(numColors, colors, DataPayloadType::kFloat4);
}

void SkPaintParamsKeyBuilder::addPointer(const void* ptr) {
SkASSERT(fPointerData.size() <= 0xFF);
uint8_t pointerIndex = (uint8_t)fPointerData.size();

this->addToKey(1, &pointerIndex, DataPayloadType::kPointerIndex);
fPointerData.push_back(ptr);
}

SkPaintParamsKey SkPaintParamsKeyBuilder::lockAsKey() {
if (!fStack.empty()) {
// SKGPU_LOG_W("Mismatched beginBlock/endBlocks.");
Expand All @@ -216,9 +206,7 @@ SkPaintParamsKey SkPaintParamsKeyBuilder::lockAsKey() {
fIsValid = true;
fStack.rewind();

return SkPaintParamsKey(SkSpan(fData.begin(), fData.count()),
SkSpan(fPointerData.begin(), fPointerData.count()),
this);
return SkPaintParamsKey(SkSpan(fData.begin(), fData.count()), this);
}

void SkPaintParamsKeyBuilder::makeInvalid() {
Expand All @@ -227,7 +215,6 @@ void SkPaintParamsKeyBuilder::makeInvalid() {

fStack.rewind();
fData.rewind();
fPointerData.rewind();
this->beginBlock(SkBuiltInCodeSnippetID::kError);
this->endBlock();

Expand All @@ -237,10 +224,8 @@ void SkPaintParamsKeyBuilder::makeInvalid() {

//--------------------------------------------------------------------------------------------------
SkPaintParamsKey::SkPaintParamsKey(SkSpan<const uint8_t> span,
SkSpan<const void*> pointerSpan,
SkPaintParamsKeyBuilder* originatingBuilder)
: fData(span)
, fPointerData(pointerSpan)
, fOriginatingBuilder(originatingBuilder) {
fOriginatingBuilder->lock();
}
Expand All @@ -257,14 +242,13 @@ SkPaintParamsKey::~SkPaintParamsKey() {
}

bool SkPaintParamsKey::operator==(const SkPaintParamsKey& that) const {
// Pointer data is intentionally ignored here; a cached key will not have pointer data.
return fData.size() == that.fData.size() &&
!memcmp(fData.data(), that.fData.data(), fData.size());
}

SkPaintParamsKey::BlockReader SkPaintParamsKey::reader(const SkShaderCodeDictionary* dict,
int headerOffset) const {
return BlockReader(dict, fData, fPointerData, headerOffset);
return BlockReader(dict, fData, headerOffset);
}

#ifdef SK_DEBUG
Expand Down Expand Up @@ -326,12 +310,10 @@ bool SkPaintParamsKey::isErrorKey() const {

SkPaintParamsKey::BlockReader::BlockReader(const SkShaderCodeDictionary* dict,
SkSpan<const uint8_t> parentSpan,
SkSpan<const void*> pointerSpan,
int offsetInParent) {
Header header = read_header(parentSpan, offsetInParent);

fBlock = parentSpan.subspan(offsetInParent, header.blockSize);
fPointerSpan = pointerSpan;
fEntry = dict->getEntry(header.codeSnippetID);
SkASSERT(fEntry);
}
Expand All @@ -349,7 +331,7 @@ SkPaintParamsKey::BlockReader SkPaintParamsKey::BlockReader::child(
childOffset += header.blockSize;
}

return BlockReader(dict, fBlock, fPointerSpan, childOffset);
return BlockReader(dict, fBlock, childOffset);
}

int32_t SkPaintParamsKey::BlockReader::codeSnippetId() const {
Expand Down Expand Up @@ -405,15 +387,6 @@ SkSpan<const SkColor4f> SkPaintParamsKey::BlockReader::colors(int fieldIndex) co
fieldIndex);
}

const void* SkPaintParamsKey::BlockReader::pointer(int fieldIndex) const {
SkASSERT(fEntry->fDataPayloadExpectations[fieldIndex].fType == DataPayloadType::kPointerIndex);
SkASSERT(fEntry->fDataPayloadExpectations[fieldIndex].fCount == 1);
SkSpan dataSpan = payload_subspan_for_field<uint8_t>(this->dataPayload(),
fEntry->fDataPayloadExpectations,
fieldIndex);
return fPointerSpan[dataSpan[0]];
}

#ifdef SK_DEBUG

int SkPaintParamsKey::BlockReader::numDataPayloadFields() const {
Expand Down
34 changes: 3 additions & 31 deletions src/core/SkPaintParamsKey.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ struct SkShaderSnippet;
// Header, consisting of 2 bytes:
// 4 bytes: code-snippet ID
// 1 byte: size of the block, in bytes (header, plus all data payload bytes)
// The rest of the data and pointers in the block are dependent on the individual code snippet.
// The rest of the data in the block is dependent on the individual code snippet.
// If a given block has child blocks, they appear in the key right after their parent
// block's header.
class SkPaintParamsKey {
Expand All @@ -56,8 +56,6 @@ class SkPaintParamsKey {
kByte,
kInt,
kFloat4,
// Represents a position inside the fPointerData span.
kPointerIndex,
};

// A given snippet's data payload is stored as an SkSpan of DataPayloadFields in the
Expand Down Expand Up @@ -89,7 +87,6 @@ class SkPaintParamsKey {
SkSpan<const uint8_t> bytes(int fieldIndex) const;
SkSpan<const int32_t> ints(int fieldIndex) const;
SkSpan<const SkColor4f> colors(int fieldIndex) const;
const void* pointer(int fieldIndex) const;

const SkShaderSnippet* entry() const { return fEntry; }

Expand All @@ -103,7 +100,6 @@ class SkPaintParamsKey {

BlockReader(const SkShaderCodeDictionary*,
SkSpan<const uint8_t> parentSpan,
SkSpan<const void*> pointerSpan,
int offsetInParent);

int32_t codeSnippetId() const;
Expand All @@ -113,7 +109,6 @@ class SkPaintParamsKey {
SkSpan<const uint8_t> dataPayload() const;

SkSpan<const uint8_t> fBlock;
SkSpan<const void*> fPointerSpan;
const SkShaderSnippet* fEntry;
};

Expand All @@ -132,10 +127,6 @@ class SkPaintParamsKey {
const uint8_t* data() const { return fData.data(); }
int sizeInBytes() const { return SkTo<int>(fData.size()); }

SkSpan<const void*> pointerSpan() const { return fPointerData; }
const void** pointerData() const { return fPointerData.data(); }
int numPointers() const { return SkTo<int>(fPointerData.size()); }

bool operator==(const SkPaintParamsKey& that) const;
bool operator!=(const SkPaintParamsKey& that) const { return !(*this == that); }

Expand All @@ -150,9 +141,7 @@ class SkPaintParamsKey {
// This ctor is to be used when paintparams keys are being consecutively generated
// by a key builder. The memory backing this key's span is shared between the
// builder and its keys.
SkPaintParamsKey(SkSpan<const uint8_t> span,
SkSpan<const void*> pointerSpan,
SkPaintParamsKeyBuilder* originatingBuilder);
SkPaintParamsKey(SkSpan<const uint8_t> span, SkPaintParamsKeyBuilder* originatingBuilder);

// This ctor is used when this key isn't being created by a builder (i.e., when the key
// is in the dictionary). In this case the dictionary will own the memory backing the span.
Expand All @@ -162,14 +151,12 @@ class SkPaintParamsKey {
const SkPaintParamsKey::BlockReader&,
SkShaderInfo*);

// The memory referenced in 'fData' and 'fPointerData' is always owned by someone else.
// The memory referenced in 'fData' is always owned by someone else.
// If 'fOriginatingBuilder' is null, the dictionary's SkArena owns the 'fData' memory and no
// explicit freeing is required.
// If 'fOriginatingBuilder' is non-null then the 'fData' memory must be explicitly locked (in
// the ctor) and unlocked (in the dtor) on the 'fOriginatingBuilder' object.
// The 'fPointerData' memory is always managed external to this class.
SkSpan<const uint8_t> fData;
SkSpan<const void*> fPointerData;

// This class should only ever access the 'lock' and 'unlock' calls on 'fOriginatingBuilder'
SkPaintParamsKeyBuilder* fOriginatingBuilder;
Expand Down Expand Up @@ -218,12 +205,6 @@ class SkPaintParamsKeyBuilder {
this->add(/*numColors=*/1, &color);
}

// `addPointer` is optional sidecar data. The pointer data in a PaintParamsKey is not checked at
// all when checking the equality of two keys; cached PaintParamsKey objects will not hold
// pointer data. However, pointer data will be required for actually painting pixels on the
// screen.
void addPointer(const void* ptr);

#ifdef SK_DEBUG
// Check that the builder has been reset to its initial state prior to creating a new key.
void checkReset();
Expand All @@ -233,7 +214,6 @@ class SkPaintParamsKeyBuilder {
SkPaintParamsKey lockAsKey();

int sizeInBytes() const { return fData.count(); }
int numPointers() const { return fPointerData.count(); }

bool isValid() const { return fIsValid; }

Expand All @@ -245,7 +225,6 @@ class SkPaintParamsKeyBuilder {
void unlock() {
SkASSERT(fLocked);
fData.rewind();
fPointerData.rewind();
#ifdef SK_GRAPHITE_ENABLED
fBlendInfo = {};
#endif
Expand Down Expand Up @@ -288,13 +267,6 @@ class SkPaintParamsKeyBuilder {
SkTDArray<StackFrame> fStack;
SkTDArray<uint8_t> fData;

// The pointer data is used by some paint types, when the key data is not sufficient to
// reconstruct all the information needed to draw. (For instance, the key for a runtime effect
// contains a hash of the shader text, but to draw, we need entire compiled shader program.)
// Cached paint-param keys will discard the pointer data. When comparing paint-param keys,
// pointer data (if any) will be ignored.
SkTDArray<const void*> fPointerData;

#ifdef SK_GRAPHITE_ENABLED
skgpu::BlendInfo fBlendInfo;
#endif
Expand Down
1 change: 0 additions & 1 deletion src/core/SkShaderCodeDictionary.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,6 @@ static constexpr SkUniform kRuntimeShaderUniforms[] = {
static constexpr DataPayloadField kRuntimeShaderDataPayload[] = {
{"runtime effect hash", DataPayloadType::kByte, 4},
{"uniform data size (bytes)", DataPayloadType::kByte, 4},
{"SkRuntimeEffect pointer", DataPayloadType::kPointerIndex, 1},
};

//--------------------------------------------------------------------------------------------------
Expand Down
87 changes: 0 additions & 87 deletions tests/graphite/KeyTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,6 @@ SkPaintParamsKey create_key_with_data(SkPaintParamsKeyBuilder* builder,
return builder->lockAsKey();
}

SkPaintParamsKey create_key_with_ptr(SkPaintParamsKeyBuilder* builder,
int snippetID,
SkSpan<const uint8_t> dataPayload,
void* pointerData) {
SkDEBUGCODE(builder->checkReset());

builder->beginBlock(snippetID);
builder->addBytes(dataPayload.size(), dataPayload.data());
builder->addPointer(pointerData);
builder->endBlock();

return builder->lockAsKey();
}

SkPaintParamsKey create_key(SkPaintParamsKeyBuilder* builder, int snippetID, int size) {
SkASSERT(size <= 1024);
static constexpr uint8_t kDummyData[1024] = {};
Expand Down Expand Up @@ -149,38 +135,6 @@ DEF_GRAPHITE_TEST_FOR_CONTEXTS(KeyEqualityChecksData, reporter, context) {
REPORTER_ASSERT(reporter, !(keyA != keyB));
}

DEF_GRAPHITE_TEST_FOR_CONTEXTS(KeyEqualityDoesNotCheckPointers, reporter, context) {

SkShaderCodeDictionary* dict = context->priv().shaderCodeDictionary();
static const int kBlockDataSize = 4;
static constexpr SkPaintParamsKey::DataPayloadField kDataFields[] = {
{"data", SkPaintParamsKey::DataPayloadType::kByte, kBlockDataSize},
{"ptrIndex", SkPaintParamsKey::DataPayloadType::kPointerIndex, 1},
};

int userSnippetID = dict->addUserDefinedSnippet("key", SkSpan(kDataFields));

static constexpr uint8_t kData[kBlockDataSize] = {1, 2, 3, 4};
int arbitraryData1 = 1;
int arbitraryData2 = 2;

SkPaintParamsKeyBuilder builderA(dict, SkBackend::kGraphite);
SkPaintParamsKeyBuilder builderB(dict, SkBackend::kGraphite);
SkPaintParamsKeyBuilder builderC(dict, SkBackend::kGraphite);
SkPaintParamsKey keyA = create_key_with_ptr(&builderA, userSnippetID, SkSpan(kData),
&arbitraryData1);
SkPaintParamsKey keyB = create_key_with_ptr(&builderB, userSnippetID, SkSpan(kData),
&arbitraryData2);
SkPaintParamsKey keyC = create_key_with_ptr(&builderC, userSnippetID, SkSpan(kData),
nullptr);

// Verify that keyA, keyB, and keyC all match, even though the pointer data does not.
REPORTER_ASSERT(reporter, keyA == keyB);
REPORTER_ASSERT(reporter, keyB == keyC);
REPORTER_ASSERT(reporter, !(keyA != keyB));
REPORTER_ASSERT(reporter, !(keyB != keyC));
}

DEF_GRAPHITE_TEST_FOR_CONTEXTS(KeyBlockReaderWorks, reporter, context) {

SkShaderCodeDictionary* dict = context->priv().shaderCodeDictionary();
Expand Down Expand Up @@ -226,44 +180,3 @@ DEF_GRAPHITE_TEST_FOR_CONTEXTS(KeyBlockReaderWorks, reporter, context) {
REPORTER_ASSERT(reporter, readerBytesZ.size() == kCountZ);
REPORTER_ASSERT(reporter, 0 == memcmp(readerBytesZ.data(), kDataZ, sizeof(kDataZ)));
}

DEF_GRAPHITE_TEST_FOR_CONTEXTS(KeyBlockReaderPointersWork, reporter, context) {

SkShaderCodeDictionary* dict = context->priv().shaderCodeDictionary();
static const int kCountX = 3;
static constexpr SkPaintParamsKey::DataPayloadField kDataFields[] = {
{"PtrIndexA", SkPaintParamsKey::DataPayloadType::kPointerIndex, 1},
{"DataX", SkPaintParamsKey::DataPayloadType::kByte, kCountX},
{"PtrIndexB", SkPaintParamsKey::DataPayloadType::kPointerIndex, 1},
};

int userSnippetID = dict->addUserDefinedSnippet("key", kDataFields);
int arbitraryDataA = 123;
int arbitraryDataB = 456;

static constexpr uint8_t kDataX[kCountX] = {1, 2, 3};
SkPaintParamsKeyBuilder builder(dict, SkBackend::kGraphite);

builder.beginBlock(userSnippetID);
builder.addPointer(&arbitraryDataA);
builder.addBytes(sizeof(kDataX), kDataX);
builder.addPointer(&arbitraryDataB);
builder.endBlock();

SkPaintParamsKey key = builder.lockAsKey();

// Verify that the block reader can extract out our data from the SkPaintParamsKey.
SkPaintParamsKey::BlockReader reader = key.reader(dict, /*headerOffset=*/0);
REPORTER_ASSERT(reporter, reader.blockSize() == (sizeof(SkPaintParamsKey::Header) +
1 + sizeof(kDataX) + 1));

const void* readerPtrA = reader.pointer(0);
REPORTER_ASSERT(reporter, readerPtrA == &arbitraryDataA);

SkSpan<const uint8_t> readerDataX = reader.bytes(1);
REPORTER_ASSERT(reporter, readerDataX.size() == kCountX);
REPORTER_ASSERT(reporter, 0 == memcmp(readerDataX.data(), kDataX, sizeof(kDataX)));

const void* readerPtrB = reader.pointer(2);
REPORTER_ASSERT(reporter, readerPtrB == &arbitraryDataB);
}

0 comments on commit 76f32c9

Please sign in to comment.