Let MovieFileWriter save temp files in a new dir#11860
Let MovieFileWriter save temp files in a new dir#11860fredrik-1 wants to merge 1 commit intomatplotlib:masterfrom
Conversation
lib/matplotlib/animation.py
Outdated
| 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.
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.
Good point. That code is definitely dangerous. I have changed to os.rmdir now.
One problem is that I believe that os.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.
fredrik-1
force-pushed
the
frame_dir
branch
3 times, most recently
from
22a9d12 to
380104a
Compare
dopplershift
left a comment
dopplershift
left a comment
There was a problem hiding this comment.
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._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.
I think tempfile.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.
Should really only delete the directory if we 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
| @property | ||
| def output_args(self): | ||
| args = ['-vcodec', self.codec] | ||
| if self.codec is not '': |
There was a problem hiding this comment.
I think this would be better as:
args = []
if self.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.
I'm not sure it's necessary to document '' as an option; given that we're eliminating vframes below, I'm not sure we want people doing it.
| # 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.
Eliminating vframes is only ok because we're putting frames in a temp directory by default, IMO. The original intent behind adding vframes 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! |
5 participants
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.