From 66b459b8d0cff496bc958f1fff1a3fe6b30b241c Mon Sep 17 00:00:00 2001 From: Dr-Blank <64108942+Dr-Blank@users.noreply.github.com> Date: Mon, 18 Mar 2024 08:57:32 -0400 Subject: [PATCH 1/6] Refactor Candidate class in fetchart.py to improve validation and resizing logic --- beetsplug/fetchart.py | 61 ++++++++++++++++++++++++++++++++----------- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/beetsplug/fetchart.py b/beetsplug/fetchart.py index 93d3f2c57..64bad4442 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 (not self.CANDIDATE_DOWNSCALE 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 (not self.CANDIDATE_REFORMAT in skip_check_for): return self.CANDIDATE_REFORMAT - else: - return self.CANDIDATE_EXACT + if plugin.deinterlace and ( + not self.CANDIDATE_DEINTERLACE in skip_check_for + ): + return self.CANDIDATE_DEINTERLACE + if downsize and (not self.CANDIDATE_DOWNSIZE 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, From f0fb1565ddd74212ea3da92450bb9dd4050ff67e Mon Sep 17 00:00:00 2001 From: Dr-Blank <64108942+Dr-Blank@users.noreply.github.com> Date: Mon, 18 Mar 2024 10:32:13 -0400 Subject: [PATCH 2/6] regression test for the bugfix - generic method to check if operation was performed - add test of deinterlace operation - add test for multiple operations performed if required (fails on master) --- test/plugins/test_art.py | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/test/plugins/test_art.py b/test/plugins/test_art.py index b2d1d74b4..17cbf7f3c 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 _assertImageOperated( + self, image_file, operation, should_operate + ): # noqa self.image_file = image_file - with patch.object(ArtResizer.shared, "resize") as mock_resize: + with patch.object(ArtResizer.shared, operation) 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,45 @@ 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._assertImageOperated(self.IMG_225x225, self.RESIZE_OP, False) + self._assertImageOperated(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._assertImageOperated(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._assertImageOperated(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._assertImageOperated(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._assertImageOperated(self.IMG_225x225, self.RESIZE_OP, True) + + def test_deinterlace(self): + self._require_backend() + self.plugin.deinterlace = True + self._assertImageOperated(self.IMG_225x225, self.DEINTERLACE_OP, True) + self.plugin.deinterlace = False + self._assertImageOperated(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._assertImageOperated(self.IMG_348x348, self.DEINTERLACE_OP, True) + self._assertImageOperated(self.IMG_348x348, self.RESIZE_OP, True) class DeprecatedConfigTest(_common.TestCase): From ada2f80de618f31bad0f6bae754eae748c4f7694 Mon Sep 17 00:00:00 2001 From: Dr-Blank <64108942+Dr-Blank@users.noreply.github.com> Date: Thu, 11 Apr 2024 13:05:46 -0400 Subject: [PATCH 3/6] add changelog entry for bug-fix --- docs/changelog.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 72186603d..d5ca4f861 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -295,6 +295,8 @@ Bug fixes: * Fix bug where all media types are reported as the first media type when importing with MusicBrainz as the data source :bug:`4947` +* Fix cover art resizing logic to support multiple steps of resizing + :bug:`5151` For plugin developers: From 6a27a8de3ca934ef73d593d4fa88a3d0a27ceea4 Mon Sep 17 00:00:00 2001 From: Dr-Blank <64108942+Dr-Blank@users.noreply.github.com> Date: Thu, 11 Apr 2024 13:15:41 -0400 Subject: [PATCH 4/6] satisfy the linter --- beetsplug/fetchart.py | 8 ++++---- test/plugins/test_art.py | 24 +++++++++++++----------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/beetsplug/fetchart.py b/beetsplug/fetchart.py index 64bad4442..5ad30aebd 100644 --- a/beetsplug/fetchart.py +++ b/beetsplug/fetchart.py @@ -190,15 +190,15 @@ class Candidate: plugin.cover_format, ) - if downscale and (not self.CANDIDATE_DOWNSCALE in skip_check_for): + if downscale and (self.CANDIDATE_DOWNSCALE not in skip_check_for): return self.CANDIDATE_DOWNSCALE - if reformat and (not self.CANDIDATE_REFORMAT in skip_check_for): + if reformat and (self.CANDIDATE_REFORMAT not in skip_check_for): return self.CANDIDATE_REFORMAT if plugin.deinterlace and ( - not self.CANDIDATE_DEINTERLACE in skip_check_for + self.CANDIDATE_DEINTERLACE not in skip_check_for ): return self.CANDIDATE_DEINTERLACE - if downsize and (not self.CANDIDATE_DOWNSIZE in skip_check_for): + if downsize and (self.CANDIDATE_DOWNSIZE not in skip_check_for): return self.CANDIDATE_DOWNSIZE return self.CANDIDATE_EXACT diff --git a/test/plugins/test_art.py b/test/plugins/test_art.py index 17cbf7f3c..3278682a4 100644 --- a/test/plugins/test_art.py +++ b/test/plugins/test_art.py @@ -896,7 +896,7 @@ class ArtForAlbumTest(UseThePlugin): else: self.assertIsNone(candidate) - def _assertImageOperated( + def _assert_image_operated( self, image_file, operation, should_operate ): # noqa self.image_file = image_file @@ -954,45 +954,47 @@ class ArtForAlbumTest(UseThePlugin): def test_resize_if_necessary(self): self._require_backend() self.plugin.maxwidth = 300 - self._assertImageOperated(self.IMG_225x225, self.RESIZE_OP, False) - self._assertImageOperated(self.IMG_348x348, self.RESIZE_OP, 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._assertImageOperated(self.IMG_225x225, self.RESIZE_OP, 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._assertImageOperated(self.IMG_225x225, self.RESIZE_OP, 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._assertImageOperated(self.IMG_225x225, self.RESIZE_OP, 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._assertImageOperated(self.IMG_225x225, self.RESIZE_OP, True) + self._assert_image_operated(self.IMG_225x225, self.RESIZE_OP, True) def test_deinterlace(self): self._require_backend() self.plugin.deinterlace = True - self._assertImageOperated(self.IMG_225x225, self.DEINTERLACE_OP, True) + self._assert_image_operated(self.IMG_225x225, self.DEINTERLACE_OP, True) self.plugin.deinterlace = False - self._assertImageOperated(self.IMG_225x225, self.DEINTERLACE_OP, 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._assertImageOperated(self.IMG_348x348, self.DEINTERLACE_OP, True) - self._assertImageOperated(self.IMG_348x348, self.RESIZE_OP, 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): From dc0b46d0cb109c449da138a061079a3ad202eed7 Mon Sep 17 00:00:00 2001 From: Dr-Blank <64108942+Dr-Blank@users.noreply.github.com> Date: Thu, 11 Apr 2024 14:17:24 -0400 Subject: [PATCH 5/6] merge #5186 --- beets/util/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 00558e90a..2b0e7ce60 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -938,7 +938,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) From 57677bd7e680d39cf2b99d83ff2b7f5e9e65a58e Mon Sep 17 00:00:00 2001 From: Dr-Blank <64108942+Dr-Blank@users.noreply.github.com> Date: Fri, 12 Apr 2024 07:36:26 -0400 Subject: [PATCH 6/6] try return path itself to avoid side effects --- test/plugins/test_art.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/plugins/test_art.py b/test/plugins/test_art.py index 3278682a4..deb6f354e 100644 --- a/test/plugins/test_art.py +++ b/test/plugins/test_art.py @@ -896,11 +896,11 @@ class ArtForAlbumTest(UseThePlugin): else: self.assertIsNone(candidate) - def _assert_image_operated( - self, image_file, operation, should_operate - ): # noqa + def _assert_image_operated(self, image_file, operation, should_operate): self.image_file = image_file - with patch.object(ArtResizer.shared, operation) as mock_operation: + 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_operation.called, should_operate)