Add av1 nvenc support #74
Conversation
…ing VMAF score and made engine/permutation_engine.rs use it in calc_vmaf_score.
… and displayed during permutor-cli execution and in the output log.
… and displayed during permutor-cli execution and in the output log.
…/encoder-benchmark into adding-cq-support
|
Thank you! I'll test this out when I get a chance (I have a 4060 so I can test this for ya). |
|
@SuperCat908809 did you get to do any comparisons on your AV1 supported GPU with regards to how AV1 is compared to H264 or the like? |
Proryanator
left a comment
There was a problem hiding this comment.
Thank you so much for contributing the NVENC AV1 encoder support and VBR support! Apologies I didn't get to reviewing your code until just now 🫠
I tested this on my 4060 using the 1080-120.y4m and the 4k-120.y4m, looks like it worked! I can't get the VBR support portion to work for me (I might be missing something though) if you could help see how I could test that out from the cli.
A few large change requests
Overall the updates look great! If you could undo some of the rust package updates that might not be needed, and see if you can get this to work with the Rust version 1.73.0 that would be great to cut down on the possible bugs introduced through these updates.
Also, if you could update the README to officially mention support for the av1_nvenc encoder that would be amazing 👏 I also believe that users would need to use/install a newer version of ffmpeg, so changing the version of ffmpeg to download to be 8.0 would be good to mention in the README (my version 5/6 didn't have this encoder). This is mentioned in a few spots you can search for 6.0 to find those updates.
| fn get_resolution_to_bitrate_map(_fps: u32) -> std::collections::HashMap<String, u32> { | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
You can probably remove this, this was a placeholder for the CQ work, right?
| [[package]] | ||
| name = "anstream" | ||
| version = "0.6.19" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "301af1932e46185686725e0fad2f8f2aa7da69dd70bf6ecc44d6b703844a3933" | ||
| dependencies = [ | ||
| "anstyle", | ||
| "anstyle-parse", | ||
| "anstyle-query", | ||
| "anstyle-wincon", | ||
| "colorchoice", | ||
| "is_terminal_polyfill", | ||
| "utf8parse", | ||
| ] |
There was a problem hiding this comment.
There are quite a few other package dependencies added in here, do you mind seeing if you need these? They would increase the .exe size, but also introduce other dependency issues that might not be worth it as part of adding AV1 support.
| pub enum NvencCodec { | ||
| H264, | ||
| HEVC, | ||
| AV1 |
There was a problem hiding this comment.
I like this change, thank you 👏 since there are now more than 2 codec types this makes it pretty clean. I would like to get away from string literals for this project more if there are other spots like that.
| }, | ||
| // leaving out vbr rate controls as these are not ideal for game streaming | ||
| rate_controls: vec!["cbr"], | ||
| rate_controls: if using_bitrate { vec!["cbr"] } else { vec!["vbr"] }, |
There was a problem hiding this comment.
You'd like to add in vbr support with the AV1 support? I do recall leaving it out before but, I can't really see any issue with adding this in there 👍
| args.push_str(" -profile:v "); | ||
| args.push_str(self.profile); | ||
| match self.codec { | ||
| NvencCodec::AV1 => {}, // don't add profile parameter for AV1 |
There was a problem hiding this comment.
Nice! AV1 doesn't have a profile parameter?
It might be good to add in error checking for if a user tries to provide -profile with this encoder.
| let bitrate = get_bitrate_for(&permutation.get_metadata(), cli.encoder.clone()); | ||
|
|
||
| permutation.bitrate = bitrate; | ||
| permutation.ffmpeg_quality = FfmpegQuality::Bitrate(bitrate); |
There was a problem hiding this comment.
I'm trying to test out the new option for VBR, but I'm not sure how I would test this out. There may need to be some changes to the benchmark/src/benchmark_cli.rs file (and the permutor-cli/src/permutor_cli.rs) files that define cli options to show users how you would essentially turn off bitrate control.
So far I can only get it to run the cbr version. I could be missing something though so if you know how I could trigger the vbr version from the cli let me know so I can test it out!
| # This file is automatically @generated by Cargo. | ||
| # It is not intended for manual editing. | ||
| version = 3 | ||
| version = 4 |
There was a problem hiding this comment.
You should be able to revert this change and the entire lock file, and make sure to use rust 1.73.0 while testing. (there might be 1 additional update to this file after you re-run cargo build though for the newly added chrono dependency which is fine).
I confirmed this, I can still build your branch with a reverted Cargo.lock file so, this could just be a side-effect from using a newer version of Rust 👍
I did open up an issue though to overall bump Rust to the latest for good house keeping that I can work on as a followup (with specific testing done for version changes, etc): #79
Threw together av1_nvenc support for both benchmark and permutor-cli. It's a bit hacky with the probably unnecessary NvencCodec enum and so on but it works.
Solution to enhancement request: #69