Grab cover art from MusicBrainz/Cover Art Archive and add it to the resulting whipper rips #436

ABCbum · 2019-12-23T16:15:11Z

Submitted as a part of GCI competition

Description
Fixes: #50

Add option --cover-art to whipper cd rip command which accepts three values:

file: save the downloaded cover image as standalone file in the rip folder (named cover.jpg)
embed: embed the download cover image into all the ripped audio tracks (no standalone file will be kept)
complete: save standalone cover image as standalone file and embed it into all the ripped audio tracks (file + embed)

Every cover art is fetched from the Cover Art Archive as JPEG thumbnail with a maximum dimension of 500px using musicbrainzngs.
Other supported values for the thumbnails are 250, 500 and 1200 (currently only some images have a corresponding 1200px sized thumbnail).

This feature introduces an optional dependency on the Pillow module which is required for the decoding of the cover file (required by the embed and complete option values).

Problems

EmbedPicTureTask shouldn't be a task.
The test checks only getCoverArt's functionality (embedding isn't tested)

MerlijnWajer · 2019-12-23T16:19:40Z

Here's a thought: should an error fetching the cover art be fatal or not? Also - let's make sure it's an optional feature.

ABCbum · 2019-12-23T16:29:27Z

Alright, i'm just testing before bed :DD . Thank for your early suggestions.

Also i think it shouldn't be fatal.

q3cpma · 2019-12-26T18:48:33Z

Isn't it possible to import conditionally? It seems strange to bloat the mandatory dependencies for an optional feature.

ABCbum · 2019-12-27T07:05:46Z

Isn't it possible to import conditionally? It seems strange to bloat the mandatory dependencies for an optional feature.

Hi @q3cpma , I don't really get what you mean right now but maybe i should move from PIL import Image to where it is used ?.

q3cpma · 2019-12-27T11:09:59Z

On Thu, Dec 26, 2019 at 11:05:48PM -0800, ABCbum wrote: > Isn't it possible to import conditionally? It seems strange to bloat the mandatory dependencies for an optional feature. I don't really get what you mean right now but maybe i should move `from PIL import Image` to where it is used ?.

Well, something like that. Sadly, I don't Python at all, so I don't know if that's a good idea. Just don't want to install pillow when I don't plan on using this feature.

ABCbum · 2019-12-27T12:32:49Z

Yeah i think thats a good point, just move the import statement down to the _make_flac_picture should solve the problem for the time being if you dont use it but i will try to find an alternative way. Thanks a lot, @q3cpma .

JoeLametta · 2019-12-28T14:12:15Z

Fixed conflicts and squashed.

ABCbum · 2019-12-28T14:12:58Z

@JoeLametta , thanks for doing that for me ;)

JoeLametta · 2019-12-28T14:31:40Z

@ABCbum I think @q3cpma means that as this one (grabbing cover art) is not a primary feature of whipper, it should be possible to install and use whipper without the new dependency (pillow).
Maybe you can leave the menu command entry as it is but handle the case in which the module is missing, report it (example: optional dependency needed for bla, bla, bla), etc.

try:
    from PIL import Image
except ImportError as e:
    logger.warning("bla, bla", e)
    ...

ABCbum · 2019-12-28T14:37:18Z

@JoeLametta , i think that's a great idea but how about requirements.txt, README.md and dockerfile ?

We can solve README.md my simply adding (optional) next to pillow. But what about the other two ? Should we remove pillow or not ?

JoeLametta · 2019-12-28T14:50:21Z

requirements / setup.py: maybe this (or we can create an optional-requirements.txt file where all optional, pip installable, dependencies will go).
README.md: just list it is as an optional dependency
Dockerfile: always install it (maybe you can use the python3-pil apt package instead)

ABCbum · 2019-12-28T15:52:49Z

I'm not sure if what i did in this commit correct tho, @JoeLametta .

whipper/command/cd.py

whipper/common/encode.py

JoeLametta · 2019-12-29T16:18:41Z

I'm not sure if what i did in this commit correct tho, @JoeLametta .

This is not enough but I'm also not sure which is the best way to handle this. @Freso do you have any advice about this?

whipper/common/program.py

JoeLametta · 2019-12-29T17:47:40Z

@ABCbum General comment: return == return None.

Freso · 2019-12-30T02:35:04Z

The "best"/canonical way of specifying optional dependencies is to specify them as "extras" in setup.py, see https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies

whipper/common/encode.py

whipper/command/cd.py

Dockerfile

README.md

whipper/common/encode.py

whipper/common/program.py

JoeLametta · 2020-01-03T14:44:40Z

Small changes + history rewritten to be easier to understand.

JoeLametta · 2020-01-03T14:45:51Z

@ABCbum could you add a test case?

requirements.txt

whipper/command/cd.py

whipper/common/encode.py

whipper/common/program.py

ABCbum · 2020-01-03T18:13:44Z

@ABCbum could you add a test case?

I have added a "draft" test case i'm not sure if it's an okay one.

JoeLametta · 2020-01-09T17:14:29Z

I have added a "draft" test case i'm not sure if it's an okay one.

If you want to test whether the embedding works too I've attached this FLAC file (the extension is ZIP because GitHub doesn't allow attaching FLAC files directly, just rename it). The file content is just 1s of silence (stereo, 44100 Hz) with the test image embedded as front cover. 😉

JoeLametta · 2020-01-10T16:46:42Z

@ABCbum Same file without attached cover: test_1s_silence_stereo_no_cover.flac.zip

I have generated the source wave file with the following code:

import wave

with wave.open('test.wav', 'wb') as f:
    duration = 1  # duration in seconds
    nchannels = 2  # Set to 1 for mono
    sampwidth = 2  # 16 bit
    framerate = 44100  # CD sample rate
    f.setnchannels(nchannels)
    f.setsampwidth(sampwidth)
    f.setframerate(framerate)
    f.setnframes(framerate * duration)
    f.writeframes(b'\x00' * nchannels * sampwidth * framerate * duration)

ABCbum · 2020-01-11T17:23:41Z

I have added a "draft" test case i'm not sure if it's an okay one.

If you want to test whether the embedding works too I've attached this FLAC file (the extension is ZIP because GitHub doesn't allow attaching FLAC files directly, just rename it). The file content is just 1s of silence (stereo, 44100 Hz) with the test image embedded as front cover. 😉

Yeah it works locally to me :D not sure if i should write a test case for that.

Add option `--cover-art` to `whipper cd rip` command which accepts three values: - `file`: save the downloaded cover image as standalone file in the rip folder (named `cover.jpg`) - `embed`: embed the download cover image into all the ripped audio tracks (no standalone file will be kept) - `complete`: save standalone cover image as standalone file and embed it into all the ripped audio tracks (`file` + `embed`) Every cover art is fetched from the Cover Art Archive as JPEG thumbnail with a maximum dimension of 500px. Other supported values for the thumbnails are 250, 500 and 1200 (currently only some images have a corresponding 1200px sized thumbnail). This feature introduces an optional dependency on the `Pillow` module which is required for the decoding of the cover file (required by the `embed` and `complete` option values). Problem: - EmbedPicTureTask shouldn't be a task. Signed-off-by: ABCbum <kimlong221002@gmail.com> Co-authored-by: JoeLametta <JoeLametta@users.noreply.github.com> Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>

Signed-off-by: ABCbum <kimlong221002@gmail.com> Co-authored-by: JoeLametta <JoeLametta@users.noreply.github.com> Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>

Mock two functions `getCoverArt`, `get_image_front` and use a locally available cover art to check if the created cover art exists. Problems: - How to check image's quality. - Not sure if only this check is enough (do we need to check the embedding part?). Signed-off-by: ABCbum <kimlong221002@gmail.com>

JoeLametta · 2020-01-14T16:05:03Z

Merged, thanks!

ABCbum · 2020-01-23T11:36:07Z

whipper/command/cd.py

@@ -290,6 +291,14 @@ def add_arguments(self):
                                 help="whether to continue ripping if "
                                 "the disc is a CD-R",
                                 default=False)
+        self.parser.add_argument('-C', '--cover-art',
+                                 action="store", dest="fetch_cover_art",
+                                 help="Fetch cover art and save it as "


Suggested change

help="Fetch cover art and save it as "

help="fetch cover art and save it as "

I just noticed a small detail that this shouldn't be capital just to be consistent 😉.

Reported by user "ABCbum" in comment (#436 (comment)). Signed-off-by: JoeLametta <JoeLametta@users.noreply.github.com>

mores · 2020-12-22T19:42:24Z

Lets say someone already issued:
whipper cd rip
<repeat 100 times>

Now they want to go back and '--cover-art complete' without re-ripping. Can you just patch existing files with just artwork ?

or is beet fetchart the way to go ?

JoeLametta · 2020-12-25T14:04:27Z

Now they want to go back and '--cover-art complete' without re-ripping. Can you just patch existing files with just artwork ?

That's not possible with the current implementation and I would say that's out of scope for whipper and best left to a specialized tool (musical tagger).

ABCbum marked this pull request as ready for review December 24, 2019 10:43

JoeLametta force-pushed the grab-cover-art branch from c071e9e to 84ca2cf Compare December 28, 2019 14:04

JoeLametta reviewed Dec 29, 2019

View reviewed changes

whipper/command/cd.py Outdated Show resolved Hide resolved

JoeLametta reviewed Dec 29, 2019

View reviewed changes

whipper/common/encode.py Outdated Show resolved Hide resolved

JoeLametta reviewed Dec 29, 2019

View reviewed changes

whipper/common/encode.py Outdated Show resolved Hide resolved

JoeLametta reviewed Dec 29, 2019

View reviewed changes

whipper/common/encode.py Outdated Show resolved Hide resolved

JoeLametta reviewed Dec 29, 2019

View reviewed changes

whipper/common/encode.py Outdated Show resolved Hide resolved

JoeLametta reviewed Dec 29, 2019

View reviewed changes

whipper/common/program.py Outdated Show resolved Hide resolved