8000 Do not run Windows' file system convert tool by cgohlke · Pull Request #5460 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Do not run Windows' file system convert tool #5460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Do not run Windows' file system convert tool #5460

wants to merge 2 commits into from

Conversation

cgohlke
Copy link
Contributor
@cgohlke cgohlke commented Nov 10, 2015

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
Copy link
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
Copy link
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
Copy link
Contributor Author
cgohlke commented Nov 13, 2015

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
Copy link
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
Copy link
Contributor Author
cgohlke commented Nov 24, 2015

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
Copy link
Member
QuLogic commented Nov 25, 2015

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

@cgohlke
Copy link
Contributor Author
cgohlke commented Nov 25, 2015

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
Copy link
Member
QuLogic commented Nov 25, 2015

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
Copy link
Member
QuLogic commented Dec 4, 2015

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
Copy link
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
Copy link
Contributor Author
cgohlke commented Dec 4, 2015

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
Copy link
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
0