SDLTest_CompareSurfaces: Decode pixels correctly on big-endian platforms #8317
Conversation
Previously, if acting on a surface with less than 32 bits per pixel, this code was placing the pixel value from the surface in the first few bytes of the Uint32 to be decoded, and unrelated data from a subsequent pixel in the remaining bytes. Because SDL_GetRGBA takes the bits to be decoded from the least-significant bits of the given value, ignoring the higher-order bits if any, this happened to be correct on little-endian platforms, where the first few bytes store the least-significant bits of an integer. However, it was incorrect on big-endian, where the first few bytes are the most-significant bits of an integer. The previous implementation also assumed that unaligned access to a 32-bit quantity is possible, which is not the case on all CPUs (but happens to be true on x86). These issues were not discovered until now because SDLTest_CompareSurfaces() is only used in testautomation, which until recently was not being run routinely at build-time, because it contained other assumptions that can fail in an autobuilder or CI environment. Resolves: libsdl-org#8315 Signed-off-by: Simon McVittie <[email protected]>
|
It seems like a trap that |
This API change might make sense for SDL3. Can you create a separate issue for this? |
Previously, if acting on a surface with less than 32 bits per pixel, this code was placing the pixel value from the surface in the first few bytes of the Uint32 to be decoded, and unrelated data from a subsequent pixel in the remaining bytes.
Because SDL_GetRGBA takes the bits to be decoded from the least-significant bits of the given value, ignoring the higher-order bits if any, this happened to be correct on little-endian platforms, where the first few bytes store the least-significant bits of an integer.
However, it was incorrect on big-endian, where the first few bytes are the most-significant bits of an integer.
The previous implementation also assumed that unaligned access to a 32-bit quantity is possible, which is not the case on all CPUs (but happens to be true on x86).
These issues were not discovered until now because SDLTest_CompareSurfaces() is only used in testautomation, which until recently was not being run routinely at build-time, because it contained other assumptions that can fail in an autobuilder or CI environment.
Resolves: #8315
I think this would also be worthwhile to cherry-pick to SDL 2 and sdl2-compat, if it's applicable there.
It might even be a good idea to have a slow, simple but fully-correct
SDL_ReadSurfacePixel(SDL_Surface *, int x, int y, Uint8 *r, Uint8 *g, Uint8 *b, Uint8 *a)as public API, but I'd prefer to fix the test with this private implementation before promoting it into public API, because I'd prefer not to have API design considerations delay the fix.