feat(import): embed cover art into imported files, behind a setting - #914
Open
m4bard wants to merge 1 commit into
Open
feat(import): embed cover art into imported files, behind a setting#914m4bard wants to merge 1 commit into
m4bard wants to merge 1 commit into
Conversation
EnableCoverArtDownload has been persisted since the Settings page was added and read by nothing. Listenarrs#833 asked which direction to take it. This takes the reading the *arr family supports: the flag governs embedding artwork into audio files, not whether artwork is fetched for the UI. That distinction comes from checking the lineage first. Readarr, Lidarr and Sonarr have no toggle at all for downloading cover images for the UI; MediaCoverService takes IConfigFileProvider only for UrlBase. The boolean of this exact shape in that family is Lidarr's EmbedCoverArt (ConfigService.cs:308, read at AudioTagService.cs:117), whose label is "Embed Cover Art In Audio Files". Gating Listenarr's image cache instead would also have blanked author and series artwork, because the cache chokepoint cannot tell which kind of image it is holding. A new column rather than the old one. EnableCoverArtDownload is true on every existing instance because that was the property initialiser, not because an operator chose it. Reusing it would mean upgrading straight into rewriting audio files with no opt-in step. EmbedCoverArtInAudioFiles is added with defaultValue false, so existing installs are off by construction and no data repair migration is needed, which the architecture tests forbid in a PR anyway. The dead flag is left in place so this stays a single additive EF scaffold; dropping it is a separate mechanical change and I did not want it riding along with behaviour. On the default, deliberately not matching Lidarr's literal true: Lidarr hides EmbedCoverArt behind WriteAudioTags, which defaults to No, so nothing embeds out of the box there. Listenarr has no equivalent master switch and its ASIN write is unconditional, so porting the default without the gate would be more aggressive than the setting it copies. Artwork and the ASIN are written in one open and save. An ASIN is a few bytes and a cover is commonly a few hundred kilobytes, going into an M4B through a generation-bound lease, so a second pass would rewrite a file that is often several gigabytes twice. The bytes are identified by their leading bytes rather than trusted from a URL or a Content-Type. An image host answering a request with an HTML error page would otherwise be embedded under image/jpeg, producing a file whose cover silently fails to render. Anything unrecognised is dropped and the ASIN write still happens. This also gives DownloadCoverArtAsync its first production caller; it has been dead since it was written. It fetches from the URL rather than the image cache: resolving a cached path means reproducing the temp-versus- library layout handling in ImagesController, which is worth doing but not in the same change as new behaviour. Scope: manual import only, which is where the one existing tag write lives. The automatic import path writes no tags at all today and is left alone. Nothing rewrites artwork for files already in the library. Tests: the setting gates both the write and the fetch, so a disabled instance makes no request; an unrecognised response still writes the ASIN; the byte sniffing names jpeg, png and webp and refuses an HTML body. Confirmed the gate test fails with the settings check removed. Backend 3124 passed, 0 failed, 130 skipped. Frontend 607 passed.
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.
Closes #833, if you want the setting kept. I offered this as a branch on that issue rather than a PR because the default is a judgement call. Opening it now so it is reviewable, with that question still genuinely open.
What #833 found
EnableCoverArtDownloadhas been persisted since the Settings page was added and read by nothing. One declaration, no reader anywhere in the backend, and a checkbox saying "Download and embed cover art for audiobooks" that describes neither half of what happens.The direction, and why it is not the obvious one
Checking the family first changed my answer. Readarr, Lidarr and Sonarr have no toggle at all for downloading cover images for the UI:
MediaCoverServicetakesIConfigFileProvideronly forUrlBase. The boolean of this exact shape is Lidarr'sEmbedCoverArt(ConfigService.cs:308, read atAudioTagService.cs:117), labelled "Embed Cover Art In Audio Files". So in that family a flag like this governs embedding, not fetching.That also rules out the change I would otherwise have made. Gating the existing image cache looked cheapest, but
ImageCacheServicecannot tell an audiobook cover from an author or series image, so a checkbox reading "cover art for audiobooks" would have silently blanked author and series artwork.Off by default, deliberately not matching Lidarr
Lidarr defaults
EmbedCoverArtto true, but hides it behindWriteAudioTags, which defaults toNo, so nothing embeds out of the box there. Listenarr has no equivalent master switch and its ASIN write is unconditional, so porting the default without the gate would be more aggressive than the setting it copies.This is the part I am least sure you will agree with. It is a one-line change if you would rather it defaulted on.
Why a new column rather than wiring the old one
EnableCoverArtDownloadis true on every existing instance because that was the property initialiser, not because an operator chose it. Two approaches did not survive:MigrationProvenanceArchitectureTestsforbids outrightRenameColumnand therefore preserves the stored true, so every existing install would silently start rewriting audio files on upgradeEmbedCoverArtInAudioFilesis added withdefaultValue: false, so existing rows are off by construction. The dead flag is left in place so this stays a single additive scaffold. Dropping it is a separate mechanical change and I did not want it riding along with behaviour.Scope
Manual import only, which is where the one existing tag write lives. Artwork and the ASIN go in one open and save, since a second pass would rewrite a file that is often several gigabytes twice. The automatic import path writes no tags at all today and is untouched, and nothing rewrites artwork for files already in the library.
The bytes are identified from their leading bytes rather than trusted from a URL or a
Content-Type, because an image host answering with an HTML error page would otherwise be embedded underimage/jpegand produce a file whose cover silently fails to render.It also gives
DownloadCoverArtAsyncits first production caller. It has been dead since it was written.Conflicts to expect
This edits the pinned migration list in
SqliteMigrationSchemaTestsand the model snapshot, so it collides with any other PR adding a migration, including your #841 against our #763. It also collides with the release blocklist branch for the same reason. Whichever lands second wants a rebase; the resolution is keeping both ids in migration-timestamp order.Tests
Backend 3124 passed, frontend 607 passed. The gate has a control: the test fails with the settings check removed, which I checked rather than assumed.
Happy to change the default, split the frontend copy fix out on its own, or close this if you would rather the setting were removed instead.
Worked through with Claude Code at my direction. The suite numbers are runs on this machine against this branch, and the family citations are source-reading against Lidarr and Readarr. I reviewed this before posting.