Change: Lock the video buffer when drawing inside the game loop to properly account for threaded drawing.

src/screenshot.cpp

@@ -25,6 +25,7 @@
 #include "window_func.h"
 #include "tile_map.h"
 #include "landscape.h"
+#include "video/video_driver.hpp"
 
 #include "table/strings.h"
 
@@ -881,6 +882,8 @@ void MakeScreenshotWithConfirm(ScreenshotType t)
  */
 bool MakeScreenshot(ScreenshotType t, const char *name)
 {
+	VideoDriver::VideoBufferLocker lock;
+
 	if (t == SC_VIEWPORT) {
 		/* First draw the dirty parts of the screen and only then change the name
 		 * of the screenshot. This way the screenshot will always show the name

src/video/video_driver.hpp

@@ -127,6 +127,25 @@ public:
 		return static_cast<VideoDriver*>(*DriverFactoryBase::GetActiveDriver(Driver::DT_VIDEO));
 	}
 
+	/**
+	 * Helper struct to ensure the video buffer is locked and ready for drawing. The destructor
+	 * will make sure the buffer is unlocked no matter how the scope is exited.
+	 */
+	struct VideoBufferLocker {
+		VideoBufferLocker()
+		{
+			this->unlock = VideoDriver::GetInstance()->LockVideoBuffer();
+		}
+
+		~VideoBufferLocker()
+		{
+			if (this->unlock) VideoDriver::GetInstance()->UnlockVideoBuffer();
+		}
+
+	private:
+		bool unlock; ///< Stores if the lock did anything that has to be undone.
+	};
+
 protected:
 	const uint ALLOWED_DRIFT = 5; ///< How many times videodriver can miss deadlines without it being overly compensated.
 

src/video/win32_v.cpp

@@ -26,8 +26,6 @@
 #include "win32_v.h"
 #include <windows.h>
 #include <imm.h>
-#include <mutex>
-#include <condition_variable>
 
 #include "../safeguards.h"
 
@@ -53,14 +51,6 @@ bool _window_maximize;
 static Dimension _bck_resolution;
 DWORD _imm_props;
 
-/** Whether the drawing is/may be done in a separate thread. */
-static bool _draw_threaded;
-/** Mutex to keep the access to the shared memory controlled. */
-static std::recursive_mutex *_draw_mutex = nullptr;
-/** Signal to draw the next frame. */
-static std::condition_variable_any *_draw_signal = nullptr;
-/** Should we keep continue drawing? */
-static volatile bool _draw_continue;
 /** Local copy of the palette for use in the drawing thread. */
 static Palette _local_palette;
 
@@ -934,34 +924,34 @@ void VideoDriver_Win32Base::MainLoop()
 
 	std::thread draw_thread;
 
-	if (_draw_threaded) {
+	if (this->draw_threaded) {
 		/* Initialise the mutex first, because that's the thing we *need*
 		 * directly in the newly created thread. */
 		try {
-			_draw_signal = new std::condition_variable_any();
+			this->draw_signal = new std::condition_variable_any();
-			_draw_mutex = new std::recursive_mutex();
+			this->draw_mutex = new std::recursive_mutex();
 		} catch (...) {
-			_draw_threaded = false;
+			this->draw_threaded = false;
 		}
 
-		if (_draw_threaded) {
+		if (this->draw_threaded) {
-			this->draw_lock = std::unique_lock<std::recursive_mutex>(*_draw_mutex);
+			this->draw_lock = std::unique_lock<std::recursive_mutex>(*this->draw_mutex);
 
-			_draw_continue = true;
+			this->draw_continue = true;
-			_draw_threaded = StartNewThread(&draw_thread, "ottd:draw-win32", &VideoDriver_Win32Base::PaintThreadThunk, this);
+			this->draw_threaded = StartNewThread(&draw_thread, "ottd:draw-win32", &VideoDriver_Win32Base::PaintThreadThunk, this);
 
 			/* Free the mutex if we won't be able to use it. */
-			if (!_draw_threaded) {
+			if (!this->draw_threaded) {
 				this->draw_lock.unlock();
 				this->draw_lock.release();
-				delete _draw_mutex;
+				delete this->draw_mutex;
-				delete _draw_signal;
+				delete this->draw_signal;
-				_draw_mutex = nullptr;
+				this->draw_mutex = nullptr;
-				_draw_signal = nullptr;
+				this->draw_signal = nullptr;
 			} else {
 				DEBUG(driver, 1, "Threaded drawing enabled");
 				/* Wait till the draw thread has started itself. */
-				_draw_signal->wait(*_draw_mutex);
+				this->draw_signal->wait(*this->draw_mutex);
 			}
 		}
 	}
@@ -982,8 +972,8 @@ void VideoDriver_Win32Base::MainLoop()
 		GdiFlush();
 
 		if (this->Tick()) {
-			if (_draw_mutex != nullptr && !HasModalProgress()) {
+			if (this->draw_mutex != nullptr && !HasModalProgress()) {
-				_draw_signal->notify_one();
+				this->draw_signal->notify_one();
 			} else {
 				this->Paint();
 			}
@@ -991,19 +981,19 @@ void VideoDriver_Win32Base::MainLoop()
 		this->SleepTillNextTick();
 	}
 
-	if (_draw_threaded) {
+	if (this->draw_threaded) {
-		_draw_continue = false;
+		this->draw_continue = false;
 		/* Sending signal if there is no thread blocked
 		 * is very valid and results in noop */
-		_draw_signal->notify_all();
+		this->draw_signal->notify_all();
 		if (this->draw_lock.owns_lock()) this->draw_lock.unlock();
 		this->draw_lock.release();
 		draw_thread.join();
 
-		delete _draw_mutex;
+		delete this->draw_mutex;
-		delete _draw_signal;
+		delete this->draw_signal;
 
-		_draw_mutex = nullptr;
+		this->draw_mutex = nullptr;
 	}
 }
 
@@ -1025,7 +1015,7 @@ void VideoDriver_Win32Base::ClientSizeChanged(int w, int h)
 bool VideoDriver_Win32Base::ChangeResolution(int w, int h)
 {
 	std::unique_lock<std::recursive_mutex> lock;
-	if (_draw_mutex != nullptr) lock = std::unique_lock<std::recursive_mutex>(*_draw_mutex);
+	if (this->draw_mutex != nullptr) lock = std::unique_lock<std::recursive_mutex>(*this->draw_mutex);
 
 	if (_window_maximize) ShowWindow(this->main_wnd, SW_SHOWNORMAL);
 
@@ -1038,25 +1028,25 @@ bool VideoDriver_Win32Base::ChangeResolution(int w, int h)
 bool VideoDriver_Win32Base::ToggleFullscreen(bool full_screen)
 {
 	std::unique_lock<std::recursive_mutex> lock;
-	if (_draw_mutex != nullptr) lock = std::unique_lock<std::recursive_mutex>(*_draw_mutex);
+	if (this->draw_mutex != nullptr) lock = std::unique_lock<std::recursive_mutex>(*this->draw_mutex);
 
 	return this->MakeWindow(full_screen);
 }
 
 void VideoDriver_Win32Base::AcquireBlitterLock()
 {
-	if (_draw_mutex != nullptr) _draw_mutex->lock();
+	if (this->draw_mutex != nullptr) this->draw_mutex->lock();
 }
 
 void VideoDriver_Win32Base::ReleaseBlitterLock()
 {
-	if (_draw_mutex != nullptr) _draw_mutex->unlock();
+	if (this->draw_mutex != nullptr) this->draw_mutex->unlock();
 }
 
 void VideoDriver_Win32Base::EditBoxLostFocus()
 {
 	std::unique_lock<std::recursive_mutex> lock;
-	if (_draw_mutex != nullptr) lock = std::unique_lock<std::recursive_mutex>(*_draw_mutex);
+	if (this->draw_mutex != nullptr) lock = std::unique_lock<std::recursive_mutex>(*this->draw_mutex);
 
 	CancelIMEComposition(this->main_wnd);
 	SetCompositionPos(this->main_wnd);
@@ -1110,7 +1100,10 @@ float VideoDriver_Win32Base::GetDPIScale()
 
 bool VideoDriver_Win32Base::LockVideoBuffer()
 {
-	if (_draw_threaded) this->draw_lock.lock();
+	if (this->buffer_locked) return false;
+	this->buffer_locked = true;
+
+	if (this->draw_threaded) this->draw_lock.lock();
 
 	_screen.dst_ptr = this->GetVideoPointer();
 
@@ -1119,7 +1112,8 @@ bool VideoDriver_Win32Base::LockVideoBuffer()
 
 void VideoDriver_Win32Base::UnlockVideoBuffer()
 {
-	if (_draw_threaded) this->draw_lock.unlock();
+	if (this->draw_threaded) this->draw_lock.unlock();
+	this->buffer_locked = false;
 }
 
 
@@ -1137,7 +1131,7 @@ const char *VideoDriver_Win32GDI::Start(const StringList &param)
 
 	MarkWholeScreenDirty();
 
-	_draw_threaded = !GetDriverParam(param, "no_threads") && !GetDriverParam(param, "no_thread") && std::thread::hardware_concurrency() > 1;
+	this->draw_threaded = !GetDriverParam(param, "no_threads") && !GetDriverParam(param, "no_thread") && std::thread::hardware_concurrency() > 1;
 
 	return nullptr;
 }
@@ -1284,19 +1278,19 @@ void VideoDriver_Win32GDI::Paint()
 void VideoDriver_Win32GDI::PaintThread()
 {
 	/* First tell the main thread we're started */
-	std::unique_lock<std::recursive_mutex> lock(*_draw_mutex);
+	std::unique_lock<std::recursive_mutex> lock(*this->draw_mutex);
-	_draw_signal->notify_one();
+	this->draw_signal->notify_one();
 
 	/* Now wait for the first thing to draw! */
-	_draw_signal->wait(*_draw_mutex);
+	this->draw_signal->wait(*this->draw_mutex);
 
-	while (_draw_continue) {
+	while (this->draw_continue) {
 		this->Paint();
 
 		/* Flush GDI buffer to ensure drawing here doesn't conflict with any GDI usage in the main WndProc. */
 		GdiFlush();
 
-		_draw_signal->wait(*_draw_mutex);
+		this->draw_signal->wait(*this->draw_mutex);
 	}
 }
 
@@ -1357,7 +1351,7 @@ const char *VideoDriver_Win32OpenGL::Start(const StringList &param)
 
 	this->ClientSizeChanged(_wnd.width, _wnd.height);
 
-	_draw_threaded = false;
+	this->draw_threaded = false;
 	MarkWholeScreenDirty();
 
 	return nullptr;

src/video/win32_v.h

@@ -11,11 +11,13 @@
 #define VIDEO_WIN32_H
 
 #include "video_driver.hpp"
+#include <mutex>
+#include <condition_variable>
 
 /** Base class for Windows video drivers. */
 class VideoDriver_Win32Base : public VideoDriver {
 public:
-	VideoDriver_Win32Base() : main_wnd(nullptr), fullscreen(false) {}
+	VideoDriver_Win32Base() : main_wnd(nullptr), fullscreen(false), draw_mutex(nullptr), draw_signal(nullptr) {}
 
 	void Stop() override;
 
@@ -36,9 +38,16 @@ public:
 	void EditBoxLostFocus() override;
 
 protected:
-	HWND    main_wnd;      ///< Handle to system window.
+	HWND main_wnd;          ///< Handle to system window.
-	bool    fullscreen;    ///< Whether to use (true) fullscreen mode.
+	bool fullscreen;        ///< Whether to use (true) fullscreen mode.
-	Rect    dirty_rect;    ///< Region of the screen that needs redrawing.
+	Rect dirty_rect;        ///< Region of the screen that needs redrawing.
+
+	bool draw_threaded;          ///< Whether the drawing is/may be done in a separate thread.
+	bool buffer_locked;          ///< Video buffer was locked by the main thread.
+	volatile bool draw_continue; ///< Should we keep continue drawing?
+
+	std::recursive_mutex *draw_mutex;         ///< Mutex to keep the access to the shared memory controlled.
+	std::condition_variable_any *draw_signal; ///< Signal to draw the next frame.
 
 	Dimension GetScreenSize() const override;
 	float GetDPIScale() override;