Improved performance of ImageData:mapPixel (now up to 2x as fast as in 0.8.0!); love.graphics.newImage, Image:refresh, love.window.setIcon, and ImageRasterizers now prevent other threads from modifying the ImageData used in those functions until they're done accessing it

src/modules/font/ImageRasterizer.cpp

@@ -73,13 +73,16 @@ GlyphData *ImageRasterizer::getGlyphData(uint32 glyph) const
 	if (gm.width == 0)
 		return g;
 
+	// We don't want another thread modifying our ImageData mid-copy.
+	love::thread::Lock lock(imageData->getMutex());
+
 	love::image::pixel *gdpixels = (love::image::pixel *) g->getData();
 	love::image::pixel *imagepixels = (love::image::pixel *) imageData->getData();
 
 	// copy glyph pixels from imagedata to glyphdata
 	for (int i = 0; i < g->getWidth() * g->getHeight(); i++)
 	{
-		love::image::pixel p = imagepixels[ it->second.x + (i % gm.width) + (imageData->getWidth() * (i / gm.width)) ];
+		love::image::pixel p = imagepixels[it->second.x + (i % gm.width) + (imageData->getWidth() * (i / gm.width))];
 
 		// Use transparency instead of the spacer color
 		if (equal(p, spacer))
@@ -98,6 +101,9 @@ void ImageRasterizer::load()
 	int imgw = imageData->getWidth();
 	int imgh = imageData->getHeight();
 
+	// We don't want another thread modifying our ImageData mid-parse.
+	love::thread::Lock lock(imageData->getMutex());
+
 	// Set the only metric that matters
 	metrics.height = imgh;
 

src/modules/graphics/opengl/Image.cpp

@@ -165,6 +165,7 @@ void Image::uploadCompressedMipmaps()
 
 void Image::createMipmaps()
 {
+	// Only valid for Images created with ImageData.
 	if (!data)
 		return;
 
@@ -183,6 +184,9 @@ void Image::createMipmaps()
 
 	bind();
 
+	// Prevent other threads from changing the ImageData while we upload it.
+	love::thread::Lock lock(data->getMutex());
+
 	if (hasNpot() && (GLEE_VERSION_3_0 || GLEE_ARB_framebuffer_object))
 	{
 		// AMD/ATI drivers have several bugs when generating mipmaps,
@@ -376,6 +380,9 @@ bool Image::loadVolatilePOT()
 	vertices[2].s = s;
 	vertices[3].s = s;
 
+	// We want this lock to potentially cover mipmap creation as well.
+	love::thread::EmptyLock lock;
+
 	while (glGetError() != GL_NO_ERROR); // clear errors
 
 	if (isCompressed() && cdata)
@@ -407,6 +414,7 @@ bool Image::loadVolatilePOT()
 		             GL_UNSIGNED_BYTE,
 		             0);
 
+		lock.setLock(data->getMutex());
 		glTexSubImage2D(GL_TEXTURE_2D,
 		                0,
 		                0, 0,
@@ -436,6 +444,9 @@ bool Image::loadVolatileNPOT()
 	filter.anisotropy = gl.setTextureFilter(filter);
 	gl.setTextureWrap(wrap);
 
+	// We want this lock to potentially cover mipmap creation as well.
+	love::thread::EmptyLock lock;
+
 	while (glGetError() != GL_NO_ERROR); // clear errors
 
 	if (isCompressed() && cdata)
@@ -452,6 +463,7 @@ bool Image::loadVolatileNPOT()
 	}
 	else if (data)
 	{
+		lock.setLock(data->getMutex());
 		glTexImage2D(GL_TEXTURE_2D,
 		             0,
 		             GL_RGBA8,
@@ -490,6 +502,9 @@ bool Image::refresh()
 	if (texture == 0)
 		return false;
 
+	// We want this lock to potentially cover mipmap creation as well.
+	love::thread::EmptyLock lock;
+
 	bind();
 
 	if (isCompressed() && cdata)
@@ -497,7 +512,7 @@ bool Image::refresh()
 		GLenum format = getCompressedFormat(cdata->getType());
 		glCompressedTexSubImage2DARB(GL_TEXTURE_2D,
 		                             0,
-								     0, 0,
+		                             0, 0,
 		                             cdata->getWidth(0),
 		                             cdata->getHeight(0),
 		                             format,
@@ -506,6 +521,7 @@ bool Image::refresh()
 	}
 	else if (data)
 	{
+		lock.setLock(data->getMutex());
 		glTexSubImage2D(GL_TEXTURE_2D,
 		                0,
 		                0, 0,

src/modules/image/ImageData.cpp

@@ -70,7 +70,16 @@ void ImageData::setPixel(int x, int y, pixel c)
 
 	Lock lock(mutex);
 
-	pixel *pixels = (pixel *)getData();
+	pixel *pixels = (pixel *) getData();
+	pixels[y*getWidth()+x] = c;
+}
+
+void ImageData::setPixelUnsafe(int x, int y, love::image::pixel c)
+{
+	if (!inside(x, y))
+		throw love::Exception("Attempt to set out-of-range pixel!");
+
+	pixel *pixels = (pixel *) getData();
 	pixels[y*getWidth()+x] = c;
 }
 
@@ -79,7 +88,7 @@ pixel ImageData::getPixel(int x, int y) const
 	if (!inside(x, y))
 		throw love::Exception("Attempt to get out-of-range pixel!");
 
-	pixel *pixels = (pixel *)getData();
+	const pixel *pixels = (const pixel *) getData();
 	return pixels[y*getWidth()+x];
 }
 
@@ -151,6 +160,11 @@ void ImageData::paste(ImageData *src, int dx, int dy, int sx, int sy, int sw, in
 	}
 }
 
+love::thread::Mutex *ImageData::getMutex() const
+{
+	return mutex;
+}
+
 bool ImageData::getConstant(const char *in, ImageData::Format &out)
 {
 	return formats.find(in, out);

src/modules/image/ImageData.h

@@ -105,6 +105,12 @@ public:
 	 **/
 	void setPixel(int x, int y, pixel p);
 
+	/**
+	 * Sets the pixel at location (x,y).
+	 * Not thread-safe!
+	 **/
+	void setPixelUnsafe(int x, int y, pixel p);
+
 	/**
 	 * Gets the pixel at location (x,y).
 	 * @param x The location along the x-axis.
@@ -120,6 +126,8 @@ public:
 	 **/
 	virtual void encode(love::filesystem::File *f, Format format) = 0;
 
+	love::thread::Mutex *getMutex() const;
+
 	// Implements Data.
 	virtual void *getData() const;
 	virtual int getSize() const;

src/modules/image/wrap_ImageData.cpp

@@ -88,19 +88,19 @@ int w_ImageData_setPixel(lua_State *L)
 		for (int i = 1; i <= 4; i++)
 			lua_rawgeti(L, 4, i);
 
-		c.r = (unsigned char)luaL_checkint(L, -4);
+		c.r = (unsigned char)luaL_checkinteger(L, -4);
-		c.g = (unsigned char)luaL_checkint(L, -3);
+		c.g = (unsigned char)luaL_checkinteger(L, -3);
-		c.b = (unsigned char)luaL_checkint(L, -2);
+		c.b = (unsigned char)luaL_checkinteger(L, -2);
-		c.a = (unsigned char)luaL_optint(L, -1, 255);
+		c.a = (unsigned char)luaL_optinteger(L, -1, 255);
 
 		lua_pop(L, 4);
 	}
 	else
 	{
-		c.r = (unsigned char)luaL_checkint(L, 4);
+		c.r = (unsigned char)luaL_checkinteger(L, 4);
-		c.g = (unsigned char)luaL_checkint(L, 5);
+		c.g = (unsigned char)luaL_checkinteger(L, 5);
-		c.b = (unsigned char)luaL_checkint(L, 6);
+		c.b = (unsigned char)luaL_checkinteger(L, 6);
-		c.a = (unsigned char)luaL_optint(L, 7, 255);
+		c.a = (unsigned char)luaL_optinteger(L, 7, 255);
 	}
 
 	try
@@ -114,39 +114,101 @@ int w_ImageData_setPixel(lua_State *L)
 	return 0;
 }
 
-int w_ImageData_mapPixel(lua_State *L)
+// Gets the result of luaL_where as a string.
+static std::string luax_getwhere(lua_State *L, int lvl)
+{
+	std::string where;
+
+	luaL_where(L, lvl);
+	
+	const char *str = lua_tostring(L, -1);
+	if (str)
+		where = str;
+
+	lua_pop(L, 1);
+
+	return where;
+}
+
+// ImageData:mapPixel. Not thread-safe! See the wrapper function below.
+static int w_ImageData_mapPixelUnsafe(lua_State *L)
 {
 	ImageData *t = luax_checkimagedata(L, 1);
-
 	luaL_checktype(L, 2, LUA_TFUNCTION);
 
 	int w = t->getWidth();
 	int h = t->getHeight();
 
-	for (int i = 0; i < w; i++)
+	// Default pixel component values (r, g, b, a.)
+	const unsigned char pixel_defaults[4] = {0, 0, 0, 255};
+
+	for (int y = 0; y < h; y++)
 	{
-		for (int j = 0; j < h; j++)
+		for (int x = 0; x < w; x++)
 		{
 			lua_pushvalue(L, 2);
-			lua_pushnumber(L, i);
+			lua_pushnumber(L, x);
-			lua_pushnumber(L, j);
+			lua_pushnumber(L, y);
-			pixel c = t->getPixel(i, j);
+			pixel c = t->getPixel(x, y);
 			lua_pushnumber(L, c.r);
 			lua_pushnumber(L, c.g);
 			lua_pushnumber(L, c.b);
 			lua_pushnumber(L, c.a);
 			lua_call(L, 6, 4);
-			c.r = luaL_optint(L, -4, 0);
+
-			c.g = luaL_optint(L, -3, 0);
+			// If we used luaL_checkX / luaL_optX then we would get messy error
-			c.b = luaL_optint(L, -2, 0);
+			// messages (e.g. Error: bad argument #-1 to '?'), so while this is
-			c.a = luaL_optint(L, -1, 255);
+			// messier code, at least the errors are a bit more descriptive.
-			t->setPixel(i, j, c);
+
+			// Treat the pixel as an array for less code duplication. :(
+			unsigned char *parray = (unsigned char *) &c;
+			for (int i = 0; i < 4; i++)
+			{
+				int ttype = lua_type(L, -4 + i);
+				switch (ttype)
+				{
+				case LUA_TNUMBER:
+					parray[i] = lua_tointeger(L, -4 + i);
+					break;
+				case LUA_TNONE:
+				case LUA_TNIL:
+					parray[i] = pixel_defaults[i];
+					break;
+				default:
+					return luaL_error(L, "%sbad return value #%d (number expected, got %s)",
+					                  luax_getwhere(L, 2).c_str(), i+1, lua_typename(L, ttype));
+				}
+			}
+
 			lua_pop(L, 4);
+
+			// We're locking the entire function, instead of each setPixel call.
+			t->setPixelUnsafe(x, y, c);
 		}
 	}
 	return 0;
 }
 
+// Thread-safe wrapper for the above function.
+int w_ImageData_mapPixel(lua_State *L)
+{
+	ImageData *t = luax_checkimagedata(L, 1);
+	luaL_checktype(L, 2, LUA_TFUNCTION);
+
+	lua_pushcfunction(L, w_ImageData_mapPixelUnsafe);
+	lua_pushvalue(L, 1);
+	lua_pushvalue(L, 2);
+
+	// Manually lock this ImageData's mutex during the entire mapPixel.
+	love::thread::Lock lock(t->getMutex());
+	int ret = lua_pcall(L, 2, 0, 0);
+
+	if (ret != 0)
+		return lua_error(L);
+
+	return 0;
+}
+
 int w_ImageData_paste(lua_State *L)
 {
 	ImageData *t = luax_checkimagedata(L, 1);

src/modules/thread/threads.cpp

@@ -42,6 +42,37 @@ Lock::~Lock()
 	mutex->unlock();
 }
 
+EmptyLock::EmptyLock()
+	: mutex(0)
+{
+}
+
+EmptyLock::~EmptyLock()
+{
+	if (mutex)
+		mutex->unlock();
+}
+
+void EmptyLock::setLock(Mutex *m)
+{
+	if (mutex)
+		mutex->unlock();
+
+	mutex = m;
+
+	if (mutex)
+		mutex->lock();
+}
+
+void EmptyLock::setLock(Mutex &m)
+{
+	if (mutex)
+		mutex->unlock();
+
+	mutex = &m;
+	mutex->lock();
+}
+
 Threadable::Threadable()
 {
 	owner = newThread(this);

src/modules/thread/threads.h

@@ -59,6 +59,19 @@ private:
 	Mutex *mutex;
 };
 
+class EmptyLock
+{
+public:
+	EmptyLock();
+	~EmptyLock();
+
+	void setLock(Mutex *m);
+	void setLock(Mutex &m);
+
+private:
+	Mutex *mutex;
+};
+
 class Threadable
 {
 public:

src/modules/window/sdl/Window.cpp

@@ -268,6 +268,9 @@ std::string Window::getWindowTitle() const
 
 bool Window::setIcon(love::image::ImageData *imgd)
 {
+	if (!imgd)
+		return false;
+
 	Uint32 rmask, gmask, bmask, amask;
 #ifdef LOVE_BIG_ENDIAN
 	rmask = 0xFF000000;
@@ -281,11 +284,21 @@ bool Window::setIcon(love::image::ImageData *imgd)
 	amask = 0xFF000000;
 #endif
 
-	int w = static_cast<int>(imgd->getWidth());
+	int w = imgd->getWidth();
-	int h = static_cast<int>(imgd->getHeight());
+	int h = imgd->getHeight();
-	int pitch = static_cast<int>(imgd->getWidth() * 4);
+	int pitch = imgd->getWidth() * 4;
+
+	SDL_Surface *icon = 0;
+
+	{
+		// We don't want another thread modifying the ImageData mid-copy.
+		love::thread::Lock lock(imgd->getMutex());
+		icon = SDL_CreateRGBSurfaceFrom(imgd->getData(), w, h, 32, pitch, rmask, gmask, bmask, amask);
+	}
+
+	if (!icon)
+		return false;
 
-	SDL_Surface *icon = SDL_CreateRGBSurfaceFrom(imgd->getData(), w, h, 32, pitch, rmask, gmask, bmask, amask);
 	SDL_WM_SetIcon(icon, NULL);
 	SDL_FreeSurface(icon);