libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.82k stars 1.82k forks source link

Re-evaluate default GL backbuffer red/green/blue/alpha bit sizes #7462

Closed slime73 closed 3 months ago

slime73 commented 1 year ago

Right now, the default backbuffer color format when creating an OpenGL context (SDL_GL_RED_SIZE, SDL_GL_GREEN_SIZE, SDL_GL_BLUE_SIZE, and SDL_GL_ALPHA_SIZE) is 3/3/2/0. Most graphics drivers don't actually support an 8 bit per pixel backbuffer and will choose something it does support, like 8/8/8/x or 5/6/5/0 or similar. It's sort of inconsistent and unreliable cross-platform.

In my opinion 8/8/8/8 would be a more solid and expected default compared to 3/3/2/0 and even 5/6/5/0 (most desktop drivers don't really support the latter either). I don't know of any GL/GLES-capable phones that don't support 8/8/8/8, but app developers could still choose 5/6/5/0 for specific use cases they have.

Defaulting to 8/8/8/8 would also allow SDL_WINDOW_TRANSPARENT to work without fiddling with bit sizes, when using OpenGL.

Or maybe other people have different opinions, but I wanted to throw it out there since 3/3/2/0 as a default has always seemed super weird to me and has caused issues in the past (for example the Android readme recommends avoiding it).

slouken commented 1 year ago

I think for SDL3 we can default to 8/8/8/x where x is 0 for regular windows and 8 for transparent windows.

1bsyl commented 1 year ago

this parts should be updated https://github.com/libsdl-org/SDL/blob/main/src/video/SDL_video.c#L3789

but glconfig is in SDL_VideoDevice struct, whereas it should be within SDL_Window struct

https://github.com/libsdl-org/SDL/blob/main/src/video/SDL_sysvideo.h#L371

but that also contains:

     int driver_loaded;
     char driver_path[256];
     void *dll_handle;

which are 1 for all window, I guess.

... Another point: we are always setting the attributes before creating a window. So SDL_GL_SetAttribute() couldn't take the window parameter that isn't yet created.

slouken commented 1 year ago

Yes, the GL attributes need to be set before creating a window, so they're global for the application.

1bsyl commented 1 year ago

yes, so difficult to have "default to 8/8/8/x where x is 0 for regular windows and 8 for transparent windows.", if the window isn't created. a const 8888 or 8880 would be easier.

Or: starts with 8880 And if this hasn't been explicitly set with SDL_GL_SetAttribute , and we have transparency, overload to 8888 ?

slouken commented 1 year ago

Or: starts with 8880 And if this hasn't been explicitly set with SDL_GL_SetAttribute , and we have transparency, overload to 8888 ?

Yeah, that sounds reasonable.

slime73 commented 1 year ago

What benefit does 8880 give over 8888? GPUs don't like non power of two pixel sizes very much so I'd have thought the driver would just use 8888 internally in that situation.

slouken commented 1 year ago

They're both 32-bit formats, but the alpha channel can be used in whole-window compositing for transparent windows if enabled.

slime73 commented 1 year ago

If 8880 (32 bit) is requested, I believe many drivers will give back 8888 but maybe some won't. This could be confusing to developers if for example they have screenshot code which uses glReadPixels to copy the backbuffer and save it to a PNG, and the saved PNG is opaque on the developer's computer but transparent on other users'. Making it consistent instead makes sense to me.

I just don't really understand what the purpose of trying to request 8880 is instead of always requesting 8888 (since allowing window transparency is driven by a different property than the backbuffer bit sizes), if it's going to be 32 bit anyway and it's overall less consistent to have the split between requesting 8880 depending on non-GL configuration, versus getting back 8888 on different drivers.

slouken commented 1 year ago

Yeah, that's a good point. Let's try 8888 for consistency.

1bsyl commented 1 year ago

changing to 8888 also in SDL_test_common.c https://github.com/libsdl-org/SDL/pull/7482

1bsyl commented 1 year ago

marked as fixed updated sdl2-compat: https://github.com/libsdl-org/sdl2-compat/commit/a445d8b63aea6313431d1191b790b905bb663c84

1bsyl commented 1 year ago

for the record, this seems to cause some trouble using SDL+QT5: https://github.com/libsdl-org/SDL/issues/7511