Merge pull request #20096 from hrydgard/misc-commits

Misc logging/comment changes and fixes

Common/File/FileUtil.cpp

@@ -88,7 +88,7 @@ constexpr bool SIMULATE_SLOW_IO = false;
 #else
 constexpr bool SIMULATE_SLOW_IO = false;
 #endif
-constexpr bool LOG_IO = true;
+constexpr bool LOG_IO = false;
 
 #ifndef S_ISDIR
 #define S_ISDIR(m)  (((m)&S_IFMT) == S_IFDIR)

Core/Config.cpp

@@ -2047,7 +2047,7 @@ void PlayTimeTracker::Start(const std::string &gameId) {
 	if (gameId.empty()) {
 		return;
 	}
-	INFO_LOG(Log::System, "GameTimeTracker::Start(%s)", gameId.c_str());
+	VERBOSE_LOG(Log::System, "GameTimeTracker::Start(%s)", gameId.c_str());
 
 	auto iter = tracker_.find(std::string(gameId));
 	if (iter != tracker_.end()) {
@@ -2070,7 +2070,7 @@ void PlayTimeTracker::Stop(const std::string &gameId) {
 		return;
 	}
 
-	INFO_LOG(Log::System, "GameTimeTracker::Stop(%s)", gameId.c_str());
+	VERBOSE_LOG(Log::System, "GameTimeTracker::Stop(%s)", gameId.c_str());
 
 	auto iter = tracker_.find(std::string(gameId));
 	if (iter != tracker_.end()) {

Core/HLE/AtracCtx.cpp

@@ -188,6 +188,8 @@ void AtracBase::UpdateContextFromPSPMem() {
 
 	// Read in any changes from the game to the context.
 	// TODO: Might be better to just always track in RAM.
+	// TODO: It's possible that there are more changes we should read. Who knows,
+	// problem games like FlatOut might poke stuff into the context?
 	bufferState_ = context_->info.state;
 	// This value is actually abused by games to store the SAS voice number.
 	loopNum_ = context_->info.loopNum;
@@ -765,7 +767,6 @@ int AtracBase::GetSecondBufferInfo(u32 *fileOffset, u32 *desiredSize) {
 
 void Atrac::GetStreamDataInfo(u32 *writePtr, u32 *writableBytes, u32 *readOffset) {
 	u32 calculatedReadOffset;
-	// TODO: Feels like this should already have been computed?
 	CalculateStreamInfo(&calculatedReadOffset);
 
 	*writePtr = first_.addr + first_.offset;
@@ -776,7 +777,7 @@ void Atrac::GetStreamDataInfo(u32 *writePtr, u32 *writableBytes, u32 *readOffset
 void Atrac::UpdateBufferState() {
 	if (bufferMaxSize_ >= track_.fileSize) {
 		if (first_.size < track_.fileSize) {
-			// The buffer is big enough, but we don't have all the data yet.
+			// The buffer is big enough in RAM, but we don't have all the data yet.
 			bufferState_ = ATRAC_STATUS_HALFWAY_BUFFER;
 		} else {
 			bufferState_ = ATRAC_STATUS_ALL_DATA_LOADED;
@@ -793,6 +794,8 @@ void Atrac::UpdateBufferState() {
 	}
 }
 
+// The game calls this after actually writing data to the buffer, as specified by the return values from GetStreamDataInfo.
+// So, we should not have to call CalculateStreamInfo again here (although, might not be a bad idea for safety).
 int Atrac::AddStreamData(u32 bytesToAdd) {
 	u32 readOffset;
 	CalculateStreamInfo(&readOffset);
@@ -857,6 +860,7 @@ u32 Atrac::GetNextSamples() {
 	if (numSamples > track_.SamplesPerFrame())
 		numSamples = track_.SamplesPerFrame();
 	if (bufferState_ == ATRAC_STATUS_STREAMED_LOOP_FROM_END && (int)numSamples + currentSample_ > track_.endSample) {
+		// This probably only happens in PPSSPP due to our internal buffer, which needs to go away.
 		bufferState_ = ATRAC_STATUS_ALL_DATA_LOADED;
 	}
 	return numSamples;
@@ -963,7 +967,6 @@ u32 Atrac::DecodeData(u8 *outbuf, u32 outbufPtr, u32 *SamplesNum, u32 *finish, i
 		return SCE_ERROR_ATRAC_ALL_DATA_DECODED;
 	}
 
-	// TODO: This isn't at all right, but at least it makes the music "last" some time.
 	u32 numSamples = 0;
 
 	// It seems like the PSP aligns the sample position to 0x800...?
@@ -1081,6 +1084,7 @@ u32 Atrac::DecodeData(u8 *outbuf, u32 outbufPtr, u32 *SamplesNum, u32 *finish, i
 void AtracBase::SetLoopNum(int loopNum) {
 	// Spammed in MHU
 	loopNum_ = loopNum;
+	// Logic here looks wacky?
 	if (loopNum != 0 && track_.loopinfo.size() == 0) {
 		// Just loop the whole audio
 		// This is a rare modification of track_ after the fact.

Core/HLE/AtracCtx.h

@@ -51,6 +51,7 @@ const int PSP_ATRAC_LOOP_STREAM_DATA_IS_ON_MEMORY = -3;
 
 // This is not a PSP-native struct.
 // But, it's stored in its entirety in savestates, which makes it awkward to change it.
+// This is used for both first_ and second_, but the latter doesn't use all the fields.
 struct InputBuffer {
 	// Address of the buffer.
 	u32 addr;
@@ -79,17 +80,19 @@ struct AtracLoopInfo {
 
 class AudioDecoder;
 
+// This is (mostly) constant info, once a track has been loaded.
 struct Track {
 	// This both does and doesn't belong in Track - it's fixed for an Atrac instance. Oh well.
 	u32 codecType = 0;
 
 	// Size of the full track being streamed or played. Can be a lot larger than the in-memory buffer in the streaming modes.
 	u32 fileSize = 0;
 
-	// Not really used for much except queries, this keeps track of the bitrate of the track.
+	// Not really used for much except queries, this keeps track of the bitrate of the track (kbps).
 	u32 bitrate = 64;
 
-	// Signifies whether to use a more efficient coding mode with less stereo separation. For our purposes, just metadata.
+	// Signifies whether to use a more efficient coding mode with less stereo separation. For our purposes, just metadata,
+	// not actually used in decoding.
 	int jointStereo = 0;
 
 	// Number of audio channels in the track.
@@ -108,10 +111,11 @@ struct Track {
 	// sometimes not.
 	int firstSampleOffset = 0;
 
-	// Last sample number. Though, we made it so in Analyze, it's exclusive in the file.
+	// Last sample number. Inclusive. Though, we made it so that in Analyze, it's exclusive in the file.
 	// Does not take firstSampleOffset into account.
 	int endSample = 0;
 
+	// NOTE: The below CAN be written.
 	// Loop configuration. The PSP only supports one loop but we store them all.
 	std::vector<AtracLoopInfo> loopinfo;
 	// The actual used loop offsets. These appear to be raw offsets, not taking FirstSampleOffset2() into account.
@@ -195,6 +199,7 @@ class AtracBase {
 		outputChannels_ = channels;
 	}
 
+	// Refactor?
 	int atracID_ = -1;
 
 	PSPPointer<SceAtracContext> context_{};
@@ -280,14 +285,17 @@ class Atrac : public AtracBase {
 		return second_.size;
 	}
 
+	// Ask where in memory new data should be written.
 	void GetStreamDataInfo(u32 *writePtr, u32 *writableBytes, u32 *readOffset) override;
+	// Notify the player that the user has written some new data.
 	int AddStreamData(u32 bytesToAdd) override;
 	u32 AddStreamDataSas(u32 bufPtr, u32 bytesToAdd) override;
 	u32 ResetPlayPosition(int sample, int bytesWrittenFirstBuf, int bytesWrittenSecondBuf) override;
 	void GetResetBufferInfo(AtracResetBufferInfo *bufferInfo, int sample) override;
 	int SetData(u32 buffer, u32 readSize, u32 bufferSize, int outputChannels, int successCode) override;
 	u32 SetSecondBuffer(u32 secondBuffer, u32 secondBufferSize) override;
 	u32 DecodeData(u8 *outbuf, u32 outbufPtr, u32 *SamplesNum, u32 *finish, int *remains) override;
+	// Returns how many samples the next DecodeData will write.
 	u32 GetNextSamples() override;
 	void InitLowLevel(u32 paramsAddr, bool jointStereo) override;
 

Core/HLE/ErrorCodes.h

@@ -2,7 +2,7 @@
 
 #include "Common/CommonTypes.h"
 
-enum : u32 {
+enum PSPErrorCode : u32 {
 	SCE_KERNEL_ERROR_OK                               = 0,
 	SCE_KERNEL_ERROR_ALREADY                          = 0x80000020,
 	SCE_KERNEL_ERROR_BUSY                             = 0x80000021,

Core/HLE/sceAtrac.cpp

@@ -40,14 +40,14 @@
 
 // Notes about sceAtrac buffer management
 //
-// sceAtrac decodes from a buffer the game fills, where this buffer is one of:
-//   * Not yet initialized (state NO DATA = 1)
-//   * The entire size of the audio data, and filled with audio data (state ALL DATA LOADED = 2)
-//   * The entire size, but only partially filled so far (state HALFWAY BUFFER = 3)
-//   * Smaller than the audio, sliding without any loop (state STREAMED WITHOUT LOOP = 4)
-//   * Smaller than the audio, sliding with a loop at the end (state STREAMED WITH LOOP AT END = 5)
-//   * Smaller with a second buffer to help with a loop in the middle (state STREAMED WITH SECOND BUF = 6)
-//   * Not managed, decoding using "low level" manual looping etc. (LOW LEVEL = 8)
+// sceAtrac decodes from a buffer the game fills, where this buffer has a status, one of:
+//   * Not yet initialized (state NO_DATA = 1)
+//   * The entire size of the audio data, and filled with audio data (state ALL_DATA_LOADED = 2)
+//   * The entire size, but only partially filled so far (state HALFWAY_BUFFER = 3)
+//   * Smaller than the audio, sliding without any loop (state STREAMED_WITHOUT_LOOP = 4)
+//   * Smaller than the audio, sliding with a loop at the end (state STREAMED_WITH_LOOP_AT_END = 5)
+//   * Smaller with a second buffer to help with a loop in the middle (state STREAMED_WITH_SECOND_BUF = 6)
+//   * Not managed, decoding using "low level" manual looping etc. (LOW_LEVEL = 8)
 //   * Not managed, reserved externally - possibly by sceSas - through low level (RESERVED = 16)
 //
 // This buffer is generally filled by sceAtracAddStreamData, and where to fill it is given by
@@ -59,6 +59,7 @@
 // the buffer it will call sceAtracSetSecondBuffer.
 // The second buffer will just contain the data for the end of loop.  The "first" buffer may manage
 // only the looped portion, or some of the part after the loop (depending on second buf size.)
+// TODO: What games use this?
 //
 // Most files will be in RIFF format.  It's also possible to load in an OMA/AA3 format file, but
 // ultimately this will share the same buffer - it's just offset a bit more.

Core/HLE/sceKernelModule.cpp

@@ -2442,15 +2442,18 @@ static u32 sceKernelStopUnloadSelfModuleWithStatus(u32 exitCode, u32 argSize, u3
 	return __KernelStopUnloadSelfModuleWithOrWithoutStatus(exitCode, argSize, argp, statusAddr, optionAddr, true);
 }
 
-void __KernelReturnFromModuleFunc()
-{
+void __KernelReturnFromModuleFunc() {
 	// Return from the thread as normal.
 	hleSkipDeadbeef();
 	__KernelReturnFromThread();
 
 	SceUID leftModuleID = __KernelGetCurThreadModuleId();
 	SceUID leftThreadID = __KernelGetCurThread();
 	int exitStatus = __KernelGetThreadExitStatus(leftThreadID);
+	if (exitStatus < 0) {
+		ERROR_LOG(Log::sceModule, "%s=GetThreadExitStatus(%d)", KernelErrorToString(exitStatus), leftThreadID);
+	}
+	// What else should happen with the exit status?
 
 	// Reschedule immediately (to leave the thread) and delete it and its stack.
 	__KernelReSchedule("returned from module");

Core/HLE/sceKernelThread.cpp

@@ -1333,21 +1333,28 @@ int __KernelGetThreadExitStatus(SceUID threadID) {
 	u32 error;
 	PSPThread *t = kernelObjects.Get<PSPThread>(threadID, error);
 	if (!t) {
-		return hleLogError(Log::sceKernel, error);
+		return error;
 	}
 
 	// __KernelResetThread and __KernelCreateThread set exitStatus in case it's DORMANT.
 	if (t->nt.status == THREADSTATUS_DORMANT) {
-		return hleLogDebug(Log::sceKernel, t->nt.exitStatus);
+		return t->nt.exitStatus;
 	}
-	return hleLogVerbose(Log::sceKernel, SCE_KERNEL_ERROR_NOT_DORMANT, "not dormant");
+	return SCE_KERNEL_ERROR_NOT_DORMANT;
 }
 
 int sceKernelGetThreadExitStatus(SceUID threadID) {
-	u32 status = __KernelGetThreadExitStatus(threadID);
+	int status = __KernelGetThreadExitStatus(threadID);
 	// Seems this is called in a tight-ish loop, maybe awaiting an interrupt - issue #13698
 	hleEatCycles(330);
-	return hleNoLog(status);
+
+	// Some return values don't deserve to be considered errors for logging.
+	switch ((PSPErrorCode)status) {
+	case SCE_KERNEL_ERROR_NOT_DORMANT:
+		return hleLogDebug(Log::sceKernel, status);
+	default:
+		return hleLogDebugOrError(Log::sceKernel, status);
+	}
 }
 
 u32 sceKernelGetThreadmanIdType(u32 uid) {

UI/DebugOverlay.cpp

@@ -467,7 +467,6 @@ void DrawFPS(UIContext *ctx, const Bounds &bounds) {
 	}
 	if (System_GetPropertyBool(SYSPROP_CAN_READ_BATTERY_PERCENTAGE)) {
 		if (g_Config.iShowStatusFlags & (int)ShowStatusFlags::BATTERY_PERCENT) {
-			char temp[256];
 			const int percentage = System_GetPropertyInt(SYSPROP_BATTERY_PERCENTAGE);
 			// Just plain append battery. Add linebreak?
 			buffer.Printf(" Battery: %d%%", percentage);

UI/ImDebugger/ImDebugger.cpp

@@ -1000,8 +1000,8 @@ void DrawAudioDecodersView(ImConfig &cfg, ImControl &control) {
 			// Show details about the selected atrac context here.
 			char header[32];
 			snprintf(header, sizeof(header), "Atrac context %d", cfg.selectedAtracCtx);
-			if (ImGui::CollapsingHeader(header, ImGuiTreeNodeFlags_DefaultOpen)) {
-				ImGui::ProgressBar((float)ctx->CurrentSample() / ctx->GetTrack().endSample, ImVec2(200.0f, 0.0f));
+			if (ctx && ImGui::CollapsingHeader(header, ImGuiTreeNodeFlags_DefaultOpen)) {
+				ImGui::ProgressBar((float)ctx->CurrentSample() / (float)ctx->GetTrack().endSample, ImVec2(200.0f, 0.0f));
 				ImGui::Text("Status: %s", AtracStatusToString(ctx->BufferState()));
 				ImGui::Text("cur/end sample: %d/%d", ctx->CurrentSample(), ctx->GetTrack().endSample);
 				ImGui::Text("ctx addr: "); ImGui::SameLine(); ImClickableValue("addr", ctx->Decoder()->GetCtxPtr(), control, ImCmd::SHOW_IN_MEMORY_VIEWER);

UI/ImDebugger/ImDebugger.h

@@ -160,7 +160,7 @@ struct ImConfig {
 	int selectedFramebuffer = -1;
 	int selectedBreakpoint = -1;
 	int selectedMemCheck = -1;
-	int selectedAtracCtx = -1;
+	int selectedAtracCtx = 0;
 	uint64_t selectedTexAddr = 0;
 
 	bool realtimePixelPreview = false;