8000 Reset the available animation movie writer on rcParam change by jankatins · Pull Request #5628 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Reset the available animation movie writer on rcParam change #5628

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

Merged
merged 7 commits into from
Feb 7, 2016
Merged

Reset the available animation movie writer on rcParam change #5628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e3e2be9
Reset the available animation movie writer on rcParam change
jankatins Dec 6, 2015
6a01829
TST: Use a better command for annimation tests
jankatins Dec 9, 2015
6c6cf91
Fix: rerun check for imagemagick in isAvailable
jankatins Feb 7, 2016
96b439a
fix spelling
jankatins Feb 7, 2016
9b61b34
FIX: add debugging output when the MovieWriter fails
jankatins Feb 7, 2016
33f564a
Pep8 fixes
jankatins Feb 7, 2016
e783867
Add comment to explain the fix in ImageMagickBase
jankatins Feb 7, 2016
File filter

Filter by extension

Filter by extension
8000
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 61 additions & 7 deletions lib/matplotlib/animation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@
class MovieWriterRegistry(object):
def __init__(self):
self.avail = dict()
self._registered = dict()
self._dirty = False

def set_dirty(self):
"""Sets a flag to re-setup the writers"""
self._dirty = True

# Returns a decorator that can be used on classes to register them under
# a name. As in:
Expand All @@ -72,19 +78,36 @@ def __init__(self):
# pass
def register(self, name):
def wrapper(writerClass):
self._registered[name] = writerClass
if writerClass.isAvailable():
self.avail[name] = writerClass
return writerClass
return wrapper

def ensure_not_dirty(self):
"""If dirty, reasks the writers if they are available"""
if self._dirty:
self.reset_available_writers()

def reset_available_writers(self):
"""Reset the available state of all registered writers"""
self.avail = {}
for name, writerClass in self._registered.items():
if writerClass.isAvailable():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: should that be "is_available"? It's the only method in that class(es) which use CamelCase instead of underscores...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is annoying, but I don't think worth breaking user code over.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At most, we can rename and add an alias isAvailable -> is_available with the @deprecated decorator.

self.avail[name] = writerClass
self._dirty = False

def list(self):
''' Get a list of available MovieWriters.'''
self.ensure_not_dirty()
return list(self.avail.keys())

def is_available(self, name):
self.ensure_not_dirty()
return name in self.avail

def __getitem__(self, name):
self.ensure_not_dirty()
if not self.avail:
raise RuntimeError("No MovieWriters available!")
return self.avail[name]
Expand Down Expand Up @@ -315,10 +338,11 @@ def isAvailable(cls):
Check to see if a MovieWriter subclass is actually available by
running the commandline tool.
'''
if not cls.bin_path():
bin_path = cls.bin_path()
if not bin_path:
return False
try:
p = subprocess.Popen(cls.bin_path(),
p = subprocess.Popen(bin_path,
shell=False,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
Expand Down Expand Up @@ -432,9 +456,19 @@ def finish(self):
# Check error code for creating file here, since we just run
# the process here, rather than having an open pipe.
if self._proc.returncode:
raise RuntimeError('Error creating movie, return code: '
+ str(self._proc.returncode)
+ ' Try running with --verbose-debug')
try:
stdout = [s.decode() for s in self._proc._stdout_buff]
stderr = [s.decode() for s in self._proc._stderr_buff]
verbose.report("MovieWriter.finish: stdout: %s" % stdout,
level='helpful')
verbose.report("MovieWriter.finish: stderr: %s" % stderr,
level='helpful')
except Exception as e:
pass
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not my finest code, but it works on py35...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to provide improved debugging feed back right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, when the called converter fails and the user adds the mpl.verbose.set_level("helpful") before calling the failing method again...

Unfortunately, it doesn't work for the unittests unless I add such a set_level call in the unittest file :-(

msg = ('Error creating movie, return code: ' +
str(self._proc.returncode) +
' Try setting mpl.verbose.set_level("helpful")')
raise RuntimeError(msg)

def cleanup(self):
MovieWriter.cleanup(self)
Expand Down Expand Up @@ -619,12 +653,28 @@ def _init_from_registry(cls):
binpath = ''
rcParams[cls.exec_key] = rcParamsDefault[cls.exec_key] = binpath

@classmethod
def isAvailable(cls):
'''
Check to see if a ImageMagickWriter is actually available

Done by first checking the windows registry (if applicable) and then
running the commandline tool.
'''
bin_path = cls.bin_path()
if bin_path == "convert":
cls._init_from_registry()
return super(ImageMagickBase, cls).isAvailable()

ImageMagickBase._init_from_registry()


# Note: the base classes need to be in that order to get
# isAvailable() from ImageMagickBase called and not the
# one from MovieWriter. The latter is then called by the
# former.
@writers.register('imagemagick')
class ImageMagickWriter(MovieWriter, ImageMagickBase):
class ImageMagickWriter(ImageMagickBase, MovieWriter):
def _args(self):
return ([self.bin_path(),
'-size', '%ix%i' % self.frame_size, '-depth', '8',
Expand All @@ -633,8 +683,12 @@ def _args(self):
+ self.output_args)


# Note: the base classes need to be in that order to get
# isAvailable() from ImageMagickBase called and not the
# one from MovieWriter. The latter is then called by the
# former.
@writers.register('imagemagick_file')
class ImageMagickFileWriter(FileMovieWriter, ImageMagickBase):
class ImageMagickFileWriter(ImageMagickBase, FileMovieWriter):
supported_formats = ['png', 'jpeg', 'ppm', 'tiff', 'sgi', 'bmp',
'pbm', 'raw', 'rgba']

Expand Down
23 changes: 19 additions & 4 deletions lib/matplotlib/rcsetup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,21 @@ def validate_hist_bins(s):
raise ValueError("'hist.bins' must be 'auto', an int or " +
"a sequence of floats")

def validate_animation_writer_path(p):
# Make sure it's a string and then figure out if the animations
# are already loaded and reset the writers (which will validate
# the path on next call)
if not isinstance(p, six.text_type):
raise ValueError("path must be a (unicode) string")
from sys import modules
# set dirty, so that the next call to the registry will re-evaluate
# the state.
# only set dirty if already loaded. If not loaded, the load will
# trigger the checks.
if "matplotlib.animation" in modules:
modules["matplotlib.animation"].writers.set_dirty()
return p


# a map from key -> value, converter
defaultParams = {
Expand Down Expand Up @@ -1222,20 +1237,20 @@ def validate_hist_bins(s):
# Controls image format when frames are written to disk
'animation.frame_format': ['png', validate_movie_frame_fmt],
# Path to FFMPEG binary. If just binary name, subprocess uses $PATH.
'animation.ffmpeg_path': ['ffmpeg', six.text_type],
'animation.ffmpeg_path': ['ffmpeg', validate_animation_writer_path],

# Additional arguments for ffmpeg movie writer (using pipes)
'animation.ffmpeg_args': [[], validate_stringlist],
# Path to AVConv binary. If just binary name, subprocess uses $PATH.
'animation.avconv_path': ['avconv', six.text_type],
'animation.avconv_path': ['avconv', validate_animation_writer_path],
# Additional arguments for avconv movie writer (using pipes)
'animation.avconv_args': [[], validate_stringlist],
# Path to MENCODER binary. If just binary name, subprocess uses $PATH.
'animation.mencoder_path': ['mencoder', six.text_type],
'animation.mencoder_path': ['mencoder', validate_animation_writer_path],
# Additional arguments for mencoder movie writer (using pipes)
'animation.mencoder_args': [[], validate_stringlist],
# Path to convert binary. If just binary name, subprocess uses $PATH
'animation.convert_path': ['convert', six.text_type],
'animation.convert_path': ['convert', validate_animation_writer_path],
# Additional arguments for mencoder movie writer (using pipes)

'animation.convert_args': [[], validate_stringlist],
Expand Down
27 changes: 27 additions & 0 deletions lib/matplotlib/tests/test_animation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@
from matplotlib.externals import six

import os
import sys
import tempfile
import numpy as np
from numpy.testing import assert_equal
from nose import with_setup
from nose.tools import assert_false, assert_true
import matplotlib as mpl
from matplotlib import pyplot as plt
from matplotlib import animation
from matplotlib.testing.noseclasses import KnownFailureTest
Expand Down Expand Up @@ -163,6 +166,30 @@ def animate(i):
frames=iter(range(5)))


def test_movie_writer_registry():
ffmpeg_path = mpl.rcParams['animation.ffmpeg_path']
# Not sure about the first state as there could be some writer
# which set rcparams
#assert_false(animation.writers._dirty)
assert_true(len(animation.writers._registered) > 0)
animation.writers.list() # resets dirty state
assert_false(animation.writers._dirty)
mpl.rcParams['animation.ffmpeg_path'] = u"not_available_ever_xxxx"
assert_true(animation.writers._dirty)
animation.writers.list() # resets
assert_false(animation.writers._dirty)
assert_false(animation.writers.is_available("ffmpeg"))
# something which is guaranteed to be available in path
# and exits immediately
bin = u"true" if sys.platform != 'win32' else u"where"
mpl.rcParams['animation.ffmpeg_path'] = bin
assert_true(animation.writers._dirty)
animation.writers.list() # resets
assert_false(animation.writers._dirty)
assert_true(animation.writers.is_available("ffmpeg"))
mpl.rcParams['animation.ffmpeg_path'] = ffmpeg_path


if __name__ == "__main__":
import nose
nose.runmodule(argv=['-s', '--with-doctest'], exit=False)
0