Fix FP blending path to not make clamping assumptions
We had assumed in Context in various places that negative pixel values
would be clamped to zero, which is incorrect for floating-point color
attachments.
TODO: on the assumption that there is value in these blend optimizations
themselves, we should split the context blend state to be
per-attachment. This would give us the `independentBlend` optional
feature, and also allow us to keep the optimization for those
attachments which do have clamping in a mixed MRT scenario.
Bug: b/132433217
Test: dEQP-VK.pipeline.blend.format.r16*
Change-Id: I5434475aba5d21a8c8342b42cccc575b517f0900
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/31050
Tested-by: Chris Forbes <chrisforbes@google.com>
Presubmit-Ready: Chris Forbes <chrisforbes@google.com>
Kokoro-Presubmit: kokoro <noreply+kokoro@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/Device/Context.cpp b/src/Device/Context.cpp
index b2d6734..b6d811e 100644
--- a/src/Device/Context.cpp
+++ b/src/Device/Context.cpp
@@ -324,6 +324,20 @@
return destBlendFactorState;
}
+ bool Context::allTargetsColorClamp() const
+ {
+ // TODO: remove all of this and support VkPhysicalDeviceFeatures::independentBlend instead
+ for (int i = 0; i < RENDERTARGETS; i++)
+ {
+ if (renderTarget[i] && renderTarget[i]->getFormat().isFloatFormat())
+ {
+ return false;
+ }
+ }
+
+ return true;
+ }
+
VkBlendOp Context::blendOperation() const
{
if(!alphaBlendEnable) return VK_BLEND_OP_SRC_EXT;
@@ -365,7 +379,7 @@
}
}
case VK_BLEND_OP_SUBTRACT:
- if(sourceBlendFactor() == VK_BLEND_FACTOR_ZERO)
+ if(sourceBlendFactor() == VK_BLEND_FACTOR_ZERO && allTargetsColorClamp())
{
return VK_BLEND_OP_ZERO_EXT; // Negative, clamped to zero
}
@@ -405,7 +419,7 @@
}
else if(sourceBlendFactor() == VK_BLEND_FACTOR_ONE)
{
- if(destBlendFactor() == VK_BLEND_FACTOR_ZERO)
+ if(destBlendFactor() == VK_BLEND_FACTOR_ZERO && allTargetsColorClamp())
{
return VK_BLEND_OP_ZERO_EXT; // Negative, clamped to zero
}
@@ -416,7 +430,7 @@
}
else
{
- if(destBlendFactor() == VK_BLEND_FACTOR_ZERO)
+ if(destBlendFactor() == VK_BLEND_FACTOR_ZERO && allTargetsColorClamp())
{
return VK_BLEND_OP_ZERO_EXT; // Negative, clamped to zero
}
@@ -533,7 +547,7 @@
}
}
case VK_BLEND_OP_SUBTRACT:
- if(sourceBlendFactorAlpha() == VK_BLEND_FACTOR_ZERO)
+ if(sourceBlendFactorAlpha() == VK_BLEND_FACTOR_ZERO && allTargetsColorClamp())
{
return VK_BLEND_OP_ZERO_EXT; // Negative, clamped to zero
}
@@ -573,7 +587,7 @@
}
else if(sourceBlendFactorAlpha() == VK_BLEND_FACTOR_ONE)
{
- if(destBlendFactorAlpha() == VK_BLEND_FACTOR_ZERO)
+ if(destBlendFactorAlpha() == VK_BLEND_FACTOR_ZERO && allTargetsColorClamp())
{
return VK_BLEND_OP_ZERO_EXT; // Negative, clamped to zero
}
@@ -584,7 +598,7 @@
}
else
{
- if(destBlendFactorAlpha() == VK_BLEND_FACTOR_ZERO)
+ if(destBlendFactorAlpha() == VK_BLEND_FACTOR_ZERO && allTargetsColorClamp())
{
return VK_BLEND_OP_ZERO_EXT; // Negative, clamped to zero
}
diff --git a/src/Device/Context.hpp b/src/Device/Context.hpp
index 6e9408e..fcbf812 100644
--- a/src/Device/Context.hpp
+++ b/src/Device/Context.hpp
@@ -123,6 +123,7 @@
bool perspectiveActive() const;
+ bool allTargetsColorClamp() const;
bool alphaBlendActive() const;
VkBlendFactor sourceBlendFactor() const;
VkBlendFactor destBlendFactor() const;
diff --git a/src/Pipeline/PixelRoutine.cpp b/src/Pipeline/PixelRoutine.cpp
index 616647c..c533485 100644
--- a/src/Pipeline/PixelRoutine.cpp
+++ b/src/Pipeline/PixelRoutine.cpp
@@ -1672,10 +1672,14 @@
switch(blendFactorActive)
{
case VK_BLEND_FACTOR_ZERO:
- // Optimized
+ blendFactor.x = Float4(0);
+ blendFactor.y = Float4(0);
+ blendFactor.z = Float4(0);
break;
case VK_BLEND_FACTOR_ONE:
- // Optimized
+ blendFactor.x = Float4(1);
+ blendFactor.y = Float4(1);
+ blendFactor.z = Float4(1);
break;
case VK_BLEND_FACTOR_SRC_COLOR:
blendFactor.x = oC.x;
@@ -1754,10 +1758,10 @@
switch(blendFactorAlphaActive)
{
case VK_BLEND_FACTOR_ZERO:
- // Optimized
+ blendFactor.w = Float4(0);
break;
case VK_BLEND_FACTOR_ONE:
- // Optimized
+ blendFactor.w = Float4(1);
break;
case VK_BLEND_FACTOR_SRC_COLOR:
blendFactor.w = oC.w;
@@ -1928,19 +1932,13 @@
blendFactor(sourceFactor, oC, pixel, state.sourceBlendFactor);
blendFactor(destFactor, oC, pixel, state.destBlendFactor);
- if(state.sourceBlendFactor != VK_BLEND_FACTOR_ONE && state.sourceBlendFactor != VK_BLEND_FACTOR_ZERO)
- {
- oC.x *= sourceFactor.x;
- oC.y *= sourceFactor.y;
- oC.z *= sourceFactor.z;
- }
+ oC.x *= sourceFactor.x;
+ oC.y *= sourceFactor.y;
+ oC.z *= sourceFactor.z;
- if(state.destBlendFactor != VK_BLEND_FACTOR_ONE && state.destBlendFactor != VK_BLEND_FACTOR_ZERO)
- {
- pixel.x *= destFactor.x;
- pixel.y *= destFactor.y;
- pixel.z *= destFactor.z;
- }
+ pixel.x *= destFactor.x;
+ pixel.y *= destFactor.y;
+ pixel.z *= destFactor.z;
switch(state.blendOperation)
{
@@ -1989,15 +1987,8 @@
blendFactorAlpha(sourceFactor, oC, pixel, state.sourceBlendFactorAlpha);
blendFactorAlpha(destFactor, oC, pixel, state.destBlendFactorAlpha);
- if(state.sourceBlendFactorAlpha != VK_BLEND_FACTOR_ONE && state.sourceBlendFactorAlpha != VK_BLEND_FACTOR_ZERO)
- {
- oC.w *= sourceFactor.w;
- }
-
- if(state.destBlendFactorAlpha != VK_BLEND_FACTOR_ONE && state.destBlendFactorAlpha != VK_BLEND_FACTOR_ZERO)
- {
- pixel.w *= destFactor.w;
- }
+ oC.w *= sourceFactor.w;
+ pixel.w *= destFactor.w;
switch(state.blendOperationAlpha)
{