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

Add support for sRGB capable framebuffers. #981

Merged
merged 1 commit into from Mar 11, 2016

Conversation

Projects
None yet
6 participants
@binary1248
Member

binary1248 commented Oct 1, 2015

Implements #175.

@binary1248 binary1248 self-assigned this Oct 1, 2015

@binary1248 binary1248 added this to the 2.4 milestone Oct 1, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 1, 2015

Member

Looks good, except that "RGB" in identifiers should be "Rgb", according to the coding style.

Member

LaurentGomila commented Oct 1, 2015

Looks good, except that "RGB" in identifiers should be "Rgb", according to the coding style.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 1, 2015

Member

Well... strictly speaking, the whole abbreviation is not RGB but sRGB, so in that sense, the identifiers would have to all be srgb.

Member

binary1248 commented Oct 1, 2015

Well... strictly speaking, the whole abbreviation is not RGB but sRGB, so in that sense, the identifiers would have to all be srgb.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 1, 2015

Member

If we enforce strict PascalCase identifiers, it would be Srgb.

Member

LaurentGomila commented Oct 1, 2015

If we enforce strict PascalCase identifiers, it would be Srgb.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 1, 2015

Member

But in camelCase (which we use), it's srgb? 😛

Member

binary1248 commented Oct 1, 2015

But in camelCase (which we use), it's srgb? 😛

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 1, 2015

Member

Yes sorry... I forgot that we were talking about variables, not class names.

I'd say that sRgb should be ok then.

Member

LaurentGomila commented Oct 1, 2015

Yes sorry... I forgot that we were talking about variables, not class names.

I'd say that sRgb should be ok then.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 2, 2015

Member

Fixed.

Member

binary1248 commented Oct 2, 2015

Fixed.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila
Member

LaurentGomila commented Oct 2, 2015

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 22, 2015

Member

How can this best be tested?

Member

eXpl0it3r commented Oct 22, 2015

How can this best be tested?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 22, 2015

Member

Request an sRGB framebuffer and draw some sRGB stuff. You will notice that things are a "bit" different if you just go ahead and request one in the OpenGL example for example. The rendering would naturally have to be adjusted for sRGB if the framebuffer expects it.

Member

binary1248 commented Oct 22, 2015

Request an sRGB framebuffer and draw some sRGB stuff. You will notice that things are a "bit" different if you just go ahead and request one in the OpenGL example for example. The rendering would naturally have to be adjusted for sRGB if the framebuffer expects it.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 25, 2015

Member

Going to ping a few people who showed interest for this feature in the past and who might be willing to test it: @NicolBolas, @retep998, @bananu7, @elliotpotts, @dezzik

Member

eXpl0it3r commented Oct 25, 2015

Going to ping a few people who showed interest for this feature in the past and who might be willing to test it: @NicolBolas, @retep998, @bananu7, @elliotpotts, @dezzik

m_settings.depthBits = static_cast<unsigned int>(depth);
m_settings.stencilBits = static_cast<unsigned int>(stencil);
m_settings.antialiasingLevel = multiSampling ? samples : 0;
m_settings.sRgbCapable = (sRgb == True);

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Nov 20, 2015

Member

Where does that True come from?

@eXpl0it3r

eXpl0it3r Nov 20, 2015

Member

Where does that True come from?

This comment has been minimized.

@binary1248

binary1248 Nov 20, 2015

Member

From Xlib via GLX.

@binary1248

binary1248 Nov 20, 2015

Member

From Xlib via GLX.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 20, 2015

Member

Are there no other "formats" we might support in the future? I mean, wouldn't it make sense to use an enum instead of a boolean?

Member

eXpl0it3r commented Nov 20, 2015

Are there no other "formats" we might support in the future? I mean, wouldn't it make sense to use an enum instead of a boolean?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Nov 20, 2015

Member

sRGB is the only other colour space that I know of which is in widespread use besides "normal RGB". Also, enabling support for sRGB should really be a flag. The specification kind of makes clear that sRGB support is an additional capability of the framebuffer and therefore doesn't replace or gets replaced by anything else.

Member

binary1248 commented Nov 20, 2015

sRGB is the only other colour space that I know of which is in widespread use besides "normal RGB". Also, enabling support for sRGB should really be a flag. The specification kind of makes clear that sRGB support is an additional capability of the framebuffer and therefore doesn't replace or gets replaced by anything else.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 29, 2015

Member

Going to ping a few people who showed interest for this feature in the past and who might be willing to test it: @NicolBolas, @retep998, @bananu7, @elliotpotts, @dezzik

Another attempt :)

Member

Bromeon commented Nov 29, 2015

Going to ping a few people who showed interest for this feature in the past and who might be willing to test it: @NicolBolas, @retep998, @bananu7, @elliotpotts, @dezzik

Another attempt :)

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Nov 29, 2015

sRGB is basically the format that everything uses. All images are assumed to be in sRGB (unless specified otherwise). Monitors are calibrated for sRGB. The web dictates sRGB. All this is doing is converting between gamma encoded sRGB and linear RGB so that blending can be performed correctly instead of with a poor approximation. This is the format to support, therefore in my mind using a boolean is the right thing to do.

This doesn't seem like it handles loading textures as sRGB though. That's an essential counterpart to treating the output framebuffer as sRGB, otherwise when you load in an image and display it to the screen, it'll look wrong.

retep998 commented Nov 29, 2015

sRGB is basically the format that everything uses. All images are assumed to be in sRGB (unless specified otherwise). Monitors are calibrated for sRGB. The web dictates sRGB. All this is doing is converting between gamma encoded sRGB and linear RGB so that blending can be performed correctly instead of with a poor approximation. This is the format to support, therefore in my mind using a boolean is the right thing to do.

This doesn't seem like it handles loading textures as sRGB though. That's an essential counterpart to treating the output framebuffer as sRGB, otherwise when you load in an image and display it to the screen, it'll look wrong.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 21, 2016

Member

Added additional support for conversion of texture data from sRGB back to linear so that it can be processed in linear and output back to the framebuffer in sRGB. Also updated the OpenGL example to "demonstrate" its usage (there is no visible difference, as to be expected when not doing e.g. complex lighting).

Member

binary1248 commented Feb 21, 2016

Added additional support for conversion of texture data from sRGB back to linear so that it can be processed in linear and output back to the framebuffer in sRGB. Also updated the OpenGL example to "demonstrate" its usage (there is no visible difference, as to be expected when not doing e.g. complex lighting).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 4, 2016

Member

Can this be merged?

Member

eXpl0it3r commented Mar 4, 2016

Can this be merged?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 4, 2016

Member

The only "real" way to test this would be to write e.g. an HDR shader that manipulates the linearized sRGB data of some texture. The whole purpose for all of this is because such algorithms would produce wrong results if the data was not linear.

Member

binary1248 commented Mar 4, 2016

The only "real" way to test this would be to write e.g. an HDR shader that manipulates the linearized sRGB data of some texture. The whole purpose for all of this is because such algorithms would produce wrong results if the data was not linear.

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Mar 4, 2016

Several tests I can think of:

  1. Take a solid gray texture that is srgb(0.5, 0.5, 0.5) and draw it twice using additive blending. If the result is white you're not using sRGB. If the result is merely a light gray srgb(0.68, 0.68, 0.68) then sRGB is working correctly.
  2. Take a black and white checkerboard pattern (where the squares are 1 pixel big) and draw it at 50% scale using linear blending and/or mipmapping so it all blurs together. If the resulting gray is srgb(0.5, 0.5, 0.5) you're not using sRGB. If the result is srgb(0.73, 0.73, 0.73) then sRGB is working correctly.

retep998 commented Mar 4, 2016

Several tests I can think of:

  1. Take a solid gray texture that is srgb(0.5, 0.5, 0.5) and draw it twice using additive blending. If the result is white you're not using sRGB. If the result is merely a light gray srgb(0.68, 0.68, 0.68) then sRGB is working correctly.
  2. Take a black and white checkerboard pattern (where the squares are 1 pixel big) and draw it at 50% scale using linear blending and/or mipmapping so it all blurs together. If the resulting gray is srgb(0.5, 0.5, 0.5) you're not using sRGB. If the result is srgb(0.73, 0.73, 0.73) then sRGB is working correctly.
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 5, 2016

Member

Test with this then:

#include <SFML/Graphics.hpp>
#include <vector>

int main()
{
    // Enable/Disable sRGB here
    bool sRgb = true;

    sf::ContextSettings contextSettings;
    contextSettings.sRgbCapable = sRgb;

    sf::RenderWindow window(sf::VideoMode(800, 600), "sRGB Test", sf::Style::Default, contextSettings);

    sf::Image image;
    image.create(400, 300, sf::Color(127, 127, 127));

    sf::Texture texture1;
    texture1.setSrgb(sRgb);
    texture1.loadFromImage(image);

    sf::Sprite sprite1;
    sprite1.setTexture(texture1);

    std::vector<sf::Color> data(799 * 600, sf::Color::White);
    for(size_t i = 0; i < 799 * 600; i += 2)
        data[i] = sf::Color::Black;

    sf::Texture texture2;
    texture2.setSrgb(sRgb);
    texture2.setSmooth(true);
    texture2.create(799, 600);
    texture2.update(reinterpret_cast<const sf::Uint8*>(&data[0]));

    sf::Sprite sprite2;
    sprite2.setPosition(401, 300);
    sprite2.setTexture(texture2);
    sprite2.setScale(0.5f, 0.5f);

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
                return 0;
        }

        window.clear();
        window.draw(sprite1, sf::BlendAdd);
        window.draw(sprite1, sf::BlendAdd);
        window.draw(sprite2);
        window.display();
    }
}

It produces the results you would expect with/without sRGB support.

Member

binary1248 commented Mar 5, 2016

Test with this then:

#include <SFML/Graphics.hpp>
#include <vector>

int main()
{
    // Enable/Disable sRGB here
    bool sRgb = true;

    sf::ContextSettings contextSettings;
    contextSettings.sRgbCapable = sRgb;

    sf::RenderWindow window(sf::VideoMode(800, 600), "sRGB Test", sf::Style::Default, contextSettings);

    sf::Image image;
    image.create(400, 300, sf::Color(127, 127, 127));

    sf::Texture texture1;
    texture1.setSrgb(sRgb);
    texture1.loadFromImage(image);

    sf::Sprite sprite1;
    sprite1.setTexture(texture1);

    std::vector<sf::Color> data(799 * 600, sf::Color::White);
    for(size_t i = 0; i < 799 * 600; i += 2)
        data[i] = sf::Color::Black;

    sf::Texture texture2;
    texture2.setSrgb(sRgb);
    texture2.setSmooth(true);
    texture2.create(799, 600);
    texture2.update(reinterpret_cast<const sf::Uint8*>(&data[0]));

    sf::Sprite sprite2;
    sprite2.setPosition(401, 300);
    sprite2.setTexture(texture2);
    sprite2.setScale(0.5f, 0.5f);

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
                return 0;
        }

        window.clear();
        window.draw(sprite1, sf::BlendAdd);
        window.draw(sprite1, sf::BlendAdd);
        window.draw(sprite2);
        window.display();
    }
}

It produces the results you would expect with/without sRGB support.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 6, 2016

Member

sRgb = true
SRGB

sRgb = false
not SRGB

Seems to work as expected.

Member

eXpl0it3r commented Mar 6, 2016

sRgb = true
SRGB

sRgb = false
not SRGB

Seems to work as expected.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 6, 2016

Member

Same here on OS X.

Member

mantognini commented Mar 6, 2016

Same here on OS X.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 8, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Mar 8, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r merged commit e00d160 into master Mar 11, 2016

16 checks passed

debian-gcc-64 Build #103 done.
Details
freebsd-gcc-64 Build #103 done.
Details
osx-clang-universal Build #103 done.
Details
static-analysis Build #103 done.
Details
windows-gcc-471-tdm-32 Build #106 done.
Details
windows-gcc-471-tdm-64 Build #106 done.
Details
windows-gcc-481-tdm-32 Build #106 done.
Details
windows-gcc-481-tdm-64 Build #106 done.
Details
windows-gcc-520-mingw-32 Build #104 done.
Details
windows-gcc-520-mingw-64 Build #106 done.
Details
windows-vc11-32 Build #105 done.
Details
windows-vc11-64 Build #106 done.
Details
windows-vc12-32 Build #105 done.
Details
windows-vc12-64 Build #104 done.
Details
windows-vc14-32 Build #104 done.
Details
windows-vc14-64 Build #106 done.
Details

@eXpl0it3r eXpl0it3r deleted the feature/srgb branch Mar 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment