Fix color mask setting up in pipeline state

Summary: Fix color mask setting up in pipeline state NOTE: In Vulkan color write bits are part of blending. Some discussion - KhronosGroup/Vulkan-Docs#241 Reviewed By: corporateshark Differential Revision: D52217287 fbshipit-source-id: 67981b0bf41b1453871652e4d6d955b414fee4ca
facebook · Dec 19, 2023 · ee21844 · ee21844
1 parent f4f3ce4
commit ee21844
Show file tree

Hide file tree

Showing 9 changed files with 50 additions and 33 deletions.
diff --git a/src/igl/RenderPipelineState.cpp b/src/igl/RenderPipelineState.cpp
@@ -22,7 +22,7 @@ bool RenderPipelineDesc::TargetDesc::ColorAttachment::operator!=(
 
 bool RenderPipelineDesc::TargetDesc::ColorAttachment::operator==(
     const ColorAttachment& other) const {
-  return (textureFormat == other.textureFormat && colorWriteBits == other.colorWriteBits &&
+  return (textureFormat == other.textureFormat && colorWriteMask == other.colorWriteMask &&
           blendEnabled == other.blendEnabled && rgbBlendOp == other.rgbBlendOp &&
           alphaBlendOp == other.alphaBlendOp && srcRGBBlendFactor == other.srcRGBBlendFactor &&
           srcAlphaBlendFactor == other.srcAlphaBlendFactor &&
@@ -138,7 +138,7 @@ size_t std::hash<RenderPipelineDesc::TargetDesc>::operator()(
 size_t std::hash<RenderPipelineDesc::TargetDesc::ColorAttachment>::operator()(
     RenderPipelineDesc::TargetDesc::ColorAttachment const& key) const {
   size_t hash = std::hash<int>()(EnumToValue(key.textureFormat));
-  hash ^= std::hash<uint8_t>()(key.colorWriteBits);
+  hash ^= std::hash<uint8_t>()(key.colorWriteMask);
   hash ^= std::hash<bool>()(key.blendEnabled);
   hash ^= std::hash<int>()(EnumToValue(key.rgbBlendOp));
   hash ^= std::hash<int>()(EnumToValue(key.alphaBlendOp));

diff --git a/src/igl/RenderPipelineState.h b/src/igl/RenderPipelineState.h
@@ -97,14 +97,15 @@ enum class BlendFactor : uint8_t {
  * value. The values red, green, blue, and alpha select one color channel each, and they can be
  * bitwise combined.
  */
-using ColorWriteBits = uint8_t;
-enum class ColorWriteMask : ColorWriteBits {
-  Disabled = 0,
-  Red = 1 << 0,
-  Green = 1 << 1,
-  Blue = 1 << 2,
-  Alpha = 1 << 3,
-  All = Red | Green | Blue | Alpha,
+using ColorWriteMask = uint8_t;
+enum ColorWriteBits : uint8_t {
+  ColorWriteBitsDisabled = 0,
+  ColorWriteBitsRed = 1 << 0,
+  ColorWriteBitsGreen = 1 << 1,
+  ColorWriteBitsBlue = 1 << 2,
+  ColorWriteBitsAlpha = 1 << 3,
+  ColorWriteBitsAll =
+      ColorWriteBitsRed | ColorWriteBitsGreen | ColorWriteBitsBlue | ColorWriteBitsAlpha,
 };
 
 /*
@@ -130,7 +131,7 @@ struct RenderPipelineDesc {
       /*
        * @brief Identify which color channels are blended.
        */
-      ColorWriteBits colorWriteBits = EnumToValue(ColorWriteMask::All);
+      ColorWriteMask colorWriteMask = ColorWriteBitsAll;
       bool blendEnabled = false;
       /*
        * @brief Assign the blend operations for RGB and alpha pixel data.

diff --git a/src/igl/metal/Device.mm b/src/igl/metal/Device.mm
@@ -414,7 +414,7 @@
     auto& src = desc.targetDesc.colorAttachments[i];
     MTLRenderPipelineColorAttachmentDescriptor* dst = metalDesc.colorAttachments[i];
     dst.pixelFormat = Texture::textureFormatToMTLPixelFormat(src.textureFormat);
-    dst.writeMask = RenderPipelineState::convertColorWriteMask(src.colorWriteBits);
+    dst.writeMask = RenderPipelineState::convertColorWriteMask(src.colorWriteMask);
     dst.blendingEnabled = src.blendEnabled;
     dst.rgbBlendOperation = MTLBlendOperation(src.rgbBlendOp);
     dst.alphaBlendOperation = MTLBlendOperation(src.alphaBlendOp);

diff --git a/src/igl/metal/RenderPipelineState.h b/src/igl/metal/RenderPipelineState.h
@@ -43,7 +43,7 @@ class RenderPipelineState final : public IRenderPipelineState {
   PolygonFillMode getPolygonFillMode() const {
     return desc_.polygonFillMode;
   }
-  static MTLColorWriteMask convertColorWriteMask(ColorWriteBits value);
+  static MTLColorWriteMask convertColorWriteMask(ColorWriteMask value);
 
  private:
   RenderPipelineDesc desc_;

diff --git a/src/igl/metal/RenderPipelineState.mm b/src/igl/metal/RenderPipelineState.mm
@@ -44,18 +44,18 @@
   return reflection_->getIndexByName(name, stage);
 }
 
-MTLColorWriteMask RenderPipelineState::convertColorWriteMask(ColorWriteBits value) {
+MTLColorWriteMask RenderPipelineState::convertColorWriteMask(ColorWriteMask value) {
   MTLColorWriteMask result = MTLColorWriteMaskNone;
-  if (value & EnumToValue(ColorWriteMask::Red)) {
+  if (value & ColorWriteBitsRed) {
     result |= MTLColorWriteMaskRed;
   }
-  if (value & EnumToValue(ColorWriteMask::Green)) {
+  if (value & ColorWriteBitsGreen) {
     result |= MTLColorWriteMaskGreen;
   }
-  if (value & EnumToValue(ColorWriteMask::Blue)) {
+  if (value & ColorWriteBitsBlue) {
     result |= MTLColorWriteMaskBlue;
   }
-  if (value & EnumToValue(ColorWriteMask::Alpha)) {
+  if (value & ColorWriteBitsAlpha) {
     result |= MTLColorWriteMaskAlpha;
   }
   return result;

diff --git a/src/igl/opengl/RenderPipelineState.cpp b/src/igl/opengl/RenderPipelineState.cpp
@@ -190,11 +190,11 @@ Result RenderPipelineState::create(const RenderPipelineDesc& desc) {
   }
 
   if (!mFramebufferDesc.colorAttachments.empty()) {
-    ColorWriteBits colorWriteBits = mFramebufferDesc.colorAttachments[0].colorWriteBits;
-    colorMask_[0] = (colorWriteBits & EnumToValue(ColorWriteMask::Red)) != 0;
-    colorMask_[1] = (colorWriteBits & EnumToValue(ColorWriteMask::Green)) != 0;
-    colorMask_[2] = (colorWriteBits & EnumToValue(ColorWriteMask::Blue)) != 0;
-    colorMask_[3] = (colorWriteBits & EnumToValue(ColorWriteMask::Alpha)) != 0;
+    ColorWriteMask const colorWriteMask = mFramebufferDesc.colorAttachments[0].colorWriteMask;
+    colorMask_[0] = static_cast<GLboolean>((colorWriteMask & ColorWriteBitsRed) != 0);
+    colorMask_[1] = static_cast<GLboolean>((colorWriteMask & ColorWriteBitsGreen) != 0);
+    colorMask_[2] = static_cast<GLboolean>((colorWriteMask & ColorWriteBitsBlue) != 0);
+    colorMask_[3] = static_cast<GLboolean>((colorWriteMask & ColorWriteBitsAlpha) != 0);
   }
 
   if (!mFramebufferDesc.colorAttachments.empty() &&

diff --git a/src/igl/tests/Framebuffer.cpp b/src/igl/tests/Framebuffer.cpp
@@ -289,8 +289,7 @@ TEST_F(FramebufferTest, Clear) {
   ASSERT_TRUE(ret.isOk());
   ASSERT_TRUE(cmdBuf_ != nullptr);
 
-  renderPipelineDesc_.targetDesc.colorAttachments[0].colorWriteBits =
-      EnumToValue(ColorWriteMask::Disabled);
+  renderPipelineDesc_.targetDesc.colorAttachments[0].colorWriteMask = ColorWriteBitsDisabled;
   pipelineState = iglDev_->createRenderPipeline(renderPipelineDesc_, &ret);
   ASSERT_TRUE(ret.isOk());
   ASSERT_TRUE(pipelineState != nullptr);

diff --git a/src/igl/tests/metal/RenderPipelineState.mm b/src/igl/tests/metal/RenderPipelineState.mm
@@ -162,22 +162,22 @@ void TearDown() override {}
 }
 
 TEST_F(RenderPipelineStateMTLTest, ConvertColorWriteMaskRed) {
-  auto mask = pipeState_->convertColorWriteMask(EnumToValue(ColorWriteMask::Red));
+  auto mask = pipeState_->convertColorWriteMask(ColorWriteBitsRed);
   ASSERT_EQ(mask, MTLColorWriteMaskRed);
 }
 
 TEST_F(RenderPipelineStateMTLTest, ConvertColorWriteMaskGreen) {
-  auto mask = pipeState_->convertColorWriteMask(EnumToValue(ColorWriteMask::Green));
+  auto mask = pipeState_->convertColorWriteMask(ColorWriteBitsGreen);
   ASSERT_EQ(mask, MTLColorWriteMaskGreen);
 }
 
 TEST_F(RenderPipelineStateMTLTest, ConvertColorWriteMaskBlue) {
-  auto mask = pipeState_->convertColorWriteMask(EnumToValue(ColorWriteMask::Blue));
+  auto mask = pipeState_->convertColorWriteMask(ColorWriteBitsBlue);
   ASSERT_EQ(mask, MTLColorWriteMaskBlue);
 }
 
 TEST_F(RenderPipelineStateMTLTest, ConvertColorWriteMaskAlpha) {
-  auto mask = pipeState_->convertColorWriteMask(EnumToValue(ColorWriteMask::Alpha));
+  auto mask = pipeState_->convertColorWriteMask(ColorWriteBitsAlpha);
   ASSERT_EQ(mask, MTLColorWriteMaskAlpha);
 }
 

diff --git a/src/igl/vulkan/RenderPipelineState.cpp b/src/igl/vulkan/RenderPipelineState.cpp
@@ -242,6 +242,23 @@ VkBlendFactor blendFactorToVkBlendFactor(igl::BlendFactor value) {
   }
 }
 
+VkColorComponentFlags colorWriteMaskToVkColorComponentFlags(igl::ColorWriteMask value) {
+  VkColorComponentFlags result = 0;
+  if (value & igl::ColorWriteBitsRed) {
+    result |= VK_COLOR_COMPONENT_R_BIT;
+  }
+  if (value & igl::ColorWriteBitsGreen) {
+    result |= VK_COLOR_COMPONENT_G_BIT;
+  }
+  if (value & igl::ColorWriteBitsBlue) {
+    result |= VK_COLOR_COMPONENT_B_BIT;
+  }
+  if (value & igl::ColorWriteBitsAlpha) {
+    result |= VK_COLOR_COMPONENT_A_BIT;
+  }
+  return result;
+}
+
 } // namespace
 
 namespace igl {
@@ -347,7 +364,8 @@ VkPipeline RenderPipelineState::getVkPipeline(
       desc_.targetDesc.colorAttachments.end(),
       [&colorBlendAttachmentStates, dualSrcBlendSupported](auto attachment) mutable {
         if (attachment.textureFormat != TextureFormat::Invalid) {
-          if (!attachment.blendEnabled) {
+          // In Vulkan color write bits are part of blending.
+          if (!attachment.blendEnabled && attachment.colorWriteMask == igl::ColorWriteBitsAll) {
             colorBlendAttachmentStates.push_back(
                 ivkGetPipelineColorBlendAttachmentState_NoBlending());
           } else {
@@ -357,15 +375,14 @@ VkPipeline RenderPipelineState::getVkPipeline(
             checkDualSrcBlendFactor(attachment.dstAlphaBlendFactor, dualSrcBlendSupported);
 
             colorBlendAttachmentStates.push_back(ivkGetPipelineColorBlendAttachmentState(
-                attachment.blendEnabled,
+                true,
                 blendFactorToVkBlendFactor(attachment.srcRGBBlendFactor),
                 blendFactorToVkBlendFactor(attachment.dstRGBBlendFactor),
                 blendOpToVkBlendOp(attachment.rgbBlendOp),
                 blendFactorToVkBlendFactor(attachment.srcAlphaBlendFactor),
                 blendFactorToVkBlendFactor(attachment.dstAlphaBlendFactor),
                 blendOpToVkBlendOp(attachment.alphaBlendOp),
-                VK_COLOR_COMPONENT_R_BIT | VK_COLOR_COMPONENT_G_BIT | VK_COLOR_COMPONENT_B_BIT |
-                    VK_COLOR_COMPONENT_A_BIT));
+                colorWriteMaskToVkColorComponentFlags(attachment.colorWriteMask)));
           }
         }
       });