Improve draw-time bind group validation messages
Makes it clear that the index being reported by these messages is the
group index and not the binding number. This was a point of confusion
in on the bug.
Additionally, adds more information to the error message regarding
buffer sizes being too small for the current pipeline. Now includes the
pipeline name and buffer size as well as the minimum required size. Also
includes a note explaining that uniform buffer bindings must be a
multiple of 16. (This recently changed and cause several existing
samples to break for non-obvious reasons.)
The error message still does not contain the buffer or binding number,
which would be helpful. This is because we currently lack a way to look
up the binding index from the packed index that this error is generated
with.
Bug: dawn:1604
Change-Id: Ibb2b44bc9e1583ddef34d703e83bcf64ed7a3aa2
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/113602
Commit-Queue: Brandon Jones <bajones@chromium.org>
Auto-Submit: Brandon Jones <bajones@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/native/CommandBufferStateTracker.cpp b/src/dawn/native/CommandBufferStateTracker.cpp
index 6677398..872196d 100644
--- a/src/dawn/native/CommandBufferStateTracker.cpp
+++ b/src/dawn/native/CommandBufferStateTracker.cpp
@@ -14,6 +14,8 @@
#include "dawn/native/CommandBufferStateTracker.h"
+#include <optional>
+
#include "dawn/common/Assert.h"
#include "dawn/common/BitSetIterator.h"
#include "dawn/native/BindGroup.h"
@@ -31,17 +33,21 @@
namespace dawn::native {
namespace {
-bool BufferSizesAtLeastAsBig(const ityp::span<uint32_t, uint64_t> unverifiedBufferSizes,
- const std::vector<uint64_t>& pipelineMinBufferSizes) {
+// Returns nullopt if all buffers in unverifiedBufferSizes are at least large enough to satisfy the
+// minimum listed in pipelineMinBufferSizes, or the index of the first detected failing buffer
+// otherwise.
+std::optional<uint32_t> FindFirstUndersizedBuffer(
+ const ityp::span<uint32_t, uint64_t> unverifiedBufferSizes,
+ const std::vector<uint64_t>& pipelineMinBufferSizes) {
ASSERT(unverifiedBufferSizes.size() == pipelineMinBufferSizes.size());
for (uint32_t i = 0; i < unverifiedBufferSizes.size(); ++i) {
if (unverifiedBufferSizes[i] < pipelineMinBufferSizes[i]) {
- return false;
+ return i;
}
}
- return true;
+ return std::nullopt;
}
} // namespace
@@ -234,8 +240,9 @@
for (BindGroupIndex i : IterateBitSet(mLastPipelineLayout->GetBindGroupLayoutsMask())) {
if (mBindgroups[i] == nullptr ||
mLastPipelineLayout->GetBindGroupLayout(i) != mBindgroups[i]->GetLayout() ||
- !BufferSizesAtLeastAsBig(mBindgroups[i]->GetUnverifiedBufferSizes(),
- (*mMinBufferSizes)[i])) {
+ FindFirstUndersizedBuffer(mBindgroups[i]->GetUnverifiedBufferSizes(),
+ (*mMinBufferSizes)[i])
+ .has_value()) {
matches = false;
break;
}
@@ -315,7 +322,7 @@
for (BindGroupIndex i : IterateBitSet(mLastPipelineLayout->GetBindGroupLayoutsMask())) {
ASSERT(HasPipeline());
- DAWN_INVALID_IF(mBindgroups[i] == nullptr, "No bind group set at index %u.",
+ DAWN_INVALID_IF(mBindgroups[i] == nullptr, "No bind group set at group index %u.",
static_cast<uint32_t>(i));
BindGroupLayoutBase* requiredBGL = mLastPipelineLayout->GetBindGroupLayout(i);
@@ -326,8 +333,8 @@
currentBGL->GetPipelineCompatibilityToken() !=
requiredBGL->GetPipelineCompatibilityToken(),
"The current pipeline (%s) was created with a default layout, and is not "
- "compatible with the %s at index %u which uses a %s that was not created by "
- "the pipeline. Either use the bind group layout returned by calling "
+ "compatible with the %s set at group index %u which uses a %s that was not "
+ "created by the pipeline. Either use the bind group layout returned by calling "
"getBindGroupLayout(%u) on the pipeline when creating the bind group, or "
"provide an explicit pipeline layout when creating the pipeline.",
mLastPipeline, mBindgroups[i], static_cast<uint32_t>(i), currentBGL,
@@ -336,8 +343,9 @@
DAWN_INVALID_IF(
requiredBGL->GetPipelineCompatibilityToken() == PipelineCompatibilityToken(0) &&
currentBGL->GetPipelineCompatibilityToken() != PipelineCompatibilityToken(0),
- "%s at index %u uses a %s which was created as part of the default layout for "
- "a different pipeline than the current one (%s), and as a result is not "
+ "%s set at group index %u uses a %s which was created as part of the default "
+ "layout "
+ "for a different pipeline than the current one (%s), and as a result is not "
"compatible. Use an explicit bind group layout when creating bind groups and "
"an explicit pipeline layout when creating pipelines to share bind groups "
"between pipelines.",
@@ -346,15 +354,27 @@
DAWN_INVALID_IF(
mLastPipelineLayout->GetBindGroupLayout(i) != mBindgroups[i]->GetLayout(),
"Bind group layout %s of pipeline layout %s does not match layout %s of bind "
- "group %s at index %u.",
+ "group %s set at group index %u.",
requiredBGL, mLastPipelineLayout, currentBGL, mBindgroups[i],
static_cast<uint32_t>(i));
- // TODO(dawn:563): Report the binding sizes and which ones are failing.
- DAWN_INVALID_IF(!BufferSizesAtLeastAsBig(mBindgroups[i]->GetUnverifiedBufferSizes(),
- (*mMinBufferSizes)[i]),
- "Binding sizes are too small for bind group %s at index %u",
- mBindgroups[i], static_cast<uint32_t>(i));
+ // TODO(dawn:563): Report which buffer bindings are failing. This requires the ability
+ // to look up the binding index from the packed index.
+ std::optional<uint32_t> packedIndex = FindFirstUndersizedBuffer(
+ mBindgroups[i]->GetUnverifiedBufferSizes(), (*mMinBufferSizes)[i]);
+ if (packedIndex.has_value()) {
+ uint64_t bufferSize =
+ mBindgroups[i]->GetUnverifiedBufferSizes()[packedIndex.value()];
+ uint64_t minBufferSize = (*mMinBufferSizes)[i][packedIndex.value()];
+ return DAWN_VALIDATION_ERROR(
+ "Binding sizes are too small for %s set at group index %u. A bound buffer "
+ "contained %u bytes, but the current pipeline (%s) requires a buffer which is "
+ "at least %u bytes. (Note that uniform buffer bindings must be a multiple of "
+ "16 bytes, and as a result may be larger than the associated data in the "
+ "shader source.)",
+ mBindgroups[i], static_cast<uint32_t>(i), bufferSize, mLastPipeline,
+ minBufferSize);
+ }
}
// The chunk of code above should be similar to the one in |RecomputeLazyAspects|.
