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();