[Rober1208]

Closes #280

Adds -i/--interval to save-images, as discussed in #280, so the number of images per scene depends on its length:

scenedetect -i video.mp4 save-images -i 2s                                 # one image per 2 seconds of each scene
scenedetect -i video.mp4 save-images -i 2s --min-images 3                  # at least 3 images per scene
scenedetect -i video.mp4 save-images -i 2s --max-images 10                 # at most 10 images per scene
scenedetect -i video.mp4 save-images -i 2s --min-images 3 --max-images 10  # between 3 and 10 images per scene

Each scene gets floor(scene length / DURATION) + 1 images, clamped to --min-images/--max-images (default 1 and no limit). The images are chosen the same way as with -n, so the first and last frames are included and the frame margin applies. DURATION takes the same formats as -m/--frame-margin: frames (24), seconds (1.5s), or a timecode (00:00:01.500).

Design notes

• Separate options, as proposed in Dynamically adjust the number of output images of the scene #280 (comment): -i DURATION takes one value, and --min-images N / --max-images N set the limits. Each limit can be set on its own, the options use only public click API (click.IntRange), and each maps to one config key. They have long names only, like --min-out-length and --expand. (Single-option variants with comma- and space-separated values were tried and dropped after the review: [cli] Add save-images -i/--interval to base image count on scene length #588 (comment).)
• Limits: --min-images must be at least 1. --max-images 0 means no limit and is the default, so a limit from the config file can be removed on the command line. --max-images must be 0 or at least --min-images.
• Config file: interval = 2s, min-images = 1 and max-images = 0 in [save-images].
• -n and -i are mutually exclusive: setting both on the command line is an error, and so is setting both num-images and interval in the config file.
• --min-images/--max-images need an interval: on the command line they are an error unless an interval is in effect, from -i or from interval in the config file. If -n is in effect, min-images/max-images from the config file are ignored, so a config file can keep them for when an interval is used.
• Precedence: each option on the command line overrides its config key, like other options: -n overrides interval, -i overrides num-images, and --min-images/--max-images override min-images/max-images. For example, with interval = 2s and max-images = 4 in the config file, -i 1s keeps the limit of 4 and --max-images 0 removes it.
• API: save_images() gets interval, min_images and max_images arguments (max_images=None means no limit). To reject num_images and interval together, the default for num_images is now None, which means 3 unless interval is set. Existing calls behave the same. min_images and max_images are rejected without interval.
• With varying counts, the $IMAGE_NUMBER padding and the progress bar total use the actual image counts. save-html already handles a different number of images per scene.
• A very small interval can give more images than a scene has frames, so some frames repeat. Setting a large -n does the same, so I left it alone.
• I left out computing the interval automatically from the shortest and longest scenes, as mentioned in Dynamically adjust the number of output images of the scene #280.

Tests

• tests/test_output.py: image counts per scene for frame, second and timecode intervals, with and without min_images/max_images, on both the threaded and non-threaded paths; invalid arguments raise ValueError.
• tests/test_cli.py (in-process):
  • image counts per scene for -i alone (frames, seconds, timecode), with --min-images, with --max-images, with both, and with --max-images 0;
  • errors: -n with -i, --min-images/--max-images without an interval, --max-images below --min-images, --min-images 0, --max-images -1, an invalid duration, and a duration of 0;
  • config file: interval, min-images and max-images, the num-images conflict, invalid values, and precedence (-i, -n, --min-images and --max-images over the config file, and min-images/max-images ignored without an interval).

docs/cli.rst matches the output of docs/generate_cli_docs.py for save-images, and scenedetect.cfg and the changelog are updated.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Breakthrough]

Thanks, this looks great! Appreciate you taking the time to kick this off.

I left a few comments - would like to understand the tradeoffs a bit better between the different CLI approaches we already discussed. I would be okay landing this as-is, so feel free to push back if you think this is the right way to go.

If possible, can you look into how other projects handle cases like this and if there's a standardized/recommended way to specify options such as this on a command line tool? Hopefully the comments and concerns I added are useful, let me know if you have any questions.

[Breakthrough]

Do we need to support leaving MIN empty? Can we require it be declared? I'm assuming if we don't specify it just defaults to 1. I'm also curious, can we use nargs here or does that not work because of the subcommands?

E.g. it would be cool if we made -i take 1/2/3 args conditionally using spaces as separators rather than requiring commas. Not sure how much effort that would be though, so happy to punt on this if it becomes a rabbit hole - just curious as to your thoughts.

Context: It's possible in some locales timecodes use commas, so eventually we might need to support that. The key word being eventually though, which is why for today I'd say this is optional, but I do want to explore what options we have and the tradeoffs for each before shipping something.

[Rober1208]

Thanks for the questions! I switched -i to space-separated values in 43e6968. Short answers here; the survey of other tools and the tradeoffs are in a comment on the PR below.

Q1, leaving MIN empty: no, we don't need it, so I removed it.

Q2, requiring MIN: yes. MIN and MAX are now positional, so MAX requires MIN. An omitted MIN defaults to 1, as you assumed. To set only a maximum you write -i 2s 1 10, which matches your original 1/2/3-arg shapes.

Q3, nargs: nargs can't do this, and the subcommands aren't the reason.

• Click rejects nargs=-1 on every option (TypeError: nargs=-1 is not supported for options.).
• A fixed nargs=3 would force all three values.
• The chained subcommands make it worse. In a small chained-group test with nargs=3, save-images -i 1s 3 list-scenes exits 0 with ('1s', '3', 'list-scenes'), so list-scenes silently never runs.

Q4, space-separated 1/2/3 args: it wasn't a rabbit hole, so it's in the update: -i 2s, -i 2s 3, -i 2s 1 10.

• A small click.Option subclass parses -i as usual, then also takes the following arguments while they start with a number.
• That's safe here because MIN and MAX are integers and no command or option name starts with a digit. So save-images -i 1s 3 list-scenes still runs list-scenes, and -i 1s 3 -o DIR still sees -o.
• Bad values (1.5, 2s, a fourth number, -3) are taken too, so they give an -i/--interval error instead of click's No such command '1.5'.
• On locale commas: with the old comma format, -i 00:00:01,500 was silently read as DURATION 00:00:01 with MIN 500. It's now an error, and commas stay free for decimal-comma timecodes later.

[Breakthrough]

Please use a named constant for this (we might have an epsilon value declared elsewhere as well).

[Rober1208]

Done in 43e6968: it's now _INTERVAL_COUNT_TOLERANCE: float = 1e-9 at the top of scenedetect/output/image.py, with a comment explaining it. The closest existing constant, MAX_FPS_DELTA in common.py, has the same value, but it's a tolerance for comparing frame rates, and this one is added to a unitless count of intervals, so I gave it its own private name, following _NTSC_DETECTION_TOLERANCE.

[Rober1208]

Thanks for the review! I've pushed 43e6968, which answers the inline comments: -i now takes space-separated values (-i 2s, -i 2s 3, -i 2s 1 10), MAX requires MIN, and the epsilon is a named constant. You asked how other projects handle this and what the tradeoffs are, so here's what I found.

How other tools do it

There's no standard for an option that takes a variable number of separate values:

• POSIX Guideline 8: an option with several values should take them as a single argument, separated by commas or blanks inside it. That describes the comma form (or a quoted -i "2s 1 10"), not separate words. Guideline 7 also says option-arguments shouldn't be optional.
• Click: options have rejected nargs=-1 since 4.0 (changelog). The request to allow it, pallets/click#484, was closed in 2016 as out of reach for click's parser.
• argparse: an option with nargs='?', '*' or '+' can't be followed by positionals or subcommands (bpo-9338 / cpython#53584, open since 2010).
• Separate --min/--max options are the most common shape for bounds:
  • find -mindepth/-maxdepth
  • fd --min-depth/--max-depth
  • rsync --min-size/--max-size
  • nmap --min-rate/--max-rate
  • yt-dlp --min-sleep-interval/--max-sleep-interval
  • The thumbnail tools vcsi (--interval, --num-samples) and mtn (-s step, -r/-c grid) also use separate options, not one combined value.
• Compact comma forms: sort -k (F[.C][OPTS][,F[.C][OPTS]]) and ImageMagick -level (black_point{,white_point}{%}{,gamma}). -level is the closest match to the old -i 2s,1,10.
• Variable count with a numeric lookahead: GDAL's gdal_translate -scale [src_min src_max [dst_min dst_max]] takes the next values only if they're numeric (source). That's the same technique this PR now uses.

Tradeoffs

	Comma: -i 2s,1,10	Spaces: -i 2s 1 10 (now in the PR)	Separate: -i 2s --min-images 1 --max-images 10
Convention	POSIX Guideline 8; sort -k, ImageMagick -level	Uncommon; GDAL -scale; looks like our --crop X0 Y0 X1 Y1	Most common for bounds (find, fd, rsync, nmap, yt-dlp, vcsi)
Chained commands	No ambiguity	Safe, since MIN/MAX are numbers and no command starts with a digit	No ambiguity
Locale commas	Conflicts (see below)	No conflict	No conflict
Config file	interval = 2s, 1, 10	interval = 2s 1 10	separate interval, min-images, max-images keys
MAX only	2s,,10	2s 1 10	--max-images 10
Click API	Public	Private parser internals	Public

Locale commas. Testing the comma format turned up a concrete problem:

• FrameTimecode only accepts . today, but the comma parser read -i 00:00:01,500 as DURATION 00:00:01 and MIN 500, with no error.
• That passes validation, so every scene would silently get at least 500 images.
• With spaces this is now an error: "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)."
• If FrameTimecode ever accepts decimal commas, -i won't need to change.
• The config file follows the same rule, so interval = 2s, 1, 10 now gets that hint. That's a small inconsistency with crop and weights, which ignore commas, but it keeps commas free for timecodes.

What it costs

• It uses click's private parser internals: the parser's _long_opt/_short_opt, the option's process, and state.rargs.
  • They are the same in click 8.0 through 8.5, and the project pins click~=8.0,!=8.3.0.
  • The 66 interval tests pass on click 8.0.0, 8.0.4, 8.1.7, 8.1.8, 8.2.1, 8.3.1, 8.3.3, 8.4.0, 8.4.2 and 8.5.0.
• Click has marked this parser deprecated since 8.2, to be removed in 9.0.
  • If the internals are missing, the hook is skipped instead of crashing (a test checks this), and -i "2s 1 10" still works as one quoted argument.
  • The chained-command tests would fail loudly if the behaviour changed.
• A missing DURATION (-i list-scenes) takes list-scenes as DURATION and fails with the DURATION error. Any click option with a value behaves this way.

If you'd rather depend only on public API, I can switch to separate --min-images/--max-images options. It's about the same amount of code, so I'm happy either way.

[Breakthrough]

If you'd rather depend only on public API, I can switch to separate --min-images/--max-images options. It's about the same amount of code, so I'm happy either way.

Thank you, let's go this route. Sorry I steered you in this direction, I didn't realize it would be so troublesome in practice.

[Rober1208]

Thanks! No worries at all, it was worth checking, and trying the variants out made the tradeoffs much clearer.

Switched to separate options in 479a545:

• -i/--interval DURATION takes one value, and --min-images N (default 1) and --max-images N (default 0, no limit) limit the number of images per scene, e.g. save-images -i 2s --max-images 10. In the config file they are interval, min-images and max-images under [save-images].
• -n and -i are still mutually exclusive. --min-images/--max-images are an error without an interval (from -i or the config file), and --max-images must be 0 or at least --min-images. Command line options override the config file per option, and min-images/max-images in the config file are ignored when -n is in effect.
• IntervalOption and the click parser hook are gone, so it only uses public click API.

I updated the PR description to match. Ready for another look.