Make saving screenshot thread- and exception-safe

* introduce ResourceOwningThread wrapper for safely passing resources
   to new threads
 * make CEventQueue thread-safe
 * start screenshot saving thread using ResourceOwningThread
 * change direct call at end of writing screenshot to thread-safe
   event communication

src/app/app.cpp

@@ -105,7 +105,6 @@ struct ApplicationPrivate
 
 CApplication::CApplication()
  : m_private(MakeUnique<ApplicationPrivate>())
- , m_eventQueue(MakeUnique<CEventQueue>())
  , m_configFile(MakeUnique<CConfigFile>())
  , m_input(MakeUnique<CInput>())
  , m_pathManager(MakeUnique<CPathManager>())
@@ -590,6 +589,8 @@ bool CApplication::Create()
         return false;
     }
 
+    m_eventQueue = MakeUnique<CEventQueue>();
+
     // Create the robot application.
     m_controller = MakeUnique<CController>(this, !defaultValues);
 

src/common/event.cpp

@@ -505,6 +505,8 @@ void InitializeEventTypeTexts()
     EVENT_TYPE_TEXT[EVENT_STUDIO_RUN]        = "EVENT_STUDIO_RUN";
     EVENT_TYPE_TEXT[EVENT_STUDIO_REALTIME]   = "EVENT_STUDIO_REALTIME";
     EVENT_TYPE_TEXT[EVENT_STUDIO_STEP]       = "EVENT_STUDIO_STEP";
+
+    EVENT_TYPE_TEXT[EVENT_WRITE_SCENE_FINISHED] = "EVENT_WRITE_SCENE_FINISHED";
 }
 
 std::string ParseEventType(EventType eventType)
@@ -546,47 +548,72 @@ std::string ParseEventType(EventType eventType)
 
 
 CEventQueue::CEventQueue()
-{
-    Flush();
-}
+    : m_mutex{},
+      m_fifo{},
+      m_head{0},
+      m_tail{0},
+      m_total{0}
+{}
 
 CEventQueue::~CEventQueue()
-{
-}
-
-void CEventQueue::Flush()
-{
-    m_head = 0;
-    m_tail = 0;
-    m_total = 0;
-}
+{}
 
 /** If the maximum size of queue has been reached, returns \c false.
     Else, adds the event to the queue and returns \c true. */
 bool CEventQueue::AddEvent(const Event &event)
 {
-    if ( m_total >= MAX_EVENT_QUEUE )
+    bool result{};
+
+    SDL_LockMutex(*m_mutex);
+
+    if (m_total >= MAX_EVENT_QUEUE)
     {
         GetLogger()->Warn("Event queue flood!\n");
-        return false;
+
+        result = false;
+    }
+    else
+    {
+        m_fifo[m_head++] = event;
+
+        if (m_head >= MAX_EVENT_QUEUE)
+            m_head = 0;
+
+        m_total++;
+
+        result = true;
     }
 
-    m_fifo[m_head++] = event;
-    if ( m_head >= MAX_EVENT_QUEUE )  m_head = 0;
-    m_total ++;
+    SDL_UnlockMutex(*m_mutex);
 
-    return true;
+    return result;
 }
 
 /** If the queue is empty, returns \c false.
     Else, gets the event from the front, puts it into \a event and returns \c true. */
 bool CEventQueue::GetEvent(Event &event)
 {
-    if ( m_head == m_tail )  return false;
+    bool result{};
 
-    event = m_fifo[m_tail++];
-    if ( m_tail >= MAX_EVENT_QUEUE )  m_tail = 0;
-    m_total --;
+    SDL_LockMutex(*m_mutex);
 
-    return true;
+    if (m_head == m_tail)
+    {
+        result = false;
+    }
+    else
+    {
+        event = m_fifo[m_tail++];
+
+        if (m_tail >= MAX_EVENT_QUEUE)
+            m_tail = 0;
+
+        m_total--;
+
+        result = true;
+    }
+
+    SDL_UnlockMutex(*m_mutex);
+
+    return result;
 }

src/common/event.h

@@ -27,10 +27,11 @@
 
 #include "common/key.h"
 
+#include "common/thread/sdl_mutex_wrapper.h"
+
 #include "math/point.h"
 #include "math/vector.h"
 
-
 /**
   \enum EventType
   \brief Type of event message
@@ -533,6 +534,8 @@ enum EventType
     EVENT_STUDIO_REALTIME   = 2052,
     EVENT_STUDIO_STEP       = 2053,
 
+    EVENT_WRITE_SCENE_FINISHED = 2100, //!< indicates end of writing scene (writing screenshot image)
+
     //! Maximum value of standard events
     EVENT_STD_MAX,
 
@@ -736,6 +739,8 @@ std::string ParseEventType(EventType eventType);
  *
  * Provides an interface to a global FIFO queue with events (both system- and user-generated).
  * The queue has a fixed maximum size but it should not be a problem.
+ *
+ * This class is thread-safe
  */
 class CEventQueue
 {
@@ -749,14 +754,13 @@ public:
     //! Object's destructor
     ~CEventQueue();
 
-    //! Empties the FIFO of events
-    void    Flush();
     //! Adds an event to the queue
     bool    AddEvent(const Event &event);
     //! Removes and returns an event from queue front
     bool    GetEvent(Event &event);
 
 protected:
+    SDLMutexWrapper m_mutex;
     Event        m_fifo[MAX_EVENT_QUEUE];
     int          m_head;
     int          m_tail;

src/common/thread/resource_owning_thread.h

@@ -0,0 +1,98 @@
+#pragma once
+
+#include "common/thread/sdl_cond_wrapper.h"
+#include "common/thread/sdl_mutex_wrapper.h"
+
+#include <SDL_thread.h>
+
+#include <memory>
+
+/**
+ * \class ResourceOwningThread
+ * \brief Wrapper around SDL thread allowing passing of resources in safe manner
+ *
+ * This class is a workaround for passing ownership of resources in a safe
+ * manner to newly created threads. It takes a pointer to a function to call
+ * in new thread and a unique_ptr to resource which is to be passed to the new thread.
+ *
+ * This is how it works:
+ *  - in main thread: create a new thread passing to it a special temporary context,
+ *  - in main thread: wait for synchronization signal that the ownership was passed,
+ *  - in new thread: acquire the resource from the context
+ *  - in new thread: signal back to main thread that the resource was acquired,
+ *  - in main thread: clean up temporary context and exit
+ *  - in new thread: run the specified function with the acquired resource.
+ *
+ * It's a bit complicated, but that's the safe (thread-safe and exception-safe)
+ * way of doing this.
+ */
+template<typename Resource>
+class ResourceOwningThread
+{
+public:
+    using ResourceUPtr = std::unique_ptr<Resource>;
+    using ThreadFunctionPtr = void(*)(ResourceUPtr);
+
+    ResourceOwningThread(ThreadFunctionPtr threadFunction, ResourceUPtr resource)
+        : m_threadFunction(threadFunction),
+          m_resource(std::move(resource))
+    {}
+
+    void Start()
+    {
+        SDLMutexWrapper mutex;
+        SDLCondWrapper cond;
+        bool condition = false;
+
+        ThreadData data;
+        data.resource = std::move(m_resource);
+        data.threadFunction = m_threadFunction;
+        data.mutex = &mutex;
+        data.cond = &cond;
+        data.condition = &condition;
+
+        SDL_LockMutex(*mutex);
+
+        SDL_CreateThread(Run, reinterpret_cast<void*>(&data));
+
+        while (!condition)
+        {
+            SDL_CondWait(*cond, *mutex);
+        }
+
+        SDL_UnlockMutex(*mutex);
+    }
+
+private:
+    static int Run(void* data)
+    {
+        ThreadFunctionPtr threadFunction = nullptr;
+        ResourceUPtr resource;
+
+        ThreadData* threadData = reinterpret_cast<ThreadData*>(data);
+        SDL_LockMutex(**threadData->mutex);
+
+        threadFunction = threadData->threadFunction;
+        resource = std::move(threadData->resource);
+
+        *threadData->condition = true;
+        SDL_CondSignal(**threadData->cond);
+        SDL_UnlockMutex(**threadData->mutex);
+
+        threadFunction(std::move(resource));
+        return 0;
+    }
+
+private:
+    struct ThreadData
+    {
+        ResourceUPtr resource;
+        SDLMutexWrapper* mutex = nullptr;
+        SDLCondWrapper* cond = nullptr;
+        bool* condition = nullptr;
+        ThreadFunctionPtr threadFunction = nullptr;
+    };
+
+    ThreadFunctionPtr m_threadFunction;
+    ResourceUPtr m_resource;
+};

src/common/thread/sdl_cond_wrapper.h

@@ -0,0 +1,27 @@
+#pragma once
+
+#include <SDL_thread.h>
+
+class SDLCondWrapper
+{
+public:
+    SDLCondWrapper()
+        : m_cond(SDL_CreateCond())
+    {}
+
+    ~SDLCondWrapper()
+    {
+        SDL_DestroyCond(m_cond);
+    }
+
+    SDLCondWrapper(const SDLCondWrapper&) = delete;
+    SDLCondWrapper& operator=(const SDLCondWrapper&) = delete;
+
+    SDL_cond* operator*()
+    {
+        return m_cond;
+    }
+
+private:
+    SDL_cond* m_cond;
+};

src/common/thread/sdl_mutex_wrapper.h

@@ -0,0 +1,27 @@
+#pragma once
+
+#include <SDL_thread.h>
+
+class SDLMutexWrapper
+{
+public:
+    SDLMutexWrapper()
+        : m_mutex(SDL_CreateMutex())
+    {}
+
+    ~SDLMutexWrapper()
+    {
+        SDL_DestroyMutex(m_mutex);
+    }
+
+    SDLMutexWrapper(const SDLMutexWrapper&) = delete;
+    SDLMutexWrapper& operator=(const SDLMutexWrapper&) = delete;
+
+    SDL_mutex* operator*()
+    {
+        return m_mutex;
+    }
+
+private:
+    SDL_mutex* m_mutex;
+};

src/graphics/engine/engine.cpp

@@ -28,6 +28,8 @@
 #include "common/logger.h"
 #include "common/make_unique.h"
 
+#include "common/thread/resource_owning_thread.h"
+
 #include "graphics/core/device.h"
 
 #include "graphics/engine/camera.h"
@@ -484,29 +486,22 @@ void CEngine::FrameUpdate()
     }
 }
 
-struct WriteScreenShotData
+void CEngine::WriteScreenShot(const std::string& fileName, int width, int height)
 {
-    std::unique_ptr<CImage> img;
-    std::string fileName;
-};
-
-bool CEngine::WriteScreenShot(const std::string& fileName, int width, int height)
-{
-    WriteScreenShotData* data = new WriteScreenShotData();
+    auto data = MakeUnique<WriteScreenShotData>();
     data->img = MakeUnique<CImage>(Math::IntPoint(width, height));
 
     data->img->SetDataPixels(m_device->GetFrameBufferPixels());
     data->img->FlipVertically();
 
     data->fileName = fileName;
-    SDL_CreateThread(CEngine::WriteScreenShotThread, data);
-    return true;
+
+    ResourceOwningThread<WriteScreenShotData> thread(CEngine::WriteScreenShotThread, std::move(data));
+    thread.Start();
 }
 
-int CEngine::WriteScreenShotThread(void* data_ptr)
+void CEngine::WriteScreenShotThread(std::unique_ptr<WriteScreenShotData> data)
 {
-    WriteScreenShotData* data = static_cast<WriteScreenShotData*>(data_ptr);
-
     if ( data->img->SavePNG(data->fileName.c_str()) )
     {
        GetLogger()->Debug("Save screenshot saved successfully\n");
@@ -516,10 +511,8 @@ int CEngine::WriteScreenShotThread(void* data_ptr)
        GetLogger()->Error("%s!\n", data->img->GetError().c_str());
     }
 
-    delete data;
-
-    CRobotMain::GetInstancePointer()->IOWriteSceneFinished();
-    return 0;
+    Event event(EVENT_WRITE_SCENE_FINISHED);
+    CApplication::GetInstancePointer()->GetEventQueue()->AddEvent(event);
 }
 
 bool CEngine::GetPause()

src/graphics/engine/engine.h

@@ -726,7 +726,7 @@ public:
 
 
     //! Writes a screenshot containing the current frame
-    bool            WriteScreenShot(const std::string& fileName, int width, int height);
+    void            WriteScreenShot(const std::string& fileName, int width, int height);
 
 
     //! Get pause mode
@@ -1348,7 +1348,12 @@ protected:
 
     int GetEngineState(const ModelTriangle& triangle);
 
-    static int WriteScreenShotThread(void* data_ptr);
+    struct WriteScreenShotData
+    {
+        std::unique_ptr<CImage> img;
+        std::string fileName;
+    };
+    static void WriteScreenShotThread(std::unique_ptr<WriteScreenShotData> data);
 
 protected:
     CApplication*     m_app;

src/object/robotmain.cpp

@@ -660,6 +660,12 @@ bool CRobotMain::ProcessEvent(Event &event)
         return EventFrame(event);
     }
 
+    if (event.type == EVENT_WRITE_SCENE_FINISHED)
+    {
+        IOWriteSceneFinished();
+        return false;
+    }
+
     // Management of the console.
     if (event.type == EVENT_KEY_DOWN &&
         event.key.key == KEY(BACKQUOTE))  // Pause ?