-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
11df90a
to
d12dac8
Compare
d12dac8
to
54f8b50
Compare
Instead of creating a new argument 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 Because I find it kind of strange to allow for something like
|
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
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. |
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. |
Let's first find out which arguments prevent the usual FFMpegFileWriter from working. I think 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 |
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 |
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 b 8000 ad 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. |
The benefit of new features has to be weighed against the need to maintain them. |
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 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}' |
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.
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.
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.
The command is passed as a list.
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.
Indeed, I stand corrected. But using str.split after str.format will still run into issues if the filename has spaces.
String manipulation feels like the wrong tool here, maybe we're lacking the right abstraction for the command line handling? |
Thanks for the contribution, but I think the consensus was against this... |
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:
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.