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

API to get (and maybe set?) individual pixels in a surface #8319

Closed
smcv opened this issue Sep 29, 2023 · 4 comments · Fixed by #8334
Closed

API to get (and maybe set?) individual pixels in a surface #8319

smcv opened this issue Sep 29, 2023 · 4 comments · Fixed by #8334

Comments

@smcv
Copy link
Contributor

smcv commented Sep 29, 2023

When writing regression tests or assertions, particularly around image endianness, it's useful to be able to say things like "the pixel at (15,13) should have value rgba(1, 2, 3, 4)".

However, it's not at all obvious how to do this correctly for formats that are not 32 bits per pixel, as evidenced by SDLTest_CompareSurfaces() having been wrong for RGB24 on big-endian platforms ever since it was added (fixed in #8317). As seen in #8317, 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() to decode it into its colour channels.

I think it might be useful to have something like:

SDL_bool SDL_ReadSurfacePixel(SDL_Surface *, int x, int y, Uint8 *r, Uint8 *g, Uint8 *b, Uint8 *a);

which would do the rowstride arithmetic internally, and let you do something like:

SDL_assert_always (SDL_ReadSurfacePixel(surface, 15, 13, &r, &g, &b, &a));
SDL_assert_always (r == 1);
SDL_assert_always (g == 2);
...

This would have to lock the surface if it's in a non-trivial format, so it could be slow, but that's fine for something testing-oriented. (Because locking is refcounted, interfaces that iterate through an image, like SDLTest_CompareSurfaces(), could speed this up considerably by locking before iteration and unlocking afterwards.)

For completeness we could also have

SDL_bool SDL_WriteSurfacePixel(SDL_Surface *, int x, int y, Uint8 r, Uint8 g, Uint8 b, Uint8 a);

but that seems less useful in practice.

@smcv
Copy link
Contributor Author

smcv commented Sep 29, 2023

See also #8320 and particularly #8320 (comment). It would probably be useful to have this as an internal function to be shared by SDLTest_CompareSurfaces and the shape stuff, even if it isn't public API.

@sulix
Copy link
Contributor

sulix commented Sep 29, 2023

See also: #4867

IIRC, there was some pushback to making this function/macro public (as it could be very slow), but we've clearly implemented this incorrectly often enough that having some helper would make sense (even if callers are later optimised to avoid using it).

@smcv
Copy link
Contributor Author

smcv commented Sep 29, 2023

On #4867, @slouken wrote:

We don't want the overhead of a function call for each pixel

... but Standard C says memcpy() is the only valid way to do potentially-unaligned accesses, and a decent compiler will optimize memcpy() into whatever is the best way to do a potentially-unaligned access (on x86, it usually ends up being the same machine code as the pointer cast).

I think part of the problem here is that SDL carefully avoids using memcpy() because it doesn't want a C runtime library dependency, and instead uses SDL_memcpy(), but compilers don't know that they can optimize away SDL_memcpy() and turn it into inlined machine code.

@icculus
Copy link
Collaborator

icculus commented Sep 29, 2023

I would rather this not end up in the public API, but we can add it to libSDL_test.

smcv added a commit to smcv/SDL that referenced this issue Oct 2, 2023
This is essentially the same as was added in d95d2d7, but with clearer
error handling. It's implemented in a private header file so that it
can be shared with SDL_shape, which also wants this functionality.

Resolves: libsdl-org#8319
Signed-off-by: Simon McVittie <[email protected]>
slouken pushed a commit that referenced this issue Oct 10, 2023
This is essentially the same as was added in d95d2d7, but with clearer
error handling. It's implemented in a private header file so that it
can be shared with SDL_shape, which also wants this functionality.

Resolves: #8319
Signed-off-by: Simon McVittie <[email protected]>
Kontrabant pushed a commit to Kontrabant/SDL that referenced this issue Oct 22, 2023
This is essentially the same as was added in d95d2d7, but with clearer
error handling. It's implemented in a private header file so that it
can be shared with SDL_shape, which also wants this functionality.

Resolves: libsdl-org#8319
Signed-off-by: Simon McVittie <[email protected]>
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