Add padding to properly align mixed-size uniforms.
The UniformManager would previously assert if it detected an alignment
error, but did not actually have any mechanism for automatically adding
padding between misaligned elements. If a user specifies a uniform
layout like `uniform float a; uniform float4 b;`, we now insert padding
between `a` and `b` so that `b` will be properly aligned. (This reuses
the logic that was originally used to trigger the alignment assertion.)
This CL fixes the bug and adds a test. The test is only active for
elements of matching size; mixed half- and full-precision elements
will need improved test logic.
Change-Id: I8c04eb6350fa73bdbcd1a08e1a45b17fee0d4194
Bug: skia:13478
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/555440
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
diff --git a/src/gpu/graphite/UniformManager.cpp b/src/gpu/graphite/UniformManager.cpp
index cddb62b..d9145c7 100644
--- a/src/gpu/graphite/UniformManager.cpp
+++ b/src/gpu/graphite/UniformManager.cpp
@@ -329,7 +329,6 @@
}
};
-#ifdef SK_DEBUG
// To determine whether a current offset is aligned, we can just 'and' the lowest bits with the
// alignment mask. A value of 0 means aligned, any other value is how many bytes past alignment we
// are. This works since all alignments are powers of 2. The mask is always (alignment - 1).
@@ -468,19 +467,20 @@
// taking into consideration all alignment requirements. The uniformOffset is set to the offset for
// the new uniform, and currentOffset is updated to be the offset to the end of the new uniform.
static uint32_t get_ubo_aligned_offset(uint32_t* currentOffset,
- uint32_t* maxAlignment,
SkSLType type,
int arrayCount) {
+ // TODO(skia:13478): these alignments are currently Metal-specific; we are likely to need
+ // fixes for other Layout types.
uint32_t alignmentMask = sksltype_to_alignment_mask(type);
- if (alignmentMask > *maxAlignment) {
- *maxAlignment = alignmentMask;
- }
uint32_t offsetDiff = *currentOffset & alignmentMask;
if (offsetDiff != 0) {
offsetDiff = alignmentMask - offsetDiff + 1;
}
uint32_t uniformOffset = *currentOffset + offsetDiff;
SkASSERT(sizeof(float) == 4);
+
+ // TODO(skia:13478): these size calculations are currently Metal-specific; we are likely to need
+ // fixes for other Layout types.
if (arrayCount) {
*currentOffset = uniformOffset + sksltype_to_mtl_size(type) * arrayCount;
} else {
@@ -488,7 +488,6 @@
}
return uniformOffset;
}
-#endif // SK_DEBUG
SkSLType UniformManager::getUniformTypeForLayout(SkSLType type) {
if (fLayout != Layout::kMetal) {
@@ -543,17 +542,13 @@
}
void UniformManager::reset() {
-#ifdef SK_DEBUG
fCurUBOOffset = 0;
- fCurUBOMaxAlignment = 0;
-#endif
fOffset = 0;
fStorage.rewind();
}
void UniformManager::checkReset() const {
SkASSERT(fCurUBOOffset == 0);
- SkASSERT(fCurUBOMaxAlignment == 0);
SkASSERT(fOffset == 0);
SkASSERT(fStorage.empty());
}
@@ -570,17 +565,7 @@
SkASSERT(fExpectedUniforms[fExpectedUniformIndex].type() == type);
SkASSERT((fExpectedUniforms[fExpectedUniformIndex].count() == 0 && count == 1) ||
fExpectedUniforms[fExpectedUniformIndex].count() == count);
-#ifdef SK_DEBUG
- fExpectedUniformIndex++;
-
- SkSLType revisedType = this->getUniformTypeForLayout(type);
-
- uint32_t debugOffset = get_ubo_aligned_offset(&fCurUBOOffset,
- &fCurUBOMaxAlignment,
- revisedType,
- count);
- SkASSERT(debugOffset == fOffset);
-#endif
+ SkDEBUGCODE(fExpectedUniformIndex++;)
}
void UniformManager::doneWithExpectedUniforms() {
@@ -589,10 +574,18 @@
}
void UniformManager::write(SkSLType type, unsigned int count, const void* src) {
- SkDEBUGCODE(this->checkExpected(type, (count == SkUniform::kNonArray) ? 1 : count);)
+ this->checkExpected(type, (count == SkUniform::kNonArray) ? 1 : count);
SkSLType revisedType = this->getUniformTypeForLayout(type);
+ // Insert padding as needed to get the correct uniform alignment.
+ uint32_t alignedOffset = get_ubo_aligned_offset(&fCurUBOOffset, revisedType, count);
+ SkASSERT(alignedOffset >= fOffset);
+ if (alignedOffset > fOffset) {
+ fStorage.append(alignedOffset - fOffset);
+ fOffset = alignedOffset;
+ }
+
uint32_t bytesNeeded = fWriteUniform(revisedType, CType::kDefault,
/*dest=*/nullptr, count, /*src=*/nullptr);
char* dst = fStorage.append(bytesNeeded);
diff --git a/src/gpu/graphite/UniformManager.h b/src/gpu/graphite/UniformManager.h
index 7d6d08e..02fe70c 100644
--- a/src/gpu/graphite/UniformManager.h
+++ b/src/gpu/graphite/UniformManager.h
@@ -71,14 +71,12 @@
WriteUniformFn fWriteUniform;
Layout fLayout; // TODO: eventually 'fLayout' will not need to be stored
+ uint32_t fCurUBOOffset = 0;
#ifdef SK_DEBUG
- uint32_t fCurUBOOffset;
- uint32_t fCurUBOMaxAlignment;
-
SkSpan<const SkUniform> fExpectedUniforms;
int fExpectedUniformIndex = 0;
#endif // SK_DEBUG
- uint32_t fOffset;
+ uint32_t fOffset = 0;
SkTDArray<char> fStorage;
};
diff --git a/tests/graphite/UniformManagerTest.cpp b/tests/graphite/UniformManagerTest.cpp
index 1fcf6c4..0f1b8af 100644
--- a/tests/graphite/UniformManagerTest.cpp
+++ b/tests/graphite/UniformManagerTest.cpp
@@ -193,3 +193,56 @@
}
}
}
+
+DEF_TEST(UniformManagerCheckPaddingScalarVector, r) {
+ // Verify that the uniform manager properly adds padding between pairs of scalar/vector.
+ // This test doesn't check mixed 16/32-bit padding yet.
+ for (Layout layout : kLayouts) {
+ UniformManager mgr(layout);
+
+ for (SkSLType type1 : kTypes) {
+ const int vecLength1 = SkSLTypeVecLength(type1);
+ if (vecLength1 < 1) {
+ continue;
+ }
+
+ for (SkSLType type2 : kTypes) {
+ const int vecLength2 = SkSLTypeVecLength(type2);
+ if (vecLength2 < 1) {
+ continue;
+ }
+
+ // Write two scalar/vector uniforms.
+ const SkUniform expectations[] = {{"a", type1}, {"b", type2}};
+ mgr.setExpectedUniforms(SkSpan(expectations));
+ mgr.write(type1, SkUniform::kNonArray, kFloats);
+ mgr.write(type2, SkUniform::kNonArray, kFloats);
+ mgr.doneWithExpectedUniforms();
+
+ // Verify that all elements are the same size.
+ const size_t elementSize1 = element_size(layout, type1);
+ const size_t elementSize2 = element_size(layout, type2);
+ if (elementSize1 == elementSize2) {
+ // The expected uniform layout is listed as strings below.
+ // A/B: uniform values.
+ // a/b: padding as part of the uniform type (vec3 takes 4 slots)
+ // _ : padding between uniforms for alignment
+ static constexpr const char* kExpectedLayout[5][5] = {
+ { "", "", "", "", "" },
+ { "", "AB", "A_BB", "A___BBBb", "A___BBBB" },
+ { "", "AAB", "AABB", "AA__BBBb", "AA__BBBB" },
+ { "", "AAAaB", "AAAaBB", "AAAaBBBb", "AAAaBBBB" },
+ { "", "AAAAB", "AAAABB", "AAAABBBb", "AAAABBBB" },
+ };
+ const size_t size = strlen(kExpectedLayout[vecLength1][vecLength2]) *
+ elementSize1;
+ SkUniformDataBlock uniformData = mgr.peekData();
+ REPORTER_ASSERT(r, uniformData.size() == size,
+ "Layout:%d Types:%d %d padding test failed",
+ (int)layout, (int)type1, (int)type2);
+ }
+ mgr.reset();
+ }
+ }
+ }
+}
