Skip to content
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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

BoatsMcGee
Copy link

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
Copy link
Collaborator

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

Copy link
Collaborator

@shssoichiro shssoichiro left a comment

Choose a reason for hiding this comment

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

I think this is good.

@shssoichiro
Copy link
Collaborator

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

@BoatsMcGee
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants