Skip to content

Conversation

@john-parton
Copy link

Previously, if sRGB framebuffer was requested but not available on Windows, the pixel format selection would fail silently with num_formats=0, leading to undefined behavior with an invalid pixel format.

This commit adds:

  • Proper validation that wglChoosePixelFormatARB found a matching format
  • Graceful fallback: retry without sRGB if initial request fails
  • Warning message to inform users when falling back to non-sRGB

Previously, if sRGB framebuffer was requested but not available on Windows,
the pixel format selection would fail silently with num_formats=0, leading
to undefined behavior with an invalid pixel format.

This commit adds:
- Proper validation that wglChoosePixelFormatARB found a matching format
- Graceful fallback: retry without sRGB if initial request fails
- Warning message to inform users when falling back to non-sRGB
@john-parton
Copy link
Author

This resolves #220

It's a relatively simple workaround. As far as I can tell, the spec doesn't really say what happens if you use 0 as your value there, but I think 0 is really null/None in this context, so double checking and finding an actual appropriate value makes sense to me.

I didn't check the behavior on MacOS or Linux, as I wanted to keep the scope of this low, but there may be a similar issue.

@john-parton
Copy link
Author

It looks like the behavior under linux to is to return as an error:

baseview/src/gl/x11.rs

Lines 190 to 192 in 237d323

if n_configs <= 0 || fb_config.is_null() {
return Err(GlError::CreationFailed(CreationFailedError::InvalidFBConfig));
}

I think probably it would be better if Windows behaved the same and then I guess whatever downstream libraries from here would have to handle that gracefully by explicitly re-initializing.

@john-parton
Copy link
Author

My preference is that this pull request be seriously considered, but I have provided a minimal alternate of propagating the error to the user here: #222 I'm not sure that pull request could even be considered done, but it's there if we want to pivot.

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.

1 participant