fix(theme): give AttributeImage a persistent cacheId -- the uncut #120 info-page read storm (#154)#206
Merged
Merged
Conversation
…info-page read storm AndrewBento (HW, #154) bisected this for us: with a theme that renders screenshots on the MAIN page he hammered art back and forth indefinitely and never wedged; entering/exiting the PS1 game INFO page kills artwork for PS2 AND PS1 alike. "The problem isn't the screenshots or the image loading, but the game information page itself." He is right -- and the bisect does not exonerate the art pipeline, it indicts ONE CALLER. ROOT CAUSE: cacheGetTexture's negative (FAILED) memo needs BOTH out-params to persist -- a failed load writes *cacheId = -2 and *UID = gCacheGeneration, and the skip gate is (*cacheId == -2 && *UID == gCacheGeneration). drawAttributeImage passed a STACK LOCAL (`int posZ = 0;`) for the cacheId, re-zeroed every frame, so the -2 was thrown away, the gate was structurally unreachable, and EVERY FRAME re-enqueued a fresh FAILING device open for as long as the info page rendered. AttributeImage was the only art consumer in the tree that did this: every other caller (getGameImageTexture and friends) passes a persistent field/array -- which is exactly why Andrew's main-page screenshots cost ~1 open per generation and survived forever, while the info page stormed. The storm is info-page-exclusive by layout, not by engine: conf_theme_OPL.cfg puts all seven AttributeImage elements on info7..info13 and none on main. It only fires on a DISK theme (the cache branch is gated on thmGetGuiValue() != 0; the built-in theme short-circuits to embedded glyphs with zero card reads), and only for badges whose glyph the theme does not ship -- which the embedded-glyph fallback right above it exists precisely because it is the NORMAL case. This IS the "BASELINE info-entry read burst that desyncs the card in the first place" that 86da202 admitted it never cut. No #120 attempt ever went near it -- all of them sanded ELEM_TYPE_GAME_IMAGE or the VCD cover loader, i.e. the paths that ALREADY had a working memo. That is why every beta made the wedge "take longer to happen" and none of them fixed it. FIX (mirrors the proven GameImage pattern): mutable_image_t gains currentCacheId, initialised to -1 and passed to cacheGetTexture instead of the stack local. A missing glyph now costs exactly ONE open per generation instead of one per frame. It also resets in the force-refresh branch -- MANDATORY, not tidiness: the -2 gate short-circuits without comparing the value string, so a -2 left over from the previous game would suppress the new badge for the rest of the generation. SCOPE (honest): this is the TRIGGER, verified line-by-line. The PERSISTENCE (all art dead afterwards) has no code-side explanation -- no global art-kill flag exists and the request accounting balances; it rests on the fork's own HW-recorded conclusion (01aa6d3) that the card desyncs CARD-SIDE under the read load. So this is a PREVENTION fix: a card that has already desynced in-session needs a power cycle. HW must confirm before #154/#120 is called closed. Also refuted, and worth recording: the TerminateThread/RPC-corruption theory is dead on reachability. cacheEnd()'s only caller (opl.c) is gated to Exit / Power Off / Boot Disc, and gDiag.artTerminate (TK) increments inside that same branch -- so TK is MATHEMATICALLY 0 for any session the user is still browsing in. Every "TK:0" HW reading was vacuous. Info exit issues no abort at all (guiSwitchScreen only calls cacheAdvanceGeneration, whose invalidate runs with preserveLoaded=1 and never sets abortRequested). Struct changed -> validated with a full `make clean` rebuild (the Makefile does not track header deps). Builds clean, exit 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (15)
🧰 Additional context used🪛 Cppcheck (2.21.0)src/themes.c[warning] 676-676: If memory allocation fails, then there is a possible null pointer dereference (nullPointerOutOfMemory) 🔇 Additional comments (2)
📝 WalkthroughWalkthroughAdds persistent cache identity to mutable attribute images, initializes and resets it with image state, and uses it during texture lookup to retain cache results across frames. ChangesAttribute image cache state
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bisect that cracked it (AndrewBento, HW, #154)
He's right — and the bisect doesn't exonerate the art pipeline, it indicts one caller.
Root cause
cacheGetTexture's negative (FAILED) memo requires both out-params to persist. A failed load writes*cacheId = -2; *UID = gCacheGeneration;and the skip gate is(*cacheId == -2 && *UID == gCacheGeneration).So the
-2was thrown away every frame, the skip gate was structurally unreachable, and every frame re-enqueued a fresh failing device open for as long as the info page rendered.AttributeImagewas the only art consumer in the tree doing this — which is exactly why Andrew's main-page screenshots cost ~1 open per generation and survived indefinitely.Info-page-exclusive by layout, not by engine:
conf_theme_OPL.cfgputs all seven AttributeImage elements oninfo7..info13, none onmain. It only fires on a disk theme (the cache branch is gated onthmGetGuiValue() != 0; the built-in theme short-circuits to embedded glyphs with zero card reads), and only for badges whose glyph the theme doesn't ship — which the embedded-glyph fallback right above it exists for because that's the normal case.This is the "BASELINE info-entry read burst that desyncs the card in the first place" that 86da202 admitted it never cut. No #120 attempt ever went near it — all of them sanded
ELEM_TYPE_GAME_IMAGEor the VCD cover loader, i.e. the paths that already had a working memo. That's why every beta made it "take longer to happen" and none fixed it.Fix (4 lines, mirrors the proven GameImage pattern)
mutable_image_tgainscurrentCacheId, init-1, passed instead of the stack local. A missing glyph now costs one open per generation instead of one per frame.It also resets in the force-refresh branch — mandatory, not tidiness: the
-2gate short-circuits without comparing the value string, so a leftover-2from the previous game would suppress the new badge for the rest of the generation.Scope — honest
This is the trigger, verified line-by-line. The persistence (all art dead afterward) has no code-side explanation — no global art-kill flag exists, request accounting balances. It rests on the fork's own HW conclusion (01aa6d3) that the card desyncs card-side under read load. So this is a prevention fix; an already-desynced card needs a power cycle. HW must confirm before #154/#120 is called closed.
Also refuted (worth recording)
The TerminateThread/RPC-corruption theory is dead on reachability.
cacheEnd()'s only caller is gated to Exit / Power Off / Boot Disc, andgDiag.artTerminate(TK) increments inside that same branch — so TK is mathematically 0 for any session you're still browsing in. Every "TK:0" HW reading was vacuous. Info exit issues no abort at all (guiSwitchScreenonly callscacheAdvanceGeneration, whose invalidate runspreserveLoaded=1and never setsabortRequested).Validation
Struct changed → validated with a full
make cleanrebuild (the Makefile doesn't track header deps). Builds clean, exit 0.🤖 Generated with Claude Code
Summary by CodeRabbit