Merge pull request #5466 from IvanSavenko/sdl_crash_fix

[1.6.7] Possible fix for crash on attempt to free sdl surface in worked thread

client/renderSDL/SDLImage.cpp

@@ -93,7 +93,9 @@ SDLImageShared::SDLImageShared(const ImagePath & filename)
 
 void SDLImageShared::scaledDraw(SDL_Surface * where, SDL_Palette * palette, const Point & scaleTo, const Point & dest, const Rect * src, const ColorRGBA & colorMultiplier, uint8_t alpha, EImageBlitMode mode) const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (!surf)
 		return;
 
@@ -154,7 +156,9 @@ void SDLImageShared::scaledDraw(SDL_Surface * where, SDL_Palette * palette, cons
 
 void SDLImageShared::draw(SDL_Surface * where, SDL_Palette * palette, const Point & dest, const Rect * src, const ColorRGBA & colorMultiplier, uint8_t alpha, EImageBlitMode mode) const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (!surf)
 		return;
 
@@ -221,7 +225,9 @@ void SDLImageShared::optimizeSurface()
 
 std::shared_ptr<const ISharedImage> SDLImageShared::scaleInteger(int factor, SDL_Palette * palette, EImageBlitMode mode) const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (factor <= 0)
 		throw std::runtime_error("Unable to scale by integer value of " + std::to_string(factor));
 
@@ -274,7 +280,9 @@ bool SDLImageShared::isLoading() const
 
 std::shared_ptr<const ISharedImage> SDLImageShared::scaleTo(const Point & size, SDL_Palette * palette) const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (palette && surf->format->palette)
 		SDL_SetSurfacePalette(surf, palette);
 
@@ -310,7 +318,9 @@ void SDLImageShared::exportBitmap(const boost::filesystem::path& path, SDL_Palet
 	directory.remove_filename();
 	boost::filesystem::create_directories(directory);
 
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (!surf)
 		return;
 
@@ -323,7 +333,9 @@ void SDLImageShared::exportBitmap(const boost::filesystem::path& path, SDL_Palet
 
 bool SDLImageShared::isTransparent(const Point & coords) const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (surf)
 		return CSDL_Ext::isTransparent(surf, coords.x - margins.x, coords.y	- margins.y);
 	else
@@ -332,7 +344,9 @@ bool SDLImageShared::isTransparent(const Point & coords) const
 
 Rect SDLImageShared::contentRect() const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	auto tmpMargins = margins;
 	auto tmpSize = Point(surf->w, surf->h);
 	return Rect(tmpMargins, tmpSize);
@@ -340,7 +354,9 @@ Rect SDLImageShared::contentRect() const
 
 const SDL_Palette * SDLImageShared::getPalette() const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (!surf)
 		return nullptr;
 	return surf->format->palette;
@@ -348,13 +364,17 @@ const SDL_Palette * SDLImageShared::getPalette() const
 
 Point SDLImageShared::dimensions() const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	return fullSize;
 }
 
 std::shared_ptr<const ISharedImage> SDLImageShared::horizontalFlip() const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (!surf)
 		return shared_from_this();
 
@@ -370,7 +390,9 @@ std::shared_ptr<const ISharedImage> SDLImageShared::horizontalFlip() const
 
 std::shared_ptr<const ISharedImage> SDLImageShared::verticalFlip() const
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	if (!surf)
 		return shared_from_this();
 
@@ -387,7 +409,9 @@ std::shared_ptr<const ISharedImage> SDLImageShared::verticalFlip() const
 // Keep the original palette, in order to do color switching operation
 void SDLImageShared::savePalette()
 {
-	assert(upscalingInProgress == false);
+	if(upscalingInProgress)
+		throw std::runtime_error("Attempt to access images that is still being loaded!");
+
 	// For some images that don't have palette, skip this
 	if(surf->format->palette == nullptr)
 		return;

client/renderSDL/SDLImageScaler.cpp

@@ -12,6 +12,7 @@
 
 #include "SDL_Extensions.h"
 
+#include "../gui/CGuiHandler.h"
 #include "../CMT.h"
 #include "../xBRZ/xbrz.h"
 
@@ -227,12 +228,19 @@ SDLImageScaler::SDLImageScaler(SDL_Surface * surf, const Rect & virtualDimension
 		SDL_FreeSurface(intermediate);
 		intermediate = SDL_ConvertSurfaceFormat(surf, SDL_PIXELFORMAT_ARGB8888, 0);
 	}
+
+	if (intermediate == surf)
+		throw std::runtime_error("Scaler uses same surface as input!");
 }
 
 SDLImageScaler::~SDLImageScaler()
 {
-	SDL_FreeSurface(intermediate);
+	GH.dispatchMainThread([surface = intermediate]()
-	SDL_FreeSurface(ret);
+	{
+		// potentially SDL bug, execute SDL_FreeSurface in main thread to avoid thread races to its internal state
+		// may be fixed somewhere between 2.26.5 - 2.30
+		SDL_FreeSurface(surface);
+	});
 }
 
 SDL_Surface * SDLImageScaler::acquireResultSurface()

client/renderSDL/ScalableImage.cpp

@@ -271,11 +271,6 @@ void ScalableImageShared::draw(SDL_Surface * where, const Point & dest, const Re
 		return images[index];
 	};
 
-	const auto & flipAndDraw = [&](FlippedImages & images, const ColorRGBA & colorMultiplier, uint8_t alphaValue){
-
-		getFlippedImage(images)->draw(where, parameters.palette, dest, src, colorMultiplier, alphaValue, locator.layer);
-	};
-
 	bool shadowLoading = scaled.at(scalingFactor).shadow.at(0) && scaled.at(scalingFactor).shadow.at(0)->isLoading();
 	bool bodyLoading = scaled.at(scalingFactor).body.at(0) && scaled.at(scalingFactor).body.at(0)->isLoading();
 	bool overlayLoading = scaled.at(scalingFactor).overlay.at(0) && scaled.at(scalingFactor).overlay.at(0)->isLoading();
@@ -292,7 +287,7 @@ void ScalableImageShared::draw(SDL_Surface * where, const Point & dest, const Re
 	}
 
 	if (scaled.at(scalingFactor).shadow.at(0))
-		flipAndDraw(scaled.at(scalingFactor).shadow, Colors::WHITE_TRUE, parameters.alphaValue);
+		getFlippedImage(scaled.at(scalingFactor).shadow)->draw(where, parameters.palette, dest, src, Colors::WHITE_TRUE, parameters.alphaValue, locator.layer);
 
 	if (parameters.player != PlayerColor::CANNOT_DETERMINE && scaled.at(scalingFactor).playerColored.at(1+parameters.player.getNum()))
 	{
@@ -301,14 +296,14 @@ void ScalableImageShared::draw(SDL_Surface * where, const Point & dest, const Re
 	else
 	{
 		if (scaled.at(scalingFactor).body.at(0))
-			flipAndDraw(scaled.at(scalingFactor).body, parameters.colorMultiplier, parameters.alphaValue);
+			getFlippedImage(scaled.at(scalingFactor).body)->draw(where, parameters.palette, dest, src, parameters.colorMultiplier, parameters.alphaValue, locator.layer);
 
 		if (scaled.at(scalingFactor).bodyGrayscale.at(0) && parameters.effectColorMultiplier.a != ColorRGBA::ALPHA_TRANSPARENT)
-			flipAndDraw(scaled.at(scalingFactor).bodyGrayscale, parameters.effectColorMultiplier, parameters.alphaValue);
+			getFlippedImage(scaled.at(scalingFactor).bodyGrayscale)->draw(where, parameters.palette, dest, src, parameters.effectColorMultiplier, parameters.alphaValue, locator.layer);
 	}
 
 	if (scaled.at(scalingFactor).overlay.at(0))
-		flipAndDraw(scaled.at(scalingFactor).overlay, parameters.ovelayColorMultiplier, static_cast<int>(parameters.alphaValue) * parameters.ovelayColorMultiplier.a / 255);
+		getFlippedImage(scaled.at(scalingFactor).overlay)->draw(where, parameters.palette, dest, src, parameters.ovelayColorMultiplier, static_cast<int>(parameters.alphaValue) * parameters.ovelayColorMultiplier.a / 255, locator.layer);
 }
 
 const SDL_Palette * ScalableImageShared::getPalette() const