From 45727ef2beccbb25e681c4ad55db1ddda7f4410c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 17 Sep 2026 10:09:48 -0600 Subject: [PATCH] Tighten the comments on the mpeg PRX changes Cut restatement and asides that only made sense against earlier, wrong versions of the code, and prefer parentheses over paired dashes. Also fix two comments left stale by the descriptor rework, and record the rule in AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 11 +++++++ Core/HLE/sceMpegbase.cpp | 57 ++++++++++++++++-------------------- Core/HLE/sceVideocodec.cpp | 22 +++++++------- Core/HLE/sceVideocodec.h | 4 +-- Core/Util/BlockAllocator.cpp | 5 ++-- 5 files changed, 51 insertions(+), 48 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 59aa1b1804..51a5f65db1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -252,6 +252,17 @@ private: But generally follow the surrounding style. Braces are preferred on the same line. Braces are always used even when they could be omitted due the inner part being just a single line. +### Comments + +Keep comments tight. Say the thing once; don't restate what the code already shows, and don't +allude to a previous, now-corrected version ("eight addresses and nothing else" - the "and nothing +else" only makes sense against the old wrong layout, which is history). Cut filler like "the two +are the same shape" down to "(the same shape)". + +For a parenthetical aside, prefer parentheses over a pair of spaced dashes: write "the audio thread +that paces playback", or "the audio thread (which paces playback)", not "the audio thread - which +paces playback -". A single trailing dash to tack on an example is fine. + We've been inconsistent with copyright notices, but for new files, have the year at 2012, and add the "This program is free software..." as in other files. `// Copyright (c) 2012- PPSSPP Project.` diff --git a/Core/HLE/sceMpegbase.cpp b/Core/HLE/sceMpegbase.cpp index 49b29c99b9..d07d3b0ab1 100644 --- a/Core/HLE/sceMpegbase.cpp +++ b/Core/HLE/sceMpegbase.cpp @@ -132,13 +132,12 @@ std::vector MpegBaseTakePESPacket(u32 dest) { // mpeg.prx hands the ME's decoded output to these to be converted to RGB. The descriptor it // passes is 48 bytes, which matches the range mpegbase.prx bounds-checks before using it. // -// mpeg.prx builds this on its own stack right before each call (1.3 at 08805698, 1.8 at 08805898 - -// the two are the same shape), from the eight buffer addresses sceVideocodec published plus the -// dimensions out of its own context. mpegbase.prx reads exactly these fields: the dimensions at -// 0x00/0x04 and all eight buffers at 0x10..0x2c. +// mpeg.prx builds this on its own stack before each call (1.3 at 08805698, 1.8 at 08805898, the +// same shape) from the eight buffer addresses sceVideocodec published and the dimensions from its +// own context. mpegbase.prx reads the dimensions at 0x00/0x04 and the eight buffers at 0x10..0x2c. // -// The buffers can be in either place: the Media Engine's memory for a frame the decoder just -// produced, or the game's, once sceMpegBaseYCrCbCopy has moved one out. +// The buffers live either in Media Engine memory (a freshly decoded frame) or in the game's +// memory (once sceMpegBaseYCrCbCopy has moved one out). struct SceMp4AvcCscStruct { s32_le height; // 0x00 in macroblocks s32_le width; // 0x04 @@ -335,17 +334,16 @@ static int MpegBaseCscRange(u32 bufferRGB, u32 cscAddr, int bufferWidth, } } NotifyMemInfo(MemBlockFlags::WRITE, bufferRGB, destSize, "MpegBaseCsc"); - // The CPU just wrote a video frame straight into what is usually a display buffer. The hardware - // backends don't see that on their own, so without telling them, the screen keeps showing the - // last frame the GE drew - the same notification our sceMpegAvcCsc HLE does. The pixel mode - // numbering matches GEBufferFormat, as it does there. + // The CPU just wrote a video frame into what is usually a display buffer. The hardware backends + // don't see that on their own, so without telling them the screen keeps showing the last frame + // the GE drew (the same notification our sceMpegAvcCsc HLE does). The pixel mode numbering + // matches GEBufferFormat, as it does there. gpu->PerformWriteFormattedFromMemory(bufferRGB, destSize, bufferWidth, (GEBufferFormat)g_mpegBasePixelMode); - // This runs on the DMACPLUS hardware and takes real time, and a caller running the real - // mpeg.prx leans on that. A psmfplayer game blits the current video frame every render frame - // while it waits for the next one to be ready, so returning instantly turns that into a tight - // loop that never yields and starves the audio thread - which is what paces playback - so the - // whole A/V pipeline deadlocks a few frames in. (SOCOM: Tactical Strike hangs exactly here.) - // Our sceMpeg HLE delays the equivalent sceMpegAvcCsc by the same amount for the same reason. + // This runs on the DMACPLUS and takes real time. A psmfplayer game blits the current video + // frame every render frame while it waits for the next, so an instant return is a tight loop + // that never yields and starves the audio thread that paces playback, and the A/V pipeline + // deadlocks a few frames in (SOCOM: Tactical Strike hangs exactly here). Our sceMpeg HLE delays + // sceMpegAvcCsc the same way. return hleDelayResult(hleLogDebug(Log::Mpeg, 0, "%dx%d at %d,%d -> %08x stride %d", rangeWidth, rangeHeight, rangeX, rangeY, bufferRGB, bufferWidth), "mpegbase csc", 4000); } @@ -378,8 +376,8 @@ static int sceMpegBaseCscAvc(u32 bufferRGB, u32 unknown, int bufferWidth, u32 cs if (!csc.IsValid()) { return hleLogError(Log::Mpeg, -1, "bad csc struct pointer"); } - // The whole frame. MpegBaseCscRange clamps the range to the real frame size, which it gets - // from the allocation rather than from the descriptor, so ask for more than any frame can be. + // The whole frame. MpegBaseCscRange clamps to the real frame size (from the allocation, not the + // descriptor), so pass more than any frame can be. return MpegBaseCscRange(bufferRGB, cscAddr, bufferWidth, 0, 0, 1024, 1024); } @@ -395,14 +393,13 @@ static u32 sceMpegBaseCscAvcRange(u32 bufferRGB, u32 unknown, u32 rangeAddr, int return MpegBaseCscRange(bufferRGB, cscAddr, bufferWidth, rangeX, rangeY, rangeWidth, rangeHeight); } -// Moves a decoded frame between two sets of buffers - both arguments are descriptors, and what -// gets copied is the pixels they point at, not the descriptors themselves. +// Moves a decoded frame between two sets of buffers. Both arguments are descriptors; what gets +// copied is the pixels they point at, not the descriptors. // -// mpegbase.prx builds a DMA list over the eight buffers (080010f8 in mpegbase_260.prx): bit 0 of -// the flags selects buffers 0, 1, 4 and 5, bit 1 selects 2, 3, 6 and 7, and the per-buffer sizes -// are the same ones sceVideocodec lays its frame out with. mpeg.prx always passes 3, i.e. all -// eight. The source is the ME's own frame; the destination is wherever the caller wants it, which -// for psmfplayer is a slot in its output pool. +// mpegbase.prx builds a DMA list over the eight buffers (080010f8 in mpegbase_260.prx): flag bit 0 +// selects buffers 0,1,4,5 and bit 1 selects 2,3,6,7, with the per-buffer sizes sceVideocodec lays +// its frame out with. mpeg.prx always passes 3 (all eight). The source is the ME's frame; the +// destination is wherever the caller wants it (for psmfplayer, a slot in its output pool). static int sceMpegBaseYCrCbCopy(u32 dstAddr, u32 srcAddr, int flags) { auto dst = PSPPointer::Create(dstAddr); auto src = PSPPointer::Create(srcAddr); @@ -410,10 +407,8 @@ static int sceMpegBaseYCrCbCopy(u32 dstAddr, u32 srcAddr, int flags) { return hleLogError(Log::Mpeg, -1, "bad descriptor pointer"); } - // Same story as the colour conversion: the frame size comes from the allocation the source - // buffers belong to, not from the descriptor's own dimension fields. - // mpeg.prx fills the destination's dimensions in macroblocks, the same way the colour - // conversion gets them; the source descriptor it builds on its stack states them in pixels. + // The destination descriptor carries the dimensions in macroblocks (as the colour conversion's + // does); the stack-built source descriptor states them in pixels, so read the destination. const int width = dst->width << 4; const int height = dst->height << 4; if (width <= 0 || height <= 0 || width > 1024 || height > 1024) { @@ -424,8 +419,8 @@ static int sceMpegBaseYCrCbCopy(u32 dstAddr, u32 srcAddr, int flags) { int copied = 0; for (int i = 0; i < 8; i++) { - // Buffers 0,1,4,5 go with bit 0 and 2,3,6,7 with bit 1 - the even/odd row halves of luma - // and of chroma respectively. + // Buffers 0,1,4,5 go with bit 0 and 2,3,6,7 with bit 1 (the even and odd row halves of luma + // and chroma). const int bit = ((i & 3) < 2) ? 1 : 2; if (!(flags & bit) || sizes[i] <= 0) { continue; diff --git a/Core/HLE/sceVideocodec.cpp b/Core/HLE/sceVideocodec.cpp index bc403918a5..faf1e70661 100644 --- a/Core/HLE/sceVideocodec.cpp +++ b/Core/HLE/sceVideocodec.cpp @@ -227,10 +227,10 @@ u32 VideocodecFrameBufferLayout(int width, int height, int sizes[8], u32 offsets // buffer0/2 take the odd band out when the width isn't a multiple of 32. const int lumaLeft = ((width + 16) >> 5) * (height >> 1) * 16; const int lumaRight = (width >> 5) * (height >> 1) * 16; - // Chroma is paired the same way luma is - left/right of a band, then even/odd rows - which is - // what sceMpegBaseYCrCbCopy's flags assume: bit 0 selects buffers 0,1,4,5 (the even rows) and - // bit 1 selects 2,3,6,7. Sizes have to line up with that, or a copy writes the wrong count - // into a buffer someone else sized. + // Chroma is paired like luma (left/right of a band, then even/odd rows), which is what + // sceMpegBaseYCrCbCopy's flags assume: bit 0 selects buffers 0,1,4,5 and bit 1 selects 2,3,6,7. + // The sizes have to match that, or a copy writes the wrong count into a buffer someone else + // sized. const int local[8] = { lumaLeft, lumaRight, lumaLeft, lumaRight, lumaLeft >> 1, lumaRight >> 1, lumaLeft >> 1, lumaRight >> 1, @@ -248,9 +248,8 @@ u32 VideocodecFrameBufferLayout(int width, int height, int sizes[8], u32 offsets return total; } -// The descriptor mpeg.prx passes in is empty: on hardware the ME owns the frame buffers, and -// reports where it put them. So allocate them here and fill the descriptor in the shape -// sceMpegBaseCscAvc expects - dimensions in macroblocks, then the eight buffer addresses. +// The descriptor mpeg.prx passes in is empty: on hardware the ME owns the frame buffers and +// reports where it put them. So allocate them here and fill in the eight buffer addresses. static bool PublishFrameBuffers(VideocodecCtx &vctx, u32 structAddr, int width, int height, u32 buffers[8]) { u32 offsets[8]; const u32 total = VideocodecFrameBufferLayout(width, height, nullptr, offsets); @@ -282,11 +281,10 @@ static bool PublishFrameBuffers(VideocodecCtx &vctx, u32 structAddr, int width, buffers[i] = vctx.frameBuffers + offsets[i]; } - // Eight addresses and nothing else. mpeg.prx reads them straight off the front of this - // structure - `lw` at 0x00..0x1C, verified in both 1.3 (Daxter's disc copy, at 08805698) and - // 1.8 (flash0:/kd/mpeg.prx, at 08805898) - and takes the frame's dimensions from its own - // context rather than from here. Writing anything else at the front lands in buffer slots 0 - // and 1, which sceMpegBaseYCrCbCopy then DMAs to. + // mpeg.prx reads eight buffer addresses off the front of this structure (`lw` at 0x00..0x1C, + // verified in 1.3 at 08805698 and 1.8 at 08805898) and takes the frame dimensions from its own + // context. So write only the addresses; anything else at the front lands in slots 0 and 1, + // which sceMpegBaseYCrCbCopy then DMAs to. if (!Memory::IsValidRange(structAddr, 8 * 4)) { return false; } diff --git a/Core/HLE/sceVideocodec.h b/Core/HLE/sceVideocodec.h index d6cbb1f603..d46b1bcce1 100644 --- a/Core/HLE/sceVideocodec.h +++ b/Core/HLE/sceVideocodec.h @@ -53,8 +53,8 @@ void VideocodecGetCtxInfo(std::vector *infos); u8 *VideocodecMEPointer(u32 addr, u32 size); // The eight buffers of the frame starting at `firstBuffer`, and its size. Returns false if that -// isn't the start of an allocation we handed out - which is the normal answer once -// sceMpegBaseYCrCbCopy has moved a frame into the game's own memory. +// isn't the start of an allocation we handed out (the normal answer once sceMpegBaseYCrCbCopy has +// moved a frame into the game's memory). bool VideocodecGetFrameBuffers(u32 firstBuffer, u32 buffers[8], int *width = nullptr, int *height = nullptr); // How the eight buffers a frame is delivered in are sized and laid out, in the order the diff --git a/Core/Util/BlockAllocator.cpp b/Core/Util/BlockAllocator.cpp index f0aa16bf80..e96d138315 100644 --- a/Core/Util/BlockAllocator.cpp +++ b/Core/Util/BlockAllocator.cpp @@ -451,9 +451,8 @@ void BlockAllocator::DoState(PointerWrap &p) const bool compact = s >= 2; int count = 0; - // An allocator that was never Init'd (or has been Shutdown) has no blocks at all. That's a - // perfectly good state to save - it's what one that nothing has asked for memory from yet - // looks like - so zero blocks is a normal count here, not a corrupt one. + // An allocator that was never Init'd (or has been Shutdown) has no blocks, which is a valid + // state to save, so a zero block count here is normal rather than corrupt. if (p.mode == p.MODE_READ) { Shutdown();