Merge pull request #20137 from hrydgard/screenshot-speedup

Screenshot performance improvement

Author: hrydgard

CMakeLists.txt

@@ -2411,8 +2411,6 @@ add_library(${CoreLibName} ${CoreLinkType}
 	Core/Screenshot.h
 	Core/System.cpp
 	Core/System.h
-	Core/ThreadPools.cpp
-	Core/ThreadPools.h
 	Core/Util/AtracTrack.cpp
 	Core/Util/AtracTrack.h
 	Core/Util/AudioFormat.cpp

Common/Thread/Promise.h

@@ -7,6 +7,22 @@
 #include "Common/Thread/Channel.h"
 #include "Common/Thread/ThreadManager.h"
 
+// Nobody needs to wait for this (except threadpool shutdown).
+template<class T>
+class IndependentTask : public Task {
+public:
+	IndependentTask(TaskType type, TaskPriority prio, T func) : func_(std::move(func)), type_(type), prio_(prio) {}
+	TaskType Type() const override { return type_; }
+	TaskPriority Priority() const override { return prio_; }
+	void Run() override {
+		func_();
+	}
+private:
+	T func_;
+	TaskType type_;
+	TaskPriority prio_;
+};
+
 template<class T>
 class PromiseTask : public Task {
 public:

Common/Thread/ThreadManager.cpp

@@ -24,6 +24,8 @@ const int MAX_CORES_TO_USE = 16;
 const int MIN_IO_BLOCKING_THREADS = 4;
 static constexpr size_t TASK_PRIORITY_COUNT = (size_t)TaskPriority::COUNT;
 
+ThreadManager g_threadManager;
+
 struct GlobalThreadContext {
 	std::mutex mutex;
 	std::deque<Task *> compute_queue[TASK_PRIORITY_COUNT];

Core/Core.vcxproj

@@ -1084,7 +1084,6 @@
     <ClCompile Include="MIPS\MIPSStackWalk.cpp" />
     <ClCompile Include="Screenshot.cpp" />
     <ClCompile Include="System.cpp" />
-    <ClCompile Include="ThreadPools.cpp" />
     <ClCompile Include="TiltEventProcessor.cpp" />
     <ClCompile Include="Util\AtracTrack.cpp" />
     <ClCompile Include="Util\AudioFormat.cpp" />
@@ -1470,7 +1469,6 @@
     <ClInclude Include="Screenshot.h" />
     <ClInclude Include="System.h" />
     <ClInclude Include="ThreadEventQueue.h" />
-    <ClInclude Include="ThreadPools.h" />
     <ClInclude Include="TiltEventProcessor.h" />
     <ClInclude Include="Util\AtracTrack.h" />
     <ClInclude Include="Util\AudioFormat.h" />

Core/Core.vcxproj.filters

@@ -952,9 +952,6 @@
     <ClCompile Include="KeyMap.cpp">
       <Filter>Core</Filter>
     </ClCompile>
-    <ClCompile Include="ThreadPools.cpp">
-      <Filter>Core</Filter>
-    </ClCompile>
     <ClCompile Include="Debugger\WebSocket\InputSubscriber.cpp">
       <Filter>Debugger\WebSocket</Filter>
     </ClCompile>
@@ -2034,9 +2031,6 @@
     <ClInclude Include="KeyMap.h">
       <Filter>Core</Filter>
     </ClInclude>
-    <ClInclude Include="ThreadPools.h">
-      <Filter>Core</Filter>
-    </ClInclude>
     <ClInclude Include="Debugger\WebSocket\InputSubscriber.h">
       <Filter>Debugger\WebSocket</Filter>
     </ClInclude>

Core/HLE/sceKernelMemory.cpp

@@ -22,6 +22,7 @@
 #include <sstream>
 
 #include "Common/Thread/ParallelLoop.h"
+#include "Common/Thread/ThreadManager.h"
 #include "Core/CoreTiming.h"
 #include "Core/Debugger/MemBlockInfo.h"
 #include "Core/HLE/HLE.h"
@@ -30,7 +31,6 @@
 #include "Core/MemMapHelpers.h"
 #include "Core/Reporting.h"
 #include "Core/System.h"
-#include "Core/ThreadPools.h"
 #include "Common/Serialize/Serializer.h"
 #include "Common/Serialize/SerializeFuncs.h"
 #include "Common/Serialize/SerializeMap.h"

Core/HLE/sceKernelModule.cpp

@@ -483,7 +483,7 @@ void PSPModule::GetLongInfo(char *ptr, int bufSize) const {
 	StringWriter w(ptr, bufSize);
 	w.F("%s: Version %d.%d. %d segments", nm.name, nm.version[1], nm.version[0], nm.nsegment).endl();
 	w.F("Memory block: %08x (%08x/%d bytes)", memoryBlockAddr, memoryBlockSize, memoryBlockSize).endl();
-	for (int i = 0; i < nm.nsegment; i++) {
+	for (int i = 0; i < (int)nm.nsegment; i++) {
 		w.F("  %08x (%08x bytes)\n", nm.segmentaddr[i], nm.segmentsize[i]);
 	}
 	w.F("Text: %08x (%08x bytes)\n", nm.text_addr, nm.text_size);

Core/SaveState.cpp

@@ -466,13 +466,12 @@ double g_lastSaveTime = -1.0;
 		Enqueue(Operation(SAVESTATE_REWIND, Path(), -1, callback, cbUserData));
 	}
 
-	void SaveScreenshot(const Path &filename, Callback callback, void *cbUserData) {
+	static void SaveScreenshot(const Path &filename) {
 		screenshotFailures = 0;
-		Enqueue(Operation(SAVESTATE_SAVE_SCREENSHOT, filename, -1, callback, cbUserData));
+		Enqueue(Operation(SAVESTATE_SAVE_SCREENSHOT, filename, -1, nullptr, nullptr));
 	}
 
-	bool CanRewind()
-	{
+	bool CanRewind() {
 		return !rewindStates.Empty();
 	}
 
@@ -690,7 +689,7 @@ double g_lastSaveTime = -1.0;
 				DeleteIfExists(shotUndo);
 				RenameIfExists(shot, shotUndo);
 			}
-			SaveScreenshot(shot, Callback(), 0);
+			SaveScreenshot(shot);
 			Save(fn.WithExtraExtension(".tmp"), slot, renameCallback, cbUserData);
 		} else {
 			if (callback) {
@@ -966,11 +965,9 @@ double g_lastSaveTime = -1.0;
 
 		bool readbackImage = false;
 
-		for (size_t i = 0, n = operations.size(); i < n; ++i) {
-			Operation &op = operations[i];
+		for (const auto &op : operations) {
 			CChunkFileReader::Error result;
 			Status callbackResult;
-			bool tempResult;
 			std::string callbackMessage;
 			std::string title;
 
@@ -1056,14 +1053,16 @@ double g_lastSaveTime = -1.0;
 				break;
 
 			case SAVESTATE_VERIFY:
-				tempResult = CChunkFileReader::Verify(state) == CChunkFileReader::ERROR_NONE;
+			{
+				int tempResult = CChunkFileReader::Verify(state) == CChunkFileReader::ERROR_NONE;
 				callbackResult = tempResult ? Status::SUCCESS : Status::FAILURE;
 				if (tempResult) {
 					INFO_LOG(Log::SaveState, "Verified save state system");
 				} else {
 					ERROR_LOG(Log::SaveState, "Save state system verification failed");
 				}
 				break;
+			}
 
 			case SAVESTATE_REWIND:
 				INFO_LOG(Log::SaveState, "Rewinding to recent savestate snapshot");
@@ -1093,18 +1092,32 @@ double g_lastSaveTime = -1.0;
 
 			case SAVESTATE_SAVE_SCREENSHOT:
 			{
+				_dbg_assert_(!op.callback);
+
 				int maxResMultiplier = 2;
-				tempResult = TakeGameScreenshot(nullptr, op.filename, ScreenshotFormat::JPG, SCREENSHOT_DISPLAY, nullptr, nullptr, maxResMultiplier);
-				callbackResult = tempResult ? Status::SUCCESS : Status::FAILURE;
-				if (!tempResult) {
+				ScreenshotResult tempResult = TakeGameScreenshot(nullptr, op.filename, ScreenshotFormat::JPG, SCREENSHOT_DISPLAY, maxResMultiplier, [](bool success) {
+					if (success) {
+						screenshotFailures = 0;
+					}
+				});
+				
+				switch (tempResult) {
+				case ScreenshotResult::ScreenshotNotPossible:
+					// Try again soon, for a short while.
+					callbackResult = Status::FAILURE;
 					WARN_LOG(Log::SaveState, "Failed to take a screenshot for the savestate! (%s) The savestate will lack an icon.", op.filename.c_str());
 					if (coreState != CORE_STEPPING_CPU && screenshotFailures++ < SCREENSHOT_FAILURE_RETRIES) {
 						// Requeue for next frame (if we were stepping, no point, will just spam errors quickly).
-						SaveScreenshot(op.filename, op.callback, op.cbUserData);
+						SaveScreenshot(op.filename);
 					}
-				} else {
-					screenshotFailures = 0;
+					break;
+				case ScreenshotResult::DelayedResult:
+				case ScreenshotResult::Success:
+					// We might not know if the file write succeeded yet though.
+					callbackResult = Status::SUCCESS;
+					break;
 				}
+
 				readbackImage = true;
 				break;
 			}

Core/Screenshot.cpp

@@ -26,7 +26,10 @@
 #include "Common/File/FileUtil.h"
 #include "Common/File/Path.h"
 #include "Common/Log.h"
+#include "Common/System/System.h"
 #include "Common/System/Display.h"
+#include "Common/System/NativeApp.h"
+#include "Common/Thread/Promise.h"
 #include "Core/Screenshot.h"
 #include "Core/System.h"
 #include "GPU/Common/GPUDebugInterface.h"
@@ -328,56 +331,54 @@ static GPUDebugBuffer ApplyRotation(const GPUDebugBuffer &buf, DisplayRotation r
 	return rotated;
 }
 
-bool TakeGameScreenshot(Draw::DrawContext *draw, const Path &filename, ScreenshotFormat fmt, ScreenshotType type, int *width, int *height, int maxRes) {
+ScreenshotResult TakeGameScreenshot(Draw::DrawContext *draw, const Path &filename, ScreenshotFormat fmt, ScreenshotType type, int maxRes, std::function<void(bool success)> callback) {
 	GPUDebugBuffer buf;
-	bool success = false;
 	u32 w = (u32)-1;
 	u32 h = (u32)-1;
 
 	if (type == SCREENSHOT_DISPLAY || type == SCREENSHOT_RENDER) {
 		if (!gpuDebug) {
 			ERROR_LOG(Log::System, "Can't take screenshots when GPU not running");
-			return false;
+			return ScreenshotResult::ScreenshotNotPossible;
+		}
+		if (!gpuDebug->GetCurrentFramebuffer(buf, type == SCREENSHOT_RENDER ? GPU_DBG_FRAMEBUF_RENDER : GPU_DBG_FRAMEBUF_DISPLAY, maxRes)) {
+			return ScreenshotResult::ScreenshotNotPossible;
 		}
-		success = gpuDebug->GetCurrentFramebuffer(buf, type == SCREENSHOT_RENDER ? GPU_DBG_FRAMEBUF_RENDER : GPU_DBG_FRAMEBUF_DISPLAY, maxRes);
 		w = maxRes > 0 ? 480 * maxRes : PSP_CoreParameter().renderWidth;
 		h = maxRes > 0 ? 272 * maxRes : PSP_CoreParameter().renderHeight;
 	} else if (g_display.rotation != DisplayRotation::ROTATE_0) {
 		_dbg_assert_(draw);
 		GPUDebugBuffer temp;
-		success = ::GetOutputFramebuffer(draw, temp);
-		if (success) {
-			buf = ApplyRotation(temp, g_display.rotation);
+		if (!::GetOutputFramebuffer(draw, temp)) {
+			return ScreenshotResult::ScreenshotNotPossible;
 		}
+		buf = ApplyRotation(temp, g_display.rotation);
 	} else {
 		_dbg_assert_(draw);
-		success = ::GetOutputFramebuffer(draw, buf);
-	}
-
-	if (!success) {
-		return false;
-	}
-
-	if (success) {
-		u8 *flipbuffer = nullptr;
-		const u8 *buffer = ConvertBufferToScreenshot(buf, false, flipbuffer, w, h);
-		success = buffer != nullptr;
-		if (success) {
-			if (width)
-				*width = w;
-			if (height)
-				*height = h;
-
-			success = Save888RGBScreenshot(filename, fmt, buffer, w, h);
+		if (!GetOutputFramebuffer(draw, buf)) {
+			return ScreenshotResult::ScreenshotNotPossible;
 		}
-		delete[] flipbuffer;
 	}
 
-	if (!success) {
-		ERROR_LOG(Log::IO, "Failed to write screenshot.");
+	if (callback) {
+		g_threadManager.EnqueueTask(new IndependentTask(TaskType::CPU_COMPUTE, TaskPriority::LOW,
+			[buf = std::move(buf), callback = std::move(callback), filename, fmt, w, h]() {
+			u8 *flipbuffer = nullptr;
+			u32 width = w, height = h;
+			const u8 *buffer = ConvertBufferToScreenshot(buf, false, flipbuffer, width, height);
+			bool success = Save888RGBScreenshot(filename, fmt, buffer, width, height);
+			delete[] flipbuffer;
+			System_RunOnMainThread([success, callback = std::move(callback)]() {
+				callback(success);
+			});
+		}));
+		return ScreenshotResult::DelayedResult;
 	}
-
-	return success;
+	u8 *flipbuffer = nullptr;
+	const u8 *buffer = ConvertBufferToScreenshot(buf, false, flipbuffer, w, h);
+	bool success = Save888RGBScreenshot(filename, fmt, buffer, w, h);
+	delete[] flipbuffer;
+	return success ? ScreenshotResult::Success : ScreenshotResult::FailedToWriteFile;
 }
 
 bool Save888RGBScreenshot(const Path &filename, ScreenshotFormat fmt, const u8 *bufferRGB888, int w, int h) {

Core/Screenshot.h

@@ -19,6 +19,8 @@
 
 #include "Common/File/Path.h"
 
+#include <functional>
+
 struct GPUDebugBuffer;
 namespace Draw {
 class DrawContext;
@@ -29,6 +31,8 @@ enum class ScreenshotFormat {
 	JPG,
 };
 
+// NOTE: The first two may need rotation, depending on the backend and screen orientation.
+// This is handled internally in TakeGameScreenshot().
 enum ScreenshotType {
 	// What's being show on screen (e.g. including FPS, etc.)
 	SCREENSHOT_OUTPUT,
@@ -38,10 +42,19 @@ enum ScreenshotType {
 	SCREENSHOT_RENDER,
 };
 
+enum class ScreenshotResult {
+	ScreenshotNotPossible,
+	DelayedResult,  // This specifies that the actual result is one of the two below and will arrive in the callback.
+	// These result can be delayed and arrive in the callback, if one is specified.
+	FailedToWriteFile,
+	Success,
+};
+
 const u8 *ConvertBufferToScreenshot(const GPUDebugBuffer &buf, bool alpha, u8 *&temp, u32 &w, u32 &h);
 
 // Can only be used while in game.
-bool TakeGameScreenshot(Draw::DrawContext *draw, const Path &filename, ScreenshotFormat fmt, ScreenshotType type, int *width = nullptr, int *height = nullptr, int maxRes = -1);
+// If the callback is passed in, the saving action happens on a background thread.
+ScreenshotResult TakeGameScreenshot(Draw::DrawContext *draw, const Path &filename, ScreenshotFormat fmt, ScreenshotType type, int maxRes = -1, std::function<void(bool success)> callback = nullptr);
 
 bool Save888RGBScreenshot(const Path &filename, ScreenshotFormat fmt, const u8 *bufferRGB888, int w, int h);
 bool Save8888RGBAScreenshot(const Path &filename, const u8 *bufferRGBA8888, int w, int h);