Improve handling of surfaces with NULL pixels

Fixes https://github.com/libsdl-org/SDL/issues/14059

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;
     }
 

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);
-
-    rc = SDL_BlitSurfaceScaled(surface, NULL, convert, NULL, scaleMode);
+    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);
 
-    // 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);
+        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);
+    } 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;
 }

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,