Skip to content

Do not run Windows' file system convert tool#5460

Closed
cgohlke wants to merge 2 commits into
matplotlib:masterfrom
cgohlke:patch-1
Closed

Do not run Windows' file system convert tool#5460
cgohlke wants to merge 2 commits into
matplotlib:masterfrom
cgohlke:patch-1

Conversation

@cgohlke

@cgohlke cgohlke commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

See also #5446.
This should fix two test errors on Windows when ImageMagick is not set up correctly:

======================================================================
ERROR: matplotlib.tests.test_animation.test_save_animation_smoketest('imagemagick', 'gif')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\aroot\stage\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "C:\aroot\stage\lib\site-packages\matplotlib\testing\decorators.py", line 118, in wrapped_function
    func(*args, **kwargs)
  File "C:\aroot\stage\lib\site-packages\matplotlib\tests\test_animation.py", line 57, in check_save_animation
    anim.save(F.name, fps=30, writer=writer, bitrate=500)
  File "C:\aroot\stage\lib\site-packages\matplotlib\animation.py", line 780, in save
    writer.grab_frame(**savefig_kwargs)
  File "C:\aroot\stage\lib\site-packages\matplotlib\animation.py", line 225, in grab_frame
    dpi=self.dpi, **savefig_kwargs)
  File "C:\aroot\stage\lib\site-packages\matplotlib\figure.py", line 1539, in savefig
    self.canvas.print_figure(*args, **kwargs)
  File "C:\aroot\stage\lib\site-packages\matplotlib\backend_bases.py", line 2230, in print_figure
    **kwargs)
  File "C:\aroot\stage\lib\site-packages\matplotlib\backends\backend_agg.py", line 519, in print_raw
    fileobj.write(renderer._renderer.buffer_rgba())
OSError: [Errno 22] Invalid argument

======================================================================
ERROR: matplotlib.tests.test_animation.test_save_animation_smoketest('imagemagick_file', 'gif')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\aroot\stage\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "C:\aroot\stage\lib\site-packages\matplotlib\testing\decorators.py", line 118, in wrapped_function
    func(*args, **kwargs)
  File "C:\aroot\stage\lib\site-packages\matplotlib\tests\test_animation.py", line 57, in check_save_animation
    anim.save(F.name, fps=30, writer=writer, bitrate=500)
  File "C:\aroot\stage\lib\site-packages\matplotlib\animation.py", line 780, in save
    writer.grab_frame(**savefig_kwargs)
  File "C:\aroot\stage\lib\contextlib.py", line 66, in __exit__
    next(self.gen)
  File "C:\aroot\stage\lib\site-packages\matplotlib\animation.py", line 191, in saving
    self.finish()
  File "C:\aroot\stage\lib\site-packages\matplotlib\animation.py", line 382, in finish
    + ' Try running with --verbose-debug')
RuntimeError: Error creating movie, return code: 4 Try running with --verbose-debug

@dopplershift

dopplershift commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

It would seem better to me to put this in the Imagemagick-specific classes rather than MovieWriter, but I'm not passionate about it.

@jenshnielsen

jenshnielsen commented Nov 11, 2015

Copy link
Copy Markdown
Member

@dopplershift I don't feel strongly about this but I guess that in principle another movie writer could have a tool called convert? I was thinking about adding support for graphicsmagick which is a fork of imagemagick (what is it with movie tools and forks?). In that case the command line argument has changed to gm convert

@tacaswell tacaswell added this to the next bug fix release (2.0.1) milestone Nov 11, 2015
@cgohlke

cgohlke commented Nov 13, 2015

Copy link
Copy Markdown
Contributor Author

The path to ImageMagick's convert tool could be read from the Windows Registry if the full path is not already specified in rcParams. The multiple inheritance does not make this easy. The simplest I could come up with is to update rcParams and rcParamsDefault in animation.py. Is that an option?

class ImageMagickBase(object):
    exec_key = 'animation.convert_path'
    args_key = 'animation.convert_args'

    @property
    def delay(self):
        return 100. / self.fps

    @property
    def output_args(self):
        return [self.outfile]

    @classmethod
    def _init_from_registry(cls):
        if sys.platform != 'win32' or rcParams[cls.exec_key] != 'convert':
            return
        from matplotlib.externals.six.moves import winreg
        try:
            hkey = winreg.OpenKeyEx(winreg.HKEY_LOCAL_MACHINE,
                                    'Software\\Imagemagick\\Current',
                                    0, winreg.KEY_QUERY_VALUE)
            binpath = winreg.QueryValueEx(hkey, 'BinPath')[0]
            winreg.CloseKey(hkey)
            binpath += '\\convert.exe'
        except Exception:
            binpath = ''
        rcParams[cls.exec_key] = rcParamsDefault[cls.exec_key] = binpath


ImageMagickBase._init_from_registry()

The isAvailable function could then check that rcParams[cls.exec_key] is not empty.

@tacaswell

tacaswell commented Nov 24, 2015

Copy link
Copy Markdown
Member

@cgohlke 👍 I see no problem with that solution. I assume it works on the subset of windows versions you think need to be supported?

@cgohlke

cgohlke commented Nov 24, 2015

Copy link
Copy Markdown
Contributor Author

I still have to check/implement that the 64-bit version of matplotlib can detect a 32-bit installation of ImageMagick and the 32-bit version of matplotlib can detect a 64-bit installation of ImageMagick. I guess the current code does not.

@QuLogic

QuLogic commented Nov 25, 2015

Copy link
Copy Markdown
Member

Is that really necessary? It's just communicating over a pipe, isn't it?

@cgohlke

cgohlke commented Nov 25, 2015

Copy link
Copy Markdown
Contributor Author

I was referring to the code detecting the ImageMagick location via registry settings. It needs revision for handling 32 and 64 bit combinations of Python and ImageMagick because of the way Windows stores 32-bit and 64-bit Application Data in the Registry

@QuLogic

QuLogic commented Nov 25, 2015

Copy link
Copy Markdown
Member

Ah, I see. ImageMagick's page on the registry keys doesn't yet mention that little hiccup.

@cgohlke cgohlke closed this Nov 26, 2015
@cgohlke cgohlke reopened this Nov 26, 2015
This was referenced Dec 2, 2015
Closed
Closed
@jankatins jankatins mentioned this pull request Dec 3, 2015
Merged
14 tasks
@QuLogic

QuLogic commented Dec 4, 2015

Copy link
Copy Markdown
Member

I'm not sure whether this PR was totally finished, but it got pulled in via #5604. It will need a backport if this milestone is still correct.

@tacaswell

tacaswell commented Dec 4, 2015

Copy link
Copy Markdown
Member

hmm, this will be a fun one to backport. It got auto-closed because these commits went in as part of #5604

@cgohlke the code in the patch is slightly different than what you posted in the comments. It looks like it copes with the 32 vs 64 registry issues, but I do not know enough about windows to be sure (on the other hand, it seems to be working on appveyor so that is a good sign right?)

@cgohlke

cgohlke commented Dec 4, 2015

Copy link
Copy Markdown
Contributor Author

Yes, this PR is totally finished. Appveyor only tests without ImageMagick.

@cgohlke cgohlke deleted the patch-1 branch December 4, 2015 06:30
tacaswell pushed a commit that referenced this pull request Jan 2, 2016
@cgohlke @tacaswell
Manual backport of #5460 to v1.5.x
2e9ff44 
This commit squashes

 87c968b
 a30ec66

and cherry-picks them to the v1.5.x branch.  These commits were merged
to master as part of #5604 which was not backported en-mass.

Original commit messages:

 Do not run Windows' file system convert tool
 On Windows, initialize the animation.convert_path setting from the Registry
@tacaswell

tacaswell commented Jan 2, 2016

Copy link
Copy Markdown
Member

This was backported to 1.5.x as 2e9ff44

@QuLogic QuLogic modified the milestones: Critical bugfix release (1.5.1), next bug fix release (2.0.1) Jan 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

OS: Microsoft

Projects

None yet

Milestone

v1.5.1

Development

Successfully merging this pull request may close these issues.

5 participants

@cgohlke @dopplershift @jenshnielsen @tacaswell @QuLogic