-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Use the correct pixel formats for Vulkan on big endian #9905
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
base: main
Are you sure you want to change the base?
Conversation
@danginsburg, can you check this?
|
1cc7a94
to
185fd73
Compare
As an aside, it would be nice to support some of the 16-bit packed formats like |
In practice I haven't seen them being used for rendering. Usually textures are converted to RGBA or a compressed texture format. |
I don't really have the context to understand what the impact of this change would be. What is removing SDL_PIXELFORMAT_ARGB8888 and SDL_PIXELFORMAT_XRGB8888 from SDL_AddSupportedTextureFormat and replacing them with SDL_PIXELFORMAT_RGBA32 and SDL_PIXELFORMAT_BGRA32 going to do? Also, I wonder if you need to do optimalTilingSupport checks for some of these since for example R8G8B8_SRGB/UNORM have pretty limited support in the wild (see vulkan.gpuinfo.org). |
The Vulkan specs state that
That probably depends on if it's still faster than converting in software. It's listed after the R8G8B8A8 formats so it shouldn't be picked as a default for the most part, but it may still be nice to extend the SDL render API to report that certain supported formats are slower.
I'm mainly interested in the 16-bit pixel formats for dynamic textures in emulators, game reimplementations and video decoders, but it should be useful for static images on lower-end devices as well (such as .bmp and .tga). |
SDL_AddSupportedTextureFormat(renderer, SDL_PIXELFORMAT_RGBA32); | ||
SDL_AddSupportedTextureFormat(renderer, SDL_PIXELFORMAT_BGRA32); | ||
#if SDL_BYTEORDER != SDL_LIL_ENDIAN | ||
SDL_AddSupportedTextureFormat(renderer, SDL_PIXELFORMAT_ABGR8888); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that the same as:
SDL_AddSupportedTextureFormat(renderer, SDL_PIXELFORMAT_RGBA32);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're the same on little endian, but different on big endian.
185fd73
to
4898020
Compare
By the way, we're not trying to stuff in all the formats that can be supported, we're trying to include the common formats that are fast for current drivers. If a format is allowed according to the spec, but mobile GPU vendors implement slow paths for access, for example, we wouldn't list those formats. The 3-byte RGB formats come to mind, and the 16-bit formats might be in a similar situation. If a format is supported but slow, it's actually better to leave it off, since SDL will convert textures to fast formats during texture creation and upload. |
It's not a performance thing, for non-mandatory formats you need to check |
@ccawley2011, can you update this PR to check |
b1b16a4
to
165bfe9
Compare
I've updated this PR to remove the 24-bit pixel format, leaving only the endian fixes. The newly added |
165bfe9
to
88c46fa
Compare
88c46fa
to
4be706f
Compare
The new formats more closely match how the specifications describe how packed vs. byte formats are stored and should work in theory on both big and little endian. Support for
SDL_PIXELFORMAT_XRGB8888
was removed because it wasn't clear how the existing code handled missing alpha components.I'm not sure about
SDL_PIXELFORMAT_XBGR2101010
, but I don't know how to test it.