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

SDL_GetRGBA, SDL_GetRGB are hard to use correctly #8320

Closed
smcv opened this issue Sep 29, 2023 · 3 comments · Fixed by #8863
Closed

SDL_GetRGBA, SDL_GetRGB are hard to use correctly #8320

smcv opened this issue Sep 29, 2023 · 3 comments · Fixed by #8863
Assignees
Milestone

Comments

@smcv
Copy link
Contributor

smcv commented Sep 29, 2023

As seen in #8317, to do format-sensitive colour conversion, you have to memcpy the right number of bytes into the low-order bits of a Uint32 (which do not necessarily start at byte 0!), and then pass that to SDL_GetRGBA() or SDL_GetRGB() to decode it into its colour channels. This seems unnecessarily tricky to get right.

One possibility to make this easier (and make implementing #8319 easier, if we want it) would be to change their signature from the current:

void SDL_GetRGB(Uint32 pixel, const SDL_PixelFormat *, Uint8 *, Uint8 *, Uint8 *);
void SDL_GetRGBA(Uint32 pixel, const SDL_PixelFormat *, Uint8 *, Uint8 *, Uint8 *, Uint8 *);

to be something more like

void SDL_GetRGB(const void *pixel, const SDL_PixelFormat *, Uint8 *, Uint8 *, Uint8 *);
void SDL_GetRGBA(const void *pixel, const SDL_PixelFormat *, Uint8 *, Uint8 *, Uint8 *, Uint8 *);

and do the "memcpy the right number of bytes into the low-order bits of a Uint32" step internally.

This might be troublesome for src/render/software/SDL_drawline.c, though.

@smcv
Copy link
Contributor Author

smcv commented Sep 29, 2023

Relatedly, I'm not sure that the uses of SDL_GetRGBA in src/video/SDL_shape.c and test/testshape.c are actually correct for 24-bits-per-pixel images, particularly on big-endian. They're certainly wrong for CPU architectures where unaligned accesses cause a SIGBUS or a silently wrong answer. But maybe nobody ever actually uses 24bpp in those contexts?

@slouken
Copy link
Collaborator

slouken commented Nov 8, 2023

@icculus, I'd like to go ahead and export the functions @smcv added. If you're calling into the SDL API for each pixel, you don't care about speed, and we should at least make sure it works with the least amount of caveats.

@slouken slouken added this to the 3.2.0 milestone Nov 8, 2023
@icculus
Copy link
Collaborator

icculus commented Nov 8, 2023

As long as the documentation has a big "this is not fast!" warning on it, that's fine with me.

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 a pull request may close this issue.

3 participants