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

webrender adjusts gamma wrong if frame contains an OpenGL texture #3262

Open
fschutt opened this Issue Nov 1, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@fschutt
Contributor

fschutt commented Nov 1, 2018

If I push an OpenGL texture (via push_image like here), the background color of all other elements get noticably lighter. If I don't push an OpenGL color, it displays the actual correct color. My assumption is that this has to do with gamma adjustment logic inside of webrender.

Without an OpenGL texture (here the red rectangle is drawn via webrender), it displays correctly:

gamma_adjust_without_opengl

With OpenGL (the red rectangle is an OpenGL texture, the background is drawn by webrender):

gamma_adjust_with_opengl

Comparison on the same screen with the Google color picker:

gamma_adjust_comparison_with_chrome

As you can see, the resulting color is much lighter - technically, whether the texture exists or not should not affect the outcome. If I debug this in renderdoc, the lighter color is drawn, so the bug happens somewhere before the actual drawing stage. I don't do any gamma adjustment myself, except for box-shadow, so I don't think that this is a bug in azul, but rather in webrender.

Steps to reproduce

git clone https://github.com/maps4print/azul
git checkout webrender_bug
cargo run --example webrender_bug

See the /src/webrender_bug.rs for a test case.

@fschutt

This comment has been minimized.

Contributor

fschutt commented Nov 5, 2018

The bug is definitely in webrender, if I print the display list, I get the correct color, but that color doesn't end up on the screen. Since this only happens with OpenGL, not with regular images, I wonder if this relies on webrender having some special OpenGL texture or shader active and maybe the OpenGL drawing messes with that? Here is the display list if I print it out:

// Background, color is correct
GenericDisplayItem {
    item: Rectangle(
        RectangleDisplayItem {
            color: ColorF {
                r: 0.18431373,
                g: 0.19215687,
                b: 0.21176471,
                a: 1.0
            }
        }
    ),
    clip_and_scroll: ClipAndScrollInfo {
        scroll_node_id: Spatial(1, PipelineId(0, 0)),
        clip_node_id: None
    },
    info: PrimitiveInfo {
        rect: TypedRect(800.0×600.0 at (0.0,0.0)),
        clip_rect: TypedRect(800.0×600.0 at (0.0,0.0)),
        is_backface_visible: false,
        tag: None
    }
}

// Hit-testing containing rectangle for the OpenGL texture
GenericDisplayItem {
    item: Rectangle(
        RectangleDisplayItem {
            color: ColorF {
                r: 0.0,
                g: 0.0,
                b: 0.0,
                a: 0.0
            }
        }
    ),
    clip_and_scroll: ClipAndScrollInfo {
        scroll_node_id: Spatial(1, PipelineId(0, 0)),
        clip_node_id: None
    },
    info: PrimitiveInfo {
        rect: TypedRect(100.0×100.0 at (0.0,0.0)),
        clip_rect: TypedRect(100.0×100.0 at (0.0,0.0)),
        is_backface_visible: false,
        tag: None
    }
}

// Actual OpenGL texture
GenericDisplayItem {
    item: Image(
        ImageDisplayItem {
            image_key: ImageKey(
                IdNamespace(
                    1
                ),
                4
            ),
            stretch_size: 100.0×100.0,
            tile_spacing: 0.0×0.0,
            image_rendering: Auto,
            alpha_type: Alpha,
            color: ColorF {
                r: 1.0,
                g: 1.0,
                b: 1.0,
                a: 1.0
            }
        }
    ),
    clip_and_scroll: ClipAndScrollInfo {
        scroll_node_id: Spatial(1, PipelineId(0, 0)),
        clip_node_id: None
    },
    info: PrimitiveInfo {
        rect: TypedRect(100.0×100.0 at (0.0,0.0)),
        clip_rect: TypedRect(100.0×100.0 at (0.0,0.0)),
        is_backface_visible: false,
        tag: None
    }
}
@fschutt

This comment has been minimized.

Contributor

fschutt commented Nov 6, 2018

The weird thing is: In brush.glsl I can directly set:

oFragColor = vec4(0.18431373, 0.19215687, 0.21176471, 1.0);

... and it doesn't make a difference. The discrepancy between regular (pure-webrender) and texture-based rendering is still the exact same. So this means that the bug isn't in webrender itself, but has rather to do with the srgb / linear color conversion logic in the shader.

Setting

oFragColor = vec4(pow(frag.color.xyz, vec3(2.2)), 1.0);

... "fixes" the OpenGL-texture approach to be the correct color, but now the pure-webrender rendered colors are too dark, of course. It does not matter whether GL_FRAMEBUFFER_SRGB is enabled.

fschutt added a commit to maps4print/azul that referenced this issue Nov 6, 2018

Added gamma adjustment hack to migitate WR bug
See servo/webrender#3262 for details.
This commit "reverses" the effect of the webrender gamma bug by
reverse-darkening the gamma of colors of the background colors.

This doesn't fix the bug properly, but the WR bug is a bit harder
to debug.
@nical

This comment has been minimized.

Collaborator

nical commented Nov 6, 2018

I had a look at this and compared the good/bad cases using apitrace and the only differences are in commands emitted outside of WebRender. The most suspicious one being glEnable(GL_FRAMEBUFFER_SRGB) at the following stack:

#0  glium::gl::Gl::Enable (self=0x7fffe33174d0, cap=36281) at build/glium-24c3eb7df3762c1d/out/gl_bindings.rs:6400
#1  0x0000555556230472 in glium::ops::clear::clear (context=0x7fffe33174d0, framebuffer=Some = {...}, rect=None, color=Some = {...}, color_srgb=false, depth=core::option::None, stencil=core::option::None)
    at glium-0.22.0/src/ops/clear.rs:39
#2  0x0000555555663ccd in <glium::framebuffer::SimpleFrameBuffer<'a> as glium::Surface>::clear (self=0x7fffffff8350, rect=None, color=Some = {...}, color_srgb=false, depth=core::option::None, 
    stencil=core::option::None) at glium-0.22.0/src/framebuffer/mod.rs:272
#3  0x00005555556d7b50 in glium::Surface::clear_color (self=0x7fffffff8350, red=1, green=0, blue=0, alpha=1) at glium-0.22.0/src/lib.rs:687
#4  0x00005555556d7dd3 in webrender_bug::render_opengl_texture (_ptr=0x7ffff567a090, info=WindowInfo<webrender_bug::WebrenderTestCase> = {...}, dimensions=HidpiAdjustedBounds = {...})
    at examples/webrender_bug.rs:35
#5  0x00005555556159d9 in azul::display_list::push_opengl_texture (info=0x7fffffff8978, rectangle=DisplayListRectParams<webrender_bug::WebrenderTestCase> = {...}, referenced_content=0x7fffffff9268, 
    referenced_mutable_content=0x7fffffff9288) at display_list.rs:666
#6  0x0000555555616998 in azul::display_list::displaylist_handle_rect (bounds=TypedRect<f32, webrender_api::units::LayoutPixel> = {...}, rectangle=DisplayListRectParams<webrender_bug::WebrenderTestCase> = {...}, 
    referenced_content=0x7fffffff9268, referenced_mutable_content=0x7fffffff9288) at display_list.rs:613
#7  0x000055555561797c in azul::display_list::push_rectangles_into_displaylist (solved_rects=0x7fffffff9110, epoch=Epoch = {...}, z_ordered_rectangles=ZOrderedRectangles = {...}, 
    referenced_content=0x7fffffff9268, referenced_mutable_content=0x7fffffff9288) at display_list.rs:450
#8  0x000055555561a77a in <azul::display_list::DisplayList<'a, T>>::into_display_list_builder (self=0x7fffffff93e0, app_data=Arc<std::sync::mutex::Mutex<webrender_bug::WebrenderTestCase>> = {...}, 
    pipeline_id=PipelineId = {...}, current_epoch=Epoch = {...}, window_size_has_changed=true, render_api=0x7ffff5651df8, parsed_css=0x7ffff5751ea0, window_size=0x7ffff5651258, fake_window=0x7ffff56fdf80, 
    app_resources=0x7fffffffb610) at display_list.rs:244
#9  0x0000555555628787 in azul::app::render (app_data=Arc<std::sync::mutex::Mutex<webrender_bug::WebrenderTestCase>> = {...}, has_window_size_changed=true, ui_description=0x7ffff5751e40, ui_state=0x7ffff5756f40, 
    parsed_css=0x7ffff5751ea0, window=0x7ffff5651000, fake_window=0x7ffff56fdf80, app_resources=0x7fffffffb610) at app.rs:808
#10 0x0000555555626178 in <azul::app::App<T>>::run_inner (self=0x7fffffffb5d8) at app.rs:324
#11 0x000055555562489c in <azul::app::App<T>>::run (self=App<webrender_bug::WebrenderTestCase> = {...}, window=...) at app.rs:209
#12 0x00005555556d7f91 in webrender_bug::main () at examples/webrender_bug.rs:42

WebRender assumes the input textures in srgb color space while GL_FRAMEBUFFER_SRGB tells the driver to assume input colors in linear space and do the conversion.

Glium appears to have a way to override it's default stance towards color spaces by specifying for each program whether it outputs srgb: https://docs.rs/glium/0.22.0/glium/program/enum.ProgramCreationInput.html
Which probably means some internal state tracking that toggles the thing on and off as needed before submitting each draw call. You could make sure to have all glium usage specify the correct flag there but that sounds error prone. You could also manually disable GL_FRAMEBUFFER_SRGB before calling into WebRender but that will probably mess with glium's internal state tracking.

My take on this is that glium isn't very friendly to sharing access to the gl context. You'll probably run into similar issues with other of the many global states if you try using glium alongside something else that also accesses the same context.

@nical

This comment has been minimized.

Collaborator

nical commented Nov 6, 2018

@kvark perhaps we should add some debug only code that queries global states such as GL_FRAMEBUFFER_SRGB at the beginning of each frame and warns/panics if they aren't the value we expect (after having explicitly set it during initialization for good measure).

@kvark

This comment has been minimized.

Member

kvark commented Nov 7, 2018

@nical that would be fine, yeah

bors-servo added a commit that referenced this issue Nov 16, 2018

Auto merge of #3318 - nical:gl-state-check, r=kvark
Add debug checks for some global GL states.

Some simple checks to avoid confusion such as #3262. It doesn't hurt to be more explicit about the state we expect.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3318)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment