fix: embed cover art when the cover is already on disk - #230
Open
Piero-93 wants to merge 1 commit into
Open
Conversation
Cover art was only handed to ffmpeg inside the `track.CoverPath == ""` branch, that is, only when it still had to be downloaded. Custom playlists cache the cover when the playlist is created (loadCustomTracks fills CoverPath from the playlist cache), so for a Spotify, Apple Music or ListenBrainz playlist imported through the web UI the image is already on disk, the branch is skipped, and EMBED_COVER_ART silently does nothing. Fetching the cover and handing it to ffmpeg are now separate steps, and how the image is embedded depends on what the container can hold: - an attached picture stream for mp3, flac, m4a, m4b and mp4, re-encoded to mjpeg and flagged attached_pic so it is stored as artwork rather than as a regular video stream (h264 in mp4, png in mp3) - a base64 METADATA_BLOCK_PICTURE tag for the Ogg family and WavPack, which ffmpeg cannot build from an image input but does pass through - nothing at all for containers that cannot carry a picture (wav, aac, ac3), where handing ffmpeg an image makes the command fail and the track is lost with it Two other ways to lose a track are gone as well: DownloadCover returns its destination path even when the fetch failed, so the file is now checked to exist and to be non-empty, and if ffmpeg refuses the image anyway the file is written again without it. Verified by writing files and reading the artwork back with a tag library, on ffmpeg 7.0.2, 8.1.2 and 9.0.1: 96 combinations of container and cover format (jpeg, png, webp, truncated, empty, missing), no track lost, and picture type 3 wherever the container allows one.
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
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.
The bug
With
EMBED_COVER_ART=true, the cover is added as an ffmpeg input only inside thetrack.CoverPath == ""branch ofsaveVideo, that is, only when it still has to be downloaded:loadCustomTracksfillsCoverPathfrom the playlist cache, so for any custom playlist imported through the web UI (Spotify, Apple Music, ListenBrainz link) the image is already on disk when the download starts. The branch is skipped, the image never reaches ffmpeg, andEMBED_COVER_ARTsilently does nothing. Signature in the logs: an ffmpeg command with a single-i.The fix
Fetching the cover and handing it to ffmpeg are now separate steps, and how the image is embedded depends on what the output container can hold:
attached_picMETADATA_BLOCK_PICTUREtagThree failure modes that currently cost the whole track, not just the artwork, are gone as well:
wav muxer does not support any stream of type video) and the download never lands.DownloadCoverreturns its destination path even when the fetch failed — the return sits outside the status check — so a 404 or an empty body yields a non-existent input and a failed command. The file is now checked to exist and to be non-empty. I left the helper's signature alone, since the web UI depends on its current contract.Two smaller corrections in passing:
ffmpeg.Input(input).Audio()replaces the explicit"map": "0:a", which was only set on the branch without cover art. With two inputs the library maps every input, so a video stream in the downloaded file used to survive into the output.-metadata:s:v title/commentgives the picture type 3 ("front cover"); without them ffmpeg writes type 0 ("other").On mp4 the image is currently stored as an h264 video stream, and on mp3 as a re-encoded png. #219 reports the mp4 half of this and fixes it inside the old branch; here it falls out of the table above. Happy to rebase on top of #219, or to drop the overlapping part, if you would rather merge that one first.
Verification
Files written and artwork read back with a tag library, on ffmpeg 7.0.2, 8.1.2 and 9.0.1, with identical results on all three: 96 combinations of 16 containers by 6 cover inputs (jpeg, png, webp, truncated, empty, missing). No track lost in any of them, and picture type 3 wherever the container allows one.
Three judgement calls worth flagging:
.jpgbut holds whatever the CDN served; copying a WebP into mp4 fails withCould not find tag for codec webp, and re-encoding a still image is cheap. On the Vorbis comment path a WebP is skipped instead, with a warning, since there is no stdlib decoder for it.METADATA_BLOCK_PICTUREtag travels as a command-line argument, which Linux caps at 128 KiB each, so covers whose encoded tag exceeds 100 KiB are skipped. The defaultCOVER_ART_SIZE=250stays an order of magnitude below that.