diff options
Diffstat (limited to 'packages')
4 files changed, 345 insertions, 29 deletions
diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index 913ad68..ae4e1f0 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -1168,11 +1168,18 @@ class NodeDaemon: if not file_path or not file_path.exists(): continue self._enriched_attempted.add(entry.id) + # The entry's own named root, so the ancestor walk + # (enrich_audio._artist_album_from_ancestors) can tell "a + # top-level folder under this root" from "one level deeper" — + # without this boundary it used to read the root's own + # directory name as an artist for every flat top-level folder. + split = indexer.roots.split(entry.path) + root_path = split[0].path if split else None async def on_done(file_id: str, fields: dict, _indexer=indexer) -> None: await self._on_enriched(_indexer, file_id, fields) - self._audio_enricher.spawn(entry, file_path, on_done) + self._audio_enricher.spawn(entry, file_path, on_done, root_path) async def _enrich_music_now(self, group_id: str) -> None: """ diff --git a/packages/meshbay-node/src/meshbay_node/indexer/enrich_audio.py b/packages/meshbay-node/src/meshbay_node/indexer/enrich_audio.py index 17a58d6..9ecf1b9 100644 --- a/packages/meshbay-node/src/meshbay_node/indexer/enrich_audio.py +++ b/packages/meshbay-node/src/meshbay_node/indexer/enrich_audio.py @@ -19,6 +19,30 @@ flat view (docs/musicbay.md §5.2) needs nothing more than this. MusicBrainz is a separate, lazy, per-request enrichment (`music_meta_req`, handled in webrtc_server.py), the same "fetched on demand, cached once" shape TMDB already uses. + +**Revised 2026-08-24** against a real ~5700-file library (folder-per-artist +mostly, but not uniformly — see musicbay.md's own "what got measured" note +if one gets added). Two findings drove this revision, both confirmed with +real data before writing the fix: + +1. The original ancestor walk always went up two levels (parent = album, + grandparent = artist) with no idea where the group's shared root itself + was. For any file sitting in a *top-level* folder — common here: bare + `Artist/track.mp3`, no album layer at all — the "grandparent" it found + was the root directory's own name, so every such artist got relabelled + as an album *of* a fake band named after the share. Measured: 289 of + 5664 tracks (41 real, unrelated artists) collapsed into one bucket this + way — the single biggest "artist" in the whole library, by a wide + margin, which is exactly the kind of thing a person notices first and + loses trust in. Fixed by passing the root's own path down so the walk + refuses to use it as a name (`_artist_album_from_ancestors` below). +2. A tag being *present* is not the same as it being *meaningful*: some + taggers write a literal placeholder ("No Artist", a French tool's + "Nouvel artiste (334)") instead of leaving the field empty, and that + placeholder is just as truthy as a real name — it was winning over the + filename/folder fallback that would have done better. `_clean_tag` + below turns a known placeholder back into "absent" before anything else + sees it. """ import asyncio @@ -44,6 +68,25 @@ READ_TIMEOUT_SECS = 10 _TRACK_NO_RE = re.compile(r"\d+") +# Values some taggers write instead of leaving a field empty — treated as +# "absent" so the fallback chain gets a chance instead of locking in a +# placeholder. Not exhaustive by construction; each entry here was seen in +# a real file, not guessed. "Various Artists" is deliberately *not* here — +# that one is a real, meaningful compilation credit worth keeping as its +# own bucket, not a placeholder. +_PLACEHOLDER_RE = re.compile( + r"^(no\s*artist|unknown(\s+artist)?|nouvel(le)?\s+artiste\s*\(\d+\)|" + r"nouveau\s+titre\s*\(\d+\)|track\s*\d*|<unknown>)$", + re.IGNORECASE, +) + + +def _clean_tag(value: str | None) -> str | None: + if not value: + return None + v = value.strip() + return v if v and not _PLACEHOLDER_RE.match(v) else None + def _extract_cover(mf) -> bytes | None: """ @@ -68,13 +111,44 @@ def _extract_cover(mf) -> bytes | None: return None +# Loose image files sitting beside the tracks — very common for exactly the +# era of rip this library mostly is (Windows Media Player's own +# `Folder.jpg`/`AlbumArt_{guid}_(Large|Small).jpg`, or a manually dropped +# `cover`/`front.jpg`). Measured: 267 such images across one real library, +# none of them ever considered before this revision — embedded-art-only +# missed them entirely. Ranked so a deliberately-named cover file wins over +# WMP's cache thumbnail when both exist in the same folder. +_COVER_STEM_RANK = ("cover", "folder", "front", "albumart") +_COVER_EXTS = (".jpg", ".jpeg", ".png") + + +def _find_sibling_cover(folder: Path) -> Path | None: + try: + candidates = [p for p in folder.iterdir() + if p.is_file() and p.suffix.lower() in _COVER_EXTS] + except OSError: + return None + if not candidates: + return None + + def rank(p: Path) -> int: + stem = p.stem.lower() + for i, name in enumerate(_COVER_STEM_RANK): + if name in stem: + return i + return len(_COVER_STEM_RANK) + candidates.sort(key=lambda p: (rank(p), p.name)) + return candidates[0] + + def _read_tags_and_cover(path: Path) -> tuple[dict, float | None, bytes | None]: """ Synchronous — always called via asyncio.to_thread. Returns a partial - `tags` dict (only keys actually found: title/artist/album/track_no), - duration in seconds (None if unreadable), and raw cover bytes (None if - absent). Never raises for an unreadable/corrupt file — the caller falls - back to filename parsing entirely in that case. + `tags` dict (only keys actually found and not a known placeholder: + title/artist/album/track_no), duration in seconds (None if unreadable), + and raw cover bytes (None if absent, embedded and sibling-file both + checked). Never raises for an unreadable/corrupt file — the caller + falls back to filename parsing entirely in that case. """ tags: dict = {} duration: float | None = None @@ -87,13 +161,19 @@ def _read_tags_and_cover(path: Path) -> tuple[dict, float | None, bytes | None]: duration = getattr(easy.info, "length", None) for field in ("title", "artist", "album"): values = easy.get(field) - if values and str(values[0]).strip(): - tags[field] = str(values[0]).strip() + cleaned = _clean_tag(str(values[0])) if values else None + if cleaned: + tags[field] = cleaned track_raw = easy.get("tracknumber") if track_raw: m = _TRACK_NO_RE.match(str(track_raw[0])) if m: tags["track_no"] = int(m.group()) + if "title" in tags: + # Some taggers copy the bare filename into `title` verbatim, track + # number included — a tag normally wins over the filename parse, so + # that pollution would otherwise beat a cleaner one (docstring above). + tags["title"] = title_parse.strip_track_prefix(tags["title"]) or tags["title"] cover: bytes | None = None try: @@ -105,27 +185,94 @@ def _read_tags_and_cover(path: Path) -> tuple[dict, float | None, bytes | None]: cover = _extract_cover(raw) except Exception: cover = None + if cover is None: + sibling = _find_sibling_cover(path.parent) + if sibling is not None: + try: + cover = sibling.read_bytes() + except OSError: + cover = None return tags, duration, cover -def _artist_album_from_ancestors(file_path: Path) -> tuple[str | None, str | None]: +# A folder name used as a last-resort artist/album, cleaned of the +# punctuation-as-separator and release-tag noise this era of rip is full of +# ("L_Oeuf_Raide_-_Berlin_Eggsile", "Sinsemilia - Premiere Recolte [MP3 +# 320kbps Album]"). Same spirit as title_parse.naive_title for video, kept +# separate because the junk vocabulary differs (bitrates and rip tags, not +# edition/language tags). +# A whole bracketed/parenthesized group is dropped if it contains any rip-tag +# word ("[MP3 320kbps Album]", "(VBR HQ mp3s)") — matching one keyword at a +# time left the rest of a multi-word group behind ("Album]" survived a first +# version of this that only recognized "full album" as one fixed phrase). +# Bare tokens outside brackets are stripped on their own. +_RIP_TAG_GROUP_RE = re.compile( + r"[\[\(][^\[\]\(\)]*\b(vbr|cbr|flac|mp3s?|kbps|hq|album)\b[^\[\]\(\)]*[\]\)]", + re.IGNORECASE, +) +_RIP_TAG_BARE_RE = re.compile(r"\b(vbr|cbr|flac|hq)\b", re.IGNORECASE) + + +def _clean_folder_name(name: str) -> str: + cleaned = re.sub(r"[._]+", " ", name) + cleaned = _RIP_TAG_GROUP_RE.sub(" ", cleaned) + cleaned = _RIP_TAG_BARE_RE.sub(" ", cleaned) + cleaned = re.sub(r"\s+", " ", cleaned).strip(" -") + return cleaned or name + + +def _split_top_level_folder(name: str) -> tuple[str, str | None]: """ - `Artist/Album/track.mp3` is the common shape (docs/musicbay.md §2.1) — - used only to fill whatever the tags left empty. No attempt to validate - against the group's actual roots (enrich.py's ancestor walks don't - either): a flat `Artist/track.mp3` layout, or a various-artists - compilation folder, just yields a plausible-but-not-guaranteed album - name from the immediate parent and nothing further up — good enough for - a fallback, not asserted as accurate. + The one ancestor level left once a file's folder turns out to sit + directly under the group's root (§ below) — there is no further + ancestor to call "artist" without leaving the root entirely. The common + convention for a single-release folder at that level is + "Artist - Album ...junk..." ("GHOST DOG - Soundtrack", + "cypress_hill_-los_grandes__xitos_en_espa_ol"); split on the first + " - " when the cleaned name has one. Otherwise the whole (cleaned) name + becomes the artist alone, which is the *more* common real shape here — + a flat per-artist folder with no album subfolder at all ("Ben Harper", + "bob_marley", "Renaud"). """ - album_folder = file_path.parent - if album_folder == album_folder.parent: + cleaned = _clean_folder_name(name) + m = re.match(r"^(.{2,60}?)\s*-\s*(.{2,80})$", cleaned) + if m: + return m.group(1).strip(), m.group(2).strip() + return cleaned, None + + +def _artist_album_from_ancestors( + file_path: Path, root_path: Path | None, +) -> tuple[str | None, str | None]: + """ + `Artist/Album/track.mp3` is one real shape in this library, but not the + only one — measured directly against it (module docstring). This walk + refuses to climb past `root_path` (the group's own shared directory): + doing so used to read the root's own name as "the artist", which is + never true and was the single largest source of bad groupings found. + + Three cases, by how many folders separate the file from the root: + 0 (file sits directly in the root) — no folder context at all. + 1 (a top-level folder) — ambiguous by construction; see + `_split_top_level_folder`. + 2+ — the classic Artist/Album layout: immediate parent is the album, + its parent is the artist. + + `root_path` is None when the caller couldn't resolve which named root + this entry belongs to (should not happen in practice — `entry.path` + always names one — but the walk still terminates safely at the + filesystem root rather than looping, same as before this revision). + """ + leaf = file_path.parent + if root_path is not None and leaf == root_path: + return None, None + parent = leaf.parent + if root_path is not None and parent == root_path: + return _split_top_level_folder(leaf.name) + if leaf == parent: # filesystem root reached without ever matching root_path return None, None - artist_folder = album_folder.parent - album = album_folder.name or None - artist = artist_folder.name if artist_folder != artist_folder.parent else None - return artist, album + return _clean_folder_name(parent.name), _clean_folder_name(leaf.name) class AudioEnricher: @@ -139,6 +286,7 @@ class AudioEnricher: def spawn( self, entry: IndexEntry, file_path: Path, on_done: Callable[[str, dict], Awaitable[None]], + root_path: Path | None = None, ) -> asyncio.Task: """ Fire-and-forget one file's enrichment — same contract as @@ -146,9 +294,11 @@ class AudioEnricher: the index fields to merge once ready, never blocks the caller, and the returned task must be held by the caller for the same reason `WebRTCPeerSession._spawn` holds streaming tasks (a bare - `ensure_future` can be garbage-collected mid-flight). + `ensure_future` can be garbage-collected mid-flight). `root_path` is + the caller's own root boundary for this entry (daemon.py resolves + it via `RootSet.split`) — see `_artist_album_from_ancestors`. """ - task = asyncio.ensure_future(self._run(entry, file_path, on_done)) + task = asyncio.ensure_future(self._run(entry, file_path, on_done, root_path)) self._tasks.add(task) def _cleanup(t: asyncio.Task) -> None: @@ -162,6 +312,7 @@ class AudioEnricher: async def _run( self, entry: IndexEntry, file_path: Path, on_done: Callable[[str, dict], Awaitable[None]], + root_path: Path | None, ) -> None: async with self._sem: fields: dict = {} @@ -183,7 +334,7 @@ class AudioEnricher: album = tags.get("album") if not artist or not album: fallback_artist, fallback_album = await asyncio.to_thread( - _artist_album_from_ancestors, file_path) + _artist_album_from_ancestors, file_path, root_path) artist = artist or fallback_artist album = album or fallback_album fields["artist"] = artist diff --git a/packages/meshbay-node/src/meshbay_node/indexer/title_parse.py b/packages/meshbay-node/src/meshbay_node/indexer/title_parse.py index 6676e9b..5203247 100644 --- a/packages/meshbay-node/src/meshbay_node/indexer/title_parse.py +++ b/packages/meshbay-node/src/meshbay_node/indexer/title_parse.py @@ -187,6 +187,20 @@ def parse_track_filename(filename: str) -> ParsedTrack: return ParsedTrack(title=title, track_no=track_no, naive_title=naive_title(filename)) +def strip_track_prefix(text: str) -> str: + """ + Some taggers copy the bare filename into the `title` tag verbatim, + track-number prefix included (found live: a whole CD-single's worth of + `title` tags reading "01 - Venus As A Boy" rather than "Venus As A + Boy") — since a tag normally wins over the filename-parsed title + (enrich_audio.py), that pollution would otherwise beat a cleaner parse. + A no-op when there's no such prefix, so a genuinely clean tag is + returned unchanged. + """ + m = _TRACK_PREFIX_RE.match(text) + return re.sub(r"^[\s-]+", "", text[m.end():]).strip() if m else text + + def parse_episode_filename(filename: str) -> ParsedName: """ Parse an episode filename. `display_title` may come back None (e.g. diff --git a/packages/meshbay-node/tests/test_enrich_audio.py b/packages/meshbay-node/tests/test_enrich_audio.py index 62d0a0a..221f1c6 100644 --- a/packages/meshbay-node/tests/test_enrich_audio.py +++ b/packages/meshbay-node/tests/test_enrich_audio.py @@ -6,12 +6,15 @@ import subprocess from pathlib import Path import pytest - from meshbay_common.protocol import IndexEntry from meshbay_node.indexer.enrich_audio import ( - AudioEnricher, _artist_album_from_ancestors, _extract_cover, + AudioEnricher, + _artist_album_from_ancestors, + _clean_tag, + _extract_cover, + _split_top_level_folder, ) -from meshbay_node.indexer.title_parse import parse_track_filename +from meshbay_node.indexer.title_parse import parse_track_filename, strip_track_prefix from meshbay_node.media_cache import MediaCache _HAVE_FFMPEG = shutil.which("ffmpeg") and shutil.which("ffprobe") @@ -54,12 +57,100 @@ def test_artist_album_from_ancestors_reads_artist_album_track_layout(tmp_path): track = folder / "01 - A Track.mp3" track.touch() - artist, album = _artist_album_from_ancestors(track) + artist, album = _artist_album_from_ancestors(track, tmp_path) assert artist == "Some Artist" assert album == "Some Album" +def test_artist_album_from_ancestors_refuses_to_name_the_root_as_artist(tmp_path): + """ + The regression this whole revision exists for: a flat `Artist/track.mp3` + layout (no album subfolder) used to read the *root's own directory + name* as the artist, because the walk always climbed two levels with no + idea where the root was. Measured live: 289 tracks across 41 real, + unrelated artists collapsed into one fake "artist" this way — the + single biggest bucket in the whole library. + """ + folder = tmp_path / "Ben Harper" + folder.mkdir(parents=True) + track = folder / "Ashes.mp3" + track.touch() + + artist, album = _artist_album_from_ancestors(track, tmp_path) + + assert artist == "Ben Harper" + assert album is None, "no album folder exists — must not invent one, or swap artist/album" + + +def test_artist_album_from_ancestors_file_directly_in_root_has_no_context(tmp_path): + track = tmp_path / "loose_track.mp3" + track.touch() + + artist, album = _artist_album_from_ancestors(track, tmp_path) + + assert (artist, album) == (None, None) + + +def test_artist_album_from_ancestors_without_a_root_keeps_the_old_two_level_behaviour(tmp_path): + """No root resolved (should not happen in practice, but must degrade safely).""" + folder = tmp_path / "Some Artist" / "Some Album" + folder.mkdir(parents=True) + track = folder / "01 - A Track.mp3" + track.touch() + + artist, album = _artist_album_from_ancestors(track, None) + + assert artist == "Some Artist" + assert album == "Some Album" + + +def test_split_top_level_folder_splits_artist_dash_album(): + artist, album = _split_top_level_folder("GHOST DOG - Soundtrack") + assert artist == "GHOST DOG" + assert album == "Soundtrack" + + +def test_split_top_level_folder_with_no_separator_is_artist_only(): + artist, album = _split_top_level_folder("Ben Harper") + assert artist == "Ben Harper" + assert album is None + + +def test_split_top_level_folder_cleans_rip_tag_noise(): + artist, album = _split_top_level_folder( + "Sinsemilia - Premiere Recolte [MP3 320kbps Album]") + assert artist == "Sinsemilia" + assert album == "Premiere Recolte" + + +def test_clean_tag_filters_known_placeholders(): + assert _clean_tag("No Artist") is None + assert _clean_tag("unknown artist") is None + assert _clean_tag("Nouvel artiste (334)") is None + assert _clean_tag("Nouveau titre (12)") is None + assert _clean_tag("") is None + assert _clean_tag(None) is None + + +def test_clean_tag_keeps_various_artists_as_a_real_credit(): + """A real, meaningful compilation credit — not a placeholder to blank out.""" + assert _clean_tag("Various Artists") == "Various Artists" + + +def test_clean_tag_keeps_a_real_value(): + assert _clean_tag("Björk") == "Björk" + + +def test_strip_track_prefix_removes_a_leaked_filename_number(): + assert strip_track_prefix("01 - Venus As A Boy (Edited Lp Version)") == \ + "Venus As A Boy (Edited Lp Version)" + + +def test_strip_track_prefix_is_a_noop_on_a_clean_title(): + assert strip_track_prefix("Venus As A Boy") == "Venus As A Boy" + + # ── end-to-end against a real (tiny, synthetic) MP3 file ──────────────────── pytestmark_ffmpeg = pytest.mark.skipif(not _HAVE_FFMPEG, reason="ffmpeg/ffprobe not installed") @@ -130,7 +221,7 @@ async def test_enricher_falls_back_to_filename_and_folder_when_tags_absent(tmp_p async def on_done(file_id, fields): done.set_result((file_id, fields)) - enricher.spawn(entry, clip, on_done) + enricher.spawn(entry, clip, on_done, tmp_path) _, fields = await asyncio.wait_for(done, timeout=30) assert fields["display_title"] == "Filename Title" @@ -140,6 +231,59 @@ async def test_enricher_falls_back_to_filename_and_folder_when_tags_absent(tmp_p @pytestmark_ffmpeg +@pytest.mark.asyncio +async def test_enricher_falls_back_to_artist_only_for_a_flat_top_level_dir(tmp_path, media_cache): + """The real-world regression case, end to end through the whole pool.""" + folder = tmp_path / "Ben Harper" + folder.mkdir(parents=True) + clip = folder / "Ashes.mp3" + _make_clip(clip) # no metadata tags at all + entry = IndexEntry(id="fileid3", name=clip.name, + path=str(clip.relative_to(tmp_path)), + size=clip.stat().st_size, type="audio", added_at=0) + + enricher = AudioEnricher(media_cache) + done = asyncio.get_event_loop().create_future() + + async def on_done(file_id, fields): + done.set_result((file_id, fields)) + + enricher.spawn(entry, clip, on_done, tmp_path) + _, fields = await asyncio.wait_for(done, timeout=30) + + assert fields["artist"] == "Ben Harper" + assert fields["album"] is None + assert fields["artist"] != tmp_path.name, \ + "must never fall back to the shared root's own directory name" + + +@pytestmark_ffmpeg +@pytest.mark.asyncio +async def test_enricher_uses_a_sibling_cover_file_when_no_embedded_art(tmp_path, media_cache): + folder = tmp_path / "Some Artist" / "Some Album" + folder.mkdir(parents=True) + clip = folder / "01 - A Track.mp3" + _make_clip(clip) + (folder / "Folder.jpg").write_bytes(b"\xff\xd8\xff\xe0fake-jpeg-bytes") + entry = IndexEntry(id="fileid4", name=clip.name, + path=str(clip.relative_to(tmp_path)), + size=clip.stat().st_size, type="audio", added_at=0) + + enricher = AudioEnricher(media_cache) + done = asyncio.get_event_loop().create_future() + + async def on_done(file_id, fields): + done.set_result((file_id, fields)) + + enricher.spawn(entry, clip, on_done, tmp_path) + _, fields = await asyncio.wait_for(done, timeout=30) + + assert fields.get("thumb_hash"), "a Folder.jpg beside the track must be picked up as its cover" + stored = await media_cache.get_thumb(fields["thumb_hash"]) + assert stored == b"\xff\xd8\xff\xe0fake-jpeg-bytes" + + +@pytestmark_ffmpeg def test_extract_cover_returns_none_when_no_apic_frame(tmp_path): from mutagen import File as MutagenFile |