-
Notifications
You must be signed in to change notification settings - Fork 156
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
Improve setting default encode parameters #798
base: master
Are you sure you want to change the base?
Conversation
Thanks, I need to look through more in detail but a lot of this looks like an improvement. |
50cda6c
to
90bd06f
Compare
The default tiling estimation was already there, this pull request doesn't change that -- just makes it actually useful. Av1an already, by default will determine tiles and set encoder parameters accordingly. But as is, if --video-params is specified, all default parameters set by Av1an are ignored, such as the (useful) tile estimation. What this pull request fixes is that now the tiling estimation can be actually used with user-specified encoder parameters. The "settings: merge video_params..." commit is what does this, and also has the --force flag ignore Av1an's default encoder parameters altogether. Parameters in --video-params will override, so if you want multiple keyframes per chunk, just set --keyint in --video-params. If we don't want Av1an to set encoder parameters automatically, then we should remove the tiling estimation and default parameters code altogether. Considering tiles in worker estimation is better because (except for svt-av1) tiling changes the CPU resources used by each worker. Not considering tiles in worker estimation would over-commit the CPU and use more RAM than necessary. Av1an already prints the encoder parameters being used, and I added a message for that in the log file in this pull request. |
bdd705b
to
34b48b7
Compare
You can still have the old behavior, where Av1an doesn't set default parameters at all. This pull request has the --force flag ignore all of Av1an's default encoder parameters. Do you think I should put this option in a separate flag, in case a user wants to ignore default arguments, but still have the syntax check? We could add a flag to opt-out of just tile estimation e.g. The main reason I made the change to merge video_params into defaults, is because I wanted to use the tiling and worker estimation. Tiling and worker estimation, and all the default parameter code for that matter, is unused when --video-params is set -- which pretty much everyone does to at least set --crf. There has been confusion about this behavior before, see #662 . This pull request fixes that. Without default parameter merging, I'd have to clutter my encoding parameters with e.g. We may need to change the numbers used for tile estimation. I also thought it was too many tiles, and changed it in my local copy of Av1an. |
@damian101 Do you have any specific changes to propose for this pull request? I'm not sure what to do with your feedback. Does the added behavior for the The encoding parameters are printed to the screen and the log file. |
92572f5
to
4eb5806
Compare
@damian101 Per your request, I've added a "--tile-auto" flag. Av1an will now only do tile estimation (and automatic setting of tiling encode params) if this flag is present. It took me a while to get back to this, but I think now all the discussion points have been addressed. log2 conversion is done in encoder.rs::get_default_arguments for applicable encoders. |
d0e8cbc
to
1ceb499
Compare
When run in a non-interactive terminal, the default prompt option is always chosen.
Av1an is chunked encoding; encoders should never place keyframes aside from the first frame of each chunk.
This allows the user to take advantage of automatic tile and worker estimation. Parameters specified in video_params will override those in Av1an's default set. The --force cli flag now sets the old behavior of ignoring the default parameter set.
This was a sanity check, and it worked under normal conditions, but if a ffmpeg filter increases the frame count, it will cut off the added frames from the encode. follows addition of --ignore-frame-mismatch flag in e10880d TODO: update display frame count and completion time estimate to account for more frames than source.
Remove redundant print statement, since `info!` prints to the console as well.
keyint 0 does not imply no scene detection Av1an already does scene detection by default, having the encoders do it too is just wasting CPU cycles.
Made rough estimates of per-worker memory and cpu usage for each encoder. Memory scales with resolution (megapixels) approximately linearly, so memory usage for each encoder is expressed as a value of GB/MegaPixel. Consider chunk_method and pix_format in addition to resolution and tiles. chunk_method memory usage scales with resolution, and differs depending on the method. Encoders use significantly more memory when encoding in full chroma 444 vs 420 subsample.
1ceb499
to
ef8361e
Compare
This improves upon setting default encoding parameters. The gist of this pull request is improving the tile and worker estimation, and keeping those values when setting video-params. My main use-case for this is that I sometimes have batches of mixed resolution videos (e.g. 1080p and 4k) that I want to encode with the same settings -- and not have to go through one by one to specify tile count.
Some of the commits are loosely related, and I wasn't sure if I should make separate pull requests for them, so they are included here. I am a beginner with rust, so please correct me if the formatting is bad, or if there is a better way to implement the changes.