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

Improve setting default encode parameters #798

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

Conversation

FlyingWombat
Copy link

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.

@shssoichiro
Copy link
Collaborator

Thanks, I need to look through more in detail but a lot of this looks like an improvement.

@FlyingWombat
Copy link
Author

FlyingWombat commented Jan 5, 2024

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.

@FlyingWombat FlyingWombat force-pushed the default-params branch 3 times, most recently from bdd705b to 34b48b7 Compare January 6, 2024 05:45
@FlyingWombat
Copy link
Author

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. --no-tile, if you think a significant portion of people would use it.

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. --keyint 0 --scd 0 --rc 0, which only need to be set because I'm using av1an, and I'd have to make an external wrapper for av1an to estimate tiles based on resolution.

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.

@FlyingWombat
Copy link
Author

@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 --force flag meet your needs?
Or, would adding a new flag specifically to control tile estimation be better? (but then we're adding another cli flag) E.g. --no-tile to disable tile estimation; or --auto-tile to enable tile estimation. Should tile estimation be enabled by default, or disabled by default?

The encoding parameters are printed to the screen and the log file.

@FlyingWombat
Copy link
Author

FlyingWombat commented Feb 16, 2024

@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.

@FlyingWombat FlyingWombat force-pushed the default-params branch 2 times, most recently from d0e8cbc to 1ceb499 Compare February 16, 2024 00:37
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.
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