Cache every cover, not only the ones pulled out of tags
The downscaling landed and the device kept serving 1920px art, because _cover_for has two branches and only one of them went through the cache. An album with a cover.jpg already sitting beside its audio - which most of this library has - had its `cover` point straight at that file, so FileResponse served whatever the internet had given it. The cache directory was full of tidy 640px files that half the albums never used. Both branches now store through the cache, in _cover_for and in _cover_for_episode (sidecar and shared-folder art alike). The undownscaled bytes still come back alongside the path, because colour extraction wants the real thing. That re-points `cover` for every album, but only for albums that are actually rescanned, and the scanner reuses anything whose fingerprint is unchanged - so _INDEX_VERSION goes to 5. An index from 4 is discarded and rebuilt, which is what that mechanism is for and is cheap by design. Two tests asserted `cover == <the library file>`, which is precisely the behaviour being changed. The episode one was also checking something real - that an episode's own sidecar art beats the show's shared cover - and both covers now live under their own album id, so it reads the colour back out of the stored file instead of comparing paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -5,8 +5,13 @@ different amounts to produce::
|
|||||||
|
|
||||||
<cache_dir>/
|
<cache_dir>/
|
||||||
├── index.json cheap: tags and structure. Thrown away freely.
|
├── index.json cheap: tags and structure. Thrown away freely.
|
||||||
├── covers/<album_id>.jpg medium: art pulled out of an ID3 APIC frame,
|
├── covers/<album_id>.jpg medium: the album's art - out of an ID3 APIC
|
||||||
│ downscaled to MAX_COVER_PX on the way in
|
│ frame, or copied from a cover.jpg in the
|
||||||
|
│ folder - downscaled to MAX_COVER_PX on
|
||||||
|
│ the way in. An album's `cover` always
|
||||||
|
│ points here and never into the library,
|
||||||
|
│ so nothing can serve full-size art by
|
||||||
|
│ accident.
|
||||||
└── analysis/<track_key>.json expensive: minutes of DSP per track
|
└── analysis/<track_key>.json expensive: minutes of DSP per track
|
||||||
analysis/<track_key>.beats.json
|
analysis/<track_key>.beats.json
|
||||||
analysis/<track_key>.curve.json
|
analysis/<track_key>.curve.json
|
||||||
@@ -41,7 +46,11 @@ __all__ = ["Fingerprint", "LibraryCache"]
|
|||||||
#: worked out - not just when the JSON shape does. A cached entry is reused whenever its
|
#: worked out - not just when the JSON shape does. A cached entry is reused whenever its
|
||||||
#: files are untouched, so otherwise a change to that logic is invisible until somebody
|
#: files are untouched, so otherwise a change to that logic is invisible until somebody
|
||||||
#: edits their music folder.
|
#: edits their music folder.
|
||||||
_INDEX_VERSION = 4
|
# 5: every album's `cover` now points into this cache, downscaled, including the ones
|
||||||
|
# that used to point straight at a `cover.jpg` in the library folder. Bumping this is
|
||||||
|
# what re-points them: an index from version 4 is discarded and the next scan rebuilds
|
||||||
|
# it, which is cheap by design - see the module docstring.
|
||||||
|
_INDEX_VERSION = 5
|
||||||
|
|
||||||
|
|
||||||
@dataclass(frozen=True, slots=True)
|
@dataclass(frozen=True, slots=True)
|
||||||
|
|||||||
@@ -119,10 +119,20 @@ def _most_common(values: list[str]) -> str:
|
|||||||
def _cover_for(
|
def _cover_for(
|
||||||
folder: Path, paths: list[Path], identifier: str, cache: LibraryCache
|
folder: Path, paths: list[Path], identifier: str, cache: LibraryCache
|
||||||
) -> tuple[Path | None, bytes | None]:
|
) -> tuple[Path | None, bytes | None]:
|
||||||
|
"""The album's art, as a path *into the cache* plus the original bytes.
|
||||||
|
|
||||||
|
Every branch goes through :meth:`LibraryCache.store_cover`, including the one that
|
||||||
|
finds a ``cover.jpg`` already sitting in the folder. Returning that file directly
|
||||||
|
would be the obvious thing and was the original behaviour, and it quietly undid the
|
||||||
|
downscaling: a folder cover is whatever the internet gave it, often 1920px or more,
|
||||||
|
and the browser then decoded all of it to fill a 185px card. The bytes come back
|
||||||
|
undownscaled either way, because `colors_from_cover` wants the real art.
|
||||||
|
"""
|
||||||
for name in _COVER_NAMES:
|
for name in _COVER_NAMES:
|
||||||
candidate = folder / name
|
candidate = folder / name
|
||||||
if candidate.is_file():
|
if candidate.is_file():
|
||||||
return candidate, candidate.read_bytes()
|
art = candidate.read_bytes()
|
||||||
|
return cache.store_cover(identifier, art), art
|
||||||
for path in paths[:3]:
|
for path in paths[:3]:
|
||||||
# Podcast feeds sometimes art only some episodes; a couple of tries is enough.
|
# Podcast feeds sometimes art only some episodes; a couple of tries is enough.
|
||||||
art = _embedded_art(path)
|
art = _embedded_art(path)
|
||||||
@@ -143,14 +153,16 @@ def _cover_for_episode(
|
|||||||
for extension in _SIDECAR_COVER_EXTENSIONS:
|
for extension in _SIDECAR_COVER_EXTENSIONS:
|
||||||
candidate = path.with_suffix(extension)
|
candidate = path.with_suffix(extension)
|
||||||
if candidate.is_file():
|
if candidate.is_file():
|
||||||
return candidate, candidate.read_bytes()
|
art = candidate.read_bytes()
|
||||||
|
return cache.store_cover(identifier, art), art
|
||||||
art = _embedded_art(path)
|
art = _embedded_art(path)
|
||||||
if art is not None:
|
if art is not None:
|
||||||
return cache.store_cover(identifier, art), art
|
return cache.store_cover(identifier, art), art
|
||||||
for name in _COVER_NAMES:
|
for name in _COVER_NAMES:
|
||||||
candidate = folder / name
|
candidate = folder / name
|
||||||
if candidate.is_file():
|
if candidate.is_file():
|
||||||
return candidate, candidate.read_bytes()
|
shared = candidate.read_bytes()
|
||||||
|
return cache.store_cover(identifier, shared), shared
|
||||||
return None, None
|
return None, None
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -247,9 +247,14 @@ async def test_a_cover_file_is_found_and_used(config_dir: Path) -> None:
|
|||||||
Image.new("RGB", (32, 32), (200, 40, 30)).save(folder / "cover.jpg")
|
Image.new("RGB", (32, 32), (200, 40, 30)).save(folder / "cover.jpg")
|
||||||
|
|
||||||
album = album_named(await build(config_dir), "Kinderparty Lieder")
|
album = album_named(await build(config_dir), "Kinderparty Lieder")
|
||||||
assert album.cover == folder / "cover.jpg"
|
# Into the cache, not at the library file: a folder cover is whatever size it came
|
||||||
|
# in at, and serving it directly is what used to put 1920px art in a 185px card.
|
||||||
|
assert album.cover is not None
|
||||||
|
assert album.cover.parent == config_dir / ".cache" / "covers"
|
||||||
|
assert album.cover.stem == album.id
|
||||||
# A solid red cover has one usable colour; the rest fall back to the synthesised
|
# A solid red cover has one usable colour; the rest fall back to the synthesised
|
||||||
# palette rather than repeating it.
|
# palette rather than repeating it - and that still reads the real art, not the
|
||||||
|
# downscaled copy.
|
||||||
assert album.colors[0] != colors_from_id(album.id)[0]
|
assert album.colors[0] != colors_from_id(album.id)[0]
|
||||||
|
|
||||||
|
|
||||||
@@ -264,9 +269,19 @@ async def test_an_episodes_sidecar_cover_wins_over_the_shows_shared_one(config_d
|
|||||||
|
|
||||||
library = await build(config_dir)
|
library = await build(config_dir)
|
||||||
|
|
||||||
assert album_named(library, "Neu").cover == podcast / "20260101 - Neu.jpg"
|
# Both covers now live in the cache under their own album id, so which file won is
|
||||||
|
# no longer visible in the path - read the colour back out instead.
|
||||||
|
def cover_colour(title: str) -> tuple[int, int, int]:
|
||||||
|
cover = album_named(library, title).cover
|
||||||
|
assert cover is not None
|
||||||
|
with Image.open(cover) as image:
|
||||||
|
return image.convert("RGB").getpixel((0, 0)) # type: ignore[return-value]
|
||||||
|
|
||||||
|
red, green, blue = cover_colour("Neu")
|
||||||
|
assert red > 150 and green < 90, "the episode's own sidecar art should win"
|
||||||
# No sidecar for this one, so it still falls back to the show's shared cover.
|
# No sidecar for this one, so it still falls back to the show's shared cover.
|
||||||
assert album_named(library, "Alt").cover == podcast / "cover.jpg"
|
red, green, blue = cover_colour("Alt")
|
||||||
|
assert red < 60 and blue > red, "falls back to the show's shared cover"
|
||||||
|
|
||||||
|
|
||||||
# --------------------------------------------------------------------------- cache
|
# --------------------------------------------------------------------------- cache
|
||||||
@@ -779,6 +794,32 @@ class TestCoverDownscaling:
|
|||||||
with Image.open(path) as image:
|
with Image.open(path) as image:
|
||||||
assert max(image.size) == MAX_COVER_PX
|
assert max(image.size) == MAX_COVER_PX
|
||||||
|
|
||||||
|
def test_a_cover_jpg_in_the_library_folder_is_cached_downscaled_too(
|
||||||
|
self, tmp_path: Path
|
||||||
|
) -> None:
|
||||||
|
"""The branch that quietly undid all of this: an album with art already sitting
|
||||||
|
beside it used to have `cover` point straight at that file, so it was served at
|
||||||
|
whatever size the internet gave it - often 1920px into a 185px card."""
|
||||||
|
from PIL import Image
|
||||||
|
|
||||||
|
from musicmouse.library.cache import MAX_COVER_PX, LibraryCache
|
||||||
|
from musicmouse.library.scanner import _cover_for
|
||||||
|
|
||||||
|
cache = LibraryCache(tmp_path / "cache")
|
||||||
|
cache.prepare()
|
||||||
|
folder = tmp_path / "album"
|
||||||
|
folder.mkdir()
|
||||||
|
(folder / "cover.jpg").write_bytes(self._jpeg((1920, 1920)))
|
||||||
|
|
||||||
|
path, art = _cover_for(folder, [], "album1", cache)
|
||||||
|
|
||||||
|
assert path is not None
|
||||||
|
assert path.parent == cache.covers, "cover must point into the cache, not the library"
|
||||||
|
with Image.open(path) as image:
|
||||||
|
assert max(image.size) == MAX_COVER_PX
|
||||||
|
# The untouched original still comes back, because colour extraction wants it.
|
||||||
|
assert art == (folder / "cover.jpg").read_bytes()
|
||||||
|
|
||||||
def test_covers_written_by_an_older_version_are_migrated(self, tmp_path: Path) -> None:
|
def test_covers_written_by_an_older_version_are_migrated(self, tmp_path: Path) -> None:
|
||||||
"""The scanner reuses an unchanged album without re-reading its tags, so a
|
"""The scanner reuses an unchanged album without re-reading its tags, so a
|
||||||
cover stored before the limit existed would otherwise never be rewritten."""
|
cover stored before the limit existed would otherwise never be rewritten."""
|
||||||
|
|||||||
Reference in New Issue
Block a user