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

GLES: on EGL, respect the user requesting a non-sRGB surface format #3817

Merged
merged 4 commits into from
Jun 6, 2023

Conversation

liquidev
Copy link
Contributor

@liquidev liquidev commented May 30, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
    • Not applicable in this case since this is a native backend change.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description
What used to happen was that sRGB was used whether you requested it or not. This PR fixes that and now passing in a non-sRGB texture format in SurfaceConfiguration will result in a non-sRGB surface being created.

Testing
To test this, compare a surface configured with the first available non-sRGB format to a surface configured with the first available sRGB format on the OpenGL/EGL backend:

fn configure_surface(&self, size: PhysicalSize<u32>) {
   let format = self
      .capabilities // wgpu::SurfaceCapabilities
      .formats
      .iter()
      .find(|format| !format.is_srgb()) // <-- Remove `!` to test the sRGB case
      .copied()
      .unwrap_or(self.capabilities.formats[0]);
   let surface_configuration = wgpu::SurfaceConfiguration {
      usage: wgpu::TextureUsages::RENDER_ATTACHMENT,
      format,
      width: size.width,
      height: size.height,
      present_mode: wgpu::PresentMode::AutoVsync,
      alpha_mode: wgpu::CompositeAlphaMode::Opaque,
      view_formats: vec![],
   };
   self.surface.configure(&self.device, &surface_configuration);
}

(First time contribution here, not sure if/how I should write an integration test for this?)

liquidev and others added 4 commits May 30, 2023 22:12
What used to happen was that sRGB was used whether you requested it or not.
This commit fixes that and now passing in a non-sRGB texture format in SurfaceConfiguration will result in a non-sRGB surface being created.
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!
Haven't tested it myself but looks good to me. Regarding automated testing, I think we can let it slide for this one - it's usually super tricky to read the actual surface back (which is why all tests that need to reason about render output read back a different target) making this fairly hard to test.

@Wumpf Wumpf merged commit dfa5400 into gfx-rs:trunk Jun 6, 2023
@torokati44
Copy link
Contributor

Would it be possible to backport this fix to the 0.16 series please?
The greater ecosystem is still in the process of migrating over to 0.17.0 so we can't switch yet.

torokati44 pushed a commit to torokati44/wgpu that referenced this pull request Aug 12, 2023
…fx-rs#3817)

* EGL: respect the user requesting a non-sRGB surface format
What used to happen was that sRGB was used whether you requested it or not.
This commit fixes that and now passing in a non-sRGB texture format in SurfaceConfiguration will result in a non-sRGB surface being created.

* add changelog entry about the EGL non-sRGB support change
torokati44 added a commit to torokati44/ruffle that referenced this pull request Aug 12, 2023
This was a workaround for the issue properly fixed by
gfx-rs/wgpu#3817.
That fix is in wgpu 0.17.0, and soon we can migrate to that.
Even until then, with egui and rendering via MovieView, this is no
longer causing problems with the desktop player, even using OpenGL.
torokati44 added a commit to ruffle-rs/ruffle-android that referenced this pull request Aug 12, 2023
@Wumpf
Copy link
Member

Wumpf commented Aug 13, 2023

Any particular project that's blocking you? Historically we haven't done much backporting after a major release, but I don't think it's off the table :)

@torokati44
Copy link
Contributor

I have since pushed this to v0.16 in my fork, and switched to that in the project where this was important.

Any particular project that's blocking you?

One of them was egui - granted, only for the desktop build, and I asked for the backport because of the Android build, so this is not that crucial. And the 0.17 switch is in master already, just not released.

The more important is naga_oil, which is used in all builds. There is a working PR with the switch to 0.17, which is waiting for a Bevy release to be merged.

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

Successfully merging this pull request may close these issues.

3 participants