Add --log-interval option for encode & auto-encode
#334
Conversation
|
Hmmm logging isn't for machine reading though, they're for humans. I think there should probably be a machine output, like sample-encode's json output, that includes progress updates too. This would be designed to output all state changes and be appropriate for a machine usage. If we had that there probably won't be a use case for tweaking logging frequency. |
|
Good point. Happy to provide either a new PR or a modification of this PR with a draft format. I'm not sure this would change the need for log frequency changes, though. I often manually review the logs, so having some degree of control over the speed of logging frequency still feels like a plus. I would argue both together constitute my ideal scenario. What's your thinking? |
src/command/encode.rs
Outdated
| /// Interval between progress log messages when running non-interactively. | ||
| /// Accepts duration (e.g., "30s", "1m") or percentage (e.g., "2%"). | ||
| #[arg(long)] | ||
| pub log_interval: Option<LogInterval>, |
There was a problem hiding this comment.
Does make sense to define this in multiple places, indeed this arg would apply to all commands that log progress updates which is pretty much all of them so it should be defined once and included with clap(flatten) like the encode args are.
There was a problem hiding this comment.
Created shared ProgressLog struct in args.rs, flattened into encode and auto_encode.
| (None, key) => { | ||
| let b = Instant::now(); | ||
| let mut logger = ProgressLogger::new(module_path!(), b); | ||
| let mut logger = ProgressLogger::new(module_path!(), b, None); |
There was a problem hiding this comment.
log control args should affect this, and all other, loggers.
There was a problem hiding this comment.
Added --log-interval to vmaf and xpsnr commands. Left sample_encode on exponential backoff — it runs many short (~20s) samples during CRF search where percentage logging would create noise.
src/command/auto_encode.rs
Outdated
| /// Interval between progress log messages when running non-interactively. | ||
| /// Accepts duration (e.g., "30s", "1m") or percentage (e.g., "2%"). |
There was a problem hiding this comment.
Docs should mention the default is logging exponentially increasing gaps
There was a problem hiding this comment.
Added: "Default: exponentially increasing intervals (16s, 32s, 64s, ...)"
src/log.rs
Outdated
| let secs: u64 = s | ||
| .parse() | ||
| .map_err(|_| anyhow::anyhow!("invalid interval: {s}"))?; | ||
| ensure!(secs > 0, "interval must be greater than 0"); | ||
| Ok(Self::Duration(Duration::from_secs(secs))) |
There was a problem hiding this comment.
I don't think plain integers should be interpreted as seconds, should just be the humantime format or fail.
There was a problem hiding this comment.
Removed. Now requires explicit format: 30s, 1m, or 5%.
|
I don't see much use for this myself, but I'm ok with merging if you think it is useful. I've left some comments, in particular I think the arg should be available in all commands that log progress updates. |
src/command/auto_encode.rs
Outdated
|
|
||
| /// Interval between progress log messages when running non-interactively. | ||
| /// Accepts duration (e.g., "30s", "1m") or percentage (e.g., "2%"). | ||
| #[arg(long)] |
There was a problem hiding this comment.
Maybe this should also have an associated env var too? E.g. AB_AV1_LOG_INTERVAL ?
|
I'll start work right now, thanks, Alex! Would you like the machine-readable format change in a separate PR or merged? |
Yeah I think the machine format should be separate. There was some discussion in #313 (comment) it might be good to propose a json structure for crf-search output and updates there first. |
Address maintainer feedback on --log-interval implementation: - Consolidate log_interval into shared ProgressLog struct (clap flatten) - Add --log-interval to vmaf and xpsnr commands - Add AB_AV1_LOG_INTERVAL env var support - Remove plain integer parsing; require explicit format (30s, 1m, 5%) - Document default exponential backoff behavior in help text sample_encode retains exponential backoff for its short batch operations.
|
Should be good to go, Alex, passes tests/formatting on my end. |
|
I added streaming JSON output for crf-search recently in a PR (see #336), would that be a better approach long term? I'll be happy to add it for the other commands too - I only tackled crf-search for now since that's one of the "building block" commands. |
3 participants
Add
--log-intervaloption to control progress logging frequency when running non-interactively (stderr not a TTY).Accepts:
--log-interval 30s--log-interval 2%Default behavior (exponential backoff) unchanged when flag is omitted.
The current exponential backoff (16s, 32s, 64s, 128s...) creates very long gaps for large encodes, making it difficult for wrapper applications to provide responsive progress feedback.
Passes linting, automated tests, and I built a windows .exe and ran it successfully multiple times without issue with my project.
Thanks for the project, Alex, it's been such a joy building a wrapper around it.