Animation fixes #1012

jdh2358 · 2012-07-15T17:55:05Z

This PR fixes a few problems with the new animation write functionality.

The first commit addresses the problem that there was no way to get
the args to setup that you may want to customize. The
MovieWriter.setup method only gets called Animation.save in the "with
writer.saving" call, but there is no way to pass extra args through to
setup like "clear_temp=False" to the FileMovieWriter.

To fix this, I added an optional dictionary argument to Animation.save
called "writer_setup_kwargs" that get passed through to "saving" and
ultimately "setup", so you can use it like:

im_ani.save('movies/im1.mp4', writer=writer,
  writer_setup_kwargs=dict(clear_temp=False))

The second commit fixes a serious problem in the that the args and
kwargs passed to the writer were being dropped on the floor. This
ultimately is another example of why multiple inheritance is
dangerous. Effectively, what we have in master is:

class MovieWriter:
    def __init__(self, name='jdh'):
        self.name = 'jdh'

class FFMpegBase:
    # no init method
    def getx(self):
        return 1

class FFMpegWriter(MovieWriter, FFMpegBase):
    pass

mixin = FFMpegWriter(name='tom')
print mixin.name

which when you test this you will see that "mixin.name" still gets the
default value for name="jdh" because the FFMpegWriter did not define
an init method and pass the args to the proper base class Moviewriter.
The second commit fixes this. However, I strongly advise @dopplershift to
reconsider the design and get rid of multiple inheritance. There is
just not enough advantage to overweigh the dangers, as we see here.

Here is a real-world example using the existing animation code that shows the problem:

writer = animation.FFMpegFileWriter(fps=15, codec='webm', bitrate=1800)
print 'CODEC', writer.codec
print 'FPS', writer.fps

The third commit adds information to the docstrings for Animation.save to
point out that the fps, bitrate and codec args are only utilized if
the writer is a string; if the writer is a class instance they are
ignored in the current code. We may want to change this to set them
on the writer unconditionally (I actually favor this) but in this
commit I just clarify the current behavior. I also added support for
the special codec 'None'. We use None to mean "use the default codec"
but there is at least one case when you want to force the codec to not
be included on the command line, and that is when you are using a
special -f arg to ffmpeg. Eg, in order to get webm movies, I need a
line like:

ffmpeg -r 15 -i _tmp%04d.png -f webm  -y im1.webm

which I could do if I could get the writer to drop the 'vcodec' arg and add the -f arg using:

writer = Writer(fps=15, codec='None', extra_args=['-f', 'webm'])

In the fourth commit, I added support for the ipython display protocol so animations made in the notebook would display a movie window in the browser.

In the fifth commit, I fixed the basic_animation_writer.py example so
that we woud not be creatig a huge list of ax.colleciotns with every
call to pcolor. The new loop looks like:

for add in np.arange(15):
    ax.cla() # clear the last frame
    im = ax.pcolor(x, y, base + add, norm=plt.Normalize(0, 30))
    ims.append([im])

and the cla call prevents pcolor from creating 15 different
collections.

The final commit adds a commented out example to the basic_animation_writer.py example that shows how to create webm movies using ffmpeg. These are supported by chrome and firefox by default, so work using the embedded html ipython display protocol with most stock browsers.

… init of the FFMpegWriter and friends

… only work for web movies in some browsers

…ileup of ax.collections

…writer

pelson · 2012-07-16T16:12:10Z

lib/matplotlib/animation.py

@@ -135,7 +137,7 @@ def frame_size(self):
        width_inches, height_inches = self.fig.get_size_inches()
        return width_inches * self.dpi, height_inches * self.dpi

-    def setup(self, fig, outfile, dpi, *args):
+    def setup(self, fig, outfile, dpi, *args, **kwargs):


In this method, both args and kwargs are ignored. Do they need to be there?

I agree. Is there any point to have it this way? My best guess is to purposely swallow extra args from a call to this function in a sub-classed animator, but I don't know if that would be a good idea.

Note, I don't have a problem with the use of _args and *_kwargs in the saving() method below, just that given that setup() is acting as the terminus of the call tree should probably at least warn that there were extra args. Case-in-point, misspelled args to savefig() never get noticed.

pelson · 2012-07-16T16:19:53Z

I've not gone through the technical implications, but based on code style/readability this gets my +1.

EDIT: I guess there is one exception to that statement: it would be nice to have some form of a test, but I don't know if we have animation tests at this stage.

jdh2358 · 2012-07-16T16:24:49Z

lib/matplotlib/animation.py

+        # for normal users the class will just look like the filename
+        # when returned.  But we define the custom ipython html
+        # display hook to embed HTML5 video for others, but only if
+        # webm is requested because the browsers do not currently


Note to self: this comment is not correct. Initially I was just going to do this for webm, but there are plugins for chrome at least that support mpeg4, so I am returning the EmbedHTML for all filetypes and leave it to the nb user to try and add support to their browser. Just need to clan up the comment

dopplershift · 2012-07-16T16:38:11Z

I should get around to looking at it this evening. I've got a long list now of Animation and co. fixes to look at this week at SciPy 2012.

WeatherGod · 2012-07-18T20:03:55Z

lib/matplotlib/animation.py

-        codec: string or None, optional
-            The codec to use. If None (the default) the setting in the
-            rcParam `animation.codec` is used.
+        codec: string or None, optional The codec to use. If None (the


I think it was intended that "The codec to use" was supposed to be on the next line.

dopplershift · 2012-07-21T17:32:11Z

Overall, these look like good changes when all of the comments are addressed.

Regarding the multiple-inheritance and class writer attributes, I would support changes for both. I'd consider doing them myself, but I'll be doing minimal development for the next few months. If nobody picks them up by then, I'll take a look.

pelson · 2012-08-19T17:21:39Z

@jdh2358 is unlikely to be able to pick this up (John, feel free to correct me). @dopplershift: Are you willing to take the baton?

dopplershift · 2012-08-20T02:32:54Z

I'm willing, but time could be an issue. Is there a time table for when this needs to be merged by?

pelson · 2012-08-20T06:27:40Z

@dopplershift: Well, sadly the 1.2.x freeze is today. I'm sorry about how late I saw this issue. On the other hand, this has not been tagged with the 1.2.x milestone, so if it doesn't make it in this time around, it isn't critical (although it is always a shame to miss out on new features so close to a release).

pelson · 2012-12-06T09:05:44Z

@jdh2358 is unlikely to be able to pick this up (John, feel free to correct me). @dopplershift: Are you willing to take the baton?

I've re-read that so many times and it hurts every time - I didn't know about John's illness at the time and I dearly wish he was still here today to be able to correct me.

@dopplershift - There are some features in here which John used in his SciPy'12 keynote which I would love to see in mpl (ipython movie integration). Given your expertise in this, if you haven't already, would you mind taking a copy of John's branch and pulling it all together with the review actions from @WeatherGod and submitting a new PR? If your happy to do that, we can close this PR and open a new one (as we cannot update John's branch).

If your a little too busy then I would be willing to attempt it in the interest of getting the features that John described into mpl.

Thank you,

dopplershift · 2012-12-06T14:57:57Z

I can commit to getting this in--it's been on my todo list for awhile, but I just haven't been able to commit the time. I'm interested in getting this in, but I'm not sure if it will be over the next couple of weekends or (more likely) over Christmas break. I'll get it in though, both because animations is my baby and because I owe it to John's memory.

dopplershift · 2013-01-22T16:33:21Z

Some of the issues identified here (regarding multiple inheritence missing parameters) are likely related to issues I noticed when running the moviewriter.py. Changing the writer used to ffmpeg_file (from ffmpeg) causes the frame rate to change and the metadata to longer be included. The output from these two should be identical. (Also, looking at the code, it looks like the rate parameter is in the wrong spot, on input rather than output).

efiring · 2013-02-19T05:00:24Z

@pelson, @dopplershift, how much work is actually required to get this to a point where it can be merged?

efiring · 2013-04-27T19:40:00Z

@dopplershift, how do the prospects for 1.3 look?

dopplershift · 2013-04-30T13:33:56Z

Lousy. I'm sad I haven't gotten this in yet, but, sadly, real life likes to get in the way. That's not likely to change in the medium term. I can probably manage to look something over, but writing code for this right now isn't in the cards.

mdboom · 2013-05-28T16:01:35Z

Punting to 1.4.x

tacaswell · 2014-01-05T21:11:56Z

lib/matplotlib/animation.py

@@ -340,7 +342,9 @@ class FFMpegBase:
    def output_args(self):
        # The %dk adds 'k' as a suffix so that ffmpeg treats our bitrate as in
        # kbps
-        args = ['-vcodec', self.codec]
+        args = []
+        if self.codec!='None':


Should this be None not 'None'?

tacaswell · 2014-01-05T21:31:05Z

@dopplershift any progress on this?

efiring · 2016-05-31T18:37:08Z

@dopplershift Can you summarize the situation? Have any of the problems addressed by this been handled in the interim by other PRs? What would it take to rebase and merge whichever parts are still relevant?

dopplershift · 2016-05-31T20:06:22Z

I'd say:

Webm hasn't really materialized as something important--I'm not personally interested in adding anything about them
IPython/Jupyter HTML hook has already been added
We've cleaned up some of the docs (clarifying that arguments aren't used if passed a MovieWriter)

The rest of the changes are:

Clean up some keyword argument handling
Add the ability to not pass a video codec (more research needed to see if this is still needed today).

I'd say there are a few useful bits to pull out here, but most of it has been subsumed elsewhere. Might be worth rebasing (and dropping commits which aren't needed) and see how bad the resulting conflicts are.

efiring · 2018-01-09T02:08:26Z

I'm going to close this as outdated. Anyone who is ready to work on it is welcome to reopen it.

jdh2358 added 6 commits July 15, 2012 10:31

pass kwargs through to animation writer

48c1921

make sure that the MovieWriter base class gets the args and kwargs on…

a2ca359

… init of the FFMpegWriter and friends

allow codec to be None and clarify save kwargs

8602ba6

support ipy html display hook for return value of Animation.save; may…

81fae9e

… only work for web movies in some browsers

fix basic animation writer to clear prior frame so we do not have a p…

4d5d0d5

…ileup of ax.collections

show how to make webm movies in a commented out example in the basic …

ea2facc

…writer

pelson reviewed Jul 16, 2012
View reviewed changes

jdh2358 reviewed Jul 16, 2012
View reviewed changes

WeatherGod reviewed Jul 18, 2012
View reviewed changes

ghost assigned dopplershift Jul 20, 2012

pelson mentioned this pull request May 28, 2013

BUG: pass kwargs to TimedAnimation #2069

Merged

tacaswell reviewed Jan 5, 2014
View reviewed changes

tacaswell added the needs_revision label Mar 8, 2014

tacaswell modified the milestones: v1.4.x, v1.4.0 Jun 4, 2014

tacaswell modified the milestones: 1.5.0, v1.4.x Feb 7, 2015

tacaswell modified the milestones: next point release, proposed next point release Jun 18, 2015

story645 added the topic: animation label Jul 20, 2016

tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017

efiring closed this Jan 9, 2018

tacaswell removed this from the v2.2 milestone Jan 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Animation fixes #1012

Animation fixes #1012

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Animation fixes #1012

Animation fixes #1012

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!