jack-cli-cd-ripper / jack

Jack command-line CD ripper
GNU General Public License v2.0
14 stars 5 forks source link

Album art embedding #42

Open pimzand opened 2 years ago

pimzand commented 2 years ago

Jack should be able to embed local and remote album art into audio files.

pimzand commented 2 years ago

There's lots of progress in this area. I added searching and embedding of local album art files, and @zzarne added downloading album art from coverartarchive, which is thightly integrated in MusicBrainz.

@zzarne was just ahead of me with downloading remote album art.

I was going to do the same, but integrate it with the auto searching of local album art. That is, automatically download remote art if no suitable local art can be found.

What do you think?

Some other thoughts: @zzarne allows to download back covers, but the current embedding code marks all album art as front cover, and removes any album art that is not marked as front cover.

Do we need to support embedding of back covers? I don't see a use case for that.

pimzand commented 2 years ago

We might add support for querying the release group for album art, when there is no result for the specific release.

zzarne commented 2 years ago

(moved this comment where it belongs)

I agree. I'd like jack to be able to download any type of album art, but of course only the front should be embedded. Everything else just should go to reasonably named files. We should probably allow "[artist] - [album] - [art].[ext]" etc. for people who want multiple albums in one directory, so another template or naming album art files would be best.

I think albumart_ignorecase should default to 1 and the albumart_max_width and height to at least 1200, for all the hi-res devices out there.

And wishlist: scale (only down!) the image if it doesn't meet the resolution or size constraints.

zzarne commented 2 years ago

Do we need a dependency on PIL/Pillow in setup.py?

zzarne commented 2 years ago

We might add support for querying the release group for album art, when there is no result for the specific release.

I like that idea.

pimzand commented 2 years ago

We should probably allow "[artist] - [album] - [art].[ext]" etc. for people who want multiple albums in one directory, so another template or naming album art files would be best.

There's now a fetch_albumart_prefix config parameter, defaulting to "jack.". It helps me recognizing where the art came from when there is already other art in the same folder. Coverartarchive art isn't usually the source for the best quality art if available at all. I prefer iTunes art if it can be found.

There's little gain in naming art after artist or album, as there is no proper way to have multiple albums (or even media) in one folder, and still manage them with jack.

pimzand commented 2 years ago

Do we need a dependency on PIL/Pillow in setup.py?

We do. It's not unlikely that we need Pillow, as that's what is used in Fedora 35. Most users would probably install it (or already have it) through yum/dnf/rpm/apt/deb/whatever.

Other new imports are base64 (for ogg album art encoding/decoding) and hashlib (for generating unique filenames for saved art).

pimzand commented 2 years ago

We might add support for querying the release group for album art, when there is no result for the specific release.

I like that idea.

Yes, it's exactly what Kodi does if it plays a file that has MusicBrainz tags, but no album art. It may give unexpected results. Sometimes the release group art has the cover that was used on vinyl. Also, sometimes, Various Artists CD's are re-released with completely different names and different covers, and still be in the same release group.

pimzand commented 2 years ago

I think albumart_ignorecase should default to 1

There's already some case ignorance in the search patterns, ie "[Ff]ront" and "[Cc]over" and so on. But I guess it will not hurt to have full case ignorance. The only thing that I don't like about that is having to use "--albumart-ignorecase" on the command line to not ignore case. Perhaps we could make the option parsing learn about prefixing with --no, like --no-albumart-ignorecase ?

and the albumart_max_width and height to at least 1200, for all the hi-res devices out there.

Again, I was thinking of my own needs. My car player will not show album art on USB sticks larger than 1000x1000. Saving space is another thing, large images can easily be as large as the audio file itself, and be embedded in every track. I will be happy to put those restraints in my .jack3rc.

And wishlist: scale (only down!) the image if it doesn't meet the resolution or size constraints.

Yes, I have been thinking about that too. I suppose Pillow can do that. We would need new config parameters for the filename to cache the resized image in, and for the requested optimal width and height. Ideally, the user would specify width only, and let jack calculate the height using the original aspect ratio.

The album art search algorithm would do a first pass looking for files to be used as-is, and do a second pass searching for files that can be scaled down, if nothing was found in the first pass.

pimzand commented 2 years ago

Coverartarchive art isn't usually the source for the best quality art if available at all. I prefer iTunes art if it can be found.

This is my first source for album art https://github.com/bendodson/itunes-artwork-finder

It would be nice if this could be ported to python and be integrated into Jack. Not easy. with Jack being commandline/curses orientated and all that.

zzarne commented 2 years ago

For scaling, I think it would be enough to enforce the constraints only when embedding while accepting any found images and always downloading the highest resolution. That would keep it simple to implement and configure. Do you agree? I'm about to find out if and which resolutions my car can display later today :).

zzarne commented 2 years ago

I like that Coverartarchive is community-driven and my experience so far is good enough with covers being found for ~70 % of my CDs. But I'm all in favor of jack supporting multiple album art sources if anyone wants to implement it.

zzarne commented 2 years ago

There's already some case ignorance in the search patterns, ie "[Ff]ront" and "[Cc]over" and so on. But I guess it will not hurt to have full case ignorance.

There are (misguided) people who use uppercase file extensions. I see no benefit in considering case when looking for cover art, but it is configurable already, so this can stay as it is with just ignoring case being the default.

The only thing that I don't like about that is having to use "--albumart-ignorecase" on the command line to not ignore case. Perhaps we could make the option parsing learn about prefixing with --no, like --no-albumart-ignorecase ?

Toggles which change their polarity when the jackrc changes defaults were a terribly idea. I should have known better even back then. Jack already supports --longoption=yes etc. I like the --no- prefix and we totally should remove the toggle behavior even if it breaks backward compatibility. Maybe I'll look into it later.

pimzand commented 2 years ago

For scaling, I think it would be enough to enforce the constraints only when embedding while accepting any found images and always downloading the highest resolution. That would keep it simple to implement and configure. Do you agree? I'm about to find out if and which resolutions my car can display later today :).

For coverartarchive, this is how it works now, without having to scale ourselves.

Jack will always download the original, unscaled image, which is usually around 600x600. Sometimes larger. I added that code to yours. Jack will also download a scaled-down image if you specify a specific resolution (500 comes to mind). That is what the original code did. By downloading both, and letting the local searching algorithm assert them in a predictable order, we are embedding the original file if it fits, and the scaled-down version if it does not.

We would only need to scale ourselves for non-coverartarchive art.

zzarne commented 2 years ago

Perhaps we could make the option parsing learn about prefixing with --no, like --no-albumart-ignorecase ?

This has been implemented, among other related features, in https://github.com/jack-cli-cd-ripper/jack/commit/6a03afc19486192af36254462acb6396e71b154a.

pimzand commented 2 years ago

But I'm all in favor of jack supporting multiple album art sources if anyone wants to implement it.

Hi @zzarne, I figured out how to download high quality art from iTunes but I'm not sure where to put the code I wrote. Unlike coverartarchive, which is tightly integrated with MusicBrainz, iTunes art needs to be queried for using the artist name and album title. This means it could just as well be used while querying for freedb metadata. Perhaps you could create a stub function. Maybe we should separate the querying for metadata and for album art.

pimzand commented 2 years ago

The album art embedding appears to work fine, but the album art fetching is not stable yet.

At coverartarchive, I noticed that the server sometimes closes the connection between downloads. We may need to disable keepalive.

At discogs, it appears to be hard to determine what the local filename should be. The latest version uses the Content-Disposition header. If that header is not present or cannot be parsed, we now use a hash of the url to create a local filename. This needs some testing. The remote timestamps appear to be unstable, we may need to disable timestamp checking.

At iTunes, I am seeing that jack is not getting the same search results as in https://github.com/bendodson/itunes-artwork-finder Jack uses the search term "artist_name - album_name" if anyone wants to test.

pimzand commented 2 years ago

Album art downloading appears to be more stable now.

At coverartarchive, jack no longer reuses a TCP connection to download multiple files. Discogs downloads appear to change every time, jack will just download the files again if they have changed. Use --overwrite-albumart=never to prevent this from happening.

Interestingly, iTunes downloads are different for each download too, but without changing size or modification date. It's not unlikely that discogs and iTunes tag their album art secretly for tracking purposes.

coverartarchive and discogs album art is found using MusicBrainz metadata directly, but iTunes art needs to be searched for. The querying for iTunes art from MusicBrainz metadata has improved by using the artist name as credited, rather than the official artist name as in MusicBrainz.

Currently, no album art is downloaded when FreeDB metadata is used. We could enable searching for iTunes album art while performing FreeDB queries.