Merge pull request #18810 from hrydgard/more-fixes

More fixes looking at crash reports

Author: hrydgard

Common/GPU/OpenGL/GLMemory.cpp

@@ -63,8 +63,7 @@ bool GLRBuffer::Unmap() {
 }
 
 GLPushBuffer::GLPushBuffer(GLRenderManager *render, GLuint target, size_t size, const char *tag) : render_(render), size_(size), target_(target), tag_(tag) {
-	bool res = AddBuffer();
-	_assert_(res);
+	AddBuffer();
 	RegisterGPUMemoryManager(this);
 }
 
@@ -103,7 +102,6 @@ void GLPushBuffer::Unmap() {
 void GLPushBuffer::Flush() {
 	// Must be called from the render thread.
 	_dbg_assert_(OnRenderThread());
-
 	if (buf_ >= buffers_.size()) {
 		_dbg_assert_msg_(false, "buf_ somehow got out of sync: %d vs %d", (int)buf_, (int)buffers_.size());
 		return;
@@ -138,17 +136,15 @@ void GLPushBuffer::Flush() {
 	}
 }
 
-bool GLPushBuffer::AddBuffer() {
+void GLPushBuffer::AddBuffer() {
 	// INFO_LOG(G3D, "GLPushBuffer(%s): Allocating %d bytes", tag_, size_);
 	BufInfo info;
 	info.localMemory = (uint8_t *)AllocateAlignedMemory(size_, 16);
-	if (!info.localMemory)
-		return false;
+	_assert_msg_(info.localMemory != 0, "GLPushBuffer alloc fail: %d (%s)", (int)size_, tag_);
 	info.buffer = render_->CreateBuffer(target_, size_, GL_DYNAMIC_DRAW);
 	info.size = size_;
 	buf_ = buffers_.size();
 	buffers_.push_back(info);
-	return true;
 }
 
 void GLPushBuffer::Destroy(bool onRenderThread) {
@@ -178,13 +174,7 @@ void GLPushBuffer::NextBuffer(size_t minSize) {
 		while (size_ < minSize) {
 			size_ <<= 1;
 		}
-
-		bool res = AddBuffer();
-		_assert_(res);
-		if (!res) {
-			// Let's try not to crash at least?
-			buf_ = 0;
-		}
+		AddBuffer();
 	}
 
 	// Now, move to the next buffer and map it.
@@ -219,8 +209,7 @@ void GLPushBuffer::Defragment() {
 
 	// Set some sane but very free limits. If there's another spike, we'll just allocate more anyway.
 	size_ = std::min(std::max(newSize, (size_t)65536), (size_t)(512 * 1024 * 1024));
-	bool res = AddBuffer();
-	_assert_msg_(res, "AddBuffer failed");
+	AddBuffer();
 }
 
 size_t GLPushBuffer::GetTotalSize() const {

Common/GPU/OpenGL/GLMemory.h

@@ -169,7 +169,7 @@ class GLPushBuffer : public GPUMemoryManager {
 	void UnmapDevice();
 
 private:
-	bool AddBuffer();
+	void AddBuffer();  // asserts on failure
 	void NextBuffer(size_t minSize);
 	void Defragment();
 

Common/GPU/Vulkan/VulkanContext.cpp

@@ -697,6 +697,11 @@ VkResult VulkanContext::CreateDevice() {
 	deviceFeatures_.enabled.standard.shaderCullDistance = deviceFeatures_.available.standard.shaderCullDistance;
 	deviceFeatures_.enabled.standard.geometryShader = deviceFeatures_.available.standard.geometryShader;
 	deviceFeatures_.enabled.standard.sampleRateShading = deviceFeatures_.available.standard.sampleRateShading;
+	
+#ifdef _DEBUG
+	// For debugging! Although, it might hide problems, so turning it off. Can be useful to rule out classes of issues.
+	// deviceFeatures_.enabled.standard.robustBufferAccess = deviceFeatures_.available.standard.robustBufferAccess;
+#endif
 
 	deviceFeatures_.enabled.multiview = { VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_MULTIVIEW_FEATURES };
 	if (extensionsLookup_.KHR_multiview) {

Common/GPU/Vulkan/VulkanFrameData.cpp

@@ -201,7 +201,7 @@ void FrameData::Submit(VulkanContext *vulkan, FrameSubmitType type, FrameDataSha
 	}
 
 	if (hasPresentCommands) {
-		_dbg_assert_(type == FrameSubmitType::FinishFrame);
+		_dbg_assert_(type != FrameSubmitType::Pending);
 		VkResult res = vkEndCommandBuffer(presentCmd);
 
 		_assert_msg_(res == VK_SUCCESS, "vkEndCommandBuffer failed (present)! result=%s", VulkanResultToString(res));

Common/GPU/Vulkan/VulkanRenderManager.cpp

@@ -585,7 +585,9 @@ void VulkanRenderManager::PollPresentTiming() {
 	// Poll for information about completed frames.
 	// NOTE: We seem to get the information pretty late! Like after 6 frames, which is quite weird.
 	// Tested on POCO F4.
-	if (vulkan_->Extensions().GOOGLE_display_timing) {
+	// TODO: Getting validation errors that this should be called from the thread doing the presenting.
+	// Probably a fair point. For now, we turn it off.
+	if (measurePresentTime_ && vulkan_->Extensions().GOOGLE_display_timing) {
 		uint32_t count = 0;
 		vkGetPastPresentationTimingGOOGLE(vulkan_->GetDevice(), vulkan_->GetSwapchain(), &count, nullptr);
 		if (count > 0) {

Common/GPU/Vulkan/thin3d_vulkan.cpp

@@ -1543,6 +1543,10 @@ std::vector<std::string> VKContext::GetFeatureList() const {
 	AddFeature(features, "shaderCullDistance", available.shaderCullDistance, enabled.shaderCullDistance);
 	AddFeature(features, "occlusionQueryPrecise", available.occlusionQueryPrecise, enabled.occlusionQueryPrecise);
 	AddFeature(features, "multiDrawIndirect", available.multiDrawIndirect, enabled.multiDrawIndirect);
+	AddFeature(features, "robustBufferAccess", available.robustBufferAccess, enabled.robustBufferAccess);
+	AddFeature(features, "fullDrawIndexUint32", available.fullDrawIndexUint32, enabled.fullDrawIndexUint32);
+	AddFeature(features, "fragmentStoresAndAtomics", available.fragmentStoresAndAtomics, enabled.fragmentStoresAndAtomics);
+	AddFeature(features, "shaderInt16", available.shaderInt16, enabled.shaderInt16);
 
 	AddFeature(features, "multiview", vulkan_->GetDeviceFeatures().available.multiview.multiview, vulkan_->GetDeviceFeatures().enabled.multiview.multiview);
 	AddFeature(features, "multiviewGeometryShader", vulkan_->GetDeviceFeatures().available.multiview.multiviewGeometryShader, vulkan_->GetDeviceFeatures().enabled.multiview.multiviewGeometryShader);

Core/HLE/Plugins.cpp

@@ -108,7 +108,11 @@ static std::vector<PluginInfo> FindPlugins(const std::string &gameID, const std:
 		std::set<std::string> matches;
 
 		std::string gameIni;
-		if (ini.GetOrCreateSection("games")->Get(gameID.c_str(), &gameIni, "")) {
+
+		// TODO: Should just use getsection and fail the ini if not found, I guess.
+		Section *games = ini.GetOrCreateSection("games");
+
+		if (games->Get(gameID.c_str(), &gameIni, "")) {
 			if (!strcasecmp(gameIni.c_str(), "true")) {
 				matches.insert("plugin.ini");
 			} else if (!strcasecmp(gameIni.c_str(), "false")){
@@ -118,7 +122,7 @@ static std::vector<PluginInfo> FindPlugins(const std::string &gameID, const std:
 			}
 		}
 
-		if (ini.GetOrCreateSection("games")->Get("ALL", &gameIni, "")) {
+		if (games->Get("ALL", &gameIni, "")) {
 			if (!strcasecmp(gameIni.c_str(), "true")) {
 				matches.insert("plugin.ini");
 			} else if (!gameIni.empty()) {

GPU/Common/DrawEngineCommon.h

@@ -194,6 +194,7 @@ class DrawEngineCommon {
 		gpuStats.numDrawCalls += numDrawInds_;
 		gpuStats.numVertexDecodes += numDrawVerts_;
 		gpuStats.numVertsSubmitted += vertexCountInDrawCalls_;
+		gpuStats.numVertsDecoded += numDecodedVerts_;
 
 		indexGen.Reset();
 		numDecodedVerts_ = 0;

GPU/Common/PresentationCommon.cpp

@@ -801,6 +801,7 @@ void PresentationCommon::CopyToOutput(OutputFlags flags, int uvRotation, float u
 	if (usePostShader) {
 		// When we render to temp framebuffers during post, we switch position, not UV.
 		// The flipping here is only because D3D has a clip coordinate system that doesn't match their screen coordinate system.
+		// The flipping here is only because D3D has a clip coordinate system that doesn't match their screen coordinate system.
 		bool flipped = flags & OutputFlags::POSITION_FLIPPED;
 		float y0 = flipped ? 1.0f : -1.0f;
 		float y1 = flipped ? -1.0f : 1.0f;

GPU/GPU.h

@@ -80,6 +80,7 @@ struct GPUStatistics {
 		numDrawSyncs = 0;
 		numListSyncs = 0;
 		numVertsSubmitted = 0;
+		numVertsDecoded = 0;
 		numUncachedVertsDrawn = 0;
 		numTextureInvalidations = 0;
 		numTextureInvalidationsByFramebuffer = 0;
@@ -119,6 +120,7 @@ struct GPUStatistics {
 	int numBBOXJumps;
 	int numPlaneUpdates;
 	int numVertsSubmitted;
+	int numVertsDecoded;
 	int numUncachedVertsDrawn;
 	int numTextureInvalidations;
 	int numTextureInvalidationsByFramebuffer;

GPU/GPUCommonHW.cpp

@@ -1749,7 +1749,7 @@ size_t GPUCommonHW::FormatGPUStatsCommon(char *buffer, size_t size) {
 	return snprintf(buffer, size,
 		"DL processing time: %0.2f ms, %d drawsync, %d listsync\n"
 		"Draw: %d (%d dec, %d culled), flushes %d, clears %d, bbox jumps %d (%d updates)\n"
-		"Vertices: %d drawn: %d\n"
+		"Vertices: %d dec: %d drawn: %d\n"
 		"FBOs active: %d (evaluations: %d)\n"
 		"Textures: %d, dec: %d, invalidated: %d, hashed: %d kB\n"
 		"readbacks %d (%d non-block), upload %d (cached %d), depal %d\n"
@@ -1768,6 +1768,7 @@ size_t GPUCommonHW::FormatGPUStatsCommon(char *buffer, size_t size) {
 		gpuStats.numBBOXJumps,
 		gpuStats.numPlaneUpdates,
 		gpuStats.numVertsSubmitted,
+		gpuStats.numVertsDecoded,
 		gpuStats.numUncachedVertsDrawn,
 		(int)framebufferManager_->NumVFBs(),
 		gpuStats.numFramebufferEvaluations,

GPU/Vulkan/DrawEngineVulkan.cpp

@@ -224,6 +224,7 @@ void DrawEngineVulkan::DoFlush() {
 	if (gstate_c.IsDirty(DIRTY_TEXTURE_IMAGE | DIRTY_TEXTURE_PARAMS) && !gstate.isModeClear() && gstate.isTextureMapEnabled()) {
 		textureCache_->SetTexture();
 		gstate_c.Clean(DIRTY_TEXTURE_IMAGE | DIRTY_TEXTURE_PARAMS);
+		// NOTE: After this is set, we MUST call ApplyTexture before returning.
 		textureNeedsApply = true;
 	} else if (gstate.getTextureAddress(0) == (gstate.getFrameBufRawAddress() | 0x04000000)) {
 		// This catches the case of clearing a texture.

UI/EmuScreen.cpp

@@ -1326,7 +1326,7 @@ ScreenRenderFlags EmuScreen::render(ScreenRenderMode mode) {
 		// We only bind it in FramebufferManager::CopyDisplayToOutput (unless non-buffered)...
 		// We do, however, start the frame in other ways.
 
-		if ((g_Config.bSkipBufferEffects && !g_Config.bSoftwareRendering) || Core_IsStepping()) {
+		if ((skipBufferEffects && !g_Config.bSoftwareRendering) || Core_IsStepping()) {
 			// We need to clear here already so that drawing during the frame is done on a clean slate.
 			if (Core_IsStepping() && gpuStats.numFlips != 0) {
 				draw->BindFramebufferAsRenderTarget(nullptr, { RPAction::KEEP, RPAction::CLEAR, RPAction::CLEAR }, "EmuScreen_BackBuffer");
@@ -1348,12 +1348,13 @@ ScreenRenderFlags EmuScreen::render(ScreenRenderMode mode) {
 		System_Notify(SystemNotification::KEEP_SCREEN_AWAKE);
 	} else if (!Core_ShouldRunBehind() && strcmp(screenManager()->topScreen()->tag(), "DevMenu") != 0) {
 		// Just to make sure.
-		if (PSP_IsInited() && !g_Config.bSkipBufferEffects) {
+		if (PSP_IsInited() && !skipBufferEffects) {
 			PSP_BeginHostFrame();
 			gpu->CopyDisplayToOutput(true);
 			PSP_EndHostFrame();
-		} else {
-			draw->BindFramebufferAsRenderTarget(nullptr, { RPAction::CLEAR, RPAction::CLEAR, RPAction::CLEAR, }, "EmuScreen_Stepping");
+		}
+		if (!framebufferBound && !gpu->PresentedThisFrame()) {
+			draw->BindFramebufferAsRenderTarget(nullptr, { RPAction::CLEAR, RPAction::CLEAR, RPAction::CLEAR, }, "EmuScreen_Behind");
 		}
 		// Need to make sure the UI texture is available, for "darken".
 		screenManager()->getUIContext()->BeginFrame();

UI/NativeApp.cpp

@@ -1061,23 +1061,6 @@ void NativeFrame(GraphicsContext *graphicsContext) {
 	ProcessWheelRelease(NKCODE_EXT_MOUSEWHEEL_UP, startTime, false);
 	ProcessWheelRelease(NKCODE_EXT_MOUSEWHEEL_DOWN, startTime, false);
 
-	std::vector<PendingMessage> toProcess;
-	{
-		std::lock_guard<std::mutex> lock(pendingMutex);
-		toProcess = std::move(pendingMessages);
-		pendingMessages.clear();
-	}
-
-	for (const auto &item : toProcess) {
-		if (HandleGlobalMessage(item.message, item.value)) {
-			// TODO: Add a to-string thingy.
-			INFO_LOG(SYSTEM, "Handled global message: %d / %s", (int)item.message, item.value.c_str());
-		}
-		g_screenManager->sendMessage(item.message, item.value.c_str());
-	}
-
-	g_requestManager.ProcessRequests();
-
 	// it's ok to call this redundantly with DoFrame from EmuScreen
 	Achievements::Idle();
 
@@ -1104,6 +1087,24 @@ void NativeFrame(GraphicsContext *graphicsContext) {
 
 	g_screenManager->update();
 
+	// Do this after g_screenManager.update() so we can receive setting changes before rendering.
+	std::vector<PendingMessage> toProcess;
+	{
+		std::lock_guard<std::mutex> lock(pendingMutex);
+		toProcess = std::move(pendingMessages);
+		pendingMessages.clear();
+	}
+
+	for (const auto &item : toProcess) {
+		if (HandleGlobalMessage(item.message, item.value)) {
+			// TODO: Add a to-string thingy.
+			INFO_LOG(SYSTEM, "Handled global message: %d / %s", (int)item.message, item.value.c_str());
+		}
+		g_screenManager->sendMessage(item.message, item.value.c_str());
+	}
+
+	g_requestManager.ProcessRequests();
+
 	// Apply the UIContext bounds as a 2D transformation matrix.
 	// TODO: This should be moved into the draw context...
 	Matrix4x4 ortho = ComputeOrthoMatrix(g_display.dp_xres, g_display.dp_yres);

android/jni/app-android.cpp

@@ -115,8 +115,6 @@ enum class EmuThreadState {
 static std::thread emuThread;
 static std::atomic<int> emuThreadState((int)EmuThreadState::DISABLED);
 
-void UpdateRunLoopAndroid(JNIEnv *env);
-
 AndroidAudioState *g_audioState;
 
 struct FrameCommand {
@@ -202,6 +200,8 @@ int utimensat(int fd, const char *path, const struct timespec times[2]) {
 }
 #endif
 
+static void ProcessFrameCommands(JNIEnv *env);
+
 void AndroidLogger::Log(const LogMessage &message) {
 	int mode;
 	switch (message.level) {
@@ -337,8 +337,22 @@ static void EmuThreadFunc() {
 	// We just call the update/render loop here.
 	emuThreadState = (int)EmuThreadState::RUNNING;
 	while (emuThreadState != (int)EmuThreadState::QUIT_REQUESTED) {
-		UpdateRunLoopAndroid(env);
+		{
+			std::lock_guard<std::mutex> renderGuard(renderLock);
+			NativeFrame(graphicsContext);
+		}
+
+		std::lock_guard<std::mutex> guard(frameCommandLock);
+		if (!nativeActivity) {
+			ERROR_LOG(SYSTEM, "No activity, clearing commands");
+			while (!frameCommands.empty())
+				frameCommands.pop();
+			return;
+		}
+		// Still under lock here.
+		ProcessFrameCommands(env);
 	}
+
 	INFO_LOG(SYSTEM, "QUIT_REQUESTED found, left EmuThreadFunc loop. Setting state to STOPPED.");
 	emuThreadState = (int)EmuThreadState::STOPPED;
 
@@ -371,8 +385,6 @@ static void EmuThreadJoin() {
 	INFO_LOG(SYSTEM, "EmuThreadJoin - joined");
 }
 
-static void ProcessFrameCommands(JNIEnv *env);
-
 static void PushCommand(std::string cmd, std::string param) {
 	std::lock_guard<std::mutex> guard(frameCommandLock);
 	frameCommands.push(FrameCommand(cmd, param));
@@ -1162,23 +1174,6 @@ extern "C" void JNICALL Java_org_ppsspp_ppsspp_NativeApp_sendRequestResult(JNIEn
 	}
 }
 
-void UpdateRunLoopAndroid(JNIEnv *env) {
-	{
-		std::lock_guard<std::mutex> renderGuard(renderLock);
-		NativeFrame(graphicsContext);
-	}
-
-	std::lock_guard<std::mutex> guard(frameCommandLock);
-	if (!nativeActivity) {
-		ERROR_LOG(SYSTEM, "No activity, clearing commands");
-		while (!frameCommands.empty())
-			frameCommands.pop();
-		return;
-	}
-	// Still under lock here.
-	ProcessFrameCommands(env);
-}
-
 extern "C" void Java_org_ppsspp_ppsspp_NativeRenderer_displayRender(JNIEnv *env, jobject obj) {
 	// This doesn't get called on the Vulkan path.
 	_assert_(useCPUThread);
@@ -1641,7 +1636,10 @@ static void VulkanEmuThread(ANativeWindow *wnd) {
 				std::lock_guard<std::mutex> renderGuard(renderLock);
 				NativeFrame(graphicsContext);
 			}
-			ProcessFrameCommands(env);
+			{
+				std::lock_guard<std::mutex> guard(frameCommandLock);
+				ProcessFrameCommands(env);
+			}
 		}
 		INFO_LOG(G3D, "Leaving Vulkan main loop.");
 	} else {