8000 Adding the possible to add full command line in animation by fredrik-1 · Pull Request #11855 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Adding the possible to add full command line in animation #11855

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

fredrik-1
Copy link
Contributor

I added the possibility to add a full command line in the animation writers that call an outside executive.

The reason for this is that that there are a lot of different possibilities to call ffmpeg and that the default command line with options is not always general enough. This happened to me when I tried to make slowly updated mp4 animations with ffmpeg.

So I added the possibility to use a command string like for example:

'{path} -framerate {fps} -i _tmp%07d.png  -vf fps=25 -vcodec 'h264 -pix_fmt yuv420p -y {outfile}'

A general string where is it possible to use keywords to put in parameters from the code in the command.

I also added a method get_command that returns the command line that have been used with a writer, some tests and an example that show this functionality and how to use some other writers.

I believe that these changes would have made it much easier for me to get working movies when I worked with that. It also makes the writers much more general when you can use any commands to write movies from the pics or streams that the writers create for you.

I added the full command line option to the extra_args parameter.

Docstrings and the example definitively needs some editing but I want to see if things seems to work or if someone has any comments.

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@fredrik-1 fredrik-1 force-pushed the animation_full_command_line branch 2 times, most recently from 11df90a to d12dac8 Compare August 15, 2018 12:02
@fredrik-1 fredrik-1 force-pushed the animation_full_command_line branch from d12dac8 to 54f8b50 Compare August 15, 2018 12:35
@ImportanceOfBeingErnest
Copy link
Member

Instead of creating a new argument run and a method get_command, couldn't it make sense to simply write the command that has been used to the _log.debug pipe?

More generally I think I have problems understanding the underlying issue. But I feel that if the arguments passed are not correct, one would rather update the logic for those, instead of putting the option of arbitrary input into the existing writers.

Or else, one could of course provide an ArbitraryFileWriter with a single argument (=the string to call) to allow for this functionality.

Because I find it kind of strange to allow for something like

 FFMpegFileWriter(fps=5, extra_args="MicrosoftWord.exe")

@tacaswell tacaswell added this to the v3.1 milestone Aug 15, 2018
@fredrik-1
Copy link
Contributor Author

Using the log debug pipe(but I don't know how) could be an idea but this implementation also make it possible to see the command before anything has been written.

The idea is that it would be quite a lot of keywords to allow all possible ways to use ffmpeg and this make it possible to quite simple allow all possibilities. I used quite a lot of time a year ago when trying to get mp4 movies to work as expected (the problem is when the pics update every 1 or 2 seconds) and it would have been nice to as simple as possible just test all suggested solutions.

Because I find it kind of strange to allow for something like
FFMpegFileWriter(fps=5, extra_args="MicrosoftWord.exe")

I agree but does it matter? It would of course also be easy to just allow to change the command string after the executive name.

@jklymak
Copy link
Member
jklymak commented Aug 15, 2018

In my opinion this is going the wrong direction. I think if users can’t use the defaults they should just make the pngs manually and make the movies from the command line. This is a lot of machinery for what is essentially just running a command from the system.

The huge advantage of doing the movie writing manually is that you only make the pngs once and then can play with settings for making the movie without retriggering all the draws.

@ImportanceOfBeingErnest
Copy link
Member

Let's first find out which arguments prevent the usual FFMpegFileWriter from working. I think extra_args are precisely there to be able to use any custom arguments you want. Now if the way FFMpegFileWriter predetermines the arguments is incorrect or incomplete, one would surely work towards fixing it.

That being said, I have to say I'm a bit lost on where the current FFMpegFileWriter fails for the use case. Is it the -r argument which overwrites something after that?

@fredrik-1
Copy link
Contributor Author

You can run through my example and see whats fails.

It is the file writer that don't work with or without extra args with slow animation if you for example use the VLC player or windows media player (I don't know if any other player works).

The difference with -r and -framerate can also make a small difference in output.

@fredrik-1
Copy link
Contributor Author

The huge advantage of doing the movie writing manually is that you only make the pngs once and then can play with settings for making the movie without retriggering all the draws.

I don't think that that difference where that important in my applications at least. It was the making of the movie that took the most time.

I believe that I played on the command line and actually ended up with doing the animation more or less manually but I think that it would be very convenient if you actually could use the classes in the animation module in a simple way at least when you are finished with the testing of what works.

This might be a bad idea but in general I don't understand why people are so skeptical to new functionality that could help people. The animation module is also far from perfect in many ways, there are for example rc parameters that are not even used in the code and it is not that easy to understand which parameters that are actually used by the different writers,

It is so much easier if you have well documented functions that are general enough to do what you want instead of guessing how to use the functions, getting into the source code to see what happens, which commands are executed etc, writing new code to test the executive "manually" on the command line etc.

@jklymak
Copy link
Member
jklymak commented Aug 15, 2018

but in general I don't understand why people are so skeptical to new functionality that could help people.

The benefit of new features has to be weighed against the need to maintain them.

Copy link
Contributor
@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

I agree with other maintainers that it's unclear we want such a complex API for just running a subprocess.
But (I think) there are other issues that need to be fixed anyways.


an example command string is::

'ffmpeg -r {fps} -i _tmp%07d.png -r 25 -y {outfile}'
Copy link
Contributor

Choose a reason for hiding this comment

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

That'll cause all sorts of funny bugs if there are spaces in {outfile}.
Also passing a string (rather than a list) to subprocess.run simply doesn't work on Linux (unless you use shell=True, but then you also run into metacharacter issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The command is passed as a list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, I stand corrected. But using str.split after str.format will still run into issues if the filename has spaces.

@dopplershift
Copy link
Contributor

String manipulation feels like the wrong tool here, maybe we're lacking the right abstraction for the command line handling?

@jklymak
Copy link
Member
jklymak commented Feb 12, 2019

Thanks for the contribution, but I think the consensus was against this...

@jklymak jklymak closed this Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0