Fix save-state, artwork, and identity regressions from review of 680e077#7
Merged
Conversation
Snapshot validation rejected cur_frame > total_frames unconditionally, which made every state saved from a track whose decoder reports an unknown length (total_frames == 0: FLAC without a STREAMINFO total, OGG length-probe failure) unloadable. Restore additionally rejected resume_frame > total_frames, refusing states saved after playback ran past an under-reported track length (VBR MP3 estimates) where the resample cache legitimately overshoots the reported total. Treat total_frames == 0 as unknown and skip the positional bound, and drop the resume-frame bound (cur_frame is already bounded for known lengths, and the cache overshoot is real decoder state). retro_unserialize also assumed any audio_restore_state failure had disturbed the decoder, so a state that merely failed validation could cascade into audio_close() on a healthy, still-playing session when the rollback snapshot failed the same validation. Expose the validation as audio_snapshot_valid() and reject bad snapshots up front, before the live decoder is touched; rollback teardown now only happens when a restore attempt actually disturbed playback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion STBI_WINDOWS_UTF8 made stb_image convert art paths as strict UTF-8 with no fallback to plain fopen, so sidecar artwork silently stopped loading for frontends that pass ANSI/locally-encoded paths on Windows -- while the audio side of the same path still opened via path_fopen_read, which deliberately keeps that fallback. Open art files through path_fopen_read and decode with stbi_load_from_file so artwork follows the same policy. The stbi_info pre-validation pass duplicated the STBI_MAX_DIMENSIONS limit compiled into stb_image and header-parsed every image twice (per signature hit during the embedded-art scan). Drop it, and derive STBI_MAX_DIMENSIONS from ART_MAX_DIMENSION (now in metadata.h) so the limit lives in one place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The convert-UTF-8-to-wide, try-the-_w-API, fall-back-to-narrow boilerplate was copy-pasted four times across audio.c and metadata.c, so a change to the fallback policy had to be replicated in every codec open. Replace the copies with a single PATH_OPEN_WIDE_THEN_NARROW helper next to path_fopen_read (which now uses it too). The bounded find-last-separator, clamp, copy, terminate directory split was likewise duplicated between retro_load_game and metadata_load; both now share path_dirname/path_basename. The shared helpers treat the later of '/' and '\' as the separator, which also fixes splitting of mixed-separator paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The audio/art/playlist teardown sequence existed in three copies across retro_load_game and retro_unload_game, and the copies had drifted: the all-tracks-failed path skipped reset_runtime_state, leaving the shuffle order generated for the failed playlist behind with track_count at 0. Factor a single unload_session() used by retro_unload_game and every retro_load_game bail-out. The 8-pixel glyph width lived as a bare literal in four text-measuring sites (plus the font renderer itself) and the 15-frame seek-icon timer in three; name them GLYPH_WIDTH and SEEK_ICON_FRAMES so a font or timer change cannot silently desync the copies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…angling Share CoreStateSnapshot with the harness through a new core_state.h instead of hand-mirroring the struct (and CORE_MAX_TRACKS) in core_harness.c, where any future field edit would silently move the malformed-save-state corruption offsets while CI stayed green. This matches how AudioStateSnapshot is already shared via audio.h. Convert the fixtures-dir argument from the ANSI code page to UTF-8 on Windows: the core treats paths as UTF-8, so a machine whose temp path contains non-ASCII characters produced mixed-encoding track paths and a spurious suite failure (reproduced from a directory containing a u-umlaut before the fix; passes after). Parse CFLAGS with escape processing disabled: posix-mode shlex.split was eating backslashes, turning -IC:\dev\extra into -IC:devextra and silently dropping the documented instrumentation hook on Windows. Make core_identity_matches_info read display_name/display_version from music_playlist_libretro.info instead of re-asserting hardcoded string literals, so the code/.info sync it is named for is actually enforced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CONTRIBUTING.md and README.md already describe the CFLAGS instrumentation hook, but CLAUDE.md was left out, violating the stated rule that build/test convention changes update both docs. Also list core_state.h and the path-splitting helpers added to path_io.h in the source layout table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Frontends key per-core option files, remaps, and save-state folders off library_name, so this morning's rename to Music Playlist Core / 1.0 orphaned existing users' settings and made their save states invisible, with no migration. Restore the identity users' data is keyed to. The .info display fields (which had said Music Playlist Core since the file was added, mismatching the core) follow the code instead, keeping core_identity_matches_info green and the identity coherent everywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Preserve true playback positions past unreliable length metadata, replay unknown-length decoders to exact save-state positions, and reject failed seeks without falsifying runtime state. Add FLAC/Vorbis continuity fixtures and close the CFLAGS, strict-build, and identity-test review findings.
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.
Fixes all 15 findings from a deep review of 680e077 ("Harden media loading and state restoration"), independently confirmed by a second verification pass, plus the five follow-up findings from review of this PR. The original concerns remain split by commit, with one final hardening commit for exact decoder restoration and tooling regressions.
User-visible regressions fixed
audio.c): playback now preserves the true source cursor beyond unreliable length metadata. Restore uses checked native seeks when possible and replays source PCM from the freshly opened decoder when FLAC/Vorbis seek tables cannot represent the saved position.core.c):retro_unserializevalidates the incoming audio snapshot before touching the live decoder, so pure validation failures can no longer cascade intoaudio_close()on a playing session.image_codecs.c/metadata.c): art now opens throughpath_fopen_read(wide attempt, narrow fallback), matching audio path handling.UltiMedia UGC/17.0: frontends key per-core settings, remaps, and save-state folders offlibrary_name, so the rename toMusic Playlist Coreorphaned existing users' data with no migration. The.infodisplay fields now follow the code.Tooling fixes
CoreStateSnapshotis shared with the harness viasrc/core_state.hinstead of a hand-mirrored struct whose corruption offsets could silently drift.CFLAGSparsing preserves raw Windows paths such as-IC:\dev\extrawithout breaking normal escaped quotes, escaped spaces, or#characters.static inline, so-Wall -Wextra -Werrorbuilds no longer fail on unused definitions.music_playlist_libretro.infoconsistency and pins the stableUltiMedia UGClibrary name.Cleanups
unload_session()teardown shared byretro_unload_gameand everyretro_load_gamebail-out.path_io.h, including mixed-separator handling.stbi_infopre-validation removed;ART_MAX_DIMENSIONdrivesSTBI_MAX_DIMENSIONS.GLYPH_WIDTH,SEEK_ICON_FRAMES.CLAUDE.md,README.md, andCONTRIBUTING.mddocument the harness knobs and small codec regression seeds.Verification
python3 tests/run_tests.py: 23/23 passed.CFLAGS="-Wall -Wextra -Werror" python3 tests/run_tests.py: 23/23 passed.tests/SMOKE_CHECKLIST.mdpass is still outstanding.🤖 Generated with Claude Code