Add assert on illegal signed/unsigned blit
The goal of this CL is to add an assert to catch any illegal blit
between signed and unsigned formats. Because the clear code also
uses this path, the clear color must also have the same signedness
as the destination color for integer formats.
Bug: b/202978657
Change-Id: I1d4576489042e3e1772190585fb035bee1635c16
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/58228
Presubmit-Ready: Alexis Hétu <sugoi@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Alexis Hétu <sugoi@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
Commit-Queue: Alexis Hétu <sugoi@google.com>
diff --git a/src/Device/Blitter.cpp b/src/Device/Blitter.cpp
index 3d74428..4f823fd 100644
--- a/src/Device/Blitter.cpp
+++ b/src/Device/Blitter.cpp
@@ -55,7 +55,7 @@
{
}
-void Blitter::clear(void *pixel, vk::Format format, vk::Image *dest, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea)
+void Blitter::clear(const void *pixel, vk::Format format, vk::Image *dest, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea)
{
VkImageAspectFlagBits aspect = static_cast<VkImageAspectFlagBits>(subresourceRange.aspectMask);
vk::Format dstFormat = viewFormat.getAspectFormat(aspect);
@@ -64,20 +64,16 @@
return;
}
- float *pPixel = static_cast<float *>(pixel);
- if(viewFormat.isUnsignedNormalized() || viewFormat.isSRGBformat())
+ VkClearColorValue clampedPixel;
+ if(viewFormat.isSignedNormalized() || viewFormat.isUnsignedNormalized() || viewFormat.isSRGBformat())
{
- pPixel[0] = sw::clamp(pPixel[0], 0.0f, 1.0f);
- pPixel[1] = sw::clamp(pPixel[1], 0.0f, 1.0f);
- pPixel[2] = sw::clamp(pPixel[2], 0.0f, 1.0f);
- pPixel[3] = sw::clamp(pPixel[3], 0.0f, 1.0f);
- }
- else if(viewFormat.isSignedNormalized())
- {
- pPixel[0] = sw::clamp(pPixel[0], -1.0f, 1.0f);
- pPixel[1] = sw::clamp(pPixel[1], -1.0f, 1.0f);
- pPixel[2] = sw::clamp(pPixel[2], -1.0f, 1.0f);
- pPixel[3] = sw::clamp(pPixel[3], -1.0f, 1.0f);
+ const float minValue = viewFormat.isSignedNormalized() ? -1.0f : 0.0f;
+ memcpy(clampedPixel.float32, pixel, sizeof(VkClearColorValue));
+ clampedPixel.float32[0] = sw::clamp(clampedPixel.float32[0], minValue, 1.0f);
+ clampedPixel.float32[1] = sw::clamp(clampedPixel.float32[1], minValue, 1.0f);
+ clampedPixel.float32[2] = sw::clamp(clampedPixel.float32[2], minValue, 1.0f);
+ clampedPixel.float32[3] = sw::clamp(clampedPixel.float32[3], minValue, 1.0f);
+ pixel = clampedPixel.float32;
}
if(fastClear(pixel, format, dest, dstFormat, subresourceRange, renderArea))
@@ -162,7 +158,7 @@
dest->contentsChanged(subresourceRange);
}
-bool Blitter::fastClear(void *clearValue, vk::Format clearFormat, vk::Image *dest, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea)
+bool Blitter::fastClear(const void *clearValue, vk::Format clearFormat, vk::Image *dest, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea)
{
if(clearFormat != VK_FORMAT_R32G32B32A32_SFLOAT &&
clearFormat != VK_FORMAT_D32_SFLOAT &&
@@ -189,7 +185,7 @@
uint32_t s;
};
- ClearValue &c = *reinterpret_cast<ClearValue *>(clearValue);
+ const ClearValue &c = *reinterpret_cast<const ClearValue *>(clearValue);
uint32_t packed = 0;
@@ -1183,6 +1179,8 @@
bool writeA = state.writeAlpha;
bool writeRGBA = writeR && writeG && writeB && writeA;
+ ASSERT(state.sourceFormat.isUnsigned() == state.destFormat.isUnsigned());
+
switch(state.destFormat)
{
case VK_FORMAT_A2B10G10R10_UINT_PACK32:
@@ -1471,8 +1469,7 @@
state.destFormat.isUnsignedComponent(3) ? 0.0f : -scale.w));
}
- // TODO(b/203068380): create proper functions to check for signedness
- if(!state.sourceFormat.isUnsignedComponent(0) && state.destFormat.isUnsignedComponent(0))
+ if(!state.sourceFormat.isUnsigned() && state.destFormat.isUnsigned())
{
value = Max(value, Float4(0.0f));
}
diff --git a/src/Device/Blitter.hpp b/src/Device/Blitter.hpp
index 3792992..6ff900f 100644
--- a/src/Device/Blitter.hpp
+++ b/src/Device/Blitter.hpp
@@ -102,7 +102,7 @@
struct BlitData
{
- void *source;
+ const void *source;
void *dest;
int sPitchB;
int dPitchB;
@@ -142,7 +142,7 @@
Blitter();
virtual ~Blitter();
- void clear(void *clearValue, vk::Format clearFormat, vk::Image *dest, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea = nullptr);
+ void clear(const void *clearValue, vk::Format clearFormat, vk::Image *dest, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea = nullptr);
void blit(const vk::Image *src, vk::Image *dst, VkImageBlit region, VkFilter filter);
void resolve(const vk::Image *src, vk::Image *dst, VkImageResolve region);
@@ -160,7 +160,7 @@
LEFT
};
- bool fastClear(void *clearValue, vk::Format clearFormat, vk::Image *dest, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea);
+ bool fastClear(const void *clearValue, vk::Format clearFormat, vk::Image *dest, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea);
bool fastResolve(const vk::Image *src, vk::Image *dst, VkImageResolve region);
Float4 readFloat4(Pointer<Byte> element, const State &state);
diff --git a/src/System/Half.hpp b/src/System/Half.hpp
index 4576e4f..d25b33e 100644
--- a/src/System/Half.hpp
+++ b/src/System/Half.hpp
@@ -57,7 +57,7 @@
unsigned int E : 5;
public:
- RGB9E5(float rgb[3])
+ RGB9E5(const float rgb[3])
: RGB9E5(rgb[0], rgb[1], rgb[2])
{
}
@@ -128,7 +128,7 @@
class R11G11B10F
{
public:
- R11G11B10F(float rgb[3])
+ R11G11B10F(const float rgb[3])
{
R = float32ToFloat11(rgb[0]);
G = float32ToFloat11(rgb[1]);
diff --git a/src/Vulkan/VkFormat.cpp b/src/Vulkan/VkFormat.cpp
index 968a6c0..16cc6e8 100644
--- a/src/Vulkan/VkFormat.cpp
+++ b/src/Vulkan/VkFormat.cpp
@@ -256,6 +256,22 @@
return format;
}
+VkFormat Format::getClearFormat() const
+{
+ // Set the proper format for the clear value, as described here:
+ // https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#clears-values
+ if(isSignedUnnormalizedInteger())
+ {
+ return VK_FORMAT_R32G32B32A32_SINT;
+ }
+ else if(isUnsignedUnnormalizedInteger())
+ {
+ return VK_FORMAT_R32G32B32A32_UINT;
+ }
+
+ return VK_FORMAT_R32G32B32A32_SFLOAT;
+}
+
bool Format::isStencil() const
{
switch(format)
@@ -1311,6 +1327,12 @@
return 1;
}
+bool Format::isUnsigned() const
+{
+ // TODO(b/203068380): create a proper check for signedness
+ return isUnsignedComponent(0);
+}
+
bool Format::isUnsignedComponent(int component) const
{
switch(format)
diff --git a/src/Vulkan/VkFormat.hpp b/src/Vulkan/VkFormat.hpp
index dd26314..89e9284 100644
--- a/src/Vulkan/VkFormat.hpp
+++ b/src/Vulkan/VkFormat.hpp
@@ -35,9 +35,11 @@
bool isSignedUnnormalizedInteger() const;
bool isUnsignedUnnormalizedInteger() const;
bool isUnnormalizedInteger() const;
+ bool isUnsigned() const;
VkImageAspectFlags getAspects() const;
Format getAspectFormat(VkImageAspectFlags aspect) const;
+ VkFormat getClearFormat() const;
bool isStencil() const;
bool isDepth() const;
bool isSRGBformat() const;
diff --git a/src/Vulkan/VkImage.cpp b/src/Vulkan/VkImage.cpp
index 7adb5e3..9fb7ac4 100644
--- a/src/Vulkan/VkImage.cpp
+++ b/src/Vulkan/VkImage.cpp
@@ -1038,22 +1038,6 @@
device->getBlitter()->resolveDepthStencil(src, dst, dsResolve);
}
-VkFormat Image::getClearFormat() const
-{
- // Set the proper format for the clear value, as described here:
- // https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#clears-values
- if(format.isSignedUnnormalizedInteger())
- {
- return VK_FORMAT_R32G32B32A32_SINT;
- }
- else if(format.isUnsignedUnnormalizedInteger())
- {
- return VK_FORMAT_R32G32B32A32_UINT;
- }
-
- return VK_FORMAT_R32G32B32A32_SFLOAT;
-}
-
uint32_t Image::getLastLayerIndex(const VkImageSubresourceRange &subresourceRange) const
{
return ((subresourceRange.layerCount == VK_REMAINING_ARRAY_LAYERS) ? arrayLayers : (subresourceRange.baseArrayLayer + subresourceRange.layerCount)) - 1;
@@ -1064,16 +1048,16 @@
return ((subresourceRange.levelCount == VK_REMAINING_MIP_LEVELS) ? mipLevels : (subresourceRange.baseMipLevel + subresourceRange.levelCount)) - 1;
}
-void Image::clear(void *pixelData, VkFormat pixelFormat, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D &renderArea)
+void Image::clear(const void *pixelData, VkFormat pixelFormat, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea)
{
- device->getBlitter()->clear(pixelData, pixelFormat, this, viewFormat, subresourceRange, &renderArea);
+ device->getBlitter()->clear(pixelData, pixelFormat, this, viewFormat, subresourceRange, renderArea);
}
void Image::clear(const VkClearColorValue &color, const VkImageSubresourceRange &subresourceRange)
{
ASSERT(subresourceRange.aspectMask == VK_IMAGE_ASPECT_COLOR_BIT);
- device->getBlitter()->clear((void *)color.float32, getClearFormat(), this, format, subresourceRange);
+ clear(color.float32, format.getClearFormat(), format, subresourceRange, nullptr);
}
void Image::clear(const VkClearDepthStencilValue &color, const VkImageSubresourceRange &subresourceRange)
@@ -1085,14 +1069,14 @@
{
VkImageSubresourceRange depthSubresourceRange = subresourceRange;
depthSubresourceRange.aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT;
- device->getBlitter()->clear((void *)(&color.depth), VK_FORMAT_D32_SFLOAT, this, format, depthSubresourceRange);
+ clear(&color.depth, VK_FORMAT_D32_SFLOAT, format, depthSubresourceRange, nullptr);
}
if(subresourceRange.aspectMask & VK_IMAGE_ASPECT_STENCIL_BIT)
{
VkImageSubresourceRange stencilSubresourceRange = subresourceRange;
stencilSubresourceRange.aspectMask = VK_IMAGE_ASPECT_STENCIL_BIT;
- device->getBlitter()->clear((void *)(&color.stencil), VK_FORMAT_S8_UINT, this, format, stencilSubresourceRange);
+ clear(&color.stencil, VK_FORMAT_S8_UINT, format, stencilSubresourceRange, nullptr);
}
}
@@ -1104,7 +1088,7 @@
if(subresourceRange.aspectMask == VK_IMAGE_ASPECT_COLOR_BIT)
{
- clear((void *)(clearValue.color.float32), getClearFormat(), viewFormat, subresourceRange, renderArea);
+ clear(clearValue.color.float32, viewFormat.getClearFormat(), viewFormat, subresourceRange, &renderArea);
}
else
{
@@ -1112,14 +1096,14 @@
{
VkImageSubresourceRange depthSubresourceRange = subresourceRange;
depthSubresourceRange.aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT;
- clear((void *)(&clearValue.depthStencil.depth), VK_FORMAT_D32_SFLOAT, viewFormat, depthSubresourceRange, renderArea);
+ clear(&clearValue.depthStencil.depth, VK_FORMAT_D32_SFLOAT, viewFormat, depthSubresourceRange, &renderArea);
}
if(subresourceRange.aspectMask & VK_IMAGE_ASPECT_STENCIL_BIT)
{
VkImageSubresourceRange stencilSubresourceRange = subresourceRange;
stencilSubresourceRange.aspectMask = VK_IMAGE_ASPECT_STENCIL_BIT;
- clear((void *)(&clearValue.depthStencil.stencil), VK_FORMAT_S8_UINT, viewFormat, stencilSubresourceRange, renderArea);
+ clear(&clearValue.depthStencil.stencil, VK_FORMAT_S8_UINT, viewFormat, stencilSubresourceRange, &renderArea);
}
}
}
diff --git a/src/Vulkan/VkImage.hpp b/src/Vulkan/VkImage.hpp
index 2c89be2..aef05eb 100644
--- a/src/Vulkan/VkImage.hpp
+++ b/src/Vulkan/VkImage.hpp
@@ -131,8 +131,7 @@
VkExtent3D imageExtentInBlocks(const VkExtent3D &extent, VkImageAspectFlagBits aspect) const;
VkOffset3D imageOffsetInBlocks(const VkOffset3D &offset, VkImageAspectFlagBits aspect) const;
VkExtent2D bufferExtentInBlocks(const VkExtent2D &extent, const VkBufferImageCopy ®ion) const;
- VkFormat getClearFormat() const;
- void clear(void *pixelData, VkFormat pixelFormat, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D &renderArea);
+ void clear(const void *pixelData, VkFormat pixelFormat, const vk::Format &viewFormat, const VkImageSubresourceRange &subresourceRange, const VkRect2D *renderArea);
int borderSize() const;
bool requiresPreprocessing() const;
