fix(media): align browse container resolution with media.meta and bound its batch - #1383
Open
wizzomafizzo wants to merge 2 commits into
Open
fix(media): align browse container resolution with media.meta and bound its batch#1383wizzomafizzo wants to merge 2 commits into
wizzomafizzo wants to merge 2 commits into
Conversation
ResolveSingletonContainerAliases built one ParentDir IN list for every candidate directory on a browse page, then passed every resolved media ID to GetMediaWithTitleAndSystemByIDs in a single call. A browse page size is client-supplied, so on a page of ~1000 container directories that call exceeded the 30s request budget: the whole resolution was abandoned and the page came back with every directory unpromoted after a 30 second wait. The ParentDir scan now runs in aliasCandidatesPerQuery-sized chunks, and the two shared by-ID lookups chunk at source rather than at the call site, since GetMediaWithTitleAndSystemByIDs is also reached from the media.meta batch path and GetMediaTagsByMediaDBIDs from four more callers, all with caller-sized ID lists. Each loop checks the context between chunks so a cancelled request stops rather than running the rest. Candidates and IDs are deduplicated before chunking. One IN list returns a row once however many times its key appears, but a key landing in two chunks would read its rows twice, and the alias scan appends per ParentDir: a repeated directory would look like it held twice the files and stop collapsing. Measured on MiSTer against 1000 one-cue directories: maxResults before after 100 0.83s, 100 promoted 0.80s, 100 500 1.36s, 500 promoted 1.62s, 500 999 30.1s, 0 promoted 2.61s, 999 1000 30.1s, 0 promoted 2.44s, 1000 Closes #1377
isSingletonDirectoryAliasCandidate dropped any directory whose recursive FileCount exceeded 64 before browse asked whether it collapsed. FindSingleContainerLaunchMedia, behind media.meta and media.image, and container.Index, behind the scrapers, apply no such cap, so the three disagreed about what counts as a container. A folder holding one .cue and 70 .bin tracks browsed as a plain, coverless, non-launchable directory while media.meta resolved it to the cue, the media-folder scraper attached folder-named artwork to that row, and a <folder> entry in gamelist.xml wrote metadata to it. Real containers were excluded outright: a disc folder over 64 tracks, or an .m3u set spanning enough discs. The cap was a batch-size guard, as its own comment said, sitting in front of the semantics instead of in front of the batch. Its stated fear was misplaced twice over: the batch query reads only ParentDir = <dir>/ rows, so a tree of subdirectories returns nothing and costs one index seek, and the example it named, MiSTer's _Arcade/_alternatives, holds media for a dozen systems and is already excluded by the single-system check above it. Bounding the batch is now that query's own job. Every directory holding media for the single system in scope is a candidate, and the container rule alone decides what collapses. docs/api/methods.md and docs/scraper.md already described the rule without a size limit. Verified on MiSTer against a purpose-built corpus, browse against media.meta on the same paths. 64 files promoted and 65 did not before the change; both promote after, along with the 71-file cue set and a 101-file m3u set, while an ambiguous two-cue folder and one holding media in a subdirectory stay plain directories. The promoted 71-file folder reports hasCover for the artwork the media-folder scraper wrote under its name. Removing the cap makes larger directories candidates, which costs about 0.2ms per direct row on pages that hold big flat ROM folders. On MiSTer's stock library that is browse of /media/fat/games/MegaDrive going from 82ms to 225ms over 745 rows, and /media/fat/games/NES from 179ms to 316ms over 584 rows. Closes #1378
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change chunks media ID, tag, and singleton alias queries. It removes browse’s 64-file candidate limit while excluding empty directories. Tests cover chunk boundaries, duplicate candidates, cancellation, and promotion of a 71-file disc folder. ChangesMedia alias resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to Large directories can now be promoted consistently, but a browse request may scan and retain all direct media rows for each candidate, consuming significant database and browse capacity on unusually large libraries. Existing chunking, cancellation, and concurrency limits reduce the exposure; the PR is mergeable with explicit owner awareness of adding an aggregate work limit. Sequence Diagram(s)sequenceDiagram
participant BrowseAPI
participant ResolveSingletonContainerAliases
participant MediaDB
BrowseAPI->>ResolveSingletonContainerAliases: submit candidate directories
ResolveSingletonContainerAliases->>MediaDB: scan ParentDir values in chunks
MediaDB-->>ResolveSingletonContainerAliases: return direct media rows
ResolveSingletonContainerAliases->>MediaDB: fetch media metadata in ID chunks
MediaDB-->>ResolveSingletonContainerAliases: return titles and systems
ResolveSingletonContainerAliases-->>BrowseAPI: return resolved aliases
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy both linked issues. [ Full details: Out of Scope Changes checkExplanation The implementation and tests remain within the linked objectives. The added helpers, counters, test stubs, and regression coverage directly support bounded alias resolution and consistent large-container handling.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Summary
pkg/database/containerexists so one rule decides when a directory of indexed media collapses to a single launch target. Browse did not use that rule as written:isSingletonDirectoryAliasCandidatedropped any directory whose recursiveFileCountexceeded 64 before the resolver was asked.FindSingleContainerLaunchMedia, behindmedia.metaandmedia.image, andcontainer.Index, behind the scrapers, apply no size limit, so the three disagreed about the same folder.docs/api/methods.mdanddocs/scraper.mdalready described the rule without a limit, so the code was the outlier and neither needed an edit.Removing the cap makes every directory on a page a candidate, which is the input that blows up #1377, so the batch is bounded first.
ResolveSingletonContainerAliasesruns itsParentDir INscan inaliasCandidatesPerQuery-sized chunks.GetMediaWithTitleAndSystemByIDsandGetMediaTagsByMediaDBIDschunk at source rather than at the call site: the first is also reached from themedia.metabatch path, the second from four more callers, all with caller-sized ID lists. Each loop checks the context between chunks so a cancelled request stops instead of running the rest.INlist returns a row once however many times its key appears, but a key landing in two chunks reads its rows twice, and the alias scan appends perParentDir— a repeated directory would look like it held twice the files and stop collapsing.isSingletonDirectoryAliasCandidateis gone. A directory qualifies onFileCount > 0, and the container rule alone decides what collapses.inScanChunks.The cap's own comment called it a batch-size guard. Its stated fear was misplaced twice over: the batch query reads only
ParentDir = <dir>/rows, so a tree of subdirectories returns nothing and costs one index seek, and the example it named — MiSTer's_Arcade/_alternatives— holds media for a dozen systems and is already excluded by the single-system check above it. The two tests that encoded the cap are retargeted rather than deleted: one now asserts the multi-system guard that genuinely protects that directory, the other that a large directory is offered to the resolver and stays plain because no alias comes back. Three unrelated browse tests held fixtures that now reach the resolver and gained alias stubs.Closes #1377
Closes #1378
Verified
task test,task lint,task cross-lint:all.Both fixes were reproduced and confirmed on the MiSTer test device, A/B against
origin/mainwith the same corpus under both binaries.media.browseof the parent againstmedia.metaon the folder path:.cue+ 3.bin.cue+ 63.bin.cue+ 64.bin.cue+ 70.bin.m3u+ 100.chd.cue+ 68.bin64 promoting and 65 not is the cap exactly. The
media-folderscraper attachedmedia/boxart/Y_ManyBins.pngto the 71-file folder's cue, and the promoted browse entry reportshasCoverfor it — only that folder.media.browseof 1000 one-cue directories:Cost
Larger directories are now candidates, at roughly 0.2ms per direct row, spent proving directories are not containers. On the device's stock library:
/media/fat/games/MegaDrive/media/fat/games/NESThis is the price of one shared definition. A guard would buy back a tenth of a second on pages of large flat ROM folders and reintroduce the disagreement.
Summary by CodeRabbit