Add option to convert cover art file format. #4133

ArsenArsen · 2021-11-02T12:32:08Z

Description

Introduces an option to force an image format, as some IEMs (notably, Rockbox) might lack support for some image formats.

To Do

Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
Tests. (Encouraged but not strictly required.)

I want to do tests, but I am unsure as to how to implement a test that changes the resulting cover file name.

PS: there might be a bug in cleanup of the temporary files here, however, my /tmp is too polluted to test, so consider this a call for help. I also don't have Windows anywhere to test on.

sampsyo

Seems great so far! I too think it would be awesome to get some help trying out the whole /tmp cleanup stuff here, if anyone has any extra cycles…

sampsyo · 2021-11-02T16:56:57Z

beets/util/artresizer.py

+            return func(path_in)
+
+    def reformat(self, path_in, new_format, deinterlaced=True):
+        """Converts image to desired format, updating it's extension, but


Suggested change

"""Converts image to desired format, updating it's extension, but

"""Converts image to desired format, updating its extension, but

sampsyo · 2021-11-02T16:57:08Z

beets/util/artresizer.py

+    def get_format(self, path_in):
+        """Returns the format of the image as a string.
+
+        Only available locally"""


Suggested change

Only available locally"""

Only available locally.

"""

sampsyo · 2021-11-02T16:57:17Z

beets/util/artresizer.py

+        """Converts image to desired format, updating it's extension, but
+        keeping the same filename.
+
+        Only available locally"""


Suggested change

Only available locally"""

Only available locally.

"""

sampsyo · 2021-11-02T16:57:45Z

beets/util/artresizer.py

+            'jpeg': 'jpg',
+        }.get(new_format, new_format)
+
+        (fname, ext) = os.path.splitext(path_in)


Suggested change

(fname, ext) = os.path.splitext(path_in)

fname, ext = os.path.splitext(path_in)

sampsyo · 2021-11-02T16:58:17Z

beets/util/artresizer.py

+        }.get(new_format, new_format)
+
+        (fname, ext) = os.path.splitext(path_in)
+        path_new = fname + b'.' + new_format.encode('us-ascii')


I guess I usually think 'utf8' is usually the most sensible default, even if you don't normally expect non-ASCII characters?

sampsyo · 2021-11-02T16:59:29Z

docs/plugins/fetchart.rst

+- **cover_format**: If enabled, forced the cover image into the specified
+  format. Valid values line up with ImageMagick/Pillow decoder names (for
+  instance, ``PNG`` or ``JPEG``). Also respects ``deinterlace``.
+  Default: ``None`` (leave unchanged).


Suggested change

Default: ``None`` (leave unchanged).

Default: None (leave unchanged).

Probably no need for code formatting here, because it's not literally what you type into the file.

sampsyo · 2021-11-02T17:00:51Z

docs/plugins/fetchart.rst

@@ -90,6 +90,10 @@ file. The available options are:
  instructed to store cover art as non-progressive JPEG. You might need this if
  you use DAPs that don't support progressive images.
  Default: ``no``.
+- **cover_format**: If enabled, forced the cover image into the specified
+  format. Valid values line up with ImageMagick/Pillow decoder names (for
+  instance, ``PNG`` or ``JPEG``). Also respects ``deinterlace``.


Maybe we should just give a list of all the formats we expect to work? We could just start with PNG and JPEG, for example, and not bother people with the somewhat subtle advice to look in the ImageMagick/Pillow documentation. (And if folks want other formats, maybe we can test those and add them to the docs.)

This might be a bit more complex. These two are the ones I tested and my confidence isn't high that the two libraries use matching names for others. I'll look for the lists in a bit.

The list can be obtained via magick identify -list format and via PIL.features.pilinfo() for Pillow.

Not sure how to integrate this into the docs. Do I just mention that and list a few common formats (I frankly doubt anyone would use anything but PNG, JPEG, or WEBP if they're really eccentric (my build of ImageMagick didn't even support WebP up to this point). All three of these have matching names across Pillow/ImageMagick on my machine.

I expect those two to work, but it could be rephrased in terms of those plus a link to docs explaining how to get a list of all formats.

Yeah, I think that's the practical thing—just listing a few common formats that 99.99% of people will want to use seems like the most helpful thing. And yes, supplementing that with a docs link will help satisfy the curiosity of the remaining 0.01%.

Got it, I'll change that and squash the commits.

ArsenArsen · 2021-11-02T18:45:30Z

I figured out a solution for testing: firejail --noprofile --private-tmp. There is, indeed, an issue with cleanup, I'll fix it.

ArsenArsen · 2021-11-02T19:12:29Z

No more junk left in /tmp!

sampsyo · 2021-11-03T14:29:05Z

Seems awesome! Thanks for your work on massaging the documentation, which looks very neat now. I'll merge this immediately! ✨

ArsenArsen · 2021-11-03T15:38:25Z

Thank you!

khnsky · 2021-11-08T18:53:02Z

This is great, thank you!
But does this convert images when resize/downscale is needed? I think that candidate.validate will return CANDIDATE_RESIZE/CANDIDATE_DOWNSCALE and candidate.resize won't convert formats.

ArsenArsen · 2021-11-08T18:59:20Z

oh, that's true, I haven't used those so I'd have forgotten to also add that special case, good catch

EDIT: do you think it'd be a good idea to modify this a bit, and instead of returning REFORMAT/DEINTERLACE, merge those into one and make them a flag, so that it happens for all kinds of candidates?

wisp3rwind · 2021-11-14T11:01:11Z

EDIT: do you think it'd be a good idea to modify this a bit, and instead of returning REFORMAT/DEINTERLACE, merge those into one and make them a flag, so that it happens for all kinds of candidates?

In fact, with the various recent additions to fetchart/artresizer, I think that the design of these function needs to be rethought. The current code is quite hard to reason about. Maybe, instead of these mutually exclusive flags, we should have something like a bitfield (in python, probably a list of required actions?), which is passed directly to a single artresizer function, which handles them all? For example, this could take the form of an options=["downsize", "reformat", "deinterlace"] argument for ArtResizer.shared.resize.

ArsenArsen · 2021-11-14T14:38:00Z

a set is more likely the appropriate function, and I disagree

I'd do one of two things, one of which is more drastic:

reformat all inputs unconditionally (which will simplify the code a lot and realistically not add an unreasonable cost in the modern age), or
run a single all-purpose reencode if something should be changed (i.e. if any parameter doesn't match)

sampsyo · 2021-11-14T15:45:12Z

You're right that a refactoring is in order here, @wisp3rwind. I like your idea of returning a set of options (perhaps a bitfield, or any other way of "unioning" enum values) instead of these mutually exclusive action flags, which have outlived their usefulness.

ArsenArsen force-pushed the convert-cover-format branch from 289d6a9 to 7fdd435 Compare November 2, 2021 12:34

sampsyo reviewed Nov 2, 2021

View reviewed changes

ArsenArsen added 3 commits November 3, 2021 13:31

fetchart: add option to force cover format

0b578a3

docs: add fetchart cover_format option

96be184

changelog: add entry about fetch_art cover_format

3de6574

ArsenArsen force-pushed the convert-cover-format branch from a58d732 to 3de6574 Compare November 3, 2021 12:31

sampsyo merged commit 3b53181 into beetbox:master Nov 3, 2021

wisp3rwind mentioned this pull request Aug 22, 2022

fetchart cover_format does not seem to be working (PNG => JPEG) #4452

Open

wisp3rwind mentioned this pull request May 7, 2024

Add fetchart typing #5213

Open

mlbaquerizo mentioned this pull request Sep 26, 2024

Allow fetchart to manipulate cover art candidates using all selected options #5440

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to convert cover art file format. #4133

Add option to convert cover art file format. #4133

ArsenArsen commented Nov 2, 2021

sampsyo left a comment

sampsyo Nov 2, 2021

sampsyo Nov 2, 2021

sampsyo Nov 2, 2021

sampsyo Nov 2, 2021

sampsyo Nov 2, 2021

sampsyo Nov 2, 2021

sampsyo Nov 2, 2021

ArsenArsen Nov 2, 2021

ArsenArsen Nov 2, 2021 •

edited

Loading

sampsyo Nov 3, 2021

ArsenArsen Nov 3, 2021

ArsenArsen commented Nov 2, 2021

ArsenArsen commented Nov 2, 2021

sampsyo commented Nov 3, 2021

ArsenArsen commented Nov 3, 2021

khnsky commented Nov 8, 2021

ArsenArsen commented Nov 8, 2021 •

edited

Loading

wisp3rwind commented Nov 14, 2021

ArsenArsen commented Nov 14, 2021 •

edited

Loading

sampsyo commented Nov 14, 2021

	"""Converts image to desired format, updating it's extension, but
	"""Converts image to desired format, updating its extension, but

	(fname, ext) = os.path.splitext(path_in)
	fname, ext = os.path.splitext(path_in)

	Default: ``None`` (leave unchanged).
	Default: None (leave unchanged).

Add option to convert cover art file format. #4133

Add option to convert cover art file format. #4133

Conversation

ArsenArsen commented Nov 2, 2021

Description

To Do

sampsyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArsenArsen Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArsenArsen commented Nov 2, 2021

ArsenArsen commented Nov 2, 2021

sampsyo commented Nov 3, 2021

ArsenArsen commented Nov 3, 2021

khnsky commented Nov 8, 2021

ArsenArsen commented Nov 8, 2021 • edited Loading

wisp3rwind commented Nov 14, 2021

ArsenArsen commented Nov 14, 2021 • edited Loading

sampsyo commented Nov 14, 2021

ArsenArsen Nov 2, 2021 •

edited

Loading

ArsenArsen commented Nov 8, 2021 •

edited

Loading

ArsenArsen commented Nov 14, 2021 •

edited

Loading