diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 1cb35d3e8..cae081dd4 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -891,7 +891,7 @@ def command_output( if proc.returncode: raise subprocess.CalledProcessError( returncode=proc.returncode, - cmd=" ".join(cmd), + cmd=" ".join(map(str, cmd)), output=stdout + stderr, ) return CommandOutput(stdout, stderr) diff --git a/beetsplug/fetchart.py b/beetsplug/fetchart.py index 1bcced49a..a3bac19a1 100644 --- a/beetsplug/fetchart.py +++ b/beetsplug/fetchart.py @@ -67,10 +67,15 @@ class Candidate: self.match = match self.size = size - def _validate(self, plugin): + def _validate(self, plugin, skip_check_for=None): """Determine whether the candidate artwork is valid based on its dimensions (width and ratio). + `skip_check_for` is a check or list of checks to skip. This is used to + avoid redundant checks when the candidate has already been + validated for a particular operation without changing + plugin configuration. + Return `CANDIDATE_BAD` if the file is unusable. Return `CANDIDATE_EXACT` if the file is usable as-is. Return `CANDIDATE_DOWNSCALE` if the file must be rescaled. @@ -82,6 +87,11 @@ class Candidate: if not self.path: return self.CANDIDATE_BAD + if skip_check_for is None: + skip_check_for = [] + if isinstance(skip_check_for, int): + skip_check_for = [skip_check_for] + if not ( plugin.enforce_ratio or plugin.minwidth @@ -180,30 +190,51 @@ class Candidate: plugin.cover_format, ) - if downscale: + if downscale and (self.CANDIDATE_DOWNSCALE not in skip_check_for): return self.CANDIDATE_DOWNSCALE - elif downsize: - return self.CANDIDATE_DOWNSIZE - elif plugin.deinterlace: - return self.CANDIDATE_DEINTERLACE - elif reformat: + if reformat and (self.CANDIDATE_REFORMAT not in skip_check_for): return self.CANDIDATE_REFORMAT - else: - return self.CANDIDATE_EXACT + if plugin.deinterlace and ( + self.CANDIDATE_DEINTERLACE not in skip_check_for + ): + return self.CANDIDATE_DEINTERLACE + if downsize and (self.CANDIDATE_DOWNSIZE not in skip_check_for): + return self.CANDIDATE_DOWNSIZE + return self.CANDIDATE_EXACT - def validate(self, plugin): - self.check = self._validate(plugin) + def validate(self, plugin, skip_check_for=None): + self.check = self._validate(plugin, skip_check_for) return self.check def resize(self, plugin): - if self.check == self.CANDIDATE_DOWNSCALE: + """Resize the candidate artwork according to the plugin's + configuration until it is valid or no further resizing is + possible. + """ + # validate the candidate in case it hasn't been done yet + current_check = self.validate(plugin) + checks_performed = [] + + # we don't want to resize the image if it's valid or bad + while current_check not in [self.CANDIDATE_BAD, self.CANDIDATE_EXACT]: + self._resize(plugin, current_check) + checks_performed.append(current_check) + current_check = self.validate( + plugin, skip_check_for=checks_performed + ) + + def _resize(self, plugin, check=None): + """Resize the candidate artwork according to the plugin's + configuration and the specified check. + """ + if check == self.CANDIDATE_DOWNSCALE: self.path = ArtResizer.shared.resize( plugin.maxwidth, self.path, quality=plugin.quality, max_filesize=plugin.max_filesize, ) - elif self.check == self.CANDIDATE_DOWNSIZE: + elif check == self.CANDIDATE_DOWNSIZE: # dimensions are correct, so maxwidth is set to maximum dimension self.path = ArtResizer.shared.resize( max(self.size), @@ -211,9 +242,9 @@ class Candidate: quality=plugin.quality, max_filesize=plugin.max_filesize, ) - elif self.check == self.CANDIDATE_DEINTERLACE: + elif check == self.CANDIDATE_DEINTERLACE: self.path = ArtResizer.shared.deinterlace(self.path) - elif self.check == self.CANDIDATE_REFORMAT: + elif check == self.CANDIDATE_REFORMAT: self.path = ArtResizer.shared.reformat( self.path, plugin.cover_format, diff --git a/docs/changelog.rst b/docs/changelog.rst index 24068b879..f02ba8980 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -350,6 +350,8 @@ Bug fixes: :bug:`5130` * Fix bug where some plugin commands hang indefinitely due to a missing `requests` timeout. +* Fix cover art resizing logic to support multiple steps of resizing + :bug:`5151` For plugin developers: diff --git a/test/plugins/test_art.py b/test/plugins/test_art.py index 30f08d50f..6e877917d 100644 --- a/test/plugins/test_art.py +++ b/test/plugins/test_art.py @@ -862,6 +862,10 @@ class ArtForAlbumTest(UseThePlugin): IMG_225x225_SIZE = os.stat(util.syspath(IMG_225x225)).st_size IMG_348x348_SIZE = os.stat(util.syspath(IMG_348x348)).st_size + RESIZE_OP = "resize" + DEINTERLACE_OP = "deinterlace" + REFORMAT_OP = "reformat" + def setUp(self): super().setUp() @@ -892,11 +896,13 @@ class ArtForAlbumTest(UseThePlugin): else: self.assertIsNone(candidate) - def _assertImageResized(self, image_file, should_resize): # noqa + def _assert_image_operated(self, image_file, operation, should_operate): self.image_file = image_file - with patch.object(ArtResizer.shared, "resize") as mock_resize: + with patch.object( + ArtResizer.shared, operation, return_value=self.image_file + ) as mock_operation: self.plugin.art_for_album(self.album, [""], True) - self.assertEqual(mock_resize.called, should_resize) + self.assertEqual(mock_operation.called, should_operate) def _require_backend(self): """Skip the test if the art resizer doesn't have ImageMagick or @@ -948,31 +954,47 @@ class ArtForAlbumTest(UseThePlugin): def test_resize_if_necessary(self): self._require_backend() self.plugin.maxwidth = 300 - self._assertImageResized(self.IMG_225x225, False) - self._assertImageResized(self.IMG_348x348, True) + self._assert_image_operated(self.IMG_225x225, self.RESIZE_OP, False) + self._assert_image_operated(self.IMG_348x348, self.RESIZE_OP, True) def test_fileresize(self): self._require_backend() self.plugin.max_filesize = self.IMG_225x225_SIZE // 2 - self._assertImageResized(self.IMG_225x225, True) + self._assert_image_operated(self.IMG_225x225, self.RESIZE_OP, True) def test_fileresize_if_necessary(self): self._require_backend() self.plugin.max_filesize = self.IMG_225x225_SIZE - self._assertImageResized(self.IMG_225x225, False) + self._assert_image_operated(self.IMG_225x225, self.RESIZE_OP, False) self._assertImageIsValidArt(self.IMG_225x225, True) def test_fileresize_no_scale(self): self._require_backend() self.plugin.maxwidth = 300 self.plugin.max_filesize = self.IMG_225x225_SIZE // 2 - self._assertImageResized(self.IMG_225x225, True) + self._assert_image_operated(self.IMG_225x225, self.RESIZE_OP, True) def test_fileresize_and_scale(self): self._require_backend() self.plugin.maxwidth = 200 self.plugin.max_filesize = self.IMG_225x225_SIZE // 2 - self._assertImageResized(self.IMG_225x225, True) + self._assert_image_operated(self.IMG_225x225, self.RESIZE_OP, True) + + def test_deinterlace(self): + self._require_backend() + self.plugin.deinterlace = True + self._assert_image_operated(self.IMG_225x225, self.DEINTERLACE_OP, True) + self.plugin.deinterlace = False + self._assert_image_operated( + self.IMG_225x225, self.DEINTERLACE_OP, False + ) + + def test_deinterlace_and_resize(self): + self._require_backend() + self.plugin.maxwidth = 300 + self.plugin.deinterlace = True + self._assert_image_operated(self.IMG_348x348, self.DEINTERLACE_OP, True) + self._assert_image_operated(self.IMG_348x348, self.RESIZE_OP, True) class DeprecatedConfigTest(_common.TestCase):