From 6e3a7288dd0a5afccc735d5797f110a4ab5ed7a1 Mon Sep 17 00:00:00 2001 From: Alex Szpakowski Date: Tue, 25 Jun 2013 05:28:20 -0300 Subject: [PATCH] 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 | 8 +- src/modules/graphics/opengl/Image.cpp | 18 ++++- src/modules/image/ImageData.cpp | 18 ++++- src/modules/image/ImageData.h | 8 ++ src/modules/image/wrap_ImageData.cpp | 102 +++++++++++++++++++++----- src/modules/thread/threads.cpp | 31 ++++++++ src/modules/thread/threads.h | 13 ++++ src/modules/window/sdl/Window.cpp | 21 +++++- 8 files changed, 191 insertions(+), 28 deletions(-) diff --git a/src/modules/font/ImageRasterizer.cpp b/src/modules/font/ImageRasterizer.cpp index 18cf2fe91..89ca622e1 100644 --- a/src/modules/font/ImageRasterizer.cpp +++ b/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; diff --git a/src/modules/graphics/opengl/Image.cpp b/src/modules/graphics/opengl/Image.cpp index ddbecb164..69afd6ecb 100644 --- a/src/modules/graphics/opengl/Image.cpp +++ b/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, diff --git a/src/modules/image/ImageData.cpp b/src/modules/image/ImageData.cpp index 133b0d1e4..7532bbc80 100644 --- a/src/modules/image/ImageData.cpp +++ b/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); diff --git a/src/modules/image/ImageData.h b/src/modules/image/ImageData.h index 290e4f422..7d8f70b52 100644 --- a/src/modules/image/ImageData.h +++ b/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; diff --git a/src/modules/image/wrap_ImageData.cpp b/src/modules/image/wrap_ImageData.cpp index 7605d7876..c32c4f83b 100644 --- a/src/modules/image/wrap_ImageData.cpp +++ b/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.g = (unsigned char)luaL_checkint(L, -3); - c.b = (unsigned char)luaL_checkint(L, -2); - c.a = (unsigned char)luaL_optint(L, -1, 255); + c.r = (unsigned char)luaL_checkinteger(L, -4); + c.g = (unsigned char)luaL_checkinteger(L, -3); + c.b = (unsigned char)luaL_checkinteger(L, -2); + c.a = (unsigned char)luaL_optinteger(L, -1, 255); lua_pop(L, 4); } else { - c.r = (unsigned char)luaL_checkint(L, 4); - c.g = (unsigned char)luaL_checkint(L, 5); - c.b = (unsigned char)luaL_checkint(L, 6); - c.a = (unsigned char)luaL_optint(L, 7, 255); + c.r = (unsigned char)luaL_checkinteger(L, 4); + c.g = (unsigned char)luaL_checkinteger(L, 5); + c.b = (unsigned char)luaL_checkinteger(L, 6); + 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, j); - pixel c = t->getPixel(i, j); + lua_pushnumber(L, x); + lua_pushnumber(L, y); + 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); - c.b = luaL_optint(L, -2, 0); - c.a = luaL_optint(L, -1, 255); - t->setPixel(i, j, c); + + // If we used luaL_checkX / luaL_optX then we would get messy error + // messages (e.g. Error: bad argument #-1 to '?'), so while this is + // messier code, at least the errors are a bit more descriptive. + + // 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); diff --git a/src/modules/thread/threads.cpp b/src/modules/thread/threads.cpp index 0dd6dc602..ecb33e1cd 100644 --- a/src/modules/thread/threads.cpp +++ b/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); diff --git a/src/modules/thread/threads.h b/src/modules/thread/threads.h index a978991ba..f8bdd487e 100644 --- a/src/modules/thread/threads.h +++ b/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: diff --git a/src/modules/window/sdl/Window.cpp b/src/modules/window/sdl/Window.cpp index aa54814a6..e65b184b9 100644 --- a/src/modules/window/sdl/Window.cpp +++ b/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(imgd->getWidth()); - int h = static_cast(imgd->getHeight()); - int pitch = static_cast(imgd->getWidth() * 4); + int w = imgd->getWidth(); + int h = imgd->getHeight(); + 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);