Fixed non-sRGB default framebuffer in OpenGL (close #124)

Author: TheMostDiligent

Graphics/GraphicsEngineOpenGL/include/DeviceContextGLImpl.hpp

@@ -316,6 +316,8 @@ class DeviceContextGLImpl final : public DeviceContextBase<EngineGLImplTraits>
     GLContextState m_ContextState;
 
 private:
+    void CommitDefaultFramebuffer();
+
     __forceinline void PrepareForDraw(DRAW_FLAGS Flags, bool IsIndexed, GLenum& GlTopology);
     __forceinline void PrepareForIndexedDraw(VALUE_TYPE IndexType, Uint32 FirstIndexLocation, GLenum& GLIndexType, size_t& FirstIndexByteOffset);
     __forceinline void PrepareForIndirectDraw(IBuffer* pAttribsBuffer);

Graphics/GraphicsEngineOpenGL/include/GLContextState.hpp

@@ -49,7 +49,7 @@ class GLContextState
     void SetProgram        (const GLObjectWrappers::GLProgramObj&     GLProgram);
     void SetPipeline       (const GLObjectWrappers::GLPipelineObj&    GLPipeline);
     void BindVAO           (const GLObjectWrappers::GLVertexArrayObj& VAO);
-    void BindFBO           (const GLObjectWrappers::GLFrameBufferObj& FBO);
+    void BindFBO           (const GLObjectWrappers::GLFrameBufferObj& FBO, bool DoEnableFramebufferSRGB = true);
     void SetActiveTexture  (Int32 Index);
     void BindTexture       (Int32 Index, GLenum BindTarget, const GLObjectWrappers::GLTextureObj& Tex);
     void BindUniformBuffer (Int32 Index,       const GLObjectWrappers::GLBufferObj& Buff, GLintptr Offset, GLsizeiptr Size);
@@ -85,6 +85,7 @@ class GLContextState
     void GetColorWriteMask(Uint32 RTIndex, Uint32& WriteMask, Bool& bIsIndexed);
     void SetColorWriteMask(Uint32 WriteMask);
     void SetColorWriteMaskIndexed(Uint32 RTIndex, Uint32 WriteMask);
+    void EnableFramebufferSRGB(Bool bEnable);
 
     Uint8 GetStencilWriteMask() const { return static_cast<Uint8>(m_DSState.m_StencilWriteMask); }
 
@@ -118,6 +119,7 @@ class GLContextState
         bool  IsFillModeSelectionSupported = true;
         bool  IsProgramPipelineSupported   = true;
         bool  IsDepthClampSupported        = true;
+        bool  IsFramebufferSRGBSupported   = false;
         GLint MaxCombinedTexUnits          = 0;
         GLint MaxDrawBuffers               = 0;
         GLint MaxUniformBufferBindings     = 0;
@@ -310,6 +312,7 @@ class GLContextState
 
     Uint32            m_ColorWriteMasks[MAX_RENDER_TARGETS] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
     EnableStateHelper m_bIndexedWriteMasks;
+    EnableStateHelper m_bFramebufferSRGB;
     Int32             m_iActiveTexture   = -1;
     Int32             m_NumPatchVertices = -1;
 

Graphics/GraphicsEngineOpenGL/src/DeviceContextGLImpl.cpp

@@ -366,6 +366,34 @@ GLuint DeviceContextGLImpl::GetDefaultFBO() const
     return m_pSwapChain ? m_pSwapChain->GetDefaultFBO() : 0;
 }
 
+void DeviceContextGLImpl::CommitDefaultFramebuffer()
+{
+    GLuint DefaultFBOHandle = m_pSwapChain->GetDefaultFBO();
+    if (m_DefaultFBO != DefaultFBOHandle)
+    {
+        m_DefaultFBO = GLObjectWrappers::GLFrameBufferObj{true, GLObjectWrappers::GLFBOCreateReleaseHelper{DefaultFBOHandle}};
+    }
+
+    // In theory, we should not be messing with GL_FRAMEBUFFER_SRGB, but sRGB conversion
+    // control for default framebuffers is totally broken.
+    // https://github.com/DiligentGraphics/DiligentCore/issues/124
+    bool EnableFramebufferSRGB = true;
+    if (m_ContextState.GetContextCaps().IsFramebufferSRGBSupported)
+    {
+        if (DefaultFBOHandle == 0 && m_NumBoundRenderTargets == 1)
+        {
+            if (const TextureViewGLImpl* pRT0 = m_pBoundRenderTargets[0])
+            {
+                const TEXTURE_FORMAT Fmt = pRT0->GetDesc().Format;
+                EnableFramebufferSRGB    = (Fmt == TEX_FORMAT_RGBA8_UNORM_SRGB || Fmt == TEX_FORMAT_BGRA8_UNORM_SRGB);
+            }
+        }
+    }
+
+    m_ContextState.BindFBO(m_DefaultFBO, EnableFramebufferSRGB);
+    m_DrawFBO = nullptr;
+}
+
 void DeviceContextGLImpl::CommitRenderTargets()
 {
     DEV_CHECK_ERR(m_pActiveRenderPass == nullptr, "This method must not be called inside render pass");
@@ -375,13 +403,7 @@ void DeviceContextGLImpl::CommitRenderTargets()
 
     if (m_IsDefaultFBOBound)
     {
-        GLuint DefaultFBOHandle = m_pSwapChain->GetDefaultFBO();
-        if (m_DefaultFBO != DefaultFBOHandle)
-        {
-            m_DefaultFBO = GLObjectWrappers::GLFrameBufferObj{true, GLObjectWrappers::GLFBOCreateReleaseHelper{DefaultFBOHandle}};
-        }
-        m_ContextState.BindFBO(m_DefaultFBO);
-        m_DrawFBO = nullptr;
+        CommitDefaultFramebuffer();
     }
     else
     {
@@ -475,13 +497,7 @@ void DeviceContextGLImpl::BeginSubpass()
     }
     else
     {
-        GLuint DefaultFBOHandle = m_pSwapChain->GetDefaultFBO();
-        if (m_DefaultFBO != DefaultFBOHandle)
-        {
-            m_DefaultFBO = GLObjectWrappers::GLFrameBufferObj{true, GLObjectWrappers::GLFBOCreateReleaseHelper{DefaultFBOHandle}};
-        }
-        m_ContextState.BindFBO(m_DefaultFBO);
-        m_DrawFBO = nullptr;
+        CommitDefaultFramebuffer();
     }
 
 

Graphics/GraphicsEngineOpenGL/src/GLContextState.cpp

@@ -48,6 +48,7 @@ GLContextState::GLContextState(RenderDeviceGLImpl* pDeviceGL)
     m_Caps.IsFillModeSelectionSupported    = AdapterInfo.Features.WireframeFill;
     m_Caps.IsProgramPipelineSupported      = AdapterInfo.Features.SeparablePrograms;
     m_Caps.IsDepthClampSupported           = AdapterInfo.Features.DepthClamp;
+    m_Caps.IsFramebufferSRGBSupported      = pDeviceGL->GetGLCaps().FramebufferSRGB;
 
     {
         m_Caps.MaxCombinedTexUnits = 0;
@@ -109,13 +110,14 @@ void GLContextState::Invalidate()
     m_BoundUniformBuffers.clear();
     m_BoundStorageBlocks.clear();
 
-    m_DSState = DepthStencilGLState();
-    m_RSState = RasterizerGLState();
+    m_DSState = DepthStencilGLState{};
+    m_RSState = RasterizerGLState{};
 
     for (Uint32 rt = 0; rt < _countof(m_ColorWriteMasks); ++rt)
         m_ColorWriteMasks[rt] = 0xFF;
 
-    m_bIndexedWriteMasks = EnableStateHelper();
+    m_bIndexedWriteMasks = EnableStateHelper{};
+    m_bFramebufferSRGB   = EnableStateHelper{};
 
     m_iActiveTexture   = -1;
     m_NumPatchVertices = -1;
@@ -177,7 +179,7 @@ void GLContextState::BindVAO(const GLVertexArrayObj& VAO)
     }
 }
 
-void GLContextState::BindFBO(const GLFrameBufferObj& FBO)
+void GLContextState::BindFBO(const GLFrameBufferObj& FBO, bool DoEnableFramebufferSRGB)
 {
     GLuint FBOHandle = 0;
     if (UpdateBoundObject(m_FBOId, FBO, FBOHandle))
@@ -193,6 +195,8 @@ void GLContextState::BindFBO(const GLFrameBufferObj& FBO)
         DEV_CHECK_GL_ERROR("Failed to bind FBO as draw framebuffer");
         glBindFramebuffer(GL_READ_FRAMEBUFFER, FBOHandle);
         DEV_CHECK_GL_ERROR("Failed to bind FBO as read framebuffer");
+
+        EnableFramebufferSRGB(DoEnableFramebufferSRGB);
     }
 }
 
@@ -906,6 +910,27 @@ void GLContextState::GetColorWriteMask(Uint32 RTIndex, Uint32& WriteMask, Bool&
     bIsIndexed = m_bIndexedWriteMasks;
 }
 
+void GLContextState::EnableFramebufferSRGB(Bool bEnable)
+{
+    if (!m_Caps.IsFramebufferSRGBSupported)
+        return;
+
+    if (m_bFramebufferSRGB != bEnable)
+    {
+        if (bEnable)
+        {
+            glEnable(GL_FRAMEBUFFER_SRGB);
+            DEV_CHECK_GL_ERROR("Failed to enable framebuffer sRGB");
+        }
+        else
+        {
+            glDisable(GL_FRAMEBUFFER_SRGB);
+            DEV_CHECK_GL_ERROR("Failed to disable framebuffer sRGB");
+        }
+        m_bFramebufferSRGB = bEnable;
+    }
+}
+
 void GLContextState::SetNumPatchVertices(Int32 NumVertices)
 {
 #if GL_ARB_tessellation_shader

Graphics/GraphicsEngineOpenGL/src/GLContextWindows.cpp

@@ -127,6 +127,9 @@ GLContext::GLContext(const EngineGLCreateInfo& InitAttribs,
         if (nPixelFormat == 0)
             LOG_ERROR_AND_THROW("Invalid Pixel Format");
 
+        // NB: An application can only set the pixel format of a window one time.
+        //     Once a window's pixel format is set, it cannot be changed.
+        //     https://learn.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-setpixelformat
         BOOL bResult = SetPixelFormat(m_WindowHandleToDeviceContext, nPixelFormat, &pfd);
         if (!bResult)
             LOG_ERROR_AND_THROW("Failed to set Pixel Format");