Made FFmpeg executable path configurable

[ad-chaos]

Overview: What does this pull request change?

FFmpeg:

• Removes the FFMPEG_BIN constant from constants.py
• adds a config option ffmpeg_path to manually specify where ffmpeg is located without changing the PATH variable
   

LaTeX:

• Add new attributes to the class TexTemplate namely dvisvgm_compiler
• Adds the ability to pass a Path like string into the tex_compiler and dvisvgm_compiler arguments of the TexTemplate constructor.

Further Information and Comments

No tests

Reviewer Checklist

• The PR title is descriptive enough for the changelog, and the PR is labeled correctly
• If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
• If applicable: newly added functions and classes are tested

[behackl]

I've skimmed through this; looks good to me overall! Two questions:

• Can you clarify the relation of this PR to Configurable LaTeX and FFmpeg executables  #2660?
• I think it would be a good idea to explicitly check, before calling the ffmpeg commands, whether config.ffmpeg_path exists and has a ffmpeg executable in it; practically this could happen with a _ensure_ffmpeg_exists utility function or so. I'd go as far as to raise a corresponding error with a very explicit hint regarding installing ffmpeg correctly.

[ad-chaos]

Can you clarify the relation of this PR to #2660?

Well, I didn't know Naveen was already working on it 😅. His implementation is very different from mine so depending in which one we like we can choose to merge one and close the other.

I think it would be a good idea to explicitly check, before calling the ffmpeg commands, whether config.ffmpeg_path exists and has a ffmpeg executable in it; practically this could happen with a _ensure_ffmpeg_exists utility function or so. I'd go as far as to raise a corresponding error with a very explicit hint regarding installing ffmpeg correctly.

Sure, will not be a problem. Will push the changes as soon as I get some time 👍

[naveen521kk]

How would one change location to the latex executable? There are no changes about them in the default config file?

[ad-chaos]

How would one change location to the latex executable?

Using the TexTemplate class and passing paths to the tex_compiler or dvisvgm_compiler arguments of the constructor.

What does this path key mean? [...] Does this mean the path to the executable of FFmpeg, in which case I would suggest renaming it to executable

path (to me atleast) seems more intuitive given it is under [ffmpeg]

I feel that we should ask users interactively about where the location of the executable is. What do you think?

No, because what if the user doesn't know the path to the executable when the error occurs?
Run manim again after getting to know the path? Plus it also adds some extra complexity.

[naveen521kk]

Using the TexTemplate class and passing paths to the tex_compiler or dvisvgm_compiler arguments of the constructor.

Doesn't it confuse users? I think LaTeX should also be configurable using config files. Also, when the latex.exe is found in a directory all the other executables like dvisvgm should be used from the same directory; I have implemented them in #2660.

path (to me atleast) seems more intuitive given it is under [ffmpeg]

path confuses with PATH(the variable used in terminals) and users may be confused with that; having it as executable is better, IMO.

No, because what if the user doesn't know the path to the executable when the error occurs?

When informing about that to users, the documentation page should be linked.

Run manim again after getting to know the path? Plus it also adds some extra complexity.

Check before rendering the scene itself?

Also, as I explained in #2511, Manim should save the path to the executables in the config files (or somewhere else) and should always use it. This is mainly to make sure that if the user changed the PATH variable, manim still uses the executables that were configured the first time.

I would also like to confirm with the user the FFmpeg and LaTeX executable that manim uses for the first time; this would make sure that the indented executables are used.

[ad-chaos]

Doesn't it confuse users?

How? The current way of configuring LaTeX (preamble, documentclass, tex_compiler) is through the TexTemplate, I just added the ability to add paths to an already present option.
Though now that you mention it maybe it would be better if it resided in the config file.

But I personally still do think asking interactively right after the error has occurred is not a good idea.
We already have manim cfg write for interactively generating a config file, so if you insist maybe we could repurpose and refine that logic and have something like manim cfg write --header=ffmpeg --key=path or something like that and tell the user that they could run that in case they want to interactively set the config option.

[naveen521kk]

Doesn't it confuse users?

How?

While FFmpeg can be configured by the config file and LaTeX can't be configured so creates confusion, IMO.

But I personally still do think asking interactively right after the error has occurred is not a good idea.

I would like to interactively ask whenever manim runs (at least for the first time) and that could be disabled by a command-line option; not just when an error has occurred.

We already have manim cfg write for interactively generating a config file, so if you insist maybe we could repurpose and refine that logic and have something like manim cfg write --header=ffmpeg --key=path or something like that and tell the user that they could run that in case they want to interactively set the config option.

The main issue I would like to open that issue is that Manim shouldn't execute random executables present on PATH; instead, it should ask the user and save it for future use. If the executable is missing/doesn't work manim should ask the user again which executable to use. See the original motivation why I would want this feature here (private).

[ad-chaos]

The main issue I would like to open that issue is that Manim shouldn't execute random executables present on PATH [...]

In that case we can stop relying on the PATH variable entirely and only have the set via config options.

[...] Instead, it should ask the user and save it for future use. If the executable is missing/doesn't work manim should ask the user again which executable to use. See the original motivation why I would want this feature.

How will you detect manim is being run the first time?
Also I have a slight bias towards setting it from a config file, it is much simpler and straight forward rather than asking interactively.
This is my personal opinion, if other devs feel that asking interactively by default is nice then this PR can be closed in favour of #2660 👍

[naveen521kk]

In that case we can stop relying on the PATH variable entirely and only have the set via config options.

That's what I want to do, we would only use stuff in PATH when initialising the config for the first time (if it is not already set).

How will you detect manim is being run the first time?

If the path to the executables in config files is empty or invalid then manim is run for the first time. If the user has set the config value beforehand nothing would be asked to the users and manim would just use the values present in config files.

[behackl]

I am not sure I understand the current state of the suggested solution here.

• What exactly should happen when manim is run for the first time and ffmpeg is found in the users PATH?
• I understand that if there is no or an invalid path stored in the config, there should be a dialog (good idea!) -- where exactly would the dialog store the path? In the global, default config shipped with manim? If so, is it a good practice to modify "library files"?

[naveen521kk]

What exactly should happen when manim is run for the first time and ffmpeg is found in the users PATH?

A dialog should be shown asking the user whether to use the executable manim found in PATH. If the answer is a no, then they would be asked to enter the location of the executable manually and finally stored it in config files. If the answer is yes, then the path to the executable is stored in config files for future use.

where exactly would the dialog store the path? In the global, default config shipped with manim? If so, is it a good practice to modify "library files"?

Usually storing them in the default.cfg isn't great because package managers (like say Pacman) has an option to check whether the files have changed, which will fail if we change it. Also, in some cases, those files are not writable. I would like it to be stored in user config file.

[kilacoda]

Request for status since it's been a while since last activity on this.

[ad-chaos]

Request for status since it's been a while since last activity on this.

Well, this PR is ready for review and merging, but the approach taken here is not accepted by everyone. You can give it a go for yourself and see if you like it.

There is also #2660 which also has the same motive. You take take a look at that as well

[ad-chaos]

Alright, rebased and ready to merge

[naveen521kk]

lgtm

[behackl]

lgtm, thanks!