ENH: add AVConv movie writer for animations #1536

Merged
merged 5 commits into from Dec 7, 2012

Projects

None yet

6 participants

@jakevdp
jakevdp commented Nov 27, 2012

This PR creates a separate MovieWriter interface for avconv. AVConv is a fork that has diverged from ffmpeg and been renamed, though retains a very similar interface.

Technically users could probably just set ffmpeg_path to 'avconv' in the rc file (the interface for the two is nearly identical), but I think this solution is cleaner and less confusing for the user. It will also by default find avconv on systems where ffmpeg is unavailable.

@jenshnielsen jenshnielsen commented on an outdated diff Nov 27, 2012
matplotlibrc.template
@@ -434,7 +434,11 @@ text.hinting_factor : 8 # Specifies the amount of softness for hinting in the
#animation.frame_format: 'png' # Controls frame format used by temp files
#animation.ffmpeg_path: 'ffmpeg' # Path to ffmpeg binary. Without full path
# $PATH is searched
-#animation.ffmpeg_args: '' # Additional arugments to pass to mencoder
-#animation.mencoder_path: 'ffmpeg' # Path to mencoder binary. Without full path
+#animation.ffmpeg_args: '' # Additional arugments to pass to ffmpeg
+#animation.avconv_path: 'avconv' # Path to ffmpeg binary. Without full path
@jenshnielsen
jenshnielsen Nov 27, 2012 Matplotlib Developers member

Shouldn't this be av_conv not ffmpeg ? (Also below)

@jenshnielsen
Member

Looks like the right solution to me. I did not test it yet but have one single documentation comment above

@pelson pelson commented on an outdated diff Nov 27, 2012
lib/matplotlib/animation.py
@@ -390,6 +390,57 @@ def _args(self):
self._base_temp_name()] + self.output_args
+# Base class of avconv information. Has the config keys and the common set
+# of arguments that controls the *output* side of things.
+class AVConvBase:
@pelson
pelson Nov 27, 2012 Matplotlib Developers member

Could this not just inherit from FFMegBase - or make a real baseclass-mixin that is the superclass of both this and FFMegBase?

@pelson pelson commented on an outdated diff Nov 27, 2012
lib/matplotlib/animation.py
+ # The %dk adds 'k' as a suffix so that avconv treats our bitrate as in
+ # kbps
+ args = ['-vcodec', self.codec]
+ if self.bitrate > 0:
+ args.extend(['-b', '%dk' % self.bitrate])
+ if self.extra_args:
+ args.extend(self.extra_args)
+ for k, v in self.metadata.items():
+ args.extend(['-metadata', '%s=%s' % (k, v)])
+
+ return args + ['-y', self.outfile]
+
+
+# Combine AVConv options with pipe-based writing
+@writers.register('avconv')
+class AVConvWriter(MovieWriter, AVConvBase):
@pelson
pelson Nov 27, 2012 Matplotlib Developers member

Same goes for this class. It is identical to the FFMpegWriter save the registered name and the class name. A simpler inheritance scheme could reduce the redundancy here too.

@pelson pelson commented on an outdated diff Nov 27, 2012
lib/matplotlib/animation.py
+ def _args(self):
+ # Returns the command line parameters for subprocess to use
+ # avconv to create a movie using a pipe.
+ args = [self.bin_path(), '-f', 'rawvideo', '-vcodec', 'rawvideo',
+ '-s', '%dx%d' % self.frame_size, '-pix_fmt', self.frame_format,
+ '-r', str(self.fps)]
+ # Logging is quieted because subprocess.PIPE has limited buffer size.
+ if not verbose.ge('debug'):
+ args += ['-loglevel', 'quiet']
+ args += ['-i', 'pipe:'] + self.output_args
+ return args
+
+
+#Combine AVConv options with temp file-based writing
+@writers.register('avconv_file')
+class AVConvFileWriter(FileMovieWriter, AVConvBase):
@pelson
pelson Nov 27, 2012 Matplotlib Developers member

As above.

@pelson
Member
pelson commented Nov 27, 2012

Nice addition - thanks @jakevdp. I think this is a good idea, my biggest concern comes from the fact that we would now need to remember to update both the FFMpeg* and the AVConv* classes at the same time and so I therefore think that improving the class inheritance scheme here (just for the AVConv and FFMpeg writers) would mean we could reduce that redundancy and still provide distinct FFMpeg and AVConv writers.
What makes me even more nervous with having to maintain this redundancy is that there are no tests for this functionality (not to be resolved by this PR as I think that is a hard problem).

Please feel free to disagree with me about the class structuring - I think debating these issues is important and I could definitely be talked out of my assertions by a convincing argument 😄.

Cheers,

Phil

@mdboom
Member
mdboom commented Nov 27, 2012

+1 to this improvement, and I agree with @pelson's suggestions.

As for testing -- at a minimum, it would be nice to be able to generate a short movie and at least assert (using an external tool, perhaps) that it has the right number of frames. It also would have to be marked skipped if the platform doesn't have the necessary third-party tools -- the Travis test environment, for example, probably won't have them.

@jakevdp
jakevdp commented Nov 27, 2012

Hi - thanks for the comments. I thought about using the inheritance structure, but remembered some CS prof who drilled into us that inheritance is not about reducing code duplication, but about encoding relationships. So deriving AVConv from FFMpeg is arguably not good OO practice...

That being said, I'd be happy to change it to ease maintenance 😁

@jakevdp
jakevdp commented Nov 28, 2012

If we're going to play with the class inheritance structure, I'd also think about using a metaclass rather than a "writers.register" decorator for each of the MovieWriter objects. I think it would be a cleaner implementation.

@jakevdp
jakevdp commented Nov 28, 2012

I added a quick smoke test for the various writers. It raises a KnownFailure error if the writer is not available on the system running the tests.

Eventually it would be nice to actually do a frame-by-frame comparison to a known result, but that's probably for another day.

Any more thoughts about inheritance structure?

@pelson pelson and 1 other commented on an outdated diff Nov 29, 2012
lib/matplotlib/tests/test_animation.py
+ def init():
+ line.set_data([], [])
+ return line,
+
+ def animate(i):
+ x = np.linspace(0, 10, 100)
+ y = np.sin(x + i)
+ line.set_data(x, y)
+ return line,
+
+ fid, fname = tempfile.mkstemp(suffix=extension)
+
+ anim = animation.FuncAnimation(fig, animate, init_func=init, frames=5)
+ anim.save(fname, fps=30, writer=writer)
+
+ os.remove(fname)
@pelson
pelson Nov 29, 2012 Matplotlib Developers member

probably best to wrap this in a:

try:
   # do animation
   ...
except:
   os.remove(fname)
   raise

So the temp file always gets nuked.

@WeatherGod
WeatherGod Nov 29, 2012 Matplotlib Developers member

Or, one could use tempfile.NamedTemporaryFile instead (get the filename from the "name" attribute of the file object).

@pelson
Member
pelson commented Nov 29, 2012

Any more thoughts about inheritance structure?

I'm less concerned about the duplication now that we have some tests. If your not keen to implement, I wouldn't want to hold up the PR.

On that basis, unless you want to look at the class structure (which is probably best done in a separate PR anyway) 👍 from me.

@jenshnielsen jenshnielsen commented on an outdated diff Nov 29, 2012
matplotlibrc.template
@@ -434,7 +434,11 @@ text.hinting_factor : 8 # Specifies the amount of softness for hinting in the
#animation.frame_format: 'png' # Controls frame format used by temp files
#animation.ffmpeg_path: 'ffmpeg' # Path to ffmpeg binary. Without full path
# $PATH is searched
-#animation.ffmpeg_args: '' # Additional arugments to pass to mencoder
-#animation.mencoder_path: 'ffmpeg' # Path to mencoder binary. Without full path
+#animation.ffmpeg_args: '' # Additional arugments to pass to ffmpeg
+#animation.avconv_path: 'avconv' # Path to avconv binary. Without full path
+ # $PATH is searched
+#animation.avconv_args: '' # Additional arugments to pass to ffmpeg
@jenshnielsen
jenshnielsen Nov 29, 2012 Matplotlib Developers member

arugments -> arguments This appears to be a typo inherited from the ffmpeg case above sine the same is in the lines above.
ffmpeg should also be changed to avconv here in the comment.

@WeatherGod WeatherGod commented on an outdated diff Nov 29, 2012
lib/matplotlib/tests/test_animation.py
+import numpy as np
+from matplotlib import pyplot as plt
+from matplotlib import animation
+from matplotlib.testing.noseclasses import KnownFailureTest
+
+
+# Smoke test for saving animations. In the future, we should probably
+# design more sophisticated tests which compare resulting frames a-la
+# matplotlib.testing.image_comparison
+def test_save_animation_smoketest():
+ writers = ['ffmpeg', 'ffmpeg_file',
+ 'mencoder', 'mencoder_file',
+ 'avconv', 'avconv_file',
+ 'imagemagick', 'imagemagick_file']
+
+ for writer in writers:
@WeatherGod
WeatherGod Nov 29, 2012 Matplotlib Developers member

Maybe each writer should have a default extension?

@jakevdp
jakevdp commented Nov 29, 2012

I implemented all the changes discussed above. I ended up just deriving AVConv* from the corresponding FFMpeg* classes: I think this makes the most sense (and will allow for flexibility if/when avconv further diverges from ffmpeg). Also fixed typos and cleaned up the tests. Let me know if anything else looks awry!

@pelson
Member
pelson commented Nov 29, 2012

👍 - I'd be happy for this to be merged. @mdboom / @WeatherGod - feel free to do the honours when your happy.

Thanks @jakevdp

@dopplershift
Collaborator

Looks good to me. I definitely think the current way things are added is feeling a bit awkward. That can come later and shouldn't hold this up. Thanks @jakevdp .

@dopplershift dopplershift merged commit 7924451 into matplotlib:master Dec 7, 2012

1 check failed

Details default The Travis build failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment