Fixed wallpaper prescaling issues

Image::SDLgetScaledImage was changed so that it tries to find an existing
scaled version of the image first, and generates it when none exists.
When it needs to generate one, this resource is added to the resource
manager, partly to avoid duplicating the work later but mainly to keep
memory management straightforward.

This function also used to leak the scaled SDL_Surface since it wrongly
assumed that Image::load would free it.

To avoid filling up the memory with scaled wallpapers that are waiting
30 seconds until they will be deleted, the Resource::decRef function was
extended with a parameter that allows telling it what to do with orphans.
Calling decRef with Resource::DeleteImmediately will delete the resource
immediately in case the resource is orphaned.

Reviewed-by: Yohann Ferreira

9 files changed, 174 insertions, 90 deletions

diff --git a/src/graphics.cpp b/src/graphics.cpp
index dfdef349..7b56d974 100644
--- a/src/graphics.cpp
+++ b/src/graphics.cpp
@@ -161,14 +161,16 @@ bool Graphics::drawRescaledImage(Image *image, int srcX, int srcY,
                                bool useColor)
 {
     // Check that preconditions for blitting are met.
-    if (!mTarget || !image) return false;
-    if (!image->mSDLSurface) return false;
+    if (!mTarget || !image)
+        return false;
+    if (!image->mSDLSurface)
+        return false;
 
     Image *tmpImage = image->SDLgetScaledImage(desiredWidth, desiredHeight);
     bool returnValue = false;
 
-    if (!tmpImage) return false;
-    if (!tmpImage->mSDLSurface) return false;
+    if (!tmpImage)
+        return false;
 
     dstX += mClipStack.top().xOffset;
     dstY += mClipStack.top().yOffset;
@@ -183,9 +185,10 @@ bool Graphics::drawRescaledImage(Image *image, int srcX, int srcY,
     srcRect.w = width;
     srcRect.h = height;
 
-    returnValue = !(SDL_BlitSurface(tmpImage->mSDLSurface, &srcRect, mTarget, &dstRect) < 0);
+    returnValue = !(SDL_BlitSurface(tmpImage->mSDLSurface, &srcRect,
+                                    mTarget, &dstRect) < 0);
 
-    delete tmpImage;
+    tmpImage->decRef(Resource::DeleteImmediately);
 
     return returnValue;
 }
@@ -228,13 +231,16 @@ void Graphics::drawImage(gcn::Image const *image, int srcX, int srcY,
 void Graphics::drawImagePattern(Image *image, int x, int y, int w, int h)
 {
     // Check that preconditions for blitting are met.
-    if (!mTarget || !image) return;
-    if (!image->mSDLSurface) return;
+    if (!mTarget || !image)
+        return;
+    if (!image->mSDLSurface)
+        return;
 
     const int iw = image->getWidth();
     const int ih = image->getHeight();
 
-    if (iw == 0 || ih == 0) return;
+    if (iw == 0 || ih == 0)
+        return;
 
     for (int py = 0; py < h; py += ih)     // Y position on pattern plane
     {
@@ -259,22 +265,32 @@ void Graphics::drawImagePattern(Image *image, int x, int y, int w, int h)
     }
 }
 
-void Graphics::drawRescaledImagePattern(Image *image, int x, int y,
-               int w, int h, int scaledWidth, int scaledHeight)
+void Graphics::drawRescaledImagePattern(Image *image,
+                                        int x, int y,
+                                        int w, int h,
+                                        int scaledWidth, int scaledHeight)
 {
     // Check that preconditions for blitting are met.
-    if (!mTarget || !image) return;
-    if (!image->mSDLSurface) return;
+    if (!mTarget || !image)
+        return;
+    if (!image->mSDLSurface)
+        return;
 
-    if (scaledHeight == 0 || scaledWidth == 0) return;
+    if (scaledHeight == 0 || scaledWidth == 0)
+        return;
 
     Image *tmpImage = image->SDLgetScaledImage(scaledWidth, scaledHeight);
-    if (!tmpImage) return;
+    if (!tmpImage)
+        return;
 
     const int iw = tmpImage->getWidth();
     const int ih = tmpImage->getHeight();
 
-    if (iw == 0 || ih == 0) return;
+    if (iw == 0 || ih == 0)
+    {
+        tmpImage->decRef(Resource::DeleteImmediately);
+        return;
+    }
 
     for (int py = 0; py < h; py += ih)     // Y position on pattern plane
     {
@@ -298,7 +314,7 @@ void Graphics::drawRescaledImagePattern(Image *image, int x, int y,
         }
     }
 
-    delete tmpImage;
+    tmpImage->decRef(Resource::DeleteImmediately);
 }
 
 void Graphics::drawImageRect(int x, int y, int w, int h,
diff --git a/src/gui/widgets/desktop.cpp b/src/gui/widgets/desktop.cpp
index bf833898..baf3e951 100644
--- a/src/gui/widgets/desktop.cpp
+++ b/src/gui/widgets/desktop.cpp
@@ -105,30 +105,39 @@ void Desktop::draw(gcn::Graphics *graphics)
 
 void Desktop::setBestFittingWallpaper()
 {
-    const std::string wallpaperName =
-            Wallpaper::getWallpaper(getWidth(), getHeight());
+    const int width = getWidth();
+    const int height = getHeight();
 
-    Image *nWallPaper = ResourceManager::getInstance()->getImage(wallpaperName);
+    if (width == 0 || height == 0)
+        return;
 
-    if (nWallPaper)
+    const std::string wallpaperName = Wallpaper::getWallpaper(width, height);
+
+    if (wallpaperName.empty())
+        return;
+
+    ResourceManager *resman = ResourceManager::getInstance();
+    Image *wallpaper = resman->getImage(wallpaperName);
+
+    if (wallpaper)
     {
         if (mWallpaper)
-            mWallpaper->decRef();
+            mWallpaper->decRef(Resource::DeleteImmediately);
+
+        mWallpaper = wallpaper;
 
-        if (!nWallPaper->useOpenGL() && (nWallPaper->getWidth() != getWidth()
-            || nWallPaper->getHeight() != getHeight()))
+        // In software mode we try to prescale the image for performance
+        if (!wallpaper->useOpenGL() &&
+                (wallpaper->getWidth() != width ||
+                 wallpaper->getHeight() != height))
         {
-            // We rescale to obtain a fullscreen wallpaper...
-            Image *newRsclWlPpr = nWallPaper->SDLgetScaledImage(getWidth(), getHeight());
-            std::string idPath = nWallPaper->getIdPath();
-
-            // We replace the resource in the resource manager
-            nWallPaper->decRef();
-            ResourceManager::getInstance()->addResource(idPath, newRsclWlPpr);
-            mWallpaper = newRsclWlPpr;
+            if (Image *prescaled = wallpaper->SDLgetScaledImage(width, height))
+            {
+                // Make sure the original can be freed
+                wallpaper->decRef();
+                mWallpaper = prescaled;
+            }
         }
-            else
-                mWallpaper = nWallPaper;
     }
     else
     {
diff --git a/src/resources/ambientlayer.cpp b/src/resources/ambientlayer.cpp
index d1fc93d3..c31afbac 100644
--- a/src/resources/ambientlayer.cpp
+++ b/src/resources/ambientlayer.cpp
@@ -32,6 +32,7 @@ AmbientLayer::AmbientLayer(Image *img, float parallax,
     mSpeedX(speedX), mSpeedY(speedY),
     mKeepRatio(keepRatio)
 {
+    mImage->incRef();
 
     if (keepRatio && !mImage->useOpenGL()
         && defaultScreenWidth != 0
@@ -47,17 +48,10 @@ AmbientLayer::AmbientLayer(Image *img, float parallax,
 
         if (rescaledOverlay)
         {
-            // Replace the resource with the new one...
-            std::string idPath = mImage->getIdPath() + "_rescaled";
-            ResourceManager::getInstance()->addResource(idPath, rescaledOverlay);
+            mImage->decRef();
             mImage = rescaledOverlay;
-            rescaledOverlay->incRef();
         }
-        else
-            mImage->incRef();
     }
-    else
-        mImage->incRef();
 }
 
 AmbientLayer::~AmbientLayer()
diff --git a/src/resources/image.cpp b/src/resources/image.cpp
index 3a6f7236..975bd647 100644
--- a/src/resources/image.cpp
+++ b/src/resources/image.cpp
@@ -326,31 +326,52 @@ void Image::setAlpha(float alpha)
     }
 }
 
-Image* Image::SDLgetScaledImage(int width, int height)
+Image *Image::SDLgetScaledImage(int width, int height)
 {
-    // No scaling on incorrect new values.
     if (width == 0 || height == 0)
-    return NULL;
+        return 0;
 
-    // No scaling when there is ... no different given size ...
+    // Increase our reference count and return ourselves in case of same size
     if (width == getWidth() && height == getHeight())
-        return NULL;
+    {
+        incRef();
+        return this;
+    }
 
-    Image* scaledImage = NULL;
-    SDL_Surface* scaledSurface = NULL;
+    if (!mSDLSurface)
+        return 0;
 
-    if (mSDLSurface)
+    ResourceManager *resman = ResourceManager::getInstance();
+
+    // Generate a unique ID path for storing the scaled version in the
+    // resource manager.
+    std::string idPath = getIdPath();
+    idPath += ":scaled:";
+    idPath += toString(width);
+    idPath += "x";
+    idPath += toString(height);
+
+    // Try whether a scaled version is already available
+    Image *scaledImage = static_cast<Image*>(resman->get(idPath));
+
+    if (!scaledImage)
     {
-        scaledSurface = zoomSurface(mSDLSurface,
-                    (double) width / getWidth(),
-                    (double) height / getHeight(),
-                    1);
+        // No scaled version with this size exists already, so create one
+        SDL_Surface *scaledSurface = zoomSurface(mSDLSurface,
+                                                 (double) width / getWidth(),
+                                                 (double) height / getHeight(),
+                                                 1);
 
-        // The load function takes care of the SDL<->OpenGL implementation
-        // and about freeing the given SDL_surface*.
         if (scaledSurface)
+        {
             scaledImage = load(scaledSurface);
+            SDL_FreeSurface(scaledSurface);
+
+            // Place the scaled image in the resource manager
+            resman->addResource(idPath, scaledImage);
+        }
     }
+
     return scaledImage;
 }
 
diff --git a/src/resources/image.h b/src/resources/image.h
index b826112d..7df74c3f 100644
--- a/src/resources/image.h
+++ b/src/resources/image.h
@@ -147,14 +147,15 @@ class Image : public Resource
         { return mDisableTransparency; }
 
         /**
-         * Gets an scaled instance of an image.
+         * Gets an scaled instance of an image. The returned image is managed
+         * by the ResourceManager.
          *
-         * @param width The desired width of the scaled image.
+         * @param width  The desired width of the scaled image.
          * @param height The desired height of the scaled image.
          *
-         * @return A new Image* object.
+         * @return An Image resource, or 0 on failure.
          */
-        Image* SDLgetScaledImage(int width, int height);
+        Image *SDLgetScaledImage(int width, int height);
 
         /**
          * Get the alpha Channel of a SDL surface.
diff --git a/src/resources/resource.cpp b/src/resources/resource.cpp
index 4f6a2519..cdff8060 100644
--- a/src/resources/resource.cpp
+++ b/src/resources/resource.cpp
@@ -27,16 +27,7 @@
 
 #include <cassert>
 
-Resource::~Resource()
-{
-}
-
-void Resource::incRef()
-{
-    ++mRefCount;
-}
-
-void Resource::decRef()
+void Resource::decRef(OrphanPolicy orphanPolicy)
 {
     // Reference may not already have reached zero
     if (mRefCount == 0) {
@@ -48,8 +39,18 @@ void Resource::decRef()
 
     if (mRefCount == 0)
     {
-        // Warn the manager that this resource is no longer used.
         ResourceManager *resman = ResourceManager::getInstance();
-        resman->release(this);
+
+        switch (orphanPolicy)
+        {
+            case DeleteLater:
+            default:
+                resman->release(this);
+                break;
+            case DeleteImmediately:
+                resman->remove(this);
+                delete this;
+                break;
+        }
     }
 }
diff --git a/src/resources/resource.h b/src/resources/resource.h
index 53b05ee8..af688eb0 100644
--- a/src/resources/resource.h
+++ b/src/resources/resource.h
@@ -33,21 +33,26 @@ class Resource
     friend class ResourceManager;
 
     public:
-        Resource(): mRefCount(0) {}
+        enum OrphanPolicy {
+            DeleteLater,
+            DeleteImmediately
+        };
+
+        Resource():
+            mRefCount(0)
+        {}
 
         /**
          * Increments the internal reference count.
          */
-        void incRef();
+        void incRef() { ++mRefCount; }
 
         /**
-         * Decrements the reference count and deletes the object
-         * if no references are left.
-         *
-         * @return <code>true</code> if the object was deleted
-         *         <code>false</code> otherwise.
+         * Decrements the reference count. When no references are left, either
+         * schedules the object for deletion or deletes it immediately,
+         * depending on the \a orphanPolicy.
          */
-        void decRef();
+        void decRef(OrphanPolicy orphanPolicy = DeleteLater);
 
         /**
          * Return the path identifying this resource.
@@ -56,7 +61,7 @@ class Resource
         { return mIdPath; }
 
     protected:
-        virtual ~Resource();
+        virtual ~Resource() {}
 
     private:
         std::string mIdPath; /**< Path identifying this resource. */
diff --git a/src/resources/resourcemanager.cpp b/src/resources/resourcemanager.cpp
index 15d7c8eb..a9e7e565 100644
--- a/src/resources/resourcemanager.cpp
+++ b/src/resources/resourcemanager.cpp
@@ -117,9 +117,11 @@ void ResourceManager::cleanOrphans()
     timeval tv;
     gettimeofday(&tv, NULL);
     // Delete orphaned resources after 30 seconds.
-    time_t oldest = tv.tv_sec, threshold = oldest - 30;
+    time_t oldest = tv.tv_sec;
+    time_t threshold = oldest - 30;
 
-    if (mOrphanedResources.empty() || mOldestOrphan >= threshold) return;
+    if (mOrphanedResources.empty() || mOldestOrphan >= threshold)
+        return;
 
     ResourceIterator iter = mOrphanedResources.begin();
     while (iter != mOrphanedResources.end())
@@ -128,7 +130,8 @@ void ResourceManager::cleanOrphans()
         time_t t = res->mTimeStamp;
         if (t >= threshold)
         {
-            if (t < oldest) oldest = t;
+            if (t < oldest)
+                oldest = t;
             ++iter;
         }
         else
@@ -234,6 +237,17 @@ bool ResourceManager::addResource(const std::string &idPath,
     return false;
 }
 
+Resource *ResourceManager::get(const std::string &idPath)
+{
+    ResourceIterator resIter = mResources.find(idPath);
+    if (resIter != mResources.end())
+    {
+        resIter->second->incRef();
+        return resIter->second;
+    }
+    return 0;
+}
+
 Resource *ResourceManager::get(const std::string &idPath, generator fun,
                                void *data)
 {
@@ -392,12 +406,18 @@ void ResourceManager::release(Resource *res)
     time_t timestamp = tv.tv_sec;
 
     res->mTimeStamp = timestamp;
-    if (mOrphanedResources.empty()) mOldestOrphan = timestamp;
+    if (mOrphanedResources.empty())
+        mOldestOrphan = timestamp;
 
     mOrphanedResources.insert(*resIter);
     mResources.erase(resIter);
 }
 
+void ResourceManager::remove(Resource *res)
+{
+    mResources.erase(res->mIdPath);
+}
+
 ResourceManager *ResourceManager::getInstance()
 {
     // Create a new instance if necessary.
diff --git a/src/resources/resourcemanager.h b/src/resources/resourcemanager.h
index 5295be5c..5abc81e2 100644
--- a/src/resources/resourcemanager.h
+++ b/src/resources/resourcemanager.h
@@ -107,6 +107,16 @@ class ResourceManager
         std::string getPath(const std::string &file);
 
         /**
+         * Returns the resource with the specified idPath, or 0 when no such
+         * resource is available. Increments the reference count.
+         *
+         * @param idPath The resource identifier path.
+         * @return A valid resource or <code>NULL</code> if the resource could
+         *         not be found.
+         */
+        Resource *get(const std::string &idPath);
+
+        /**
          * Creates a resource and adds it to the resource map.
          *
          * @param idPath The resource identifier path.
@@ -178,11 +188,6 @@ class ResourceManager
         SpriteDef *getSprite(const std::string &path, int variant = 0);
 
         /**
-         * Releases a resource, placing it in the set of orphaned resources.
-         */
-        void release(Resource *);
-
-        /**
          * Allocates data into a buffer pointer for raw data loading. The
          * returned data is expected to be freed using <code>free()</code>.
          *
@@ -225,6 +230,18 @@ class ResourceManager
 
     private:
         /**
+         * Releases a resource, placing it in the set of orphaned resources.
+         * Only called from Resource::decRef,
+         */
+        void release(Resource *);
+
+        /**
+         * Removes a resource from the list of resources managed by the
+         * resource manager. Only called from Resource::decRef,
+         */
+        void remove(Resource *);
+
+        /**
          * Deletes the resource after logging a cleanup message.
          */
         static void cleanUp(Resource *resource);