master-of-zen / Av1an

Cross-platform command-line AV1 / VP9 / HEVC / H264 encoding framework with per scene quality encoding
GNU General Public License v3.0
1.51k stars 156 forks source link

Add Richly-Typed loadscript VapourSynth Script to Source Control #886

Open BoatsMcGee opened 3 weeks ago

BoatsMcGee commented 3 weeks ago

The current way Av1an writes vpy files is prone to errors from the developer side. Currently, the Rust source code itself contains a separate string for each "import" method which is tailor-made for that method. Most of the strings are identical between methods and are needlessly repetitive. More crucially, these strings are not recognized as Python by the IDE because understandably the IDE isn't expecting to encounter a different language in a random string. Due to this limitation, developers won't appreciate rich typings and intellisense the IDE would offer should that Python code be written where it is expected: in a .py file. This leaves contributors prone to making mistakes they might otherwise avoid should they have the guidance modern IDEs provide. This is easily solved by designing and writing the python file directly into the project files structure and having that source-controlled.

Moving Python Into a Dedicated File

This is a proposal to use the include_str! macro which can include an entire text file, or in this case a Python script, directly into the built executable. Essentially, we can move Python code out of .rs files and into their respective .py or .vpy files and use them however we want in the Rust code. In this PR, I unified the scripts for each import method into 1 .vpy file and modified the string itself for each method to set variables for the script to access during runtime. We can also avoid modifying the text altogether by instead providing the values through environment variables when the script is invoked. This simplifies the embedded Python code and the Rust code all while making it more palatable for potential contributors.

Enhancements

This change also allows us to leverage the Python runtime as seen in the bestsource case when different versions produce different outputs. In short, the recent release of R8 allows setting the cache file path to an absolute path where previous versions did not allow this whatsoever. For a fuller picture, see the comments in the .vpy script.

This suggestion falls outside the scope of this PR but the .vpy script can be further modified to support a potential feature where users can specify a preferred order of chunking methods to attempt before the script fails. For instance, users may prefer DgDecNV, BestSource, and LSmashWorks in that order and the script can fallback to less preferred methods should previous ones fail or not be installed.

Considerations

I have very limited experience with Rust currently and the create_vs_file function could be further improved from my introduced changes so please feel free to make or suggest necessary tweaks.

Thank you, - Boats

shssoichiro commented 3 weeks ago

This looks interesting and I like the idea. I'll take a look through the implementation most likely later today.

shssoichiro commented 3 weeks ago

I spoke too soon, looks like it needs a cargo fmt real quick.

BoatsMcGee commented 3 weeks ago

Sorry about that, I assume it's just a style issue and not anything related to the logic, right? Having run cargo fmt locally the only related change I see is line 254 in vapoursynth.rs was collapsed into a single line which is totally fine by me. However, I also see that util.rs and scenes.rs also changed in the imports. Everything seems harmless so I'll go ahead and commit that.

BoatsMcGee commented 2 weeks ago

Is there anything else that needs to be done? I synced the fork for the latest changes and made sure that there's no other formatting issues.

Thanks, - Boats

shssoichiro commented 2 weeks ago

It seems like there's some issue going on where the tests are failing. see https://github.com/master-of-zen/Av1an/actions/runs/11674361550/job/32550276127?pr=886

I may have time to help look into this later today, been quite busy lately but I'll see what I can figure out.