From 8830b466d0bfd2429879b7e2189119c0898ceda7 Mon Sep 17 00:00:00 2001 From: Sam Lantinga Date: Wed, 8 Oct 2025 10:00:21 -0700 Subject: [PATCH] Improve handling of surfaces with NULL pixels Fixes https://github.com/libsdl-org/SDL/issues/14059 --- src/video/SDL_fillrect.c | 11 ++- src/video/SDL_surface.c | 134 +++++++++++++++++++----------- test/testautomation_surface.c | 150 +++++++++++++++++++++++++++++++++- 3 files changed, 237 insertions(+), 58 deletions(-) diff --git a/src/video/SDL_fillrect.c b/src/video/SDL_fillrect.c index a32c3c6c64..5be1eec527 100644 --- a/src/video/SDL_fillrect.c +++ b/src/video/SDL_fillrect.c @@ -266,17 +266,16 @@ bool SDL_FillSurfaceRects(SDL_Surface *dst, const SDL_Rect *rects, int count, Ui return SDL_InvalidParamError("SDL_FillSurfaceRects(): dst"); } - // Perform software fill - CHECK_PARAM(!dst->pixels) { - return SDL_SetError("SDL_FillSurfaceRects(): You must lock the surface"); - } - CHECK_PARAM(!rects) { return SDL_InvalidParamError("SDL_FillSurfaceRects(): rects"); } + if (!dst->pixels && SDL_MUSTLOCK(dst)) { + return SDL_SetError("SDL_FillSurfaceRects(): You must lock the surface"); + } + // Nothing to do - if (dst->w == 0 || dst->h == 0) { + if (dst->w == 0 || dst->h == 0 || !dst->pixels) { return true; } diff --git a/src/video/SDL_surface.c b/src/video/SDL_surface.c index f2cffae99f..82a24c60eb 100644 --- a/src/video/SDL_surface.c +++ b/src/video/SDL_surface.c @@ -598,7 +598,7 @@ bool SDL_SetSurfaceRLE(SDL_Surface *surface, bool enabled) { int flags; - CHECK_PARAM(!SDL_SurfaceValid(surface)) { + CHECK_PARAM(!SDL_SurfaceValid(surface) || SDL_ISPIXELFORMAT_FOURCC(surface->format)) { return SDL_InvalidParamError("surface"); } @@ -1017,10 +1017,10 @@ bool SDL_BlitSurface(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst SDL_Rect r_src, r_dst; // Make sure the surfaces aren't locked - CHECK_PARAM(!SDL_SurfaceValid(src)) { + CHECK_PARAM(!SDL_SurfaceValid(src) || (!src->pixels && !SDL_MUSTLOCK(src))) { return SDL_InvalidParamError("src"); } - CHECK_PARAM(!SDL_SurfaceValid(dst)) { + CHECK_PARAM(!SDL_SurfaceValid(dst) || (!dst->pixels && !SDL_MUSTLOCK(dst))) { return SDL_InvalidParamError("dst"); } CHECK_PARAM((src->flags & SDL_SURFACE_LOCKED) || (dst->flags & SDL_SURFACE_LOCKED)) { @@ -1094,12 +1094,6 @@ bool SDL_BlitSurface(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst static bool SDL_BlitSurfaceClippedScaled(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst, const SDL_Rect *dstrect, SDL_ScaleMode scaleMode) { // We need to scale first, then blit into dst because we're clipping in the destination surface pixel coordinates - if (SDL_MUSTLOCK(src)) { - if (!SDL_LockSurface(src)) { - return false; - } - } - bool result; int saved_w = src->w; int saved_h = src->h; @@ -1117,22 +1111,19 @@ static bool SDL_BlitSurfaceClippedScaled(SDL_Surface *src, const SDL_Rect *srcre src->w = saved_w; src->h = saved_h; src->pixels = saved_pixels; - - if (SDL_MUSTLOCK(src)) { - SDL_UnlockSurface(src); - } return result; } bool SDL_BlitSurfaceScaled(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surface *dst, const SDL_Rect *dstrect, SDL_ScaleMode scaleMode) { SDL_Rect r_src, r_dst; + bool result; // Make sure the surfaces aren't locked - CHECK_PARAM(!SDL_SurfaceValid(src) || !src->pixels) { + CHECK_PARAM(!SDL_SurfaceValid(src) || (!src->pixels && !SDL_MUSTLOCK(src))) { return SDL_InvalidParamError("src"); } - CHECK_PARAM(!SDL_SurfaceValid(dst) || !dst->pixels) { + CHECK_PARAM(!SDL_SurfaceValid(dst) || (!dst->pixels && !SDL_MUSTLOCK(dst))) { return SDL_InvalidParamError("dst"); } CHECK_PARAM((src->flags & SDL_SURFACE_LOCKED) || (dst->flags & SDL_SURFACE_LOCKED)) { @@ -1214,7 +1205,29 @@ bool SDL_BlitSurfaceScaled(SDL_Surface *src, const SDL_Rect *srcrect, SDL_Surfac return SDL_BlitSurfaceClippedScaled(src, &r_src, dst, &r_dst, scaleMode); } - return SDL_BlitSurfaceUncheckedScaled(src, &r_src, dst, &r_dst, scaleMode); + if (SDL_MUSTLOCK(src)) { + if (!SDL_LockSurface(src)) { + return false; + } + } + if (SDL_MUSTLOCK(dst)) { + if (!SDL_LockSurface(dst)) { + if (SDL_MUSTLOCK(src)) { + SDL_UnlockSurface(src); + } + return false; + } + } + + result = SDL_BlitSurfaceUncheckedScaled(src, &r_src, dst, &r_dst, scaleMode); + + if (SDL_MUSTLOCK(src)) { + SDL_UnlockSurface(src); + } + if (SDL_MUSTLOCK(dst)) { + SDL_UnlockSurface(dst); + } + return result; } /** @@ -1900,7 +1913,11 @@ SDL_Surface *SDL_ConvertSurfaceAndColorspace(SDL_Surface *surface, SDL_PixelForm src_properties = surface->props; // Create a new surface with the desired format - convert = SDL_CreateSurface(surface->w, surface->h, format); + if (surface->pixels || SDL_MUSTLOCK(surface)) { + convert = SDL_CreateSurface(surface->w, surface->h, format); + } else { + convert = SDL_CreateSurfaceFrom(surface->w, surface->h, format, NULL, 0); + } if (!convert) { goto error; } @@ -1992,7 +2009,11 @@ SDL_Surface *SDL_ConvertSurfaceAndColorspace(SDL_Surface *surface, SDL_PixelForm } } - result = SDL_BlitSurfaceUnchecked(surface, &bounds, convert, &bounds); + if (surface->pixels) { + result = SDL_BlitSurfaceUnchecked(surface, &bounds, convert, &bounds); + } else { + result = true; + } // Restore colorkey alpha value if (palette_ck_transform) { @@ -2177,40 +2198,49 @@ SDL_Surface *SDL_ScaleSurface(SDL_Surface *surface, int width, int height, SDL_S } // Create a new surface with the desired size - convert = SDL_CreateSurface(width, height, surface->format); + if (surface->pixels || SDL_MUSTLOCK(surface)) { + convert = SDL_CreateSurface(width, height, surface->format); + } else { + convert = SDL_CreateSurfaceFrom(width, height, surface->format, NULL, 0); + } if (!convert) { goto error; } SDL_SetSurfacePalette(convert, surface->palette); SDL_SetSurfaceColorspace(convert, surface->colorspace); + SDL_SetSurfaceRLE(convert, SDL_SurfaceHasRLE(surface)); - // Save the original copy flags - copy_flags = surface->map.info.flags; - copy_color.r = surface->map.info.r; - copy_color.g = surface->map.info.g; - copy_color.b = surface->map.info.b; - copy_color.a = surface->map.info.a; - surface->map.info.r = 0xFF; - surface->map.info.g = 0xFF; - surface->map.info.b = 0xFF; - surface->map.info.a = 0xFF; - surface->map.info.flags = (copy_flags & (SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY)); - SDL_InvalidateMap(&surface->map); + if (surface->pixels || SDL_MUSTLOCK(surface)) { + // Save the original copy flags + copy_flags = surface->map.info.flags; + copy_color.r = surface->map.info.r; + copy_color.g = surface->map.info.g; + copy_color.b = surface->map.info.b; + copy_color.a = surface->map.info.a; + surface->map.info.r = 0xFF; + surface->map.info.g = 0xFF; + surface->map.info.b = 0xFF; + surface->map.info.a = 0xFF; + surface->map.info.flags = (copy_flags & (SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY)); + SDL_InvalidateMap(&surface->map); - rc = SDL_BlitSurfaceScaled(surface, NULL, convert, NULL, scaleMode); + rc = SDL_BlitSurfaceScaled(surface, NULL, convert, NULL, scaleMode); - // Clean up the original surface, and update converted surface - convert->map.info.r = copy_color.r; - convert->map.info.g = copy_color.g; - convert->map.info.b = copy_color.b; - convert->map.info.a = copy_color.a; - convert->map.info.flags = (copy_flags & ~(SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY)); - surface->map.info.r = copy_color.r; - surface->map.info.g = copy_color.g; - surface->map.info.b = copy_color.b; - surface->map.info.a = copy_color.a; - surface->map.info.flags = copy_flags; - SDL_InvalidateMap(&surface->map); + // Clean up the original surface, and update converted surface + convert->map.info.r = copy_color.r; + convert->map.info.g = copy_color.g; + convert->map.info.b = copy_color.b; + convert->map.info.a = copy_color.a; + convert->map.info.flags = (copy_flags & ~(SDL_COPY_RLE_COLORKEY | SDL_COPY_RLE_ALPHAKEY)); + surface->map.info.r = copy_color.r; + surface->map.info.g = copy_color.g; + surface->map.info.b = copy_color.b; + surface->map.info.a = copy_color.a; + surface->map.info.flags = copy_flags; + SDL_InvalidateMap(&surface->map); + } else { + rc = true; + } // SDL_BlitSurfaceScaled failed, and so the conversion if (!rc) { @@ -2239,8 +2269,18 @@ SDL_Surface *SDL_ConvertSurface(SDL_Surface *surface, SDL_PixelFormat format) SDL_Surface *SDL_DuplicatePixels(int width, int height, SDL_PixelFormat format, SDL_Colorspace colorspace, void *pixels, int pitch) { - SDL_Surface *surface = SDL_CreateSurface(width, height, format); - if (surface) { + SDL_Surface *surface; + if (pixels) { + surface = SDL_CreateSurface(width, height, format); + } else { + surface = SDL_CreateSurfaceFrom(width, height, format, NULL, 0); + } + if (!surface) { + return NULL; + } + SDL_SetSurfaceColorspace(surface, colorspace); + + if (surface->pixels) { int length = width * SDL_BYTESPERPIXEL(format); Uint8 *src = (Uint8 *)pixels; Uint8 *dst = (Uint8 *)surface->pixels; @@ -2250,8 +2290,6 @@ SDL_Surface *SDL_DuplicatePixels(int width, int height, SDL_PixelFormat format, dst += surface->pitch; src += pitch; } - - SDL_SetSurfaceColorspace(surface, colorspace); } return surface; } diff --git a/test/testautomation_surface.c b/test/testautomation_surface.c index c3aededf91..13b979a12e 100644 --- a/test/testautomation_surface.c +++ b/test/testautomation_surface.c @@ -707,6 +707,140 @@ static int SDLCALL surface_testBlitMultiple(void *arg) return TEST_COMPLETED; } +/** + * Tests operations on surfaces with NULL pixels + */ +static int SDLCALL surface_testSurfaceNULLPixels(void *arg) +{ + SDL_Surface *a, *b, *face; + bool result; + + face = SDLTest_ImageFace(); + SDLTest_AssertCheck(face != NULL, "Verify face surface is not NULL"); + if (face == NULL) { + return TEST_ABORTED; + } + + /* Test blitting with NULL pixels */ + a = SDL_CreateSurfaceFrom(face->w, face->h, SDL_PIXELFORMAT_ARGB8888, NULL, 0); + SDLTest_AssertCheck(a != NULL, "Verify result from SDL_CreateSurfaceFrom() with NULL pixels is not NULL"); + result = SDL_BlitSurface(a, NULL, face, NULL); + SDLTest_AssertCheck(!result, "Verify result from SDL_BlitSurface() with src having NULL pixels is false"); + result = SDL_BlitSurface(face, NULL, a, NULL); + SDLTest_AssertCheck(!result, "Verify result from SDL_BlitSurface() with dst having NULL pixels is false"); + + b = SDL_CreateSurfaceFrom(face->w * 2, face->h * 2, SDL_PIXELFORMAT_ARGB8888, NULL, 0); + SDLTest_AssertCheck(b != NULL, "Verify result from SDL_CreateSurfaceFrom() with NULL pixels is not NULL"); + result = SDL_BlitSurfaceScaled(b, NULL, face, NULL, SDL_SCALEMODE_NEAREST); + SDLTest_AssertCheck(!result, "Verify result from SDL_BlitSurfaceScaled() with src having NULL pixels is false"); + result = SDL_BlitSurfaceScaled(face, NULL, b, NULL, SDL_SCALEMODE_NEAREST); + SDLTest_AssertCheck(!result, "Verify result from SDL_BlitSurfaceScaled() with dst having NULL pixels is false"); + SDL_DestroySurface(b); + b = NULL; + + /* Test conversion with NULL pixels */ + b = SDL_ConvertSurfaceAndColorspace(a, SDL_PIXELFORMAT_ABGR8888, NULL, SDL_COLORSPACE_UNKNOWN, 0); + SDLTest_AssertCheck(b != NULL, "Verify result from SDL_ConvertSurfaceAndColorspace() with NULL pixels is not NULL"); + SDL_DestroySurface(b); + b = NULL; + + /* Test duplication with NULL pixels */ + b = SDL_DuplicateSurface(a); + SDLTest_AssertCheck(b != NULL, "Verify result from SDL_DuplicateSurface() with NULL pixels is not NULL"); + SDL_DestroySurface(b); + b = NULL; + + /* Test scaling with NULL pixels */ + b = SDL_ScaleSurface(a, a->w * 2, a->h * 2, SDL_SCALEMODE_NEAREST); + SDLTest_AssertCheck(b != NULL, "Verify result from SDL_ScaleSurface() with NULL pixels is not NULL"); + SDLTest_AssertCheck(b->pixels == NULL, "Verify pixels from SDL_ScaleSurface() is NULL"); + SDL_DestroySurface(b); + b = NULL; + + /* Test filling surface with NULL pixels */ + result = SDL_FillSurfaceRect(a, NULL, 0); + SDLTest_AssertCheck(result, "Verify result from SDL_FillSurfaceRect() with dst having NULL pixels is true"); + + /* Clean up. */ + SDL_DestroySurface(face); + SDL_DestroySurface(a); + SDL_DestroySurface(b); + + return TEST_COMPLETED; +} + +/** + * Tests operations on surfaces with RLE pixels + */ +static int SDLCALL surface_testSurfaceRLEPixels(void *arg) +{ + SDL_Surface *face, *a, *b, *tmp; + bool result; + + face = SDLTest_ImageFace(); + SDLTest_AssertCheck(face != NULL, "Verify face surface is not NULL"); + if (face == NULL) { + return TEST_ABORTED; + } + + /* Create a temporary surface to trigger RLE encoding during blit */ + tmp = SDL_DuplicateSurface(face); + SDLTest_AssertCheck(tmp != NULL, "Verify result from SDL_DuplicateSurface() with RLE pixels is not NULL"); + + result = SDL_SetSurfaceRLE(face, true); + SDLTest_AssertCheck(result, "Verify result from SDL_SetSurfaceRLE() is true"); + + /* Test duplication with RLE pixels */ + a = SDL_DuplicateSurface(face); + SDLTest_AssertCheck(a != NULL, "Verify result from SDL_DuplicateSurface() with RLE pixels is not NULL"); + SDLTest_AssertCheck(SDL_SurfaceHasRLE(a), "Verify result from SDL_DuplicateSurface() with RLE pixels has RLE set"); + + /* Verify that blitting from an RLE surface does RLE encode it */ + SDLTest_AssertCheck(!SDL_MUSTLOCK(a), "Verify initial RLE surface does not need to be locked"); + SDLTest_AssertCheck(a->pixels != NULL, "Verify initial RLE surface has pixels available"); + result = SDL_BlitSurface(a, NULL, tmp, NULL); + SDLTest_AssertCheck(result, "Verify result from SDL_BlitSurface() with RLE surface is true"); + SDLTest_AssertCheck(SDL_MUSTLOCK(a), "Verify RLE surface after blit needs to be locked"); + SDLTest_AssertCheck(a->pixels == NULL, "Verify RLE surface after blit does not have pixels available"); + + /* Test scaling with RLE pixels */ + b = SDL_ScaleSurface(a, a->w * 2, a->h * 2, SDL_SCALEMODE_NEAREST); + SDLTest_AssertCheck(b != NULL, "Verify result from SDL_ScaleSurface() is not NULL"); + SDLTest_AssertCheck(SDL_SurfaceHasRLE(b), "Verify result from SDL_ScaleSurface() with RLE pixels has RLE set"); + + /* Test scaling blitting with RLE pixels */ + result = SDL_BlitSurfaceScaled(a, NULL, b, NULL, SDL_SCALEMODE_NEAREST); + SDLTest_AssertCheck(result, "Verify result from SDL_BlitSurfaceScaled() with src having RLE pixels is true"); + SDL_BlitSurface(a, NULL, tmp, NULL); + SDL_DestroySurface(b); + b = NULL; + + /* Test conversion with RLE pixels */ + b = SDL_ConvertSurfaceAndColorspace(a, SDL_PIXELFORMAT_ABGR8888, NULL, SDL_COLORSPACE_UNKNOWN, 0); + SDLTest_AssertCheck(b != NULL, "Verify result from SDL_ConvertSurfaceAndColorspace() with RLE pixels is not NULL"); + SDLTest_AssertCheck(SDL_SurfaceHasRLE(b), "Verify result from SDL_ConvertSurfaceAndColorspace() with RLE pixels has RLE set"); + SDL_BlitSurface(a, NULL, tmp, NULL); + SDL_DestroySurface(b); + b = NULL; + +#if 0 /* This will currently fail, you must lock the surface first */ + /* Test filling surface with RLE pixels */ + result = SDL_FillSurfaceRect(a, NULL, 0); + SDLTest_AssertCheck(result, "Verify result from SDL_FillSurfaceRect() with dst having RLE pixels is true"); +#endif + + /* Make sure the RLE surface still needs to be locked after surface operations */ + SDLTest_AssertCheck(a->pixels == NULL, "Verify RLE surface after operations does not have pixels available"); + + /* Clean up. */ + SDL_DestroySurface(face); + SDL_DestroySurface(a); + SDL_DestroySurface(b); + SDL_DestroySurface(tmp); + + return TEST_COMPLETED; +} + /** * Tests surface conversion. */ @@ -740,9 +874,7 @@ static int SDLCALL surface_testSurfaceConversion(void *arg) /* Clean up. */ SDL_DestroySurface(face); - face = NULL; SDL_DestroySurface(rface); - rface = NULL; return TEST_COMPLETED; } @@ -991,9 +1123,9 @@ static int SDLCALL surface_testBlitInvalid(void *arg) SDLTest_AssertCheck(invalid->pixels == NULL, "Check surface pixels are NULL"); result = SDL_BlitSurface(invalid, NULL, valid, NULL); - SDLTest_AssertCheck(result == true, "SDL_BlitSurface(invalid, NULL, valid, NULL), result = %s\n", result ? "true" : "false"); + SDLTest_AssertCheck(result == false, "SDL_BlitSurface(invalid, NULL, valid, NULL), result = %s\n", result ? "true" : "false"); result = SDL_BlitSurface(valid, NULL, invalid, NULL); - SDLTest_AssertCheck(result == true, "SDL_BlitSurface(valid, NULL, invalid, NULL), result = %s\n", result ? "true" : "false"); + SDLTest_AssertCheck(result == false, "SDL_BlitSurface(valid, NULL, invalid, NULL), result = %s\n", result ? "true" : "false"); result = SDL_BlitSurfaceScaled(invalid, NULL, valid, NULL, SDL_SCALEMODE_NEAREST); SDLTest_AssertCheck(result == false, "SDL_BlitSurfaceScaled(invalid, NULL, valid, NULL, SDL_SCALEMODE_NEAREST), result = %s\n", result ? "true" : "false"); @@ -1767,6 +1899,14 @@ static const SDLTest_TestCaseReference surfaceTestLoadFailure = { surface_testLoadFailure, "surface_testLoadFailure", "Tests sprite loading. A failure case.", TEST_ENABLED }; +static const SDLTest_TestCaseReference surfaceTestNULLPixels = { + surface_testSurfaceNULLPixels, "surface_testSurfaceNULLPixels", "Tests surface operations with NULL pixels.", TEST_ENABLED +}; + +static const SDLTest_TestCaseReference surfaceTestRLEPixels = { + surface_testSurfaceRLEPixels, "surface_testSurfaceRLEPixels", "Tests surface operations with RLE surfaces.", TEST_ENABLED +}; + static const SDLTest_TestCaseReference surfaceTestSurfaceConversion = { surface_testSurfaceConversion, "surface_testSurfaceConversion", "Tests surface conversion.", TEST_ENABLED }; @@ -1857,6 +1997,8 @@ static const SDLTest_TestCaseReference *surfaceTests[] = { &surfaceTestBlit9Grid, &surfaceTestBlitMultiple, &surfaceTestLoadFailure, + &surfaceTestNULLPixels, + &surfaceTestRLEPixels, &surfaceTestSurfaceConversion, &surfaceTestCompleteSurfaceConversion, &surfaceTestBlitColorMod,