From ad9f256058fefed9df9b476a7314fd14929ed7d2 Mon Sep 17 00:00:00 2001 From: ababyduck Date: Mon, 22 Apr 2019 23:12:51 -0700 Subject: [PATCH 1/9] Update artresizer's ImageMagick commands to use the magick binary Updated artresizer's ImageMagick commands to use the magick binary added in ImageMagick 7.x, rather than the legacy utilities ('convert', 'identify', etc.) This resolves an issue where beets is failing to detect or use ImageMagick on Windows even when it is set correctly on the PATH, which in turn restores functionality to the fetchart and embedart plugins on Windows. Closes #2093 --- beets/util/artresizer.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index e58b356bef..31554ceb9e 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -93,7 +93,7 @@ def im_resize(maxwidth, path_in, path_out=None): # with regards to the height. try: util.command_output([ - 'convert', util.syspath(path_in, prefix=False), + 'magick', util.syspath(path_in, prefix=False), '-resize', '{0}x>'.format(maxwidth), util.syspath(path_out, prefix=False), ]) @@ -121,7 +121,7 @@ def pil_getsize(path_in): def im_getsize(path_in): - cmd = ['identify', '-format', '%w %h', + cmd = ['magick', 'identify', '-format', '%w %h', util.syspath(path_in, prefix=False)] try: out = util.command_output(cmd) @@ -235,7 +235,7 @@ def get_im_version(): Try invoking ImageMagick's "convert". """ try: - out = util.command_output(['convert', '--version']) + out = util.command_output(['magick', '-version']) if b'imagemagick' in out.lower(): pattern = br".+ (\d+)\.(\d+)\.(\d+).*" From f15f8a08f9b70ea0d5f092ee26003a18ae7d98dc Mon Sep 17 00:00:00 2001 From: ababyduck Date: Wed, 24 Apr 2019 05:30:17 -0700 Subject: [PATCH 2/9] Added fallback to ImageMagick's legacy utilities When the `magick` binary is not available, artresizer will fall back to the "legacy" binaries (`convert`, `identify`, etc.) --- beets/util/artresizer.py | 61 ++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 31554ceb9e..a7e5ea1705 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -81,7 +81,8 @@ def pil_resize(maxwidth, path_in, path_out=None): def im_resize(maxwidth, path_in, path_out=None): - """Resize using ImageMagick's ``convert`` tool. + """Resize using ImageMagick's ``magick`` tool + (or fall back to ``convert`` for older versions.) Return the output path of resized image. """ path_out = path_out or temp_file_for(path_in) @@ -92,11 +93,13 @@ def im_resize(maxwidth, path_in, path_out=None): # than the given width while maintaining the aspect ratio # with regards to the height. try: - util.command_output([ - 'magick', util.syspath(path_in, prefix=False), + cmds = (['magick'],['convert']) + cmd = cmds[0] if not im_legacy else cmds[1] + args = [util.syspath(path_in, prefix=False), '-resize', '{0}x>'.format(maxwidth), - util.syspath(path_out, prefix=False), - ]) + util.syspath(path_out, prefix=False)] + + util.command_output(cmd + args) except subprocess.CalledProcessError: log.warning(u'artresizer: IM convert failed for {0}', util.displayable_path(path_in)) @@ -121,10 +124,12 @@ def pil_getsize(path_in): def im_getsize(path_in): - cmd = ['magick', 'identify', '-format', '%w %h', - util.syspath(path_in, prefix=False)] + cmds = (['magick', 'identify'],['identify']) + cmd = cmds[0] if not im_legacy else cmds[1] + args = ['-format', '%w %h', util.syspath(path_in, prefix=False)] + try: - out = util.command_output(cmd) + out = util.command_output(cmd + args) except subprocess.CalledProcessError as exc: log.warning(u'ImageMagick size query failed') log.debug( @@ -229,26 +234,32 @@ def _check_method(): return WEBPROXY, (0) - +im_legacy = None def get_im_version(): """Return Image Magick version or None if it is unavailable - Try invoking ImageMagick's "convert". + Try invoking ImageMagick's "magick". If "magick" is unavailable, + as with older versions, fall back to "convert" """ - try: - out = util.command_output(['magick', '-version']) - - if b'imagemagick' in out.lower(): - pattern = br".+ (\d+)\.(\d+)\.(\d+).*" - match = re.search(pattern, out) - if match: - return (int(match.group(1)), - int(match.group(2)), - int(match.group(3))) - return (0,) - - except (subprocess.CalledProcessError, OSError) as exc: - log.debug(u'ImageMagick check `convert --version` failed: {}', exc) - return None + cmds = ('magick','convert') + for isLegacy, cmd in enumerate(cmds): + + try: + out = util.command_output([cmd, '--version']) + + if b'imagemagick' in out.lower(): + pattern = br".+ (\d+)\.(\d+)\.(\d+).*" + match = re.search(pattern, out) + if match: + im_legacy = bool(isLegacy) + return (int(match.group(1)), + int(match.group(2)), + int(match.group(3))) + + except (subprocess.CalledProcessError, OSError) as exc: + log.debug(u'ImageMagick version check failed: {}', exc) + return None + + return (0,) def get_pil_version(): From 1a6e0a7a29c21d4b62839a52c4c0e8b834cfcda0 Mon Sep 17 00:00:00 2001 From: ababyduck Date: Wed, 24 Apr 2019 06:18:43 -0700 Subject: [PATCH 3/9] Fix whitespace for flake8 compliance Fixed various whitespace issues and a global variable reference to comply with flake8 linting. --- beets/util/artresizer.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index a7e5ea1705..dc5cff9aa9 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -93,11 +93,11 @@ def im_resize(maxwidth, path_in, path_out=None): # than the given width while maintaining the aspect ratio # with regards to the height. try: - cmds = (['magick'],['convert']) + cmds = (['magick'], ['convert']) cmd = cmds[0] if not im_legacy else cmds[1] args = [util.syspath(path_in, prefix=False), - '-resize', '{0}x>'.format(maxwidth), - util.syspath(path_out, prefix=False)] + '-resize', '{0}x>'.format(maxwidth), + util.syspath(path_out, prefix=False)] util.command_output(cmd + args) except subprocess.CalledProcessError: @@ -124,7 +124,7 @@ def pil_getsize(path_in): def im_getsize(path_in): - cmds = (['magick', 'identify'],['identify']) + cmds = (['magick', 'identify'], ['identify']) cmd = cmds[0] if not im_legacy else cmds[1] args = ['-format', '%w %h', util.syspath(path_in, prefix=False)] @@ -234,13 +234,16 @@ def _check_method(): return WEBPROXY, (0) + im_legacy = None + + def get_im_version(): """Return Image Magick version or None if it is unavailable - Try invoking ImageMagick's "magick". If "magick" is unavailable, + Try invoking ImageMagick's "magick". If "magick" is unavailable, as with older versions, fall back to "convert" """ - cmds = ('magick','convert') + cmds = ('magick', 'convert') for isLegacy, cmd in enumerate(cmds): try: @@ -250,6 +253,7 @@ def get_im_version(): pattern = br".+ (\d+)\.(\d+)\.(\d+).*" match = re.search(pattern, out) if match: + global im_legacy im_legacy = bool(isLegacy) return (int(match.group(1)), int(match.group(2)), @@ -258,7 +262,7 @@ def get_im_version(): except (subprocess.CalledProcessError, OSError) as exc: log.debug(u'ImageMagick version check failed: {}', exc) return None - + return (0,) From 666790bd83be2afddb8395552d0da8c659d1068b Mon Sep 17 00:00:00 2001 From: ababyduck Date: Wed, 24 Apr 2019 09:07:38 -0700 Subject: [PATCH 4/9] Refactor to eliminate use of global variable `get_im_version` now returns an additional bool `isLegacy`, which indicates whether the the `magick` binary is accessible. It is stored in `self.im_legacy` on initialization of an `ArtResizer` object, and can be accessed via `ArtResizer.shared.im_legacy` --- beets/util/artresizer.py | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index dc5cff9aa9..49dc21a93d 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -94,7 +94,7 @@ def im_resize(maxwidth, path_in, path_out=None): # with regards to the height. try: cmds = (['magick'], ['convert']) - cmd = cmds[0] if not im_legacy else cmds[1] + cmd = cmds[0] if not ArtResizer.shared.im_legacy else cmds[1] args = [util.syspath(path_in, prefix=False), '-resize', '{0}x>'.format(maxwidth), util.syspath(path_out, prefix=False)] @@ -125,7 +125,7 @@ def pil_getsize(path_in): def im_getsize(path_in): cmds = (['magick', 'identify'], ['identify']) - cmd = cmds[0] if not im_legacy else cmds[1] + cmd = cmds[0] if not ArtResizer.shared.im_legacy else cmds[1] args = ['-format', '%w %h', util.syspath(path_in, prefix=False)] try: @@ -178,6 +178,9 @@ def __init__(self): log.debug(u"artresizer: method is {0}", self.method) self.can_compare = self._can_compare() + if self.method[0] == IMAGEMAGICK: + self.im_legacy = self.method[2] + def resize(self, maxwidth, path_in, path_out=None): """Manipulate an image file according to the method, returning a new path. For PIL or IMAGEMAGIC methods, resizes the image to a @@ -224,9 +227,9 @@ def _can_compare(self): @staticmethod def _check_method(): """Return a tuple indicating an available method and its version.""" - version = get_im_version() + version, isLegacy = get_im_version() if version: - return IMAGEMAGICK, version + return IMAGEMAGICK, version, isLegacy version = get_pil_version() if version: @@ -235,13 +238,13 @@ def _check_method(): return WEBPROXY, (0) -im_legacy = None - - def get_im_version(): """Return Image Magick version or None if it is unavailable Try invoking ImageMagick's "magick". If "magick" is unavailable, as with older versions, fall back to "convert" + + Our iterator `isLegacy` will be non-zero when the first command + fails, and will be returned in a tuple along with the version """ cmds = ('magick', 'convert') for isLegacy, cmd in enumerate(cmds): @@ -253,11 +256,11 @@ def get_im_version(): pattern = br".+ (\d+)\.(\d+)\.(\d+).*" match = re.search(pattern, out) if match: - global im_legacy - im_legacy = bool(isLegacy) - return (int(match.group(1)), + return ((int(match.group(1)), int(match.group(2)), - int(match.group(3))) + int(match.group(3))), + bool(isLegacy) + ) except (subprocess.CalledProcessError, OSError) as exc: log.debug(u'ImageMagick version check failed: {}', exc) From e00640e7dd4d8feca8070ea1314e79d7f8f226c2 Mon Sep 17 00:00:00 2001 From: ababyduck Date: Wed, 24 Apr 2019 09:35:56 -0700 Subject: [PATCH 5/9] Handle TypeError exception when no ImageMagick install is present Fixes an error introduced in 1a6e0a7 where a TypeError exception was raised when calling `_check_method()` with no ImageMagick installation present. --- beets/util/artresizer.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 49dc21a93d..ad1521bcf8 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -227,9 +227,12 @@ def _can_compare(self): @staticmethod def _check_method(): """Return a tuple indicating an available method and its version.""" - version, isLegacy = get_im_version() - if version: - return IMAGEMAGICK, version, isLegacy + try: + version, isLegacy = get_im_version() + if version: + return IMAGEMAGICK, version, isLegacy + except TypeError: + pass version = get_pil_version() if version: From a8f137bf9571c0671ce02e359c98c9c3aae90318 Mon Sep 17 00:00:00 2001 From: ababyduck Date: Wed, 24 Apr 2019 10:09:31 -0700 Subject: [PATCH 6/9] Rename `isLegacy` to `im_legacy` for consistency and flake8 compliance --- beets/util/artresizer.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index ad1521bcf8..5e3e983867 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -228,9 +228,9 @@ def _can_compare(self): def _check_method(): """Return a tuple indicating an available method and its version.""" try: - version, isLegacy = get_im_version() + version, im_legacy = get_im_version() if version: - return IMAGEMAGICK, version, isLegacy + return IMAGEMAGICK, version, im_legacy except TypeError: pass @@ -246,11 +246,11 @@ def get_im_version(): Try invoking ImageMagick's "magick". If "magick" is unavailable, as with older versions, fall back to "convert" - Our iterator `isLegacy` will be non-zero when the first command + Our iterator `im_legacy` will be non-zero when the first command fails, and will be returned in a tuple along with the version """ cmds = ('magick', 'convert') - for isLegacy, cmd in enumerate(cmds): + for im_legacy, cmd in enumerate(cmds): try: out = util.command_output([cmd, '--version']) @@ -262,7 +262,7 @@ def get_im_version(): return ((int(match.group(1)), int(match.group(2)), int(match.group(3))), - bool(isLegacy) + bool(im_legacy) ) except (subprocess.CalledProcessError, OSError) as exc: From 48be3a7eafd9534f210ff9c534d6b506127f3bc4 Mon Sep 17 00:00:00 2001 From: ababyduck Date: Wed, 24 Apr 2019 18:43:50 -0700 Subject: [PATCH 7/9] Make requested changes for PR 3236 - Refactored convert and identify command names to an ArtResizer member variable, set on ArtResizer init. Functions that use this info will now access it from there. - Changed the way `cmd` variables are written so that the command name and command args are assigned directly to `cmd`, rather than doing `command_output(cmd + args)` - `get_im_version()` will now always return a tuple containing two values: a tuple representing the version, and either a bool or None flag representing whether we should send legacy commands to ImageMagick - Improved readability of successful return value in `get_im_version()` --- beets/util/artresizer.py | 47 ++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 5e3e983867..427a5411d9 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -93,13 +93,12 @@ def im_resize(maxwidth, path_in, path_out=None): # than the given width while maintaining the aspect ratio # with regards to the height. try: - cmds = (['magick'], ['convert']) - cmd = cmds[0] if not ArtResizer.shared.im_legacy else cmds[1] - args = [util.syspath(path_in, prefix=False), + cmd = ArtResizer.shared.im_convert_cmd + \ + [util.syspath(path_in, prefix=False), '-resize', '{0}x>'.format(maxwidth), util.syspath(path_out, prefix=False)] - util.command_output(cmd + args) + util.command_output(cmd) except subprocess.CalledProcessError: log.warning(u'artresizer: IM convert failed for {0}', util.displayable_path(path_in)) @@ -124,12 +123,11 @@ def pil_getsize(path_in): def im_getsize(path_in): - cmds = (['magick', 'identify'], ['identify']) - cmd = cmds[0] if not ArtResizer.shared.im_legacy else cmds[1] - args = ['-format', '%w %h', util.syspath(path_in, prefix=False)] - try: - out = util.command_output(cmd + args) + cmd = ArtResizer.shared.im_identify_cmd + \ + ['-format', '%w %h', util.syspath(path_in, prefix=False)] + + out = util.command_output(cmd) except subprocess.CalledProcessError as exc: log.warning(u'ImageMagick size query failed') log.debug( @@ -180,6 +178,12 @@ def __init__(self): if self.method[0] == IMAGEMAGICK: self.im_legacy = self.method[2] + if self.im_legacy: + self.im_convert_cmd = ['convert'] + self.im_identify_cmd = ['identify'] + else: + self.im_convert_cmd = ['magick'] + self.im_identify_cmd = ['magick', 'identify'] def resize(self, maxwidth, path_in, path_out=None): """Manipulate an image file according to the method, returning a @@ -246,30 +250,31 @@ def get_im_version(): Try invoking ImageMagick's "magick". If "magick" is unavailable, as with older versions, fall back to "convert" - Our iterator `im_legacy` will be non-zero when the first command - fails, and will be returned in a tuple along with the version + Our iterator will be non-zero when the first command fails, and will + be returned in a tuple along with the version. """ - cmds = ('magick', 'convert') - for im_legacy, cmd in enumerate(cmds): + cmd_names = (['magick'], + ['convert']) + for i, cmd_name in enumerate(cmd_names): try: - out = util.command_output([cmd, '--version']) + cmd = cmd_name + ['--version'] + out = util.command_output(cmd) if b'imagemagick' in out.lower(): pattern = br".+ (\d+)\.(\d+)\.(\d+).*" match = re.search(pattern, out) + version = (int(match.group(1)), + int(match.group(2)), + int(match.group(3))) + legacy = bool(i) if match: - return ((int(match.group(1)), - int(match.group(2)), - int(match.group(3))), - bool(im_legacy) - ) + return (version, legacy) except (subprocess.CalledProcessError, OSError) as exc: log.debug(u'ImageMagick version check failed: {}', exc) - return None - return (0,) + return (0, None) def get_pil_version(): From 09abd9802aa337f20ba83c3a0a6155cbf964b7c4 Mon Sep 17 00:00:00 2001 From: ababyduck Date: Thu, 25 Apr 2019 05:50:11 -0700 Subject: [PATCH 8/9] Make `get_im_version()` return same types across all conditions `get_im_version` should now always return a tuple containing: - index 0: a tuple representing the version - index 1: a bool or None, representing legacy status --- beets/util/artresizer.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 427a5411d9..7ae5fd63f4 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -232,9 +232,9 @@ def _can_compare(self): def _check_method(): """Return a tuple indicating an available method and its version.""" try: - version, im_legacy = get_im_version() - if version: - return IMAGEMAGICK, version, im_legacy + version, legacy = get_im_version() + if version > (0, 0, 0): + return IMAGEMAGICK, version, legacy except TypeError: pass @@ -274,7 +274,7 @@ def get_im_version(): except (subprocess.CalledProcessError, OSError) as exc: log.debug(u'ImageMagick version check failed: {}', exc) - return (0, None) + return ((0,), None) def get_pil_version(): From 278d87f25aec3e59a197b6d1d5f8325707edab8e Mon Sep 17 00:00:00 2001 From: ababyduck Date: Thu, 25 Apr 2019 07:50:12 -0700 Subject: [PATCH 9/9] Make more requested changes for PR 3236 - Moved several variable assignments outside of try blocks - Added and clarified various comments and docstrings - Modified the command loop in `get_im_version()` to a slightly more readable approach - `get_im_version()` now returns None when ImageMagick is unavailable - Updated `ArtResizer._check_method` to handle our new returns in a way that is more readable - Fixed an issue where `get_im_version()` could crash if the regex search failed to find a match --- beets/util/artresizer.py | 67 ++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 7ae5fd63f4..5a00363761 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -82,7 +82,7 @@ def pil_resize(maxwidth, path_in, path_out=None): def im_resize(maxwidth, path_in, path_out=None): """Resize using ImageMagick's ``magick`` tool - (or fall back to ``convert`` for older versions.) + (or fall back to ``convert`` for older versions). Return the output path of resized image. """ path_out = path_out or temp_file_for(path_in) @@ -92,17 +92,18 @@ def im_resize(maxwidth, path_in, path_out=None): # "-resize WIDTHx>" shrinks images with the width larger # than the given width while maintaining the aspect ratio # with regards to the height. - try: - cmd = ArtResizer.shared.im_convert_cmd + \ - [util.syspath(path_in, prefix=False), - '-resize', '{0}x>'.format(maxwidth), - util.syspath(path_out, prefix=False)] + cmd = ArtResizer.shared.im_convert_cmd + \ + [util.syspath(path_in, prefix=False), + '-resize', '{0}x>'.format(maxwidth), + util.syspath(path_out, prefix=False)] + try: util.command_output(cmd) except subprocess.CalledProcessError: log.warning(u'artresizer: IM convert failed for {0}', util.displayable_path(path_in)) return path_in + return path_out @@ -123,10 +124,10 @@ def pil_getsize(path_in): def im_getsize(path_in): - try: - cmd = ArtResizer.shared.im_identify_cmd + \ - ['-format', '%w %h', util.syspath(path_in, prefix=False)] + cmd = ArtResizer.shared.im_identify_cmd + \ + ['-format', '%w %h', util.syspath(path_in, prefix=False)] + try: out = util.command_output(cmd) except subprocess.CalledProcessError as exc: log.warning(u'ImageMagick size query failed') @@ -176,6 +177,9 @@ def __init__(self): log.debug(u"artresizer: method is {0}", self.method) self.can_compare = self._can_compare() + # Use ImageMagick's magick binary when it's available. If it's + # not, fall back to the older, separate convert and identify + # commands. if self.method[0] == IMAGEMAGICK: self.im_legacy = self.method[2] if self.im_legacy: @@ -230,13 +234,14 @@ def _can_compare(self): @staticmethod def _check_method(): - """Return a tuple indicating an available method and its version.""" - try: - version, legacy = get_im_version() - if version > (0, 0, 0): - return IMAGEMAGICK, version, legacy - except TypeError: - pass + """Return a tuple indicating an available method and its version. + If the method is ImageMagick, also return a bool indicating whether to + use the `magick` binary or legacy utils (`convert`, `identify`, etc.) + """ + version = get_im_version() + if version: + version, legacy = version + return IMAGEMAGICK, version, legacy version = get_pil_version() if version: @@ -246,40 +251,36 @@ def _check_method(): def get_im_version(): - """Return Image Magick version or None if it is unavailable - Try invoking ImageMagick's "magick". If "magick" is unavailable, - as with older versions, fall back to "convert" + """Return ImageMagick version/legacy-flag pair or None if the check fails. - Our iterator will be non-zero when the first command fails, and will - be returned in a tuple along with the version. + Try invoking ImageMagick's `magick` binary first, then `convert` if + `magick` is unavailable. """ - cmd_names = (['magick'], - ['convert']) - for i, cmd_name in enumerate(cmd_names): + for cmd_name, legacy in ((['magick'], False), (['convert'], True)): + cmd = cmd_name + ['--version'] try: - cmd = cmd_name + ['--version'] out = util.command_output(cmd) if b'imagemagick' in out.lower(): pattern = br".+ (\d+)\.(\d+)\.(\d+).*" match = re.search(pattern, out) - version = (int(match.group(1)), - int(match.group(2)), - int(match.group(3))) - legacy = bool(i) if match: - return (version, legacy) + version = (int(match.group(1)), + int(match.group(2)), + int(match.group(3))) + return version, legacy except (subprocess.CalledProcessError, OSError) as exc: log.debug(u'ImageMagick version check failed: {}', exc) - return ((0,), None) + return None def get_pil_version(): - """Return Image Magick version or None if it is unavailable - Try importing PIL.""" + """Return Pillow version or None if it is unavailable + Try importing PIL. + """ try: __import__('PIL', fromlist=[str('Image')]) return (0,)