Add internal function to read individual pixels from a surface #8334
Conversation
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]>
This shares its implementation with SDLTest_ReadSurfacePixel: the same code is compiled twice, to get it into the static test library and also the public shared library. Signed-off-by: Simon McVittie <[email protected]>
This avoids assuming that the pixels are suitably aligned for direct access, which there's no guarantee that they are; in particular, 3-bytes-per-pixel RGB images are likely to have 3 out of 4 pixels misaligned. On x86, dereferencing a misaligned pointer does what you would expect, but on other architectures it's undefined whether it will work, crash with SIGBUS, or silently give a wrong answer. Signed-off-by: Simon McVittie <[email protected]>
| if (surface == NULL || surface->format == NULL || surface->pixels == NULL) { | ||
| return SDL_InvalidParamError("surface"); | ||
| } | ||
|
|
||
| if (x < 0 || x >= surface->w) { | ||
| return SDL_InvalidParamError("x"); | ||
| } | ||
|
|
||
| if (y < 0 || y >= surface->h) { | ||
| return SDL_InvalidParamError("y"); | ||
| } | ||
|
|
||
| if (r == NULL) { | ||
| return SDL_InvalidParamError("r"); | ||
| } | ||
|
|
||
| if (g == NULL) { | ||
| return SDL_InvalidParamError("g"); | ||
| } | ||
|
|
||
| if (b == NULL) { | ||
| return SDL_InvalidParamError("b"); | ||
| } | ||
|
|
||
| if (a == NULL) { | ||
| return SDL_InvalidParamError("a"); | ||
| } |
There was a problem hiding this comment.
Perhaps all this invalid-parameter handling is unnecessary for a function that's only exposed internally within SDL (SDL_ReadSurfacePixel), and to tests (SDLTest_ReadSurfacePixel)?
| case (3): | ||
| pixel_value = *(Uint32 *)pixel & (~shape->format->Amask); | ||
| break; |
There was a problem hiding this comment.
I am suspicious about this code, which is why I'm replacing it. If the pointer pixel is misaligned, that's undefined behaviour, and on some CPU architectures (mostly RISC), it will either crash or give silently wrong answers (on ARM I think it traditionally gave the result that would have been correct for dereferencing the last correctly-aligned pointer less than or equal to this one).
|
This is clearly not a very efficient thing to do, but it's certainly good enough for unit tests, and probably also good enough for its use in SDL_shape, which is not something you're likely to do in an inner loop. |
test: Extract SDLTest_ReadSurfacePixel
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: API to get (and maybe set?) individual pixels in a surface #8319
surface: Add a private SDL_ReadSurfacePixel
This shares its implementation with SDLTest_ReadSurfacePixel: the same
code is compiled twice, to get it into the static test library and also
the public shared library.
shape: Use SDL[Test]_ReadSurfacePixel
This avoids assuming that the pixels are suitably aligned for direct
access, which there's no guarantee that they are; in particular,
3-bytes-per-pixel RGB images are likely to have 3 out of 4 pixels
misaligned. On x86, dereferencing a misaligned pointer does what you
would expect, but on other architectures it's undefined whether it will
work, crash with SIGBUS, or silently give a wrong answer.