New ImageMagick params for im_getsize()

[Tstassin]

Prevents im_getsize from crashing when using ImageMagick native package on Synology NAS

Issue #2121

[sampsyo]

Interesting; thank you for discovering the problem and investigating!

There's a downside to this exact fix: it can, unfortunately, decrease the chance of success on Windows. See #2093—the summary is that convert gets shadowed by a built-in command. In the end, then, I don't know what the right path is here. One breaks partial IM installs like yours, and the other breaks normal IM installs on Windows. I think the only right solution may be to make these commands configurable.

[Tstassin]

@sampsyo We could detect Windows (or Linux) installations using platform.system()and activate the right command regarding our result ?
We have a bug with Windows (convert command overriding) and a bug with Synology NAS devices (stripped down ImageMagick kit), we could address both easily.

[sampsyo]

Sure, good point—an explicit platform check could be an OK shortcut without adding a full configuration option.

[ghost]

We are going to have to do something a little different from the way we do it now in any case.

Compare http://packages.ubuntu.com/trusty/amd64/imagemagick/filelist to http://packages.ubuntu.com/precise/amd64/imagemagick/filelist

You'll see that they now use an .im6 suffix. This is likely due to the lack of package namespacing on standard Linux distros and imagemagick is grabby with a bunch of common names

[sampsyo]

That's rather frustrating too—but it's another argument that we should just make the path configurable, if there really is a substantial diversity of names where IM can be installed.

[ghost]

I think we should first go with a descending priority list (like bash completion). It should cover most cases. I do think there's a case for configurable path though. It'd be easy to imagine a situation where beets could otherwise run, but the packaged imagemagick version can't handle what we want.

[jackwilsdon]

Something we could try out would be a "lookup" table, where we have a table with a list of commands to try and the expected output. We would try them in order to work out what the real binary is, e.g.

IMAGEMAGICK_LOOKUP = [
    {
        'command': 'identify',
        'check': {
            'command': 'identify --version',
            'output_match': '^Version: ImageMagick'
        }
    },
    {
        'command': 'convert -identify',
        'check': {
            'command': 'convert --version',
            'output_match': '^Version: ImageMagick'
        }
    },
    {
        'command': 'identify.im6',
        'check': {
            'command': 'identify.im6 --version',
            'output_match': '^Version: ImageMagick'
        }
    },
    {
        'command': 'convert.im6 -identify',
        'check': {
            'command': 'convert.im6 --version',
            'output_match': '^Version: ImageMagick'
        }
    }
]

We'd then iterate each check property and run the command, matching the output to output_match as a regular expression. If the output matches, we use the original command property (the one before check, not inside check). It's not exactly fast but it seems like it might do what we want and be a more robust method of checking?

[sampsyo]

Yeah! That would be a great way to make the adaptation extensible. We could do this series of tests in place of the current simple existence check.

[ghost]

as long as we memoize it, it should be ok.

[Tstassin]

Looks good to me

[ghost]

@sampsyo: it looks like imagemagick 7 now skips with a magick command that we can use in place of convert, and perhaps other friends. I haven't yet tried it myself though.

[ghost]

So, i talked to some folks who actually use Debian and learned that the .im6 suffixed stuff is already handled via the alternatives command when installed. We shouldn't have to worry unless we don't actually work with graphicsmagick when installed, as that's the only other provider of the command that I know of.

[sampsyo]

Good to know; thanks for investigating. It would be good to eventually transition to that magick command, which looks like a much more sane solution than the panoply of generically-named tools.

[ghost]

@sampsyo : we'll be stuck with convert and friends for awhile though! If we can provide a platform specific fix for windows, then I think we should be able to rely on convert -identify everywhere else. The we can look for magick if convert doesn't exist.

[sampsyo]

Yep! I agree completely.

[ghost]

so instead of the full path, we could ask the windows registry!!

http://www.imagemagick.org/Usage/windows/#convert_issue

ImageMagick writes a few Registry keys to HKEY Local Machine\Software\ImageMagick. If you do deal with several installed versions of IM, the most important key is HKEY Local Machine\Software\ImageMagick\Current, where you can also find the path to IM's binarys, called BinPath.

[ghost]

We use these tools in all sorts of places.. I'm a little confused as to how and where we can centralize these imagemagick binary lookups and still provide the full path option.

should it be in art.py?

[sampsyo]

Yep, an abstraction layer in the art module seems right.

Sorry for terse responses lately! I swear I have a good excuse...

[jtpavlock]

@Tstassin I know it's been some time since you opened this pull request, but do you still intend to continue work on it?

[Tstassin]

@Tstassin I know it's been some time since you opened this pull request, but do you still intend to continue work on it?

I don't remember what I changed or if this has been corrected since then, but it seems to work now for me and I don't intend to work anymore on it.

[wisp3rwind]

I guess that this is fixed by #3236