Refactor determining the aspects to clear
This change uses additive logic for determining which aspects to clear,
to improve readability and avoid future bugs in case more aspects can
be cleared during `VK_ATTACHMENT_LOAD_OP_CLEAR` execution.
Note that previously, if present, aspect bits other than COLOR, DEPTH,
and STENCIL would have been preserved. However, the Vulkan spec
explicitly disallows METADATA and MEMORY_PLANE_i aspect bits for
attachments, and PLANE_i would make no sense since multi-planar images
have different sizes of planes, so there's no single set of components
for each pixel to render to if it was used by an attachment, with the
possible exception of 444 formats. SwiftShader doesn't support 444
multi-planar formats though, and implementations that do, don't set the
COLOR_ATTACHMENT feature bit (nor on any other multi-planar format for
that matter). In addition, we have asserts in Image::clear() that check
that only COLOR, DEPTH, and/or STENCIL aspects are cleared. So this is
still a pure refactoring.
An additional assert was added to a missing else clause, for good
measure.
Fixes: b/197695836
Change-Id: I4eb4ba41863e21b028b33c4e4361390cf169e815
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/56629
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
diff --git a/src/Vulkan/VkFramebuffer.cpp b/src/Vulkan/VkFramebuffer.cpp
index e686c13..4235fc1 100644
--- a/src/Vulkan/VkFramebuffer.cpp
+++ b/src/Vulkan/VkFramebuffer.cpp
@@ -81,17 +81,16 @@
for(uint32_t i = 0; i < count; i++)
{
const VkAttachmentDescription attachment = renderPass->getAttachment(i);
- VkImageAspectFlags aspectMask = Format(attachment.format).getAspects();
+ VkImageAspectFlags clearMask = 0;
switch(attachment.loadOp)
{
case VK_ATTACHMENT_LOAD_OP_CLEAR:
- // Keep the color aspect for clearing.
+ clearMask |= VK_IMAGE_ASPECT_COLOR_BIT | VK_IMAGE_ASPECT_DEPTH_BIT;
break;
case VK_ATTACHMENT_LOAD_OP_LOAD:
case VK_ATTACHMENT_LOAD_OP_DONT_CARE:
- // Don't clear the attachment's color aspect.
- aspectMask &= VK_IMAGE_ASPECT_STENCIL_BIT;
+ // Don't clear the attachment's color or depth aspect.
break;
default:
UNSUPPORTED("attachment.loadOp %d", attachment.loadOp);
@@ -100,30 +99,32 @@
switch(attachment.stencilLoadOp)
{
case VK_ATTACHMENT_LOAD_OP_CLEAR:
- // Keep the stencil aspect for clearing.
+ clearMask |= VK_IMAGE_ASPECT_STENCIL_BIT;
break;
case VK_ATTACHMENT_LOAD_OP_LOAD:
case VK_ATTACHMENT_LOAD_OP_DONT_CARE:
// Don't clear the attachment's stencil aspect.
- aspectMask &= ~VK_IMAGE_ASPECT_STENCIL_BIT;
break;
default:
UNSUPPORTED("attachment.stencilLoadOp %d", attachment.stencilLoadOp);
}
- if(!aspectMask || !renderPass->isAttachmentUsed(i))
+ // Image::clear() demands that we only specify existing aspects.
+ clearMask &= Format(attachment.format).getAspects();
+
+ if(!clearMask || !renderPass->isAttachmentUsed(i))
{
continue;
}
if(renderPass->isMultiView())
{
- attachments[i]->clearWithLayerMask(pClearValues[i], aspectMask, renderArea,
+ attachments[i]->clearWithLayerMask(pClearValues[i], clearMask, renderArea,
renderPass->getAttachmentViewMask(i));
}
else
{
- attachments[i]->clear(pClearValues[i], aspectMask, renderArea);
+ attachments[i]->clear(pClearValues[i], clearMask, renderArea);
}
}
}
@@ -173,6 +174,8 @@
}
}
}
+ else
+ UNSUPPORTED("attachment.aspectMask %X", attachment.aspectMask);
}
void Framebuffer::setAttachment(ImageView *imageView, uint32_t index)
