Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color space issues with SRGB framebuffer in OpenGL #1915

Closed
parasyte opened this issue Apr 8, 2018 · 4 comments
Closed

Color space issues with SRGB framebuffer in OpenGL #1915

parasyte opened this issue Apr 8, 2018 · 4 comments

Comments

@parasyte
Copy link

parasyte commented Apr 8, 2018

Short info header:

  • GFX version: 0.17.1
  • OS: macOS 10.13.4
  • GPU: AMD Radeon R9 M370X 2048 MB graphics / Intel Iris Pro 1536 MB graphics

Today I started updating the gfx dependency in imgui-rs. One of the things I noticed is that the imgui-gfx-renderer looks washed out compared to imgui-glium-renderer. It took me all day to find the cause, which turned out to be a color space mismatch between the (linear space) colors provided by imgui in the vertex attribute buffer, and the gfx_device_gl backend forcefully enabling SRGB on the framebuffer.

I was able to reproduce it by patching the gamma example to use the purple title bar color from imgui instead of the grayscale. Here's the patch:

diff --git a/examples/gamma/main.rs b/examples/gamma/main.rs
index af29c5b4..b95a8c82 100644
--- a/examples/gamma/main.rs
+++ b/examples/gamma/main.rs
@@ -31,7 +31,7 @@ pub type DepthFormat = gfx::format::DepthStencil;
 gfx_defines!{
     vertex Vertex {
         pos: [f32; 2] = "a_Pos",
-        color: [f32; 3] = "a_Color",
+        color: [f32; 4] = "a_Color",
     }

     pipeline pipe {
@@ -41,13 +41,14 @@ gfx_defines!{
 }

 const QUAD: [Vertex; 4] = [
-    Vertex { pos: [ -0.5,  0.5 ], color: [0.0, 0.0, 0.0] },
-    Vertex { pos: [ -0.5, -0.5 ], color: [0.0, 0.0, 0.0] },
-    Vertex { pos: [  0.5, -0.5 ], color: [1.0, 1.0, 1.0] },
-    Vertex { pos: [  0.5,  0.5 ], color: [1.0, 1.0, 1.0] }
+    // Use imgui TitleBg color: https://github.com/ocornut/imgui/blob/v1.52/imgui.cpp#L744
+    Vertex { pos: [ -0.5,  0.5 ], color: [0.27, 0.27, 0.54, 0.83] },
+    Vertex { pos: [ -0.5, -0.5 ], color: [0.27, 0.27, 0.54, 0.83] },
+    Vertex { pos: [  0.5, -0.5 ], color: [0.27, 0.27, 0.54, 0.83] },
+    Vertex { pos: [  0.5,  0.5 ], color: [0.27, 0.27, 0.54, 0.83] }
 ];

-const CLEAR_COLOR: [f32; 4] = [0.5, 0.5, 0.5, 1.0];
+const CLEAR_COLOR: [f32; 4] = [1.0, 1.0, 1.0, 1.0];

 type SurfaceData = <<ColorFormat as Formatted>::Surface as SurfaceTyped>::DataType;

diff --git a/examples/gamma/shader/quad_120.glslv b/examples/gamma/shader/quad_120.glslv
index a64fe208..44a270ed 100644
--- a/examples/gamma/shader/quad_120.glslv
+++ b/examples/gamma/shader/quad_120.glslv
@@ -1,10 +1,10 @@
 #version 120

 attribute vec2 a_Pos;
-attribute vec3 a_Color;
+attribute vec4 a_Color;
 varying vec4 v_Color;

 void main() {
-    v_Color = vec4(a_Color, 1.0);
+    v_Color = a_Color;
     gl_Position = vec4(a_Pos, 0.0, 1.0);
 }
diff --git a/examples/gamma/shader/quad_150_core.glslv b/examples/gamma/shader/quad_150_core.glslv
index 2ce6461b..c438e2ba 100644
--- a/examples/gamma/shader/quad_150_core.glslv
+++ b/examples/gamma/shader/quad_150_core.glslv
@@ -1,10 +1,10 @@
 #version 150 core

 in vec2 a_Pos;
-in vec3 a_Color;
+in vec4 a_Color;
 out vec4 v_Color;

 void main() {
-    v_Color = vec4(a_Color, 1.0);
+    v_Color = a_Color;
     gl_Position = vec4(a_Pos, 0.0, 1.0);
 }
diff --git a/examples/gamma/shader/quad_300_es.glslv b/examples/gamma/shader/quad_300_es.glslv
index f0f59952..3df5b91e 100644
--- a/examples/gamma/shader/quad_300_es.glslv
+++ b/examples/gamma/shader/quad_300_es.glslv
@@ -1,10 +1,10 @@
 #version 300 es

 in vec2 a_Pos;
-in vec3 a_Color;
+in vec4 a_Color;
 out vec4 v_Color;

 void main() {
-    v_Color = vec4(a_Color, 1.0);
+    v_Color = a_Color;
     gl_Position = vec4(a_Pos, 0.0, 1.0);
 }

And here's a screenshot, showing the color rendered incorrectly:

screen shot 2018-04-07 at 11 53 31 pm

When I disable the SRGB framebuffer (using unsafe code), the rendering works as expected:

// XXX: Fix incorrect render results
unsafe {
    extern crate gfx_gl;
    device.with_gl(|gl| {
        gl.Disable(gfx_gl::FRAMEBUFFER_SRGB);
    });
}

screen shot 2018-04-07 at 11 56 27 pm


As a final data point, here are some screenshots of the imgui-rs hello_gfx example; the latter screenshot has the SRGB-disable workaround to fix the #colors:

screen shot 2018-04-07 at 11 04 03 pm

screen shot 2018-04-07 at 11 04 20 pm

May be related to: #992 #997

@parasyte
Copy link
Author

parasyte commented Apr 8, 2018

Related: ocornut/imgui#578

@amitksinha
Copy link

https://www.mapd.com/docs/latest/getting-started/hwRefCfgGuide/
Currently, the optimal GPUs on which to run the MapD Analytics Platform are:

NVIDIA Tesla P40
NVIDIA Tesla P100

Will it handle any GPU card such as radeon?

@amitksinha
Copy link

may be i posted the question incorrectly.
Should I take that mapd will support any GPU, or is it vendor specific?
thanks

@parasyte
Copy link
Author

parasyte commented Apr 8, 2018

This actually looks like a problem with imgui colors in its default style. The gfx sRGB framebuffer is rendering colors exactly as they should appear. (It's just that color spaces always confuse the hell out of me.)

The TitleBg color above, for example is the sRGB components divided by 255 to incorrectly place them into linear space without a gamma transform. (128,128,128) in sRGB is not mid-gray, but (0.5,0.5,0.5) in linear-space is mid-gray. That's the mistake they made.

So I'm going to close this in favor of #997

@parasyte parasyte closed this as completed Apr 8, 2018
bors bot added a commit that referenced this issue Jun 5, 2018
2110: dx12: Add dynamically sized CPU descriptor heaps r=kvark a=msiglreith

Dynamic allocator for CPU descriptor heaps. We currently have 2 ways of allocating CPU descriptors: from a device global heap and command local linear allocators. This PR cleans up the two paths a bit and fixes the current size limitation for the first kind by dynamically creating new small fixed size CPU heaps.

Freeing currently not support but that's not an regression as we didn't support it before either.
Small 'regression' concerning `fill_buffer` which was implemented incorrectly as far as I can see. Probably would work only on AMD. This breaks the fill reftests.

Fixes #1915
PR checklist:
- [x] tested quad, relevant CTS and a few vulkan samples with the following backends: dx12


Co-authored-by: msiglreith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants