Multithreading image-loading problem with SDL_FreeSurface()

Hello, good people.
For the past couple of weeks I’ve been dealing with an annoying multithreading problem which I’m really struggling to resolve.
I will be very happy and proud of the community if you help me address the problem.

  • Error code:
    double free or corruption (faststop)

  • Explanation of the problem and behaviour of the program:
    When I execute the program on a single thread - there is no problem, but when when I allow multithreading - my program crashes.
    For slower machines it could crash once in 50 runs, but with faster machines (bigger and faster number of cores) - it crashesh like 25 of 50 times. (unpreddicted behaviour)

  • Source code:
    At the bottom of the post you can find all the detailed 1:1 source code that causes the crash.

Useful info:
From what I’ve read about multithread resource loading - only the thread that created the SDL_Renderer can create valid SDL_Texture’s with that renderer.
The other threads can only “help” by loading the image from harddrive with IMG_Load() since it’s only CPU work.
That’s exatcly what I am doing.

  • Miltithread approach:
  1. I construct thread safe queue with absolute image paths on the harddrive from the main thread (still on single core at this moment);
  2. I spawn worker threads and pass them the above thread safe queue, which they start to consume.
  3. The worker threads execute IMG_Load() (Loading images /or SDL_Surfaces/ with the CPU from the harddrive) and store the loaded SDL_Surfaces into another
    thread safe queue.
  4. The main thread is waiting with a condition variable on the second thread safe queue and start to consume it and construct SDL_Textures out of the popped SDL_Surfaces;
  5. Only the main thread calls Texture::freeSurface() on a surface once, when the SDL_Texture has been successfully constructed (GPU work).

Where it crashes:
In Texture::freeSurface() method at the SDL_FreeSurface() call.

void Texture::freeSurface(SDL_Surface *& surface)
{
if(surface && surface->refcount) //sanity check
{
SDL_FreeSurface(surface);
surface = nullptr;
}
}

Important note:
When the crash occures - in the Texture::freeSurface() function - the value of “surface” is not nullptr and the value of surface->refcount is 0.
SDL2 updates the value of refcount when a surface is freed, so again this points to a multithreading issue.

Important note 2:
Happened to me only once but I did get a SEGFAULT on the destructor of a ThreadSafeQueue.

Thank you for your time.

/*
 * ThreadSafeQueue.hpp
 *  @brief: thread-safe multiple producer, multiple consumer queue
 */

#ifndef THREADSAFEQUEUE_HPP_
#define THREADSAFEQUEUE_HPP_

//C++ system headers
#include <mutex>
#include <condition_variable>
#include <queue>
#include <memory>
#include <utility>
#include <atomic>

template<typename T>
class ThreadSafeQueue
{
    public:
        //default constructor
        ThreadSafeQueue() : _isKilled(false) {}

        //forbid the move constructor and move assignment operator
        ThreadSafeQueue(const ThreadSafeQueue && movedOther) = delete;
        ThreadSafeQueue & operator= (const ThreadSafeQueue && movedOther) =
                                                                        delete;

        //forbid the copy constructor and copy assignment operator
        ThreadSafeQueue(const ThreadSafeQueue & other) = delete;
        ThreadSafeQueue & operator= (const ThreadSafeQueue & other) = delete;

        //default destructor
        virtual ~ThreadSafeQueue() = default;

        /** @brief used to push new elements to the queue
         *
         *         NOTE: push() method moves the pushed data,
         *                                              rather than copying it.
         *
         *  @param const T & - reference to the data that is being pushed
         * */
        void push(const T & newData)
        {
            //lock the queue
            std::unique_lock<std::mutex> lock(_mutex);

            //push the data to queue by moving it
            _data.push(std::move(newData));

            /** Manually unlock the queue since we want to notify any threads
              * that are waiting for data
              * */
            lock.unlock();

            //notify one blocked thread
            _condVar.notify_one();
        }

        /** @brief used to push new elements to the queue
         *
         *         NOTE: use pushWithCopy() method only when you have a
         *               good reason not to have your original resource moved.
         *               pushWithCopy() method copies the pushed data,
         *                                              rather than moving it.
         *
         *  @param const T & - reference to the data that is being pushed
         * */
        void pushWithCopy(const T & newData)
        {
            //lock the queue
            std::unique_lock<std::mutex> lock(_mutex);

            //push the data to queue by copying it
            _data.push(newData);

            /** Manually unlock the mutex since we want to notify any threads
              * that are waiting for data
              * */
            lock.unlock();

            //notify one blocked thread
            _condVar.notify_one();
        }

        /** @brief used to acquire data from the queue.
         *
         *         NOTE: if queue is empty - the thread is put to sleep with
         *               a condition variable and when new data is being pushed
         *               into the queue - the thread is woken up again
         *
         *  @param T & - reference to the value that is being popped
         *                                                       from the queue
         * */
        void waitAndPop(T & value)
        {
            //lock the queue
            std::unique_lock<std::mutex> lock(_mutex);

            /** Condition variables can be subject to spurious wake-ups,
             *  so it is important to check the actual condition
             *  being waited for when the call to wait returns
             * */
            while (_data.empty() && !_isKilled)
            {
                _condVar.wait(lock);
            }

            //user has requested shutdown
            if(_isKilled)
            {
                return;
            }

            //acquire the data from the queue
            value = std::move(_data.front());

            //pop the 'moved' queue node
            _data.pop();
        }

        /** @brief used to acquire data from the queue.
         *
         *         NOTE: if queue is empty - the thread is put to sleep with
         *               a condition variable and when new data is being pushed
         *               into the queue - the thread is woken up again
         *
         *  @param bool - unique_ptr to the value
         *                                  that is being popped from the queue
         * */
        std::unique_ptr<T> waitAndPop()
        {
            //lock the queue
            std::unique_lock<std::mutex> lock(_mutex);

            /** Condition variables can be subject to spurious wake-ups,
             *  so it is important to check the actual condition
             *  being waited for when the call to wait returns
             * */
            while (_data.empty() && !_isKilled)
            {
                _condVar.wait(lock);
            }

            //user has requested shutdown
            if(_isKilled)
            {
                return std::unique_ptr<T>();
            }

            //acquire the data from the queue
            std::unique_ptr<T> res(std::make_unique<T>(
                                                     std::move(_data.front())));

            //pop the 'moved' queue node
            _data.pop();

            return res;
        }

        /** @brief used to try and acquire data from the queue.
         *
         *         NOTE: if queue is not empty - the data from the queue is
         *               being moved to the T & value function argument
         *
         *  @param bool - pop was successful or not
         * */
        bool tryPop(T & value)
        {
            //lock the queue
            std::lock_guard<std::mutex> lock(_mutex);

            //if queue is empty - try_pop() fails -> return false status
            if (_data.empty())
            {
                return false;
            }

            //acquire the data from the queue
            value = std::move(_data.front());

            //pop the 'moved' queue node
            _data.pop();

            //try_pop() succeeded -> return true status
            return true;
        }

        /** @brief used to try and acquire data from the queue.
         *
         *         NOTE: if queue is not empty - the data from the queue is
         *               being moved to the T & value function argument
         *
         *  @param std::unique_ptr<T> - unique_ptr to the value
         *                                  that is being popped from the queue.
         *
         *                              If unique_ptr is empty -
         *                                            the pop was not successful
         * */
        std::unique_ptr<T> tryPop()
        {
            //lock the queue
            std::lock_guard<std::mutex> lock(_mutex);

            //if queue is empty - try_pop() fails -> return empty qnique_ptr
            if (_data.empty())
            {
                return std::unique_ptr<T>();
            }

            //acquire the data from the queue and pop
            std::unique_ptr<T> res(std::make_unique<T>(
                                                     std::move(_data.front())));

            //pop the 'moved' queue node
            _data.pop();

            //try_pop() succeeded -> return qnique_ptr to the popped data
            return res;
        }

        /** @brief used check the empty status of the queue.
         *
         *  @param bool - is queue empty or not
         * */
        inline bool isEmpty() const
        {
            //lock the queue
            std::lock_guard<std::mutex> lock(_mutex);

            //return empty status
            return _data.empty();
        }

        /** @brief used acquire the size of the queue.
         *
         *  @param uint64_t - the size of the queue container
         * */
        inline uint64_t size() const
        {
            //lock the queue
            std::lock_guard<std::mutex> lock(_mutex);

            //return actual queue size
            return _data.size();
        }

        /** @brief used initiate shutdown of the queue
         *         (usually invoking of this method by the developer should be
         *          followed by joining thread producers and thread consumers)
         * */
        inline void shutdown()
        {
            //lock the queue
            std::lock_guard<std::mutex> lock(_mutex);

            _isKilled = true;
        }

    private:
        //the actual queue holding the data
        std::queue<T>           _data;

        //the queue mutex used for locking on thread-shared resources
        mutable std::mutex      _mutex;

        /** Conditional variable used for waking threads that are currently
          * waiting for data to be pushed to the queue (starvation)
          * so they can process it
          * */
        std::condition_variable _condVar;

        /** An atomic flag used to signal waiting thread that user requested
         *  queue shutdown with ::shutdown() method (and will probably
         *  join the threads) afterwards
         *  */
        std::atomic<bool>       _isKilled;
};


#endif /* THREADSAFEQUEUE_HPP_ */
/*
 * Texture.h
 */

#ifndef TEXTURE_H_
#define TEXTURE_H_

//C++ system headers
#include <cstdint>

//Forward declarations
struct SDL_Surface;
struct SDL_Texture;
struct SDL_Renderer;


class Texture
{
    public:
        //forbid the default constructor and destructor
        Texture() = delete;
        virtual ~Texture() = delete;

        //forbid the copy and move constructors
        Texture(const Texture & other) = delete;
        Texture(Texture && other) = delete;

        //forbid the copy and move assignment operators
        Texture & operator=(const Texture & other) = delete;
        Texture & operator=(Texture && other) = delete;

        /** @brief used to free SDL_Surface
         *
         *  @param SDL_Surface *& the surface to be freed
         * */
        static void freeSurface(SDL_Surface *& surface);

        /** @brief used to free SDL_Texture
         *
         *  @param SDL_Texture*& the texture to be freed
         * */
        static void freeTexture(SDL_Texture *& texture);

        /** @brief used to load SDL_Surface from file on the hard drive
         *
         *  @param const char *   - absolute path to file
         *  @param SDL_Surface *& - dynamically created SDL_Surface
         *
         *  @returns int32_t      - error code
         * */
        static int32_t loadSurfaceFromFile(const char *   path,
                                           SDL_Surface *& outSurface);

        /** @brief used to create SDL_Texture from provided SDL_Surface
         *         NOTE: if SDL_Texture is successful - the input SDL_Surface
         *                       is not longer needed -> therefore it is freed.
         *
         *  @param SDL_Surface *& - input SDL_Surface
         *  @param SDL_Texture *& - dynamically created SDL_Texture
         *
         *  @returns int32_t     - error code
         * */
        static int32_t loadTextureFromSurface(SDL_Surface *& surface,
                                              SDL_Texture *& outTexture);

        /** @brief used to acquire renderer pointer that will be performing
          *                                         the graphical render calls.
          *
          *  @param SDL_Renderer * - the actual hardware renderer
          * */
        static void setRenderer(SDL_Renderer * renderer);

    private:
        /** Keep pointer to the actual renderer,
          * since Texture class function will be used all the time
          * and in order not to pass pointers on every single function call
          * */
        static SDL_Renderer * _renderer;
};

#endif /* TEXTURE_H_ */
/*
 * Texture.cpp
 */

//Corresponding header
#include "Texture.h"

//Other libraries headers
#include <SDL.h>
#include <SDL_image.h>

//Own components headers
#include "Log.h"

SDL_Renderer * Texture::_renderer = nullptr;


int32_t Texture::loadSurfaceFromFile(const char *   path,
                                     SDL_Surface *& outTexture)
{
    int32_t err = EXIT_SUCCESS;

    //memory leak check
    if(nullptr != outTexture)
    {
        LOGERR("Warning non-nullptr detected! Will no create Surface. "
                                                      "Memory leak prevented!");

        err = EXIT_FAILURE;
    }
    else
    {
        //Load image at specified path
        outTexture = IMG_Load(path);
        if(nullptr == outTexture)
        {
            LOGERR("Unable to load image %s! SDL_image Error: %s",
                                                         path, IMG_GetError());

            err = EXIT_FAILURE;
        }
    }

    return err;
}

int32_t Texture::loadTextureFromSurface(SDL_Surface *& surface,
                                        SDL_Texture *& outTexture)
{
    int32_t err = EXIT_SUCCESS;

    if(nullptr == surface)
    {
       LOGERR("Nullptr surface detected. Unable to loadFromSurface()");

       err = EXIT_FAILURE;
    }

    if(EXIT_SUCCESS == err)
    {
        //Check for memory leaks and get rid of preexisting texture
        if(outTexture)
        {
            LOGERR("Warning, trying to dynamically load a new texture before"
                                               "calling delete on the old one");
            freeTexture(outTexture);
        }

        //Create texture from surface pixels
        outTexture = SDL_CreateTextureFromSurface(_renderer, surface);

        if(nullptr == outTexture)
        {
            LOGERR("Unable to create texture! SDL Error: %s", SDL_GetError());

            err = EXIT_FAILURE;
        }

        //Get rid of old loaded surface
        freeSurface(surface);
    }

    return err;
}

void Texture::freeSurface(SDL_Surface *& surface)
{
    if(surface && surface->refcount) //sanity check
    {
        SDL_FreeSurface(surface);
        surface = nullptr;
    }
}

void Texture::freeTexture(SDL_Texture *& texture)
{
    if(texture) //sanity check
    {
        SDL_DestroyTexture(texture);
        texture = nullptr;
    }
}

void Texture::setRenderer(SDL_Renderer * renderer)
{
    _renderer = renderer;
}
/*
 * ResourceContainer.h
 */

#ifndef RESOURCECONTAINER_H_
#define RESOURCECONTAINER_H_

//C++ system headers
#include <cstdint>
#include <vector>
#include <unordered_map>

//Own components headers
#include "CommonResourceStructs.h"

//Forward declarations
//Since ThreadSafeQueue is a very heavy include -> use forward declaration to it
template <typename T> class ThreadSafeQueue;

struct SDL_Surface;
struct SDL_Texture;


class ResourceContainer
{
    public:
        ResourceContainer();

        virtual ~ResourceContainer() = default;

        /** @brief used to initialise the Resource container
         *
         *  @param const uint64_t - number of widgets to be loaded
         *
         *  @return int32_t       - error code
         * */
        int32_t init(const uint64_t widgetsCount);

        /** @brief used to deinitialize
         *                          (free memory occupied by Resource container)
         * */
        void deinit();

        /** @brief used to load all stored resources from the _rsrsDataMap
         *       > as SDL_Textute * in the _rsrsMap (for Hardware Renderer);
         *
         *  @param const bool - is multithread init allowed
         * */
        void loadAllStoredResources(const bool isMultithreadInitAllowed);

    private:
        /** @brief used to load a single resource at program start up
         *
         *  @param const ResourceData & - populated structure with
         *                                                 Widget specific data
         *  @param SDL_Surface *&       - created SDL_Surface
         *
         *  @returns int32_t            - error code
         * */
        int32_t loadSurface(const ResourceData & rsrcData,
                            SDL_Surface *&       outSurface);

        /** @brief used to internally load all stored resources
         *                                          only using the main thread
         * */
        void loadAllStoredResourcesSingleCore();

        /** @brief used to internally load all stored resources
         *     only using all possible hardware threads (if such are supported)
         *
         *  @param const uint32_t - number of worker threads to spawn
         *                                        (main thread is not included)
         * */
        void loadAllStoredResourcesMultiCore(const uint32_t workerThreadsNum);

        //_rsrcMap holds all Images
        std::unordered_map<uint64_t, SDL_Texture *> _rsrcMap;

        //_rsrcDataMap holds resource specific information for every Image
        std::unordered_map<uint64_t, ResourceData>  _rsrcDataMap;

        /** A copy of the resourceData's (used for
         *                                     multithread loading of resources)
         *  */
        ThreadSafeQueue<ResourceData> *             _resDataThreadQueue;

        /** Holds all loaded SDL_Surface's during initiliazation (used for
         *                                     multithread loading of resources)
         *
         *  std::pair first : unique rsrcId for currently processed ResourceData
         *  std::pair second: SDL_Surface * that will be created from the
         *                                              processed ResourceData
         *  */
        ThreadSafeQueue<std::pair<uint64_t, SDL_Surface *>> *
                                                    _loadedSurfacesThreadQueue;
};

#endif /* RESOURCECONTAINER_H_ */
/*
 * ResourceContainer.cpp
 */

//Corresponding header
#include "ResourceContainer.h"

//C++ system headers
#include <cstdlib>
#include <thread>

//Other libraries headers
#include "Texture.h"

//Own components headers
#include "ThreadSafeQueue.hpp"

#include "Log.h"

/** @brief used to load SDL_Surface's from file system async
 *
 *  @param resQueue              - the resource Data queue (input)
 *  @param outSurfQueue          - the loaded surfaces queue (output)
 *  @param threadsLeftToComplete - number of threads still working on the async
 *                                 surface load from the file system
 *  */
static void loadSurfacesFromFileSystemAsync(
           ThreadSafeQueue<ResourceData> * resQueue,
           ThreadSafeQueue<std::pair< uint64_t, SDL_Surface *>> * outSurfQueue,
           std::atomic<int32_t> & threadsLeftToComplete)
{
    ResourceData resData;

    SDL_Surface * surface = nullptr;

    while(resQueue->tryPop(resData))
    {
        if(EXIT_SUCCESS !=
             Texture::loadSurfaceFromFile(resData.header.path.c_str(), surface))
        {
            LOGERR("Warning, error in loadSurfaceFromFile() for file %s. "
                            "Terminating other resourceLoading",
                                                resData.header.path.c_str());

            return;
        }

        //push the newly generated SDL_Surface to the ThreadSafe Surface Queue
        outSurfQueue->push(std::make_pair(resData.header.hashValue, surface));

        //reset the variable so it can be reused
        surface = nullptr;
    }

    //decrement number of worker threads to be complete
    --threadsLeftToComplete;

    if(0 == threadsLeftToComplete.load())
    {
        //all worker threads are done -> shutdown the resourceQueue
        resQueue->shutdown();
    }
}


ResourceContainer::ResourceContainer() : _resDataThreadQueue(nullptr),
                                         _loadedSurfacesThreadQueue(nullptr)
{
}

int32_t ResourceContainer::init(const uint64_t widgetsCount)
{
    int32_t err = EXIT_SUCCESS;

    _rsrcDataMap.reserve(widgetsCount);

    _rsrcMap.reserve(widgetsCount);

    _resDataThreadQueue = new ThreadSafeQueue<ResourceData>;

    if(nullptr == _resDataThreadQueue)
    {
        LOGERR("Error, bad alloc for ThreadSafeQueue<ResourceData>");

        err = EXIT_FAILURE;
    }

    if(EXIT_SUCCESS == err)
    {
        _loadedSurfacesThreadQueue =
                        new ThreadSafeQueue<std::pair<uint64_t, SDL_Surface *>>;

        if(nullptr == _loadedSurfacesThreadQueue)
        {
            LOGERR("Error, bad alloc for ThreadSafeQueue<SDL_Surface *>");

            err = EXIT_FAILURE;
        }
    }

    return err;
}

void ResourceContainer::deinit()
{
    //free Image/Sprite Textures
    for(_rsrcMapIt it = _rsrcMap.begin(); it != _rsrcMap.end(); ++it)
    {
        Texture::freeTexture(it->second);
    }

    //clear rsrcMap unordered_map and shrink size
    _rsrcMap.clear();


    //clear rsrcDataMap unordered_map and shrink size
    _rsrcDataMap.clear();


    //release memory for resourceData thread safe queue
    if(_resDataThreadQueue) //sanity check
    {
        delete _resDataThreadQueue;
        _resDataThreadQueue = nullptr;
    }

    //release memory for loaded SDL_Surface's thread safe queue
    if(_loadedSurfacesThreadQueue) //sanity check
    {
        delete _loadedSurfacesThreadQueue;
        _loadedSurfacesThreadQueue = nullptr;
    }
}

void ResourceContainer::loadAllStoredResources(
                                            const bool isMultithreadInitAllowed)
{
    if(!isMultithreadInitAllowed)
    {
        LOGG("Starting Single Core resource loading ");

        loadAllStoredResourcesSingleCore();

        return;
    }

    /** Multi Threading strategy:
     *
     *  N - hardware supported number of cores.
     *
     *  Only the Hardware Renderer can perform GPU operations. With this said:
     *      > spawn N - 1 worker threads that will perform  only the
     *        CPU intensive work (reading files from disk,creating
     *        SDL_Surface's from them and storing those SDL_Surface's into a
     *        ThreadSafeQueue);
     *
     *      > use the main thread to pop generated SDL_Surface's from the
     *        ThreadSafeQueue that the worker threads are populating and start
     *        to create SDL_Texture's (GPU Accelerated Textures) out of those
     *        SDL_Surface's;
     *
     *  NOTE: in the case of Software Renderer usage:
     *        Since there will be no GPU operations and there is only CPU work
     *        to be executed -> spawn N - 1 worker threads for generating
     *        the SDL_Surface's and also use the main thread for the same job.
     * */

    /** Hardware_concurrency may return 0 if its not supported
      * if this happens -> run the resourceLoad in single core
      * */
    const uint32_t HARDWARE_THREAD_NUM = std::thread::hardware_concurrency();

    /** If threads are <= 1 no need to spawn second thread,
     *  because there will be a performance
     *  lost from the constant threads context switches
     * */
    if(HARDWARE_THREAD_NUM > 1) //multi core case
    {
        /* Generate THREAD_NUM - 1 worker CPU threads and leave the main
         * thread to operate ot GPU + CPU operations
         * */
        const uint32_t WORKER_THREAD_NUM = HARDWARE_THREAD_NUM - 1;

        LOGG("Starting Multi Core resource loading on %u threads",
                                                        HARDWARE_THREAD_NUM);

        loadAllStoredResourcesMultiCore(WORKER_THREAD_NUM);
    }
    else //single core case
    {
        LOGR("Multi Threading is not supported on this hardware. ");
        LOGG("Starting Single Core resource loading ");

        loadAllStoredResourcesSingleCore();
    }
}

int32_t ResourceContainer::loadSurface(const ResourceData & rsrcData,
                                       SDL_Surface *&       outSurface)
{
    int32_t err = EXIT_SUCCESS;

    if(EXIT_SUCCESS !=
            Texture::loadSurfaceFromFile(rsrcData.header.path.c_str(),
                                         outSurface))
    {
        LOGERR("Error in loadSurfaceFromFile() for rsrcId: %#16lX",
                                                rsrcData.header.hashValue);

        err = EXIT_FAILURE;
    }

    return err;
}

int32_t ResourceContainer::loadTextureFromSurface(SDL_Surface *  surface,
                                                  SDL_Texture *& outTexture)
{
    int32_t err = EXIT_SUCCESS;

    if(EXIT_SUCCESS != Texture::loadTextureFromSurface(surface, outTexture))
    {
        LOGERR("Error in Texture::loadTextureFromSurface()");

        err = EXIT_FAILURE;
    }

    return err;
}

void ResourceContainer::loadAllStoredResourcesSingleCore()
{
    //NOTE: some of the thread safe mechanism such as ThreadSafeQueue as reused.
    //the overhead is minimal and the source will be reused.

    ResourceData resData;

    SDL_Surface * newSurface = nullptr;

    while(_resDataThreadQueue->tryPop(resData))
    {
        if(EXIT_SUCCESS !=
                    Texture::loadSurfaceFromFile(resData.header.path.c_str(),
                                                 newSurface))
        {
            LOGERR("Warning, error in loadSurfaceFromFile() for file %s. "
                            "Terminating other resourceLoading",
                                                resData.header.path.c_str());

            return;
        }

        //push the newly generated SDL_Surface to the ThreadSafe Surface Queue
        _loadedSurfacesThreadQueue->push(
                                    std::make_pair(resData.header.hashValue,
                                                   newSurface));

        //reset the variable so it can be reused
        newSurface = nullptr;
    }

    //temporary variables used for _loadedSurfacesThreadQueue::pop operation
    std::pair<uint64_t, SDL_Surface *> currResSurface(0, nullptr);

    //on main thread return
    SDL_Texture * newTexture = nullptr;

    //generate GPU textures from the stored SDL_Surface's
    while(_loadedSurfacesThreadQueue->tryPop(currResSurface))
    {
        if(EXIT_SUCCESS != 
                  Texture::loadTextureFromSurface(currResSurface.second,
                                                  newTexture))
        {
            LOGERR("Error in Texture::loadTextureFromSurface() for rsrcId: "
                                          "%#16lX", currResSurface.first);

            return;
        }

        //store the generates SDL_Texture into the rsrcMap
        _rsrcMap[currResSurface.first] = newTexture;

        //reset the variable so it can be reused
        newTexture = nullptr;
    }
}

void ResourceContainer::loadAllStoredResourcesMultiCore(
                                                const uint32_t workerThreadsNum)
{
    //create a vector pool for worker threads
    std::vector<std::thread> workerThreadPool(workerThreadsNum);

    //used to indicate to the main thread for all worker threads job finished
    std::atomic<int32_t> threadsLeftToComplete(workerThreadsNum);

    //temporary variables used for _loadedSurfacesThreadQueue::pop operation
    std::pair<uint64_t, SDL_Surface *> currResSurface(0, nullptr);

    for(uint32_t i = 0; i < workerThreadsNum; ++i)
    {
        //spawn the worker threads
        workerThreadPool[i] =
                std::thread(loadSurfacesFromFileSystemAsync,
                            _resDataThreadQueue,
                            _loadedSurfacesThreadQueue,
                            std::ref(threadsLeftToComplete));
    }

    //temporary variables used for calculations
    SDL_Texture * newTexture = nullptr;

    //start work on the main thread
    //while waiting for the worker threads to be done
    while(0 != threadsLeftToComplete.load())
    {
        /** Block main thread and wait resources to be pushed
          * into the _loadedSurfacesThreadQueue
          * */
        _loadedSurfacesThreadQueue->waitAndPop(currResSurface);

        if(EXIT_SUCCESS !=
                Texture::loadTextureFromSurface(currResSurface.second,
                                                newTexture))
        {
            LOGERR("Error in Texture::loadTextureFromSurface() for rsrcId: "
                                          "%#16lX", currResSurface.first);

            return;
        }
        else
        {
            //emplace GPU Texture to the rsrcMap
            _rsrcMap[currResSurface.first] = newTexture;

            //reset the variable so it can be reused
            newTexture = nullptr;
        }
    }

    //all worker threads are done with their work - join them
    for(uint32_t i = 0; i < workerThreadsNum; ++i)
    {
        workerThreadPool[i].join();
    }

    //finish rest of the work with main thread (if such is left)
    while(_loadedSurfacesThreadQueue->tryPop(currResSurface))
    {
        if(EXIT_SUCCESS !=
                Texture::loadTextureFromSurface(currResSurface.second,
                                                newTexture))
        {
            LOGERR("Error in Texture::loadTextureFromSurface() for rsrcId: "
                                           "%#16lX", currResSurface.first);

            return;
        }
        else
        {
            //emplace GPU Texture to the rsrcMap
            _rsrcMap[currResSurface.first] = newTexture;

            //reset the variable so it can be reused
            newTexture = nullptr;
        }
    }
}

I am facing the exactly same problem. Any clue?

If you haven’t found a solution to your issue: shouldn’t you be pointing to the dereferenced variable like that instead?

@Eduardo_Fernandes You should probably create your own thread if you’re not copying nmviper’s code exactly.

I am not copying this code, by mine is very similar to it. Actually my game is made with pure C instead of C++.

The problem only occurs when I call IMG_Load in a thread and call the SDL_FreeSurface in the main thread.
If everything runs in the same thread no exception is throwed.

Unless you have some mutexes protecting the surfaces, there will be trouble, always.

You haven’t posted any code, so the best I could potentially give you is theory and speculation, and I’ve been doing very poorly with both of those recently.

As Blerg mentioned you should probably wrap critical parts of the code with a mutex, If you have multiple writes and reads to a container and its not thread safe this could cause problems.

Is multithreading needed to load surfaces / textures. How many times are you going to load / release textures?

If this is a learning project then go ahead otherwise it may be overkill.

In may case, is a professional project. The problem is that there are literally thousands of images to load, so that’s why I’m working on the multithread loading.
In theory the surfaces can be generated in parallel threads, but the texture needs to be created in the main thread. This is working perfectly here. The problem occurs only when I call the SDL_FreeSurface.
This is called from the main thread, but sometimes throws a (double free or corruption) error.

May I ask how the SDL_Surfaces are stored as in container. And is the releasing of the surfaces done in the same thread ?

I have the same issue. I create surface in one thread and free it in another. It causes critical error on windows.
SLD2-2.0.7

it seems that issue is in following function
void
SDL_FreeFormat(SDL_PixelFormat *format)
{
SDL_PixelFormat *prev;

if (!format) {
    SDL_InvalidParamError("format");
    return;
}
if (--format->refcount > 0) {
    return;
}

/* Remove this format from our list */
if (format == formats) {
    formats = format->next;
} else if (formats) {
    for (prev = formats; prev->next; prev = prev->next) {
        if (prev->next == format) {
            prev->next = format->next;
            break;
        }
    }
}

if (format->palette) {
    SDL_FreePalette(format->palette);
}
SDL_free(format);

}

list operations are not thread safe

1 Like

Thanks everyone for participating in solving the problem. You are the best! :wink:

Giving feedback to everyone that are having the same issue as me:

Short version:
There seems to be a problem with the internal SDL2 implementation on this one as @salos pointed out.
Good news is for the sake of the test I updated my SDL2 to the latest stable version 2.0.8 and the problem no longer seems to appear.

============
Long version:
@Smiles to answer your questions

Yes, I am working on a professional project with “modules” where images are constantly loaded/unloaded from the program.

The SDL_Surfaces are stored in a thread safe queue, which operations are protected internally by mutexes. I’ve pasted the original source code in my first post.

And for the releasing in the same thread question:
The SDL_Surfaces are created in worker threads(not main thread), then pushed into the thread safe queue.
The main thread(which created the SDL_renderer consumes this SDL_Surface queue, create the SDL_Texture from it and free the SDL_Surface). So you could say the the SDL_Surface is freed in a different thread(from which it was created).

@salos You are my hero, thank you!
The problem was not how I was using the SDL_Surfaces. The problem was how they were internally stored by the SDL.

Again, switching to SDL2 version 2.0.8 seems to solve my problem (not a single crash in a couple of weeks).

But even if that would not be the case - I now know where the problem lies and can think of a way of fixing it.