8000 Use tempfile.mkdtemp for animation.FileMovieWriter. by anntzer · Pull Request #3536 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Use tempfile.mkdtemp for animation.FileMovieWriter. #3536

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Use tempfile.mkdtemp for animation.FileMovieWriter. #3536

wants to merge 4 commits into from

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Sep 18, 2014

Currently, animation.FileMovieWriter (e.g. FFMpegFileWriter)
creates temporaries in the current folder, without even checking
whether the file existed before. This is obviously risky (e.g., create
a file named _tmp0000000.png in the current folder before running
moviewriter.py from the docs examples and it will be overwritten.
Moveover, temporaries are not cleaned up if an exception is raised
(e.g., KeyboardInterrupt).

This is relatively easy to fix using tempfile.mkdtemp, although I
had to modify finish so that it doesn't call cleanup by itself
(obviously, this can be fixed by keeping a cleaned_up attribute
but it feels like a minor API change is preferable; moreover the
context-manager API stays the same).

Only basic, manual testing done.

@anntzer
Use tempfile.mkdtemp for animation.FileMovieWriter.
f10180a 
Currently, `animation.FileMovieWriter` (e.g. `FFMpegFileWriter`)
creates temporaries in the current folder, without even checking
whether the file existed before.  This is obviously risky (e.g., create
a file named `_tmp0000000.png` in the current folder before running
`moviewriter.py` from the docs examples and it will be overwritten.
Moveover, temporaries are not cleaned up if an exception is raised
(e.g., KeyboardInterrupt).

This is relatively easy to fix using `tempfile.mkdtemp`, although I
had to modify `finish` so that it doesn't call `cleanup` by itself
(obviously, this can be fixed by keeping a `cleaned_up` attribute
but it feels like a minor API change is preferable; moreover the
context-manager API stays the same).

Only basic, manual testing done.
@tacaswell tacaswell added this to the v1.5.x milestone Sep 18, 2014
@tacaswell
Copy link
Member

Thanks. I would also suggest using the streaming writing, it is much faster and avoids these problems.

@jenshnielsen
Copy link
Member

I seem to remember that there were some issues with temporary files on windows @dopplershift probably remembers better than me.

WeatherGod
WeatherGod reviewed Sep 18, 2014
View reviewed changes
lib/matplotlib/animation.py
@@ -193,7 +199,6 @@ def _run(self):

def finish(self):
'Finish any processing for writing the movie.'
self.cleanup()
Copy link
Member

Choose a reason for hiding this comment

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

Is this even valid? Doesn't there have to at least be a "pass" statement? Plus, I don't like the idea of a function that is documented to finish something actually not do anything.

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, this is valid(!) Moreover it is not that easy to write code with the proper behavior without making this change (because in case of failure you want to call cleanup without calling finish).
In my opinion the finish and cleanup methods should be private anyways. I can also rename the new ones _finish and _cleanup, have the saving contextmanager just call the private methods, and deprecate the wrappers finish and cleanup which keep the previous behavior... What do you think?

@anntzer
Copy link
Contributor Author
anntzer commented Sep 18, 2014

re: Using streaming writing: it doesn't support bbox_inches=tight, which I need.

@tacaswell
Copy link
Member

Sorry to derail the issue a bit, but why do you need bbox_inches=tight?

I suspect that (with out really knowing the problem) you would be better off a) sizing the figure properly and b) using tight_layout.

PS how is ffmpeg re-assembling non-uniformily shaped files later? I assume it is just re-sampling to what ever size it wants?

@anntzer
Copy link
Contributor Author
anntzer commented Sep 18, 2014

I am not using tight_layout but am passing zero-margins to GridSpec, which should be more or less the same. Still, it is not easy to force a proper aspect ratio, as I have a matplotlib canvas embedded as a widget of a Qt application, so the aspect ratio of the canvas is essentially determined by the Qt layout engine.

@dopplershift
Copy link
Contributor

I like the feature, thanks for writing it up. I have no problems outside the aforementioned isinstance() checks.

Regarding bbox_inches, we drop it because in most cases it breaks things. If when using it, though, the images stay the same size, it should be ok. Not sure if there's a better way to support an ok but dangerous case.

@jenshnielsen I don't remember anything about temp files on windows, but I'm not a windows guy.

@jenshnielsen
Copy link
Member

Ok I just remember that someone had some issues with the animations at Scipy which I think was on windows but I can't remember any details so I am fine with these changes apart from your comments.

@WeatherGod
Copy link
Member

The issue at Scipy was 8000 that the image-magick convert.exe executable
collides with a Windows executable by the same name.

On Thu, Sep 18, 2014 at 4:04 PM, Jens Hedegaard Nielsen <
notifications@github.com> wrote:

Ok I just remember that someone had some issues with the animations at
Scipy which I think was on windows but I can't remember any details so I am
fine with these changes apart from your comments.

—
Reply to this email directly or view it on GitHub
#3536 (comment)
.

@anntzer
Deprecate setup, finish and cleanup; saving takes kwargs.
1d010d3 
The old setup, finish and cleanup methods interact poorly with the
context-manager approach anyways so the saving context-manager now calls
_setup, _finish and _cleanup.

The saving context-manager now supports keyword-arguments, e.g.
clear_temp.

Do not overload the _temp_names attribute anymore.

Minor layout changes.
@anntzer
Copy link
Contributor Author
anntzer commented Sep 18, 2014

The new commit takes into account most suggestions. It deprecates the old non-context-manager API though, in order to keep things reasonably simple.

@dopplershift
Copy link
Contributor

I don't have time to see why at the moment, but it seems that this PR is causing test failures.

@anntzer
Copy link
Contributor Author
anntzer commented Sep 19, 2014

Should be fixed now.

@dopplershift
Copy link
Contributor

I'll let this settle for a bit, but I'm good with as this stands.

@tacaswell
Copy link
Member

Can you add text to the deprecation warnings to the effect 'deprecated in 1.5 and will be removed in 1.6'?

@tacaswell
Copy link
Member

I am also in favor of using super here, but hold off making changes until other weigh in.

@anntzer
Copy link
Contributor Author
anntzer commented Sep 19, 2014

I'm all for super but I had a PR some time ago where I was discouraged from using it... is there any "official" policy about it?

@tacaswell
Copy link
Member

I think I was the one doing the discouraging. The issue (iircc) there was that only a sub-set of the classes in the classes family were getting, here there are few enough classes and little enough overriding that superifying everything isn't so bad.

tacaswell
tacaswell reviewed Oct 18, 2014
View reviewed changes
lib/matplotlib/animation.py
@@ -316,7 +351,8 @@ def _frame_sink(self):
fname = self._base_temp_name() % self._frame_counter

# Save the filename so we can delete it later if necessary
self._temp_names.append(fname)
if isinstance(self._temp_names, list):
Copy link
Member

Choose a reason for hiding this comment

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

Could you do this test as self._temp_names is not None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to do so. See below.

@anntzer
Copy link
Contributor Author
anntzer commented Oct 18, 2014

I have lost interest in this issue as I ended up just rewriting my own wrapper to ffmpeg for my use case. Feel free to close it, amend it or merge it.
As a side note, in order to handle the (equivalent of the) bbox_inches="tight" case (which, at least in my case, could initially fail due the user resizing the GUI window while the animation was generated), I use QWidget.setFixedSize for the duration of the context manager in order to prevent this from happening. Other options would be to resample each image to the size of the first image, or (if using a tempfile-based writer) pad each image to the size of the largest image.

@dopplershift dopplershift self-assigned this Jan 26, 2015
@tacaswell tacaswell modified the milestones: proposed next point release, next point release Feb 19, 2015
@tacaswell tacaswell modified the milestones: proposed next point release, next point release Jun 23, 2015
@tacaswell
Copy link
Member

@anntzer Any interest in revisiting this PR?

@anntzer
Copy link
Contributor Author
anntzer commented Mar 22, 2016

I don't really care anymore, as I have my own wrapper now: https://gist.github.com/anntzer/e0186a16b392b4abf3e1
Anyone who wants to work on it is welcome to copy parts of it.

Note that this directly takes QImage as input, but it's really just passing the RGBA bytes to the pipe; from matplotlib's POV it's probably just as good to use fig.canvas.print_raw. http://stackoverflow.com/questions/5391026/matplotlib-alternatives-to-savefig-to-improve-performance-when-saving-into-a compared saving raw bytes to saving to png and found a twofold improvement in performance (and likewise the decoding on ffmpeg's side is probably faster too), so it's probably also worth taking this into account.

@tacaswell
Copy link
Member

bump on this.

@anntzer
Copy link
Contributor Author
anntzer commented Jun 20, 2016 •
edited
Loading

Honestly I think the whole thing should just be rewritten to pipe the RGBA bytes to ffmpeg's (or whatever process is used) stdin.

Feel free to close this, I won't revisit it (plus, #5454 made rebasing not so trivial).

@tacaswell
Copy link
Member

That is what http://matplotlib.org/api/animation_api.html#matplotlib.animation.FFMpegWriter and friends do.

I think the file-base version exist either for back-compatibility or some use case where writing to stdin does not work.

@anntzer
Copy link
Contributor Author
anntzer commented Jun 20, 2016

Fair enough. Still not enough to convince me to fix the rebase :-)

@QuLogic QuLogic mentioned this pull request Jan 24, 2017
Merged
@tacaswell
Copy link
Member

Closing this as the rebase has become pretty nasty due to other changes to animation.py.

@tacaswell tacaswell closed this Aug 13, 2017
@tacaswell tacaswell modified the milestones: unassigned, 2.1 (next point release) Aug 13, 2017
@anntzer anntzer deleted the animation-tempfile branch August 23, 2017 01:11
@anntzer anntzer mentioned this pull request Jan 23, 2018
Closed
@anntzer anntzer mentioned this pull request Sep 17, 2018
Closed
This was referenced Jan 3, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dopplershift dopplershift

Labels
status: needs revision topic: animation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anntzer @tacaswell @jenshnielsen @dopplershift @WeatherGod
0