Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Let MovieFileWriter save temp files in a new dir#11860
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lib/matplotlib/animation.py Outdated
| @@ -542,6 +549,9 @@ def cleanup(self): | |||
| self._temp_names) | |||
| for fname in self._temp_names: | |||
| os.remove(fname) | |||
| folder = os.path.split(fname)[0] | |||
| if folder is not '': | |||
| shutil.rmtree(folder, ignore_errors=True) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't it dangerous to simply delete a (possibly non-empty) folder from the user's disk? I guess we don't want to have people saying "Matplotlib sucks, it just deleted all my data without warning".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point. That code is definitely dangerous. I have changed toos.rmdir now.
One problem is that I believe thatos.rmdir sometimes don't remove empty directory's on windows for some reason. It seems to work for me now and it is possible that it doesn't happen that often and never in this application.
22a9d12 to380104aCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In general, putting the frames in a directory is a good idea and something I should have done awhile ago.
Only a few minor changes.
lib/matplotlib/animation.py Outdated
| ''' | ||
| self.fig = fig | ||
| self.outfile = outfile | ||
| if dpi is None: | ||
| dpi = self.fig.dpi | ||
| self.dpi = dpi | ||
| self._adjust_frame_size() | ||
| root, _ = os.path.splitext(outfile) | ||
| if frame_dir is None: | ||
| for i in itertools.count(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thinktempfile.mkdtemp would be more straightforward here.
lib/matplotlib/animation.py Outdated
| folder = os.path.split(fname)[0] | ||
| if folder is not '': | ||
| try: | ||
| os.rmdir(folder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should really only delete the directory ifwe created it. Right now the user could pass us an existing directory, and this code would remove it when done.
lib/matplotlib/animation.py Outdated
| @@ -597,7 +613,10 @@ class FFMpegBase(object): | |||
| @property | |||
| def output_args(self): | |||
| args = ['-vcodec', self.codec] | |||
| if self.codec is not '': | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this would be better as:
args= []ifself.codec:args.extend(['-vcodec',self.codec])
lib/matplotlib/animation.py Outdated
| frame_dir : str or None, optional | ||
| Directory where the temporary files are saved. None means that a | ||
| new directory is created and '' means that no temporary directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure it's necessary to document'' as an option; given that we're eliminatingvframes below, I'm not sure we want people doing it.
| @@ -665,8 +684,7 @@ def _args(self): | |||
| # Returns the command line parameters for subprocess to use | |||
| # ffmpeg to create a movie using a collection of temp images | |||
| return [self.bin_path(), '-r', str(self.fps), | |||
| '-i', self._base_temp_name(), | |||
| '-vframes', str(self._frame_counter)] + self.output_args | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Eliminatingvframes is only ok because we're putting frames in a temp directory by default, IMO. The original intent behind addingvframes was to handle a problem where users got stale frames in their animation if they changed the number of frames.
Made the suggested changes. |
See also#3536 for what was one of my first PRs to matplotlib :p |
@fredrik-1 did you want to proceed w/ this? We are trying to tag 3.1 soonish... |
Thanks a lot for the contribution. I'm going to close this as having had no action for quite a while. OTOH, if there is still a community that wants this, please feel free to re-open, with my apologies! |
Another try to get the animation module to work better. The reason for these changes are that they make it possible to create videos of animations with a low frame rate to work in video players like the VLC player. This can be done by changing the output frame rate to a higher value by for example using
extra_args=['-r', '25']orextra_args=['-vf', 'fps=25'].The changes in this pr:
-vframesin ffmpeg file writer because it is usually unnecessary and make it impossible to change the output frame rate.MovieFileWritersave the temporary files in a new directory to avoid the possible problem that the writer reads old files.extra_argsafter the-i temp_filesby not using a codec ifcodec=''. This is not really necessary but gives more freedom and might be useful for someone.