From 18f9bfb575ebd3d7ffa4e3183a82f86cca2279b7 Mon Sep 17 00:00:00 2001 From: Piotr Dziwinski Date: Tue, 4 Aug 2015 20:30:31 +0200 Subject: [PATCH] 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 | 3 +- src/common/event.cpp | 73 +++++++++++----- src/common/event.h | 10 ++- src/common/thread/resource_owning_thread.h | 98 ++++++++++++++++++++++ src/common/thread/sdl_cond_wrapper.h | 27 ++++++ src/common/thread/sdl_mutex_wrapper.h | 27 ++++++ src/graphics/engine/engine.cpp | 27 +++--- src/graphics/engine/engine.h | 9 +- src/object/robotmain.cpp | 6 ++ 9 files changed, 234 insertions(+), 46 deletions(-) create mode 100644 src/common/thread/resource_owning_thread.h create mode 100644 src/common/thread/sdl_cond_wrapper.h create mode 100644 src/common/thread/sdl_mutex_wrapper.h diff --git a/src/app/app.cpp b/src/app/app.cpp index c19898f3..29f8a4a5 100644 --- a/src/app/app.cpp +++ b/src/app/app.cpp @@ -105,7 +105,6 @@ struct ApplicationPrivate CApplication::CApplication() : m_private(MakeUnique()) - , m_eventQueue(MakeUnique()) , m_configFile(MakeUnique()) , m_input(MakeUnique()) , m_pathManager(MakeUnique()) @@ -590,6 +589,8 @@ bool CApplication::Create() return false; } + m_eventQueue = MakeUnique(); + // Create the robot application. m_controller = MakeUnique(this, !defaultValues); diff --git a/src/common/event.cpp b/src/common/event.cpp index e4ab5919..e8cf3ab3 100644 --- a/src/common/event.cpp +++ b/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; } diff --git a/src/common/event.h b/src/common/event.h index 46a7a9dc..db33c200 100644 --- a/src/common/event.h +++ b/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; diff --git a/src/common/thread/resource_owning_thread.h b/src/common/thread/resource_owning_thread.h new file mode 100644 index 00000000..2f7de773 --- /dev/null +++ b/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 + +#include + +/** + * \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 +class ResourceOwningThread +{ +public: + using ResourceUPtr = std::unique_ptr; + 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(&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(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; +}; diff --git a/src/common/thread/sdl_cond_wrapper.h b/src/common/thread/sdl_cond_wrapper.h new file mode 100644 index 00000000..1d970083 --- /dev/null +++ b/src/common/thread/sdl_cond_wrapper.h @@ -0,0 +1,27 @@ +#pragma once + +#include + +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; +}; diff --git a/src/common/thread/sdl_mutex_wrapper.h b/src/common/thread/sdl_mutex_wrapper.h new file mode 100644 index 00000000..a43555f9 --- /dev/null +++ b/src/common/thread/sdl_mutex_wrapper.h @@ -0,0 +1,27 @@ +#pragma once + +#include + +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; +}; diff --git a/src/graphics/engine/engine.cpp b/src/graphics/engine/engine.cpp index 6ecb135b..b2a3f91d 100644 --- a/src/graphics/engine/engine.cpp +++ b/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 img; - std::string fileName; -}; - -bool CEngine::WriteScreenShot(const std::string& fileName, int width, int height) -{ - WriteScreenShotData* data = new WriteScreenShotData(); + auto data = MakeUnique(); data->img = MakeUnique(Math::IntPoint(width, height)); data->img->SetDataPixels(m_device->GetFrameBufferPixels()); data->img->FlipVertically(); data->fileName = fileName; - SDL_CreateThread(CEngine::WriteScreenShotThread, data); - return true; + + ResourceOwningThread thread(CEngine::WriteScreenShotThread, std::move(data)); + thread.Start(); } -int CEngine::WriteScreenShotThread(void* data_ptr) +void CEngine::WriteScreenShotThread(std::unique_ptr data) { - WriteScreenShotData* data = static_cast(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() diff --git a/src/graphics/engine/engine.h b/src/graphics/engine/engine.h index aa560cd4..261db0e6 100644 --- a/src/graphics/engine/engine.h +++ b/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 img; + std::string fileName; + }; + static void WriteScreenShotThread(std::unique_ptr data); protected: CApplication* m_app; diff --git a/src/object/robotmain.cpp b/src/object/robotmain.cpp index 6338c1b6..745915fe 100644 --- a/src/object/robotmain.cpp +++ b/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 ?