From a31bdd85d325830e200a0fd796877c36d7f40bab Mon Sep 17 00:00:00 2001 From: Rober <124747128+Rober1208@users.noreply.github.com> Date: Thu, 22 Oct 2026 09:34:45 +0800 Subject: [PATCH 1/4] [cli] Add save-images -i/--interval to base image count on scene length With `-i DURATION[,MIN[,MAX]]`, each scene gets floor(length / DURATION) + 1 images instead of a fixed `-n`, clamped to [MIN, MAX] (default 1, no limit; an empty MIN or MAX uses the default, e.g. `-i 2s,,10`). The images are chosen the same way as with `-n`, including the frame margin. `-n` and `-i` are mutually exclusive, on the command line and in the config file. The API gets `interval`, `min_images` and `max_images` arguments on `save_images()`. `num_images` now defaults to None (3 unless `interval` is set) so that setting both can be rejected, and `min_images`/`max_images` are rejected without `interval`. Closes #280 Co-Authored-By: Claude Opus 5.5 --- docs/cli.rst | 8 +- scenedetect.cfg | 9 +- scenedetect/_cli/__init__.py | 49 ++++++++++- scenedetect/_cli/commands.py | 8 +- scenedetect/_cli/config.py | 52 +++++++++++ scenedetect/output/__init__.py | 4 +- scenedetect/output/image.py | 133 +++++++++++++++++++++++----- tests/test_cli.py | 156 +++++++++++++++++++++++++++++++++ tests/test_output.py | 67 ++++++++++++++ website/pages/changelog.md | 1 + 10 files changed, 459 insertions(+), 28 deletions(-) diff --git a/docs/cli.rst b/docs/cli.rst index b324e275..5808bccb 100644 --- a/docs/cli.rst +++ b/docs/cli.rst @@ -649,6 +649,8 @@ Examples ``scenedetect -i video.mp4 save-images --num-images 5`` + ``scenedetect -i video.mp4 save-images --interval 2s,1,10`` + ``scenedetect -i video.mp4 save-images --width 1024`` ``scenedetect -i video.mp4 save-images --filename \$SCENE_NUMBER-img\$IMAGE_NUMBER`` @@ -670,10 +672,14 @@ Options .. option:: -n N, --num-images N - Number of images to generate per scene. Will always include start/end frame, unless -n 1, in which case the image will be the frame at the mid-point of the scene. + Number of images to generate per scene. Will always include start/end frame, unless -n 1, in which case the image will be the frame at the mid-point of the scene. Cannot be used with :option:`-i/--interval <-i>`. Default: ``3`` +.. option:: -i DURATION[,MIN[,MAX]], --interval DURATION[,MIN[,MAX]] + + Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with :option:`-n/--num-images <-n>`. MIN and MAX limit the number of images per scene (default: 1, no limit). Leave MIN empty to only set MAX (-i 2s,,10). DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with :option:`-n/--num-images <-n>`. + .. option:: -j, --jpeg Set output format to JPEG (default). diff --git a/scenedetect.cfg b/scenedetect.cfg index 53e0e079..bf99be13 100644 --- a/scenedetect.cfg +++ b/scenedetect.cfg @@ -228,9 +228,16 @@ # Image format (jpeg, png, webp). #format = jpeg -# Number of images to generate for each scene. +# Number of images to generate for each scene. Cannot be used with interval. #num-images = 3 +# Generate one image per DURATION of each scene instead of num-images, so longer scenes +# get more images: floor(length / DURATION) + 1 per scene. Specified as DURATION[, MIN[, MAX]], +# where MIN and MAX limit the number of images per scene (default: 1, no limit). Leave MIN +# empty to only set MAX (2s, , 10). DURATION accepts frames (24), seconds with `s` suffix +# (1.5s), or timecode (00:00:01.500). +#interval = 2s, 1, 10 + # Image quality (jpeg/webp). Default is 95 for jpeg, 100 for webp #quality = 95 diff --git a/scenedetect/_cli/__init__.py b/scenedetect/_cli/__init__.py index fef4ddec..9ab51ad3 100644 --- a/scenedetect/_cli/__init__.py +++ b/scenedetect/_cli/__init__.py @@ -34,6 +34,7 @@ CONFIG_MAP, DEFAULT_JPG_QUALITY, DEFAULT_WEBP_QUALITY, + ImageIntervalValue, RangeValue, ) from scenedetect._cli.context import USER_CONFIG, CliContext, check_split_video_requirements @@ -1400,6 +1401,8 @@ def split_video_command( {scenedetect_with_video} save-images --num-images 5 + {scenedetect_with_video} save-images --interval 2s,1,10 + {scenedetect_with_video} save-images --width 1024 {scenedetect_with_video} save-images --filename \\$SCENE_NUMBER-img\\$IMAGE_NUMBER @@ -1432,10 +1435,20 @@ def split_video_command( metavar="N", default=None, type=click.INT, - help="Number of images to generate per scene. Will always include start/end frame, unless -n 1, in which case the image will be the frame at the mid-point of the scene.{}".format( + help="Number of images to generate per scene. Will always include start/end frame, unless -n 1, in which case the image will be the frame at the mid-point of the scene. Cannot be used with -i/--interval.{}".format( USER_CONFIG.get_help_string("save-images", "num-images") ), ) +@click.option( + "--interval", + "-i", + metavar="DURATION[,MIN[,MAX]]", + default=None, + type=click.STRING, + help="Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with -n/--num-images. MIN and MAX limit the number of images per scene (default: 1, no limit). Leave MIN empty to only set MAX (-i 2s,,10). DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with -n/--num-images.{}".format( + USER_CONFIG.get_help_string("save-images", "interval", show_default=False) + ), +) @click.option( "--jpeg", "-j", @@ -1525,6 +1538,7 @@ def save_images_command( output: str | None = None, filename: str | None = None, num_images: int | None = None, + interval: str | None = None, jpeg: bool = False, webp: bool = False, quality: int | None = None, @@ -1577,6 +1591,34 @@ def save_images_command( raise click.BadParameter("\n".join(error_strs), param_hint="save-images") output = ctx.config.get_value("save-images", "output", output) + # -n/--num-images and -i/--interval are mutually exclusive. If neither is set, use the + # config file, where only one of them may be set. + if num_images is not None and interval is not None: + raise click.BadParameter( + "Only one of -n/--num-images or -i/--interval can be set.", param_hint="save-images" + ) + if interval is not None: + try: + interval_value = ImageIntervalValue(interval).value + except ValueError as ex: + raise click.BadParameter(str(ex), param_hint="-i/--interval") from ex + elif num_images is not None: + interval_value = None + else: + if not ctx.config.is_default("save-images", "interval") and not ctx.config.is_default( + "save-images", "num-images" + ): + raise click.BadParameter( + "Only one of num-images or interval can be set in the config file.", + param_hint="save-images", + ) + interval_value = ctx.config.get_value("save-images", "interval") + if interval_value is None: + num_images = ctx.config.get_value("save-images", "num-images") + (interval, min_images, max_images) = ( + interval_value if interval_value is not None else (None, 1, None) + ) + save_images_args = { "encoder_param": compression if png else quality, "frame_margin": ctx.config.get_value("save-images", "frame-margin", frame_margin), @@ -1584,7 +1626,10 @@ def save_images_command( "image_extension": image_extension, "filename": ctx.config.get_value("save-images", "filename", filename), "interpolation": scale_method, - "num_images": ctx.config.get_value("save-images", "num-images", num_images), + "interval": interval, + "max_images": max_images, + "min_images": min_images, + "num_images": num_images, "output": output, "scale": scale, "show_progress": not ctx.quiet_mode, diff --git a/scenedetect/_cli/commands.py b/scenedetect/_cli/commands.py index 478ff1dc..7667f163 100644 --- a/scenedetect/_cli/commands.py +++ b/scenedetect/_cli/commands.py @@ -194,7 +194,10 @@ def save_images( context: CliContext, scenes: SceneList, cuts: CutList, - num_images: int, + num_images: int | None, + interval: str | None, + min_images: int, + max_images: int | None, frame_margin: int, image_extension: str, encoder_param: int, @@ -215,6 +218,9 @@ def save_images( scene_list=scenes, video=context.video_stream, num_images=num_images, + interval=interval, + min_images=min_images, + max_images=max_images, frame_margin=frame_margin, image_extension=image_extension, encoder_param=encoder_param, diff --git a/scenedetect/_cli/config.py b/scenedetect/_cli/config.py index bc033ac0..bb556dac 100644 --- a/scenedetect/_cli/config.py +++ b/scenedetect/_cli/config.py @@ -96,6 +96,57 @@ def from_config(config_value: str, default: "TimecodeValue") -> "TimecodeValue": ) from ex +class ImageIntervalValue(ValidatedValue): + """Validator for the save-images interval, specified as DURATION[,MIN[,MAX]]. + + DURATION is a timecode, and MIN/MAX are the minimum/maximum number of images per scene. An + empty MIN or MAX uses the default (e.g. `2s,,10` only sets MAX).""" + + def __init__(self, value: str | None = None): + self._value: tuple[str, int, int | None] | None = None + if value is None: + return + values = [val.strip() for val in value.split(",")] + if not 1 <= len(values) <= 3 or not values[0]: + raise ValueError("Interval must be specified as DURATION[,MIN[,MAX]].") + duration = values[0] + try: + duration_secs = FrameTimecode(timecode=duration, fps=100.0).seconds + except ValueError as ex: + raise ValueError( + "Interval DURATION must be in frames (24), seconds (1.5s), or HH:MM:SS." + ) from ex + if duration_secs <= 0: + raise ValueError("Interval DURATION must be greater than 0.") + try: + min_images = int(values[1]) if len(values) > 1 and values[1] else 1 + max_images = int(values[2]) if len(values) > 2 and values[2] else None + except ValueError as ex: + raise ValueError("Interval MIN and MAX must be numbers of images.") from ex + if min_images < 1: + raise ValueError("Interval MIN must be at least 1.") + if max_images is not None and max_images < min_images: + raise ValueError("Interval MAX must be greater than or equal to MIN.") + self._value = (duration, min_images, max_images) + + @property + def value(self) -> tuple[str, int, int | None] | None: + return self._value + + def __str__(self) -> str: + if self._value is None: + return "(none)" + duration, min_images, max_images = self._value + return ", ".join(str(val) for val in (duration, min_images, max_images) if val is not None) + + @staticmethod + def from_config(config_value: str, default: "ImageIntervalValue") -> "ImageIntervalValue": + try: + return ImageIntervalValue(config_value) + except ValueError as ex: + raise OptionParseFailure(f"{ex}") from ex + + class RangeValue(ValidatedValue): """Validator for int/float ranges. `min_val` and `max_val` are inclusive.""" @@ -442,6 +493,7 @@ class FcpFormat(Enum): "format": "jpeg", "frame-margin": TimecodeValue(1), "height": 0, + "interval": ImageIntervalValue(), "num-images": 3, "output": None, "quality": RangeValue(_PLACEHOLDER, min_val=0, max_val=100), diff --git a/scenedetect/output/__init__.py b/scenedetect/output/__init__.py index 02f109cd..b0d56b35 100644 --- a/scenedetect/output/__init__.py +++ b/scenedetect/output/__init__.py @@ -228,8 +228,8 @@ def write_scene_list_html( the start times of each scene (besides the 0th scene) is used instead. css: String containing all the css information for the resulting html page. css_class: String containing the named css class - image_filenames: dict where key i contains a list with n elements (filenames of - the n saved images from that scene) + image_filenames: dict where key i contains a list of the filenames of the images + saved from that scene (the number of images can differ between scenes) image_width: Optional desired width of images in table in pixels image_height: Optional desired height of images in table in pixels """ diff --git a/scenedetect/output/image.py b/scenedetect/output/image.py index b7fced6a..a9feb460 100644 --- a/scenedetect/output/image.py +++ b/scenedetect/output/image.py @@ -35,13 +35,44 @@ logger = logging.getLogger("pyscenedetect") +def _get_num_images( + scene_list: SceneList, + num_images: int | None, + interval: TimecodeLike | None, + min_images: int, + max_images: int | None, +) -> list[int]: + """Get the number of images to generate for each scene in `scene_list`. + + If `interval` is set, each scene gets ``floor(scene length / interval) + 1`` images, clamped + to [`min_images`, `max_images`]. Otherwise, each scene gets `num_images`. + """ + if interval is None: + assert num_images is not None + return [num_images] * len(scene_list) + frame_rate = scene_list[0][0].frame_rate + assert frame_rate is not None + interval_secs = FrameTimecode(timecode=interval, fps=frame_rate).seconds + result = [] + for start, end in scene_list: + duration_secs = max((end - start).seconds, 0.0) + # The epsilon keeps a scene that is a whole number of intervals long from getting one + # image less due to floating point error. + scene_images = max(math.floor(duration_secs / interval_secs + 1e-9) + 1, min_images) + if max_images is not None: + scene_images = min(scene_images, max_images) + result.append(scene_images) + return result + + def _generate_timecode_list( scene_list: SceneList, - num_images: int, + num_images_per_scene: list[int], frame_margin: TimecodeLike, ) -> list[list[FrameTimecode]]: """Generate per-scene image timecodes using PTS-accurate seconds-based timing. + `num_images_per_scene` is the number of images to generate for each scene in `scene_list`. `frame_margin` accepts any :data:`TimecodeLike` value (e.g. ``int`` frames, ``float`` seconds, or ``str`` such as ``"0.1s"``). """ @@ -49,7 +80,7 @@ def _generate_timecode_list( assert frame_rate is not None margin_secs = FrameTimecode(timecode=frame_margin, fps=frame_rate).seconds result = [] - for start, end in scene_list: + for (start, end), num_images in zip(scene_list, num_images_per_scene, strict=True): duration_secs = (end - start).seconds if duration_secs <= 0: result.append([start] * num_images) @@ -72,6 +103,14 @@ def _generate_timecode_list( return result +def _describe_num_images( + num_images: int | None, interval: TimecodeLike | None, total_images: int +) -> str: + if interval is None: + return f"{num_images} images per scene" + return f"{total_images} images (interval: {interval})" + + def _scale_image( image: np.ndarray, aspect_ratio: float | None, @@ -108,7 +147,7 @@ def _scale_image( class _ImageExtractor: def __init__( self, - num_images: int = 3, + num_images: int | None = 3, frame_margin: TimecodeLike = 1, image_extension: str = "jpg", imwrite_param: list[int] | None = None, @@ -117,6 +156,9 @@ def __init__( height: int | None = None, width: int | None = None, interpolation: Interpolation = Interpolation.CUBIC, + interval: TimecodeLike | None = None, + min_images: int = 1, + max_images: int | None = None, ): """Multi-threaded implementation of save-images functionality. Uses background threads to handle image encoding and saving images to disk to improve parallelism. @@ -124,7 +166,8 @@ def __init__( This object is thread-safe. Arguments: - num_images: Number of images to generate for each scene. Minimum is 1. + num_images: Number of images to generate for each scene. Minimum is 1. Must be None + if `interval` is set. frame_margin: Padding around the beginning and end of each scene used when selecting which frames to extract. Accepts an int (frames), float (seconds), or str (e.g. ``"0.1s"``, ``"00:00:00.100"``). Can be 0, but some video files @@ -149,8 +192,15 @@ def __init__( Specifying only width will rescale the image to that number of pixels wide while preserving the aspect ratio. interpolation: Type of interpolation to use when resizing images. + interval: If set, generate one image per `interval` of each scene's length instead of + `num_images`, see :func:`save_images`. + min_images: Minimum number of images to generate for each scene with `interval`. + max_images: Maximum number of images to generate for each scene with `interval`. """ self._num_images = num_images + self._interval = interval + self._min_images = min_images + self._max_images = max_images self._frame_margin = frame_margin self._image_extension = image_extension self._image_name_template = image_name_template @@ -175,19 +225,20 @@ def run( output_dir: Directory to write files to. show_progress: If `true` and tqdm is available, shows a progress bar. """ + timecode_list = self.generate_timecode_list(scene_list) + total_images = sum(len(timecodes) for timecodes in timecode_list) + max_scene_images = max(len(timecodes) for timecodes in timecode_list) + # Setup flags and init progress bar if available. completed = True logger.info( - f"Saving {self._num_images} images per scene [format={self._image_extension}]" - f" {output_dir if output_dir else ''} " + f"Saving {_describe_num_images(self._num_images, self._interval, total_images)}" + f" [format={self._image_extension}] {output_dir if output_dir else ''} " ) progress_bar = None if show_progress: - progress_bar = tqdm( - total=len(scene_list) * self._num_images, unit="images", dynamic_ncols=True - ) + progress_bar = tqdm(total=total_images, unit="images", dynamic_ncols=True) - timecode_list = self.generate_timecode_list(scene_list) image_filenames = {i: [] for i in range(len(timecode_list))} filename_template = Template(self._image_name_template) @@ -195,7 +246,7 @@ def run( scene_num_format = "%0" scene_num_format += str(max(3, math.floor(math.log(len(scene_list), 10)) + 1)) + "d" image_num_format = "%0" - image_num_format += str(math.floor(math.log(self._num_images, 10)) + 2) + "d" + image_num_format += str(math.floor(math.log(max_scene_images, 10)) + 2) + "d" def format_filename(scene_number: int, image_number: int, image_timecode: FrameTimecode): return "{}.{}".format( @@ -337,7 +388,10 @@ def generate_timecode_list(self, scene_list: SceneList) -> list[list[FrameTimeco Uses PTS-accurate seconds-based timing so results are correct for both CFR and VFR video. """ - return _generate_timecode_list(scene_list, self._num_images, self._frame_margin) + num_images_per_scene = _get_num_images( + scene_list, self._num_images, self._interval, self._min_images, self._max_images + ) + return _generate_timecode_list(scene_list, num_images_per_scene, self._frame_margin) def resize_image( self, @@ -352,7 +406,7 @@ def resize_image( def save_images( scene_list: SceneList, video: VideoStream, - num_images: int = 3, + num_images: int | None = None, frame_margin: TimecodeLike = 1, image_extension: str = "jpg", encoder_param: int = 95, @@ -364,6 +418,9 @@ def save_images( width: int | None = None, interpolation: Interpolation = Interpolation.CUBIC, threading: bool = True, + interval: TimecodeLike | None = None, + min_images: int = 1, + max_images: int | None = None, ) -> dict[int, list[str]]: """Save a set number of images from each scene, given a list of scenes and the associated video/frame source. @@ -373,7 +430,8 @@ def save_images( from calling a SceneManager's detect_scenes() method. video: A VideoStream object corresponding to the scene list. Note that the video will be closed/re-opened and seeked through. - num_images: Number of images to generate for each scene. Minimum is 1. + num_images: Number of images to generate for each scene. Minimum is 1. Defaults to 3 + if `interval` is not set. Cannot be used with `interval`. frame_margin: Padding around the beginning and end of each scene used when selecting which frames to extract. Accepts an int (frames), float (seconds), or str (e.g. ``"0.1s"``, ``"00:00:00.100"``). Can be 0, but some video files @@ -402,6 +460,14 @@ def save_images( while preserving the aspect ratio. interpolation: Type of interpolation to use when resizing images. threading: Offload image encoding and disk IO to background threads to improve performance. + interval: If set, the number of images for each scene depends on its length instead of + being `num_images`: each scene gets ``floor(scene length / interval) + 1`` images, + spread over the scene the same way. Accepts the same values as `frame_margin`. + Cannot be used with `num_images`. + min_images: Minimum number of images to generate for each scene with `interval`. + Minimum is 1. Can only be set with `interval`. + max_images: Maximum number of images to generate for each scene with `interval`, or + None for no limit. Can only be set with `interval`. Returns: Dictionary of the format { scene_num : [image_paths] }, where scene_num is the @@ -415,7 +481,23 @@ def save_images( if not scene_list: return {} - if num_images <= 0: + if interval is not None: + if num_images is not None: + raise ValueError("num_images and interval cannot both be set") + frame_rate = scene_list[0][0].frame_rate + assert frame_rate is not None + if FrameTimecode(timecode=interval, fps=frame_rate).seconds <= 0: + raise ValueError("interval must be greater than 0") + if min_images <= 0: + raise ValueError("min_images must be greater than 0") + if max_images is not None and max_images < min_images: + raise ValueError("max_images must be greater than or equal to min_images") + else: + if min_images != 1 or max_images is not None: + raise ValueError("min_images and max_images can only be used with interval") + if num_images is None: + num_images = 3 + if num_images is not None and num_images <= 0: raise ValueError("num_images must be greater than 0") if isinstance(frame_margin, (int, float)) and frame_margin < 0: raise ValueError("frame_margin must be non-negative") @@ -440,27 +522,36 @@ def save_images( height, width, interpolation, + interval, + min_images, + max_images, ) return extractor.run(video, scene_list, output_dir, bool(show_progress)) + timecode_list = _generate_timecode_list( + scene_list, + _get_num_images(scene_list, num_images, interval, min_images, max_images), + frame_margin, + ) + total_images = sum(len(timecodes) for timecodes in timecode_list) + max_scene_images = max(len(timecodes) for timecodes in timecode_list) + # Setup flags and init progress bar if available. completed = True logger.info( - f"Saving {num_images} images per scene [format={image_extension}]" - f" {output_dir if output_dir else ''} " + f"Saving {_describe_num_images(num_images, interval, total_images)}" + f" [format={image_extension}] {output_dir if output_dir else ''} " ) progress_bar = None if show_progress: - progress_bar = tqdm(total=len(scene_list) * num_images, unit="images", dynamic_ncols=True) + progress_bar = tqdm(total=total_images, unit="images", dynamic_ncols=True) filename_template = Template(image_name_template) scene_num_format = "%0" scene_num_format += str(max(3, math.floor(math.log(len(scene_list), 10)) + 1)) + "d" image_num_format = "%0" - image_num_format += str(math.floor(math.log(num_images, 10)) + 2) + "d" - - timecode_list = _generate_timecode_list(scene_list, num_images, frame_margin) + image_num_format += str(math.floor(math.log(max_scene_images, 10)) + 2) + "d" image_filenames = {i: [] for i in range(len(timecode_list))} aspect_ratio = video.aspect_ratio diff --git a/tests/test_cli.py b/tests/test_cli.py index b96661c1..d1c881bb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -11,6 +11,7 @@ # import os +import re import subprocess # These tests validate that the CLI itself functions correctly, mainly based on the return @@ -36,6 +37,7 @@ import pytest import scenedetect +from scenedetect._cli.config import ConfigLoadFailure, ConfigRegistry, ImageIntervalValue from scenedetect.output import is_ffmpeg_available, is_mkvmerge_available from scenedetect.platform import StrPath from tests.helpers import invoke_cli @@ -668,6 +670,160 @@ def test_cli_save_images_rotation(rotated_video_file, tmp_path: Path): assert image.shape == (1280, 544, 3) +_TIME_AND_DETECTOR = ["time", *DEFAULT_TIME.split(), DEFAULT_DETECTOR] + + +def _count_images_per_scene(path: Path) -> list[int]: + return [ + len(list(path.glob(f"{DEFAULT_VIDEO_NAME}-Scene-{scene + 1:03d}-*.jpg"))) + for scene in range(DEFAULT_NUM_SCENES) + ] + + +@pytest.mark.parametrize( + "interval,expected", + [ + # The two scenes are 1.752s and 2.252s long: floor(length / 1s) + 1 images each. + ("1s", [2, 3]), + ("00:00:01.000", [2, 3]), + # MIN and MAX limit the number of images per scene. + ("1s,3", [3, 3]), + ("0.5s,1,4", [4, 4]), + ("0.5s,,4", [4, 4]), + ], +) +def test_cli_save_images_interval(tmp_path: Path, interval: str, expected: list[int]): + """Test `save-images -i/--interval`.""" + assert ( + invoke_scenedetect( + f"-i {{VIDEO}} -s {{STATS}} time {{TIME}} {{DETECTOR}} save-images -i {interval}", + output_dir=tmp_path, + ) + == 0 + ) + assert _count_images_per_scene(tmp_path) == expected + + +@pytest.mark.parametrize( + "value,expected", + [ + ("1s", ("1s", 1, None)), + ("24", ("24", 1, None)), + ("00:00:01.500", ("00:00:01.500", 1, None)), + ("2s,3", ("2s", 3, None)), + ("2s,1,10", ("2s", 1, 10)), + ("2s, 1, 10", ("2s", 1, 10)), + # An empty MIN or MAX uses the default. + ("2s,,10", ("2s", 1, 10)), + ("2s,3,", ("2s", 3, None)), + ], +) +def test_image_interval_value(value: str, expected: tuple): + assert ImageIntervalValue(value).value == expected + + +@pytest.mark.parametrize( + "value,error", + [ + ("", "Interval must be specified as DURATION[,MIN[,MAX]]."), + (",3", "Interval must be specified as DURATION[,MIN[,MAX]]."), + ("1s,1,2,3", "Interval must be specified as DURATION[,MIN[,MAX]]."), + ("abc", "Interval DURATION must be in frames"), + ("1s 2 3", "Interval DURATION must be in frames"), + ("0", "Interval DURATION must be greater than 0."), + ("0s", "Interval DURATION must be greater than 0."), + ("1s,x", "Interval MIN and MAX must be numbers of images."), + ("1s,0", "Interval MIN must be at least 1."), + ("1s,3,2", "Interval MAX must be greater than or equal to MIN."), + ], +) +def test_image_interval_value_rejects_invalid_values(value: str, error: str): + with pytest.raises(ValueError, match=re.escape(error)): + ImageIntervalValue(value) + + +@pytest.mark.parametrize( + "args,error", + [ + (["-n", "3", "-i", "1s"], "Only one of -n/--num-images or -i/--interval can be set."), + (["-i", "1s,3,2"], "Interval MAX must be greater than or equal to MIN."), + ], +) +def test_cli_save_images_interval_rejects_invalid_values( + tmp_path: Path, args: list[str], error: str +): + exit_code, output = invoke_cli( + ["-i", DEFAULT_VIDEO_PATH, "-o", str(tmp_path), *_TIME_AND_DETECTOR, "save-images", *args] + ) + assert exit_code != 0 + assert error in output + assert not list(tmp_path.glob("*.jpg")) + + +@pytest.mark.parametrize( + "config,args,expected", + [ + ("interval = 1s, 3", "", [3, 3]), + # Options on the command line take precedence over the config file. + ("interval = 1s, 3", "-n 2", [2, 2]), + ("num-images = 5", "-i 1s", [2, 3]), + ], +) +def test_cli_save_images_interval_config_file( + tmp_path: Path, config: str, args: str, expected: list[int] +): + """Test the `interval` option of `[save-images]` in a config file.""" + config_path = tmp_path.joinpath("config.cfg") + config_path.write_text(f"[save-images]\n{config}\n") + assert ( + invoke_scenedetect( + f"-i {{VIDEO}} -s {{STATS}} time {{TIME}} {{DETECTOR}} save-images {args}", + output_dir=tmp_path, + config_file=str(config_path), + ) + == 0 + ) + assert _count_images_per_scene(tmp_path) == expected + + +@pytest.mark.parametrize( + "config,error", + [ + ("interval = 1s, 0", "Interval MIN must be at least 1."), + ("interval = abc", "Interval DURATION must be in frames"), + ], +) +def test_cli_save_images_interval_config_file_rejects_invalid_values( + tmp_path: Path, config: str, error: str +): + config_path = tmp_path.joinpath("config.cfg") + config_path.write_text(f"[save-images]\n{config}\n") + with pytest.raises(ConfigLoadFailure) as ex: + ConfigRegistry(str(config_path)) + assert any(error in message for _, message in ex.value.init_log) + + +def test_cli_save_images_interval_config_file_rejects_num_images(tmp_path: Path): + """`num-images` and `interval` are mutually exclusive in the config file.""" + config_path = tmp_path.joinpath("config.cfg") + config_path.write_text("[save-images]\nnum-images = 3\ninterval = 1s\n") + exit_code, output = invoke_cli( + [ + "-c", + str(config_path), + "-i", + DEFAULT_VIDEO_PATH, + "-o", + str(tmp_path), + *_TIME_AND_DETECTOR, + "save-images", + ] + ) + assert exit_code != 0 + assert "Only one of num-images or interval can be set in the config file." in output + assert not list(tmp_path.glob("*.jpg")) + + def test_cli_save_html(tmp_path: Path): """Test `save-html` command.""" base_command = "-i {VIDEO} -s {STATS} time {TIME} {DETECTOR} {COMMAND}" diff --git a/tests/test_output.py b/tests/test_output.py index da7a23d2..78b957a7 100644 --- a/tests/test_output.py +++ b/tests/test_output.py @@ -237,6 +237,73 @@ def test_save_images_zero_width_scene(test_video_file, tmp_path: Path): assert total_images == len([path for path in tmp_path.glob(image_name_glob)]) +@pytest.mark.parametrize("threading", [True, False]) +@pytest.mark.parametrize( + "interval,min_images,max_images,expected", + [ + # Scenes are 100, 10, and 60 frames long: floor(length / 25) + 1 images each. + (25, 1, None, [5, 1, 3]), + ("25", 1, None, [5, 1, 3]), + (25, 2, 4, [4, 2, 3]), + # 1 second is 29.97 frames. + ("1s", 1, None, [4, 1, 3]), + (1.0, 1, None, [4, 1, 3]), + ], +) +def test_save_images_interval( + test_video_file, tmp_path: Path, threading, interval, min_images, max_images, expected +): + """With `interval`, the number of images saved for each scene depends on its length.""" + video = VideoStreamCv2(test_video_file) + video_fps = video.frame_rate + scene_list = [ + (FrameTimecode(start, video_fps), FrameTimecode(end, video_fps)) + for start, end in [(0, 100), (200, 210), (300, 360)] + ] + image_filenames = save_images( + scene_list=scene_list, + output_dir=tmp_path, + video=video, + interval=interval, + min_images=min_images, + max_images=max_images, + image_name_template="scenedetect.tempfile.$SCENE_NUMBER.$IMAGE_NUMBER", + threading=threading, + ) + assert [len(image_filenames[scene]) for scene in sorted(image_filenames)] == expected + for paths in image_filenames.values(): + for path in paths: + assert tmp_path.joinpath(path).exists(), f"expected {path} to exist" + assert sum(expected) == len(list(tmp_path.glob("scenedetect.tempfile.*.jpg"))) + + +@pytest.mark.parametrize( + "kwargs,error", + [ + ({"num_images": 3, "interval": 25}, "num_images and interval cannot both be set"), + ({"interval": 0}, "interval must be greater than 0"), + ({"interval": "0s"}, "interval must be greater than 0"), + ({"interval": 25, "min_images": 0}, "min_images must be greater than 0"), + ( + {"interval": 25, "min_images": 3, "max_images": 2}, + "max_images must be greater than or equal to min_images", + ), + ( + {"num_images": 3, "max_images": 2}, + "min_images and max_images can only be used with interval", + ), + ({"min_images": 2}, "min_images and max_images can only be used with interval"), + ], +) +def test_save_images_interval_rejects_invalid_arguments( + test_video_file, tmp_path: Path, kwargs, error +): + video = VideoStreamCv2(test_video_file) + scene_list = [(FrameTimecode(0, video.frame_rate), FrameTimecode(10, video.frame_rate))] + with pytest.raises(ValueError, match=error): + save_images(scene_list=scene_list, output_dir=tmp_path, video=video, **kwargs) + + # # Scene-list export API (EDL / FCPXML / FCP7 xmeml / OTIO) # diff --git a/website/pages/changelog.md b/website/pages/changelog.md index 57aa57c9..55ff909e 100644 --- a/website/pages/changelog.md +++ b/website/pages/changelog.md @@ -5,6 +5,7 @@ ### Unreleased + - [feature] Add `-i`/`--interval` to `save-images` (`interval` in the config file, and `interval`/`min_images`/`max_images` in `save_images()`) to save images based on the length of each scene: one image per `DURATION`, with an optional minimum and maximum number of images per scene, e.g. `save-images -i 2s,1,10` [#280](https://github.com/Breakthrough/PySceneDetect/issues/280) - [bugfix] `$VIDEO_NAME` is now derived from the path component of URL inputs, ignoring the query string and fragment (e.g. `https://host/clip.mp4?token=1.2` -> `clip`). URLs with no path use the host. Previously the query string leaked into output filenames, which fails on Windows. [#197](https://github.com/Breakthrough/PySceneDetect/issues/197) - [bugfix] OTIO exports use the full source duration when provided, including from the CLI, instead of declaring a fixed 1,980-frame duration. The available range remains unspecified when the duration is unknown. Scene source ranges are unchanged. [#580](https://github.com/Breakthrough/PySceneDetect/pull/580) - [bugfix] The OpenCV backend keeps `position` on the last frame after reaching the end of a VFR video, so the last scene no longer ends early (or before it starts) [#561](https://github.com/Breakthrough/PySceneDetect/issues/561) From 43e6968fbf0276722e73c5fc7038b348647ce034 Mon Sep 17 00:00:00 2001 From: Rober <124747128+Rober1208@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:27:37 +0800 Subject: [PATCH 2/4] [cli] Take -i/--interval values as separate arguments Following the review on #588, `save-images -i` now takes `DURATION [MIN [MAX]]` as separate arguments (`-i 2s 1 10`) instead of one comma-separated value, and MAX can only be set together with MIN. Click options can't take a variable number of values, so `IntervalOption` also takes the arguments that follow `-i` while they start with a number. No command or option name does, so a following command or option is left alone. This uses click parser internals that are unchanged in click 8.0 through 8.5; if they are missing, the hook is skipped and the values can still be passed as one quoted argument. Commas are no longer separators, so a timecode with a decimal comma such as `00:00:01,500` is now an error instead of DURATION `00:00:01` with MIN 500. The config file uses the same format (`interval = 2s 1 10`), and a comma gives a hint to separate the values with spaces. The tolerance used when counting intervals per scene is now the named constant `_INTERVAL_COUNT_TOLERANCE`. Co-Authored-By: Claude Opus 5.5 --- docs/cli.rst | 6 +- scenedetect.cfg | 10 +-- scenedetect/_cli/__init__.py | 48 +++++++++++- scenedetect/_cli/config.py | 22 +++--- scenedetect/output/image.py | 10 ++- tests/test_cli.py | 140 ++++++++++++++++++++++++++++++----- website/pages/changelog.md | 2 +- 7 files changed, 194 insertions(+), 44 deletions(-) diff --git a/docs/cli.rst b/docs/cli.rst index 5808bccb..5e7f56d2 100644 --- a/docs/cli.rst +++ b/docs/cli.rst @@ -649,7 +649,7 @@ Examples ``scenedetect -i video.mp4 save-images --num-images 5`` - ``scenedetect -i video.mp4 save-images --interval 2s,1,10`` + ``scenedetect -i video.mp4 save-images --interval 2s 1 10`` ``scenedetect -i video.mp4 save-images --width 1024`` @@ -676,9 +676,9 @@ Options Default: ``3`` -.. option:: -i DURATION[,MIN[,MAX]], --interval DURATION[,MIN[,MAX]] +.. option:: -i DURATION [MIN [MAX]], --interval DURATION [MIN [MAX]] - Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with :option:`-n/--num-images <-n>`. MIN and MAX limit the number of images per scene (default: 1, no limit). Leave MIN empty to only set MAX (-i 2s,,10). DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with :option:`-n/--num-images <-n>`. + Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with :option:`-n/--num-images <-n>`. MIN and MAX limit the number of images per scene (default: 1, no limit), e.g. -i 2s 1 10. MAX requires MIN. DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with :option:`-n/--num-images <-n>`. .. option:: -j, --jpeg diff --git a/scenedetect.cfg b/scenedetect.cfg index bf99be13..3724c7e3 100644 --- a/scenedetect.cfg +++ b/scenedetect.cfg @@ -232,11 +232,11 @@ #num-images = 3 # Generate one image per DURATION of each scene instead of num-images, so longer scenes -# get more images: floor(length / DURATION) + 1 per scene. Specified as DURATION[, MIN[, MAX]], -# where MIN and MAX limit the number of images per scene (default: 1, no limit). Leave MIN -# empty to only set MAX (2s, , 10). DURATION accepts frames (24), seconds with `s` suffix -# (1.5s), or timecode (00:00:01.500). -#interval = 2s, 1, 10 +# get more images: floor(length / DURATION) + 1 per scene. Specified as DURATION [MIN [MAX]], +# separated by spaces, where MIN and MAX limit the number of images per scene (default: 1, no +# limit). MAX requires MIN, e.g. 2s 1 10 for at most 10 images. DURATION accepts frames (24), +# seconds with `s` suffix (1.5s), or timecode (00:00:01.500). +#interval = 2s 1 10 # Image quality (jpeg/webp). Default is 95 for jpeg, 100 for webp #quality = 95 diff --git a/scenedetect/_cli/__init__.py b/scenedetect/_cli/__init__.py index 9ab51ad3..4362584c 100644 --- a/scenedetect/_cli/__init__.py +++ b/scenedetect/_cli/__init__.py @@ -22,6 +22,7 @@ import logging import os import os.path +import re from copy import copy import click @@ -150,6 +151,46 @@ class CommandGroup(Command, click.Group): pass +# Matches an argument that starts with a number, e.g. `10`, `-3`, `1.5`, or `2s`. No command or +# option name starts with a digit (there are no options like `-1`), so a following command (e.g. +# `list-scenes`) or option (e.g. `-o DIR`) never matches. +_STARTS_WITH_NUMBER = re.compile(r"[+-]?[0-9]") + + +class IntervalOption(click.Option): + """Option that takes 1 to 3 space-separated values: DURATION [MIN [MAX]]. + + Click options only support a fixed number of values (`nargs=-1` is not allowed), so the first + value is parsed as usual, and the arguments that follow are also taken while they start with a + number. Invalid values (e.g. `1.5` or a fourth number) are therefore reported as errors for + this option instead of as unknown commands. The values are passed on joined by spaces (e.g. + "2s 1 10"), and checked by `ImageIntervalValue`. + """ + + def add_to_parser(self, parser, ctx) -> None: + super().add_to_parser(parser, ctx) + # Click has no public API for this, so it relies on parser internals that are the same in + # click 8.0 through 8.5: the parser's `_long_opt`/`_short_opt` dicts, `obj`/`process` of + # their option objects, and `state.rargs`. If they are missing (click plans to replace its + # parser in 9.0), the hook is skipped and the values can still be quoted (-i "2s 1 10"). + parser_opts = [ + *getattr(parser, "_long_opt", {}).values(), + *getattr(parser, "_short_opt", {}).values(), + ] + parser_opt = next((opt for opt in parser_opts if getattr(opt, "obj", None) is self), None) + if parser_opt is None or not hasattr(parser_opt, "process"): + return + process = parser_opt.process + + def process_with_extra_values(value, state) -> None: + values = [value] + while state.rargs and _STARTS_WITH_NUMBER.match(state.rargs[0]): + values.append(state.rargs.pop(0)) + process(" ".join(values), state) + + parser_opt.process = process_with_extra_values + + def print_command_help(ctx: click.Context, command: click.Command): """Print help/usage for a given command. Modifies `ctx` in-place.""" ctx.info_name = command.name @@ -1401,7 +1442,7 @@ def split_video_command( {scenedetect_with_video} save-images --num-images 5 - {scenedetect_with_video} save-images --interval 2s,1,10 + {scenedetect_with_video} save-images --interval 2s 1 10 {scenedetect_with_video} save-images --width 1024 @@ -1442,10 +1483,11 @@ def split_video_command( @click.option( "--interval", "-i", - metavar="DURATION[,MIN[,MAX]]", + cls=IntervalOption, + metavar="DURATION [MIN [MAX]]", default=None, type=click.STRING, - help="Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with -n/--num-images. MIN and MAX limit the number of images per scene (default: 1, no limit). Leave MIN empty to only set MAX (-i 2s,,10). DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with -n/--num-images.{}".format( + help="Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with -n/--num-images. MIN and MAX limit the number of images per scene (default: 1, no limit), e.g. -i 2s 1 10. MAX requires MIN. DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with -n/--num-images.{}".format( USER_CONFIG.get_help_string("save-images", "interval", show_default=False) ), ) diff --git a/scenedetect/_cli/config.py b/scenedetect/_cli/config.py index bb556dac..8e7b24d3 100644 --- a/scenedetect/_cli/config.py +++ b/scenedetect/_cli/config.py @@ -97,18 +97,22 @@ def from_config(config_value: str, default: "TimecodeValue") -> "TimecodeValue": class ImageIntervalValue(ValidatedValue): - """Validator for the save-images interval, specified as DURATION[,MIN[,MAX]]. + """Validator for the save-images interval, specified as DURATION [MIN [MAX]]. - DURATION is a timecode, and MIN/MAX are the minimum/maximum number of images per scene. An - empty MIN or MAX uses the default (e.g. `2s,,10` only sets MAX).""" + DURATION is a timecode, and MIN/MAX are the minimum/maximum number of images per scene. The + values are separated by spaces, so MAX can only be set together with MIN (e.g. `2s 1 10`).""" def __init__(self, value: str | None = None): self._value: tuple[str, int, int | None] | None = None if value is None: return - values = [val.strip() for val in value.split(",")] - if not 1 <= len(values) <= 3 or not values[0]: - raise ValueError("Interval must be specified as DURATION[,MIN[,MAX]].") + if "," in value: + raise ValueError( + "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)." + ) + values = value.split() + if not 1 <= len(values) <= 3: + raise ValueError("Interval must be specified as DURATION [MIN [MAX]].") duration = values[0] try: duration_secs = FrameTimecode(timecode=duration, fps=100.0).seconds @@ -119,8 +123,8 @@ def __init__(self, value: str | None = None): if duration_secs <= 0: raise ValueError("Interval DURATION must be greater than 0.") try: - min_images = int(values[1]) if len(values) > 1 and values[1] else 1 - max_images = int(values[2]) if len(values) > 2 and values[2] else None + min_images = int(values[1]) if len(values) > 1 else 1 + max_images = int(values[2]) if len(values) > 2 else None except ValueError as ex: raise ValueError("Interval MIN and MAX must be numbers of images.") from ex if min_images < 1: @@ -137,7 +141,7 @@ def __str__(self) -> str: if self._value is None: return "(none)" duration, min_images, max_images = self._value - return ", ".join(str(val) for val in (duration, min_images, max_images) if val is not None) + return " ".join(str(val) for val in (duration, min_images, max_images) if val is not None) @staticmethod def from_config(config_value: str, default: "ImageIntervalValue") -> "ImageIntervalValue": diff --git a/scenedetect/output/image.py b/scenedetect/output/image.py index a9feb460..5388e7eb 100644 --- a/scenedetect/output/image.py +++ b/scenedetect/output/image.py @@ -34,6 +34,11 @@ logger = logging.getLogger("pyscenedetect") +# Added to the ratio of scene length to interval (a number of intervals, so it has no unit) before +# rounding down, so floating point error doesn't drop an image from a scene that is a whole number +# of intervals long (e.g. 0.3 / 0.1 gives 2.9999999999999996). +_INTERVAL_COUNT_TOLERANCE: float = 1e-9 + def _get_num_images( scene_list: SceneList, @@ -56,9 +61,8 @@ def _get_num_images( result = [] for start, end in scene_list: duration_secs = max((end - start).seconds, 0.0) - # The epsilon keeps a scene that is a whole number of intervals long from getting one - # image less due to floating point error. - scene_images = max(math.floor(duration_secs / interval_secs + 1e-9) + 1, min_images) + num_intervals = math.floor(duration_secs / interval_secs + _INTERVAL_COUNT_TOLERANCE) + scene_images = max(num_intervals + 1, min_images) if max_images is not None: scene_images = min(scene_images, max_images) result.append(scene_images) diff --git a/tests/test_cli.py b/tests/test_cli.py index d1c881bb..9d214abe 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -32,11 +32,13 @@ import sys from pathlib import Path +import click import cv2 import numpy as np import pytest import scenedetect +from scenedetect._cli import IntervalOption from scenedetect._cli.config import ConfigLoadFailure, ConfigRegistry, ImageIntervalValue from scenedetect.output import is_ffmpeg_available, is_mkvmerge_available from scenedetect.platform import StrPath @@ -687,9 +689,10 @@ def _count_images_per_scene(path: Path) -> list[int]: ("1s", [2, 3]), ("00:00:01.000", [2, 3]), # MIN and MAX limit the number of images per scene. - ("1s,3", [3, 3]), - ("0.5s,1,4", [4, 4]), - ("0.5s,,4", [4, 4]), + ("1s 3", [3, 3]), + ("0.5s 1 4", [4, 4]), + # DURATION in frames (24 frames is about 1s here). + ("24 2 2", [2, 2]), ], ) def test_cli_save_images_interval(tmp_path: Path, interval: str, expected: list[int]): @@ -704,37 +707,110 @@ def test_cli_save_images_interval(tmp_path: Path, interval: str, expected: list[ assert _count_images_per_scene(tmp_path) == expected +@pytest.mark.parametrize( + "args,expected", + [ + (["-i", "1s"], [2, 3]), + (["-i", "1s", "3"], [3, 3]), + (["-i", "0.5s", "1", "4"], [4, 4]), + (["-i", "24"], [2, 3]), + (["--interval=1s", "3"], [3, 3]), + (["-i1s", "3"], [3, 3]), + # The values can also be given as one argument. + (["-i", "1s 3"], [3, 3]), + ], +) +def test_cli_save_images_interval_followed_by_command( + tmp_path: Path, args: list[str], expected: list[int] +): + """Only the values of -i/--interval are taken, so a following command still runs.""" + exit_code, _ = invoke_cli( + [ + "-i", + DEFAULT_VIDEO_PATH, + "-o", + str(tmp_path), + *_TIME_AND_DETECTOR, + "save-images", + *args, + "list-scenes", + ] + ) + assert exit_code == 0 + assert _count_images_per_scene(tmp_path) == expected + assert tmp_path.joinpath(f"{DEFAULT_VIDEO_NAME}-Scenes.csv").exists() + + +def test_cli_save_images_interval_followed_by_option(tmp_path: Path): + """Only the values of -i/--interval are taken, so a following option is still parsed.""" + image_dir = tmp_path / "images" + exit_code, _ = invoke_cli( + [ + "-i", + DEFAULT_VIDEO_PATH, + "-o", + str(tmp_path), + *_TIME_AND_DETECTOR, + "save-images", + "-i", + "1s", + "3", + "-o", + str(image_dir), + "-q", + "90", + ] + ) + assert exit_code == 0 + assert _count_images_per_scene(image_dir) == [3, 3] + assert not list(tmp_path.glob("*.jpg")) + + +def test_interval_option_without_parser_internals(): + """IntervalOption skips its hook if click's parser internals are missing (e.g. click 9).""" + + class Parser: + def add_option(self, **kwargs): + pass + + option = IntervalOption(["--interval", "-i"]) + option.add_to_parser(Parser(), click.Context(click.Command("save-images"))) # type: ignore[arg-type] + + @pytest.mark.parametrize( "value,expected", [ ("1s", ("1s", 1, None)), ("24", ("24", 1, None)), ("00:00:01.500", ("00:00:01.500", 1, None)), - ("2s,3", ("2s", 3, None)), - ("2s,1,10", ("2s", 1, 10)), - ("2s, 1, 10", ("2s", 1, 10)), - # An empty MIN or MAX uses the default. - ("2s,,10", ("2s", 1, 10)), - ("2s,3,", ("2s", 3, None)), + ("2s 3", ("2s", 3, None)), + ("2s 1 10", ("2s", 1, 10)), + (" 2s 1\t10 ", ("2s", 1, 10)), ], ) def test_image_interval_value(value: str, expected: tuple): assert ImageIntervalValue(value).value == expected + # The string form (shown in --help when set in a config file) can be parsed again. + assert ImageIntervalValue(str(ImageIntervalValue(value))).value == expected @pytest.mark.parametrize( "value,error", [ - ("", "Interval must be specified as DURATION[,MIN[,MAX]]."), - (",3", "Interval must be specified as DURATION[,MIN[,MAX]]."), - ("1s,1,2,3", "Interval must be specified as DURATION[,MIN[,MAX]]."), + ("", "Interval must be specified as DURATION [MIN [MAX]]."), + ("1s 1 2 3", "Interval must be specified as DURATION [MIN [MAX]]."), ("abc", "Interval DURATION must be in frames"), - ("1s 2 3", "Interval DURATION must be in frames"), ("0", "Interval DURATION must be greater than 0."), ("0s", "Interval DURATION must be greater than 0."), - ("1s,x", "Interval MIN and MAX must be numbers of images."), - ("1s,0", "Interval MIN must be at least 1."), - ("1s,3,2", "Interval MAX must be greater than or equal to MIN."), + ("1s x", "Interval MIN and MAX must be numbers of images."), + ("1s 1.5", "Interval MIN and MAX must be numbers of images."), + ("1s 2s", "Interval MIN and MAX must be numbers of images."), + ("1s 0", "Interval MIN must be at least 1."), + ("1s 3 2", "Interval MAX must be greater than or equal to MIN."), + # Commas are not separators, and are not accepted in timecodes. + ("1s,3", "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)."), + ("2s, 1, 10", "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)."), + ("00:00:01,500", "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)."), ], ) def test_image_interval_value_rejects_invalid_values(value: str, error: str): @@ -746,7 +822,28 @@ def test_image_interval_value_rejects_invalid_values(value: str, error: str): "args,error", [ (["-n", "3", "-i", "1s"], "Only one of -n/--num-images or -i/--interval can be set."), - (["-i", "1s,3,2"], "Interval MAX must be greater than or equal to MIN."), + (["-i", "1s", "3", "-n", "3"], "Only one of -n/--num-images or -i/--interval can be set."), + ( + ["-i", "1s", "3", "2"], + "Invalid value for -i/--interval: Interval MAX must be greater than or equal to MIN.", + ), + (["-i", "1s", "-3"], "Invalid value for -i/--interval: Interval MIN must be at least 1."), + ( + ["-i", "1s", "3", "10", "7", "list-scenes"], + "Invalid value for -i/--interval: Interval must be specified as DURATION [MIN [MAX]].", + ), + ( + ["-i", "1s", "1.5"], + "Invalid value for -i/--interval: Interval MIN and MAX must be numbers of images.", + ), + ( + ["-i", "1s", "2s"], + "Invalid value for -i/--interval: Interval MIN and MAX must be numbers of images.", + ), + ( + ["-i", "1s,3"], + "Invalid value for -i/--interval: Interval values must be separated by spaces, not commas (e.g. 2s 1 10).", + ), ], ) def test_cli_save_images_interval_rejects_invalid_values( @@ -763,9 +860,11 @@ def test_cli_save_images_interval_rejects_invalid_values( @pytest.mark.parametrize( "config,args,expected", [ - ("interval = 1s, 3", "", [3, 3]), + ("interval = 1s 3", "", [3, 3]), + # The two scenes are 1.752s and 2.252s long: floor(length / 2s) + 1 images each. + ("interval = 2s 1 10", "", [1, 2]), # Options on the command line take precedence over the config file. - ("interval = 1s, 3", "-n 2", [2, 2]), + ("interval = 1s 3", "-n 2", [2, 2]), ("num-images = 5", "-i 1s", [2, 3]), ], ) @@ -789,8 +888,9 @@ def test_cli_save_images_interval_config_file( @pytest.mark.parametrize( "config,error", [ - ("interval = 1s, 0", "Interval MIN must be at least 1."), + ("interval = 1s 0", "Interval MIN must be at least 1."), ("interval = abc", "Interval DURATION must be in frames"), + ("interval = 1s, 3", "Interval values must be separated by spaces, not commas"), ], ) def test_cli_save_images_interval_config_file_rejects_invalid_values( diff --git a/website/pages/changelog.md b/website/pages/changelog.md index 55ff909e..0950b177 100644 --- a/website/pages/changelog.md +++ b/website/pages/changelog.md @@ -5,7 +5,7 @@ ### Unreleased - - [feature] Add `-i`/`--interval` to `save-images` (`interval` in the config file, and `interval`/`min_images`/`max_images` in `save_images()`) to save images based on the length of each scene: one image per `DURATION`, with an optional minimum and maximum number of images per scene, e.g. `save-images -i 2s,1,10` [#280](https://github.com/Breakthrough/PySceneDetect/issues/280) + - [feature] Add `-i`/`--interval` to `save-images` (`interval` in the config file, and `interval`/`min_images`/`max_images` in `save_images()`) to save images based on the length of each scene: one image per `DURATION`, with an optional minimum and maximum number of images per scene, e.g. `save-images -i 2s 1 10` [#280](https://github.com/Breakthrough/PySceneDetect/issues/280) - [bugfix] `$VIDEO_NAME` is now derived from the path component of URL inputs, ignoring the query string and fragment (e.g. `https://host/clip.mp4?token=1.2` -> `clip`). URLs with no path use the host. Previously the query string leaked into output filenames, which fails on Windows. [#197](https://github.com/Breakthrough/PySceneDetect/issues/197) - [bugfix] OTIO exports use the full source duration when provided, including from the CLI, instead of declaring a fixed 1,980-frame duration. The available range remains unspecified when the duration is unknown. Scene source ranges are unchanged. [#580](https://github.com/Breakthrough/PySceneDetect/pull/580) - [bugfix] The OpenCV backend keeps `position` on the last frame after reaching the end of a VFR video, so the last scene no longer ends early (or before it starts) [#561](https://github.com/Breakthrough/PySceneDetect/issues/561) From 479a54534f4df37b684db0a8fd8884e241eaee8d Mon Sep 17 00:00:00 2001 From: Rober <124747128+Rober1208@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:24:21 +0800 Subject: [PATCH 3/4] [cli] Use --min-images/--max-images with save-images -i Following the review decision on #588, `save-images -i/--interval` now takes a single DURATION, and the minimum and maximum number of images per scene are set with separate `--min-images` and `--max-images` options (default 1 and 0 for no limit), e.g. `-i 2s --max-images 10`. They are `min-images`/`max-images` in the config file, where `interval` is now a plain duration. `--min-images`/`--max-images` on the command line are an error unless an interval is in effect (from `-i` or the config file), and `--max-images` must be 0 or at least `--min-images`. If `-n/--num-images` is in effect, `min-images`/`max-images` from the config file are ignored. This removes `IntervalOption` and its click parser hook, along with the help text, config comments and tests for the space-separated values. Co-Authored-By: Claude Opus 5.5 --- docs/cli.rst | 18 ++- scenedetect.cfg | 16 +- scenedetect/_cli/__init__.py | 105 +++++++------ scenedetect/_cli/config.py | 54 ++----- tests/test_cli.py | 276 ++++++++++++----------------------- website/pages/changelog.md | 2 +- 6 files changed, 186 insertions(+), 285 deletions(-) diff --git a/docs/cli.rst b/docs/cli.rst index 5e7f56d2..7685db12 100644 --- a/docs/cli.rst +++ b/docs/cli.rst @@ -649,7 +649,7 @@ Examples ``scenedetect -i video.mp4 save-images --num-images 5`` - ``scenedetect -i video.mp4 save-images --interval 2s 1 10`` + ``scenedetect -i video.mp4 save-images --interval 2s --max-images 10`` ``scenedetect -i video.mp4 save-images --width 1024`` @@ -676,9 +676,21 @@ Options Default: ``3`` -.. option:: -i DURATION [MIN [MAX]], --interval DURATION [MIN [MAX]] +.. option:: -i DURATION, --interval DURATION - Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with :option:`-n/--num-images <-n>`. MIN and MAX limit the number of images per scene (default: 1, no limit), e.g. -i 2s 1 10. MAX requires MIN. DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with :option:`-n/--num-images <-n>`. + Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with :option:`-n/--num-images <-n>`. Use --min-images and --max-images to limit the number of images per scene. DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with :option:`-n/--num-images <-n>`. + +.. option:: --min-images N + + Minimum number of images per scene. Can only be used with :option:`-i/--interval <-i>`. + + Default: ``1`` + +.. option:: --max-images N + + Maximum number of images per scene, or 0 for no limit. Can only be used with :option:`-i/--interval <-i>`. + + Default: ``0`` .. option:: -j, --jpeg diff --git a/scenedetect.cfg b/scenedetect.cfg index 3724c7e3..46c36106 100644 --- a/scenedetect.cfg +++ b/scenedetect.cfg @@ -231,12 +231,16 @@ # Number of images to generate for each scene. Cannot be used with interval. #num-images = 3 -# Generate one image per DURATION of each scene instead of num-images, so longer scenes -# get more images: floor(length / DURATION) + 1 per scene. Specified as DURATION [MIN [MAX]], -# separated by spaces, where MIN and MAX limit the number of images per scene (default: 1, no -# limit). MAX requires MIN, e.g. 2s 1 10 for at most 10 images. DURATION accepts frames (24), -# seconds with `s` suffix (1.5s), or timecode (00:00:01.500). -#interval = 2s 1 10 +# Generate one image per DURATION of each scene instead of num-images, so longer +# scenes get more images: floor(length / DURATION) + 1 per scene. DURATION accepts +# frames (24), seconds with `s` suffix (1.5s), or timecode (00:00:01.500). +# Cannot be used with num-images. +#interval = 2s + +# Minimum and maximum number of images per scene with interval (ignored without +# interval). Set max-images to 0 for no limit. +#min-images = 1 +#max-images = 0 # Image quality (jpeg/webp). Default is 95 for jpeg, 100 for webp #quality = 95 diff --git a/scenedetect/_cli/__init__.py b/scenedetect/_cli/__init__.py index 4362584c..73ac84be 100644 --- a/scenedetect/_cli/__init__.py +++ b/scenedetect/_cli/__init__.py @@ -22,7 +22,6 @@ import logging import os import os.path -import re from copy import copy import click @@ -151,46 +150,6 @@ class CommandGroup(Command, click.Group): pass -# Matches an argument that starts with a number, e.g. `10`, `-3`, `1.5`, or `2s`. No command or -# option name starts with a digit (there are no options like `-1`), so a following command (e.g. -# `list-scenes`) or option (e.g. `-o DIR`) never matches. -_STARTS_WITH_NUMBER = re.compile(r"[+-]?[0-9]") - - -class IntervalOption(click.Option): - """Option that takes 1 to 3 space-separated values: DURATION [MIN [MAX]]. - - Click options only support a fixed number of values (`nargs=-1` is not allowed), so the first - value is parsed as usual, and the arguments that follow are also taken while they start with a - number. Invalid values (e.g. `1.5` or a fourth number) are therefore reported as errors for - this option instead of as unknown commands. The values are passed on joined by spaces (e.g. - "2s 1 10"), and checked by `ImageIntervalValue`. - """ - - def add_to_parser(self, parser, ctx) -> None: - super().add_to_parser(parser, ctx) - # Click has no public API for this, so it relies on parser internals that are the same in - # click 8.0 through 8.5: the parser's `_long_opt`/`_short_opt` dicts, `obj`/`process` of - # their option objects, and `state.rargs`. If they are missing (click plans to replace its - # parser in 9.0), the hook is skipped and the values can still be quoted (-i "2s 1 10"). - parser_opts = [ - *getattr(parser, "_long_opt", {}).values(), - *getattr(parser, "_short_opt", {}).values(), - ] - parser_opt = next((opt for opt in parser_opts if getattr(opt, "obj", None) is self), None) - if parser_opt is None or not hasattr(parser_opt, "process"): - return - process = parser_opt.process - - def process_with_extra_values(value, state) -> None: - values = [value] - while state.rargs and _STARTS_WITH_NUMBER.match(state.rargs[0]): - values.append(state.rargs.pop(0)) - process(" ".join(values), state) - - parser_opt.process = process_with_extra_values - - def print_command_help(ctx: click.Context, command: click.Command): """Print help/usage for a given command. Modifies `ctx` in-place.""" ctx.info_name = command.name @@ -1442,7 +1401,7 @@ def split_video_command( {scenedetect_with_video} save-images --num-images 5 - {scenedetect_with_video} save-images --interval 2s 1 10 + {scenedetect_with_video} save-images --interval 2s --max-images 10 {scenedetect_with_video} save-images --width 1024 @@ -1483,14 +1442,31 @@ def split_video_command( @click.option( "--interval", "-i", - cls=IntervalOption, - metavar="DURATION [MIN [MAX]]", + metavar="DURATION", default=None, type=click.STRING, - help="Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with -n/--num-images. MIN and MAX limit the number of images per scene (default: 1, no limit), e.g. -i 2s 1 10. MAX requires MIN. DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with -n/--num-images.{}".format( + help="Generate one image per DURATION of each scene instead of a fixed number, so longer scenes get more images. Each scene gets floor(length / DURATION) + 1 images, chosen the same way as with -n/--num-images. Use --min-images and --max-images to limit the number of images per scene. DURATION can be specified in frames (-i 24), in seconds with `s` suffix (-i 1.5s), or timecode (-i 00:00:01.500). Cannot be used with -n/--num-images.{}".format( USER_CONFIG.get_help_string("save-images", "interval", show_default=False) ), ) +@click.option( + "--min-images", + metavar="N", + default=None, + type=click.IntRange(min=1), + help="Minimum number of images per scene. Can only be used with -i/--interval.{}".format( + USER_CONFIG.get_help_string("save-images", "min-images") + ), +) +@click.option( + "--max-images", + metavar="N", + default=None, + type=click.IntRange(min=0), + help="Maximum number of images per scene, or 0 for no limit. Can only be used with -i/--interval.{}".format( + USER_CONFIG.get_help_string("save-images", "max-images") + ), +) @click.option( "--jpeg", "-j", @@ -1581,6 +1557,8 @@ def save_images_command( filename: str | None = None, num_images: int | None = None, interval: str | None = None, + min_images: int | None = None, + max_images: int | None = None, jpeg: bool = False, webp: bool = False, quality: int | None = None, @@ -1641,12 +1619,10 @@ def save_images_command( ) if interval is not None: try: - interval_value = ImageIntervalValue(interval).value + ImageIntervalValue(interval) except ValueError as ex: raise click.BadParameter(str(ex), param_hint="-i/--interval") from ex - elif num_images is not None: - interval_value = None - else: + elif num_images is None: if not ctx.config.is_default("save-images", "interval") and not ctx.config.is_default( "save-images", "num-images" ): @@ -1654,12 +1630,33 @@ def save_images_command( "Only one of num-images or interval can be set in the config file.", param_hint="save-images", ) - interval_value = ctx.config.get_value("save-images", "interval") - if interval_value is None: + interval = ctx.config.get_value("save-images", "interval") + if interval is None: num_images = ctx.config.get_value("save-images", "num-images") - (interval, min_images, max_images) = ( - interval_value if interval_value is not None else (None, 1, None) - ) + # --min-images/--max-images only apply with an interval (from the command line or the config + # file). Without one, they are an error on the command line, and ignored in the config file. + if interval is None: + if min_images is not None or max_images is not None: + raise click.BadParameter( + "--min-images and --max-images can only be used with -i/--interval.", + param_hint="save-images", + ) + min_images, max_images = 1, None + else: + min_images_value: int = ctx.config.get_value("save-images", "min-images", min_images) + max_images_value: int = ctx.config.get_value("save-images", "max-images", max_images) + # Click checks the range of values from the command line, but not from the config file. + if min_images_value < 1: + raise click.BadParameter( + "min-images in the config file must be at least 1.", param_hint="save-images" + ) + if max_images_value != 0 and max_images_value < min_images_value: + raise click.BadParameter( + "max-images must be 0 (no limit) or at least min-images.", + param_hint="save-images", + ) + # A max-images of 0 means no limit. + min_images, max_images = min_images_value, max_images_value or None save_images_args = { "encoder_param": compression if png else quality, diff --git a/scenedetect/_cli/config.py b/scenedetect/_cli/config.py index 8e7b24d3..779de30b 100644 --- a/scenedetect/_cli/config.py +++ b/scenedetect/_cli/config.py @@ -97,52 +97,26 @@ def from_config(config_value: str, default: "TimecodeValue") -> "TimecodeValue": class ImageIntervalValue(ValidatedValue): - """Validator for the save-images interval, specified as DURATION [MIN [MAX]]. + """Validator for the save-images interval: a timecode greater than 0, or None if unset. - DURATION is a timecode, and MIN/MAX are the minimum/maximum number of images per scene. The - values are separated by spaces, so MAX can only be set together with MIN (e.g. `2s 1 10`).""" + Stores value in original representation, like `TimecodeValue`.""" def __init__(self, value: str | None = None): - self._value: tuple[str, int, int | None] | None = None - if value is None: - return - if "," in value: - raise ValueError( - "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)." - ) - values = value.split() - if not 1 <= len(values) <= 3: - raise ValueError("Interval must be specified as DURATION [MIN [MAX]].") - duration = values[0] - try: - duration_secs = FrameTimecode(timecode=duration, fps=100.0).seconds - except ValueError as ex: - raise ValueError( - "Interval DURATION must be in frames (24), seconds (1.5s), or HH:MM:SS." - ) from ex - if duration_secs <= 0: - raise ValueError("Interval DURATION must be greater than 0.") - try: - min_images = int(values[1]) if len(values) > 1 else 1 - max_images = int(values[2]) if len(values) > 2 else None - except ValueError as ex: - raise ValueError("Interval MIN and MAX must be numbers of images.") from ex - if min_images < 1: - raise ValueError("Interval MIN must be at least 1.") - if max_images is not None and max_images < min_images: - raise ValueError("Interval MAX must be greater than or equal to MIN.") - self._value = (duration, min_images, max_images) + if value is not None: + try: + seconds = FrameTimecode(timecode=value, fps=100.0).seconds + except ValueError as ex: + raise ValueError( + "Interval must be in frames (24), seconds (1.5s), or HH:MM:SS." + ) from ex + if seconds <= 0: + raise ValueError("Interval must be greater than 0.") + self._value = value @property - def value(self) -> tuple[str, int, int | None] | None: + def value(self) -> str | None: return self._value - def __str__(self) -> str: - if self._value is None: - return "(none)" - duration, min_images, max_images = self._value - return " ".join(str(val) for val in (duration, min_images, max_images) if val is not None) - @staticmethod def from_config(config_value: str, default: "ImageIntervalValue") -> "ImageIntervalValue": try: @@ -498,6 +472,8 @@ class FcpFormat(Enum): "frame-margin": TimecodeValue(1), "height": 0, "interval": ImageIntervalValue(), + "max-images": 0, + "min-images": 1, "num-images": 3, "output": None, "quality": RangeValue(_PLACEHOLDER, min_val=0, max_val=100), diff --git a/tests/test_cli.py b/tests/test_cli.py index 9d214abe..cdb737f0 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -11,7 +11,6 @@ # import os -import re import subprocess # These tests validate that the CLI itself functions correctly, mainly based on the return @@ -32,14 +31,12 @@ import sys from pathlib import Path -import click import cv2 import numpy as np import pytest import scenedetect -from scenedetect._cli import IntervalOption -from scenedetect._cli.config import ConfigLoadFailure, ConfigRegistry, ImageIntervalValue +from scenedetect._cli.config import ConfigLoadFailure, ConfigRegistry from scenedetect.output import is_ffmpeg_available, is_mkvmerge_available from scenedetect.platform import StrPath from tests.helpers import invoke_cli @@ -682,50 +679,17 @@ def _count_images_per_scene(path: Path) -> list[int]: ] -@pytest.mark.parametrize( - "interval,expected", - [ - # The two scenes are 1.752s and 2.252s long: floor(length / 1s) + 1 images each. - ("1s", [2, 3]), - ("00:00:01.000", [2, 3]), - # MIN and MAX limit the number of images per scene. - ("1s 3", [3, 3]), - ("0.5s 1 4", [4, 4]), - # DURATION in frames (24 frames is about 1s here). - ("24 2 2", [2, 2]), - ], -) -def test_cli_save_images_interval(tmp_path: Path, interval: str, expected: list[int]): - """Test `save-images -i/--interval`.""" - assert ( - invoke_scenedetect( - f"-i {{VIDEO}} -s {{STATS}} time {{TIME}} {{DETECTOR}} save-images -i {interval}", - output_dir=tmp_path, - ) - == 0 - ) - assert _count_images_per_scene(tmp_path) == expected - - -@pytest.mark.parametrize( - "args,expected", - [ - (["-i", "1s"], [2, 3]), - (["-i", "1s", "3"], [3, 3]), - (["-i", "0.5s", "1", "4"], [4, 4]), - (["-i", "24"], [2, 3]), - (["--interval=1s", "3"], [3, 3]), - (["-i1s", "3"], [3, 3]), - # The values can also be given as one argument. - (["-i", "1s 3"], [3, 3]), - ], -) -def test_cli_save_images_interval_followed_by_command( - tmp_path: Path, args: list[str], expected: list[int] -): - """Only the values of -i/--interval are taken, so a following command still runs.""" - exit_code, _ = invoke_cli( +def _invoke_save_images( + tmp_path: Path, args: list[str], config: str | None = None +) -> tuple[int, str]: + """Run `save-images` with `args` in-process, using `config` as the [save-images] section of + the config file.""" + config_path = tmp_path / "config.cfg" + config_path.write_text(f"[save-images]\n{config or ''}\n") + return invoke_cli( [ + "-c", + str(config_path), "-i", DEFAULT_VIDEO_PATH, "-o", @@ -733,125 +697,63 @@ def test_cli_save_images_interval_followed_by_command( *_TIME_AND_DETECTOR, "save-images", *args, - "list-scenes", - ] - ) - assert exit_code == 0 - assert _count_images_per_scene(tmp_path) == expected - assert tmp_path.joinpath(f"{DEFAULT_VIDEO_NAME}-Scenes.csv").exists() - - -def test_cli_save_images_interval_followed_by_option(tmp_path: Path): - """Only the values of -i/--interval are taken, so a following option is still parsed.""" - image_dir = tmp_path / "images" - exit_code, _ = invoke_cli( - [ - "-i", - DEFAULT_VIDEO_PATH, - "-o", - str(tmp_path), - *_TIME_AND_DETECTOR, - "save-images", - "-i", - "1s", - "3", - "-o", - str(image_dir), - "-q", - "90", ] ) - assert exit_code == 0 - assert _count_images_per_scene(image_dir) == [3, 3] - assert not list(tmp_path.glob("*.jpg")) - - -def test_interval_option_without_parser_internals(): - """IntervalOption skips its hook if click's parser internals are missing (e.g. click 9).""" - - class Parser: - def add_option(self, **kwargs): - pass - - option = IntervalOption(["--interval", "-i"]) - option.add_to_parser(Parser(), click.Context(click.Command("save-images"))) # type: ignore[arg-type] - - -@pytest.mark.parametrize( - "value,expected", - [ - ("1s", ("1s", 1, None)), - ("24", ("24", 1, None)), - ("00:00:01.500", ("00:00:01.500", 1, None)), - ("2s 3", ("2s", 3, None)), - ("2s 1 10", ("2s", 1, 10)), - (" 2s 1\t10 ", ("2s", 1, 10)), - ], -) -def test_image_interval_value(value: str, expected: tuple): - assert ImageIntervalValue(value).value == expected - # The string form (shown in --help when set in a config file) can be parsed again. - assert ImageIntervalValue(str(ImageIntervalValue(value))).value == expected @pytest.mark.parametrize( - "value,error", + "args,expected", [ - ("", "Interval must be specified as DURATION [MIN [MAX]]."), - ("1s 1 2 3", "Interval must be specified as DURATION [MIN [MAX]]."), - ("abc", "Interval DURATION must be in frames"), - ("0", "Interval DURATION must be greater than 0."), - ("0s", "Interval DURATION must be greater than 0."), - ("1s x", "Interval MIN and MAX must be numbers of images."), - ("1s 1.5", "Interval MIN and MAX must be numbers of images."), - ("1s 2s", "Interval MIN and MAX must be numbers of images."), - ("1s 0", "Interval MIN must be at least 1."), - ("1s 3 2", "Interval MAX must be greater than or equal to MIN."), - # Commas are not separators, and are not accepted in timecodes. - ("1s,3", "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)."), - ("2s, 1, 10", "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)."), - ("00:00:01,500", "Interval values must be separated by spaces, not commas (e.g. 2s 1 10)."), + # The two scenes are 1.752s and 2.252s long: floor(length / 1s) + 1 images each. + (["-i", "1s"], [2, 3]), + (["-i", "1.5s"], [2, 2]), + (["--interval", "00:00:01.000"], [2, 3]), + # DURATION in frames (24 frames is about 1s here). + (["-i", "24"], [2, 3]), + # --min-images and --max-images limit the number of images per scene. + (["-i", "1s", "--min-images", "3"], [3, 3]), + (["-i", "0.5s", "--max-images", "4"], [4, 4]), + (["-i", "0.2s", "--min-images", "10", "--max-images", "11"], [10, 11]), + (["-i", "0.5s", "--max-images", "0"], [4, 5]), ], ) -def test_image_interval_value_rejects_invalid_values(value: str, error: str): - with pytest.raises(ValueError, match=re.escape(error)): - ImageIntervalValue(value) +def test_cli_save_images_interval(tmp_path: Path, args: list[str], expected: list[int]): + """Test `save-images -i/--interval` with --min-images/--max-images.""" + exit_code, _ = _invoke_save_images(tmp_path, args) + assert exit_code == 0 + assert _count_images_per_scene(tmp_path) == expected @pytest.mark.parametrize( "args,error", [ (["-n", "3", "-i", "1s"], "Only one of -n/--num-images or -i/--interval can be set."), - (["-i", "1s", "3", "-n", "3"], "Only one of -n/--num-images or -i/--interval can be set."), ( - ["-i", "1s", "3", "2"], - "Invalid value for -i/--interval: Interval MAX must be greater than or equal to MIN.", + ["--min-images", "2"], + "--min-images and --max-images can only be used with -i/--interval.", ), - (["-i", "1s", "-3"], "Invalid value for -i/--interval: Interval MIN must be at least 1."), ( - ["-i", "1s", "3", "10", "7", "list-scenes"], - "Invalid value for -i/--interval: Interval must be specified as DURATION [MIN [MAX]].", + ["-n", "3", "--max-images", "2"], + "--min-images and --max-images can only be used with -i/--interval.", ), ( - ["-i", "1s", "1.5"], - "Invalid value for -i/--interval: Interval MIN and MAX must be numbers of images.", + ["-i", "1s", "--min-images", "3", "--max-images", "2"], + "max-images must be 0 (no limit) or at least min-images.", ), + (["-i", "1s", "--min-images", "0"], "0 is not in the range x>=1."), + (["-i", "1s", "--max-images", "-1"], "-1 is not in the range x>=0."), ( - ["-i", "1s", "2s"], - "Invalid value for -i/--interval: Interval MIN and MAX must be numbers of images.", - ), - ( - ["-i", "1s,3"], - "Invalid value for -i/--interval: Interval values must be separated by spaces, not commas (e.g. 2s 1 10).", + ["-i", "abc"], + "Invalid value for -i/--interval: Interval must be in frames (24), seconds (1.5s), or HH:MM:SS.", ), + (["-i", "0"], "Invalid value for -i/--interval: Interval must be greater than 0."), + (["-i", "0s"], "Invalid value for -i/--interval: Interval must be greater than 0."), ], ) def test_cli_save_images_interval_rejects_invalid_values( tmp_path: Path, args: list[str], error: str ): - exit_code, output = invoke_cli( - ["-i", DEFAULT_VIDEO_PATH, "-o", str(tmp_path), *_TIME_AND_DETECTOR, "save-images", *args] - ) + exit_code, output = _invoke_save_images(tmp_path, args) assert exit_code != 0 assert error in output assert not list(tmp_path.glob("*.jpg")) @@ -860,42 +762,73 @@ def test_cli_save_images_interval_rejects_invalid_values( @pytest.mark.parametrize( "config,args,expected", [ - ("interval = 1s 3", "", [3, 3]), - # The two scenes are 1.752s and 2.252s long: floor(length / 2s) + 1 images each. - ("interval = 2s 1 10", "", [1, 2]), + ("interval = 1s", [], [2, 3]), + ("interval = 1s\nmin-images = 3", [], [3, 3]), + ("interval = 0.5s\nmax-images = 4", [], [4, 4]), # Options on the command line take precedence over the config file. - ("interval = 1s 3", "-n 2", [2, 2]), - ("num-images = 5", "-i 1s", [2, 3]), + ("interval = 2s\nmax-images = 2", ["-i", "0.5s"], [2, 2]), + ("interval = 0.5s\nmax-images = 4", ["--max-images", "2"], [2, 2]), + ("interval = 0.5s\nmax-images = 4", ["--max-images", "0"], [4, 5]), + ("interval = 2s", ["--min-images", "3"], [3, 3]), + ("num-images = 5", ["-i", "1s"], [2, 3]), + # Without an interval in effect, min-images and max-images in the config file are ignored. + ("interval = 1s\nmin-images = 5\nmax-images = 5", ["-n", "2"], [2, 2]), + ("num-images = 2\nmin-images = 5", [], [2, 2]), ], ) def test_cli_save_images_interval_config_file( - tmp_path: Path, config: str, args: str, expected: list[int] + tmp_path: Path, config: str, args: list[str], expected: list[int] ): - """Test the `interval` option of `[save-images]` in a config file.""" - config_path = tmp_path.joinpath("config.cfg") - config_path.write_text(f"[save-images]\n{config}\n") - assert ( - invoke_scenedetect( - f"-i {{VIDEO}} -s {{STATS}} time {{TIME}} {{DETECTOR}} save-images {args}", - output_dir=tmp_path, - config_file=str(config_path), - ) - == 0 - ) + """Test the `interval`, `min-images` and `max-images` options of `[save-images]`.""" + exit_code, _ = _invoke_save_images(tmp_path, args, config=config) + assert exit_code == 0 assert _count_images_per_scene(tmp_path) == expected @pytest.mark.parametrize( - "config,error", + "config,args,error", [ - ("interval = 1s 0", "Interval MIN must be at least 1."), - ("interval = abc", "Interval DURATION must be in frames"), - ("interval = 1s, 3", "Interval values must be separated by spaces, not commas"), + ( + "num-images = 3\ninterval = 1s", + [], + "Only one of num-images or interval can be set in the config file.", + ), + ( + "num-images = 3", + ["--max-images", "2"], + "--min-images and --max-images can only be used with -i/--interval.", + ), + ("interval = 1s\nmin-images = 0", [], "min-images in the config file must be at least 1."), + ( + "interval = 1s\nmin-images = 3\nmax-images = 2", + [], + "max-images must be 0 (no limit) or at least min-images.", + ), + ( + "interval = 1s\nmax-images = -1", + [], + "max-images must be 0 (no limit) or at least min-images.", + ), ], ) def test_cli_save_images_interval_config_file_rejects_invalid_values( - tmp_path: Path, config: str, error: str + tmp_path: Path, config: str, args: list[str], error: str ): + exit_code, output = _invoke_save_images(tmp_path, args, config=config) + assert exit_code != 0 + assert error in output + assert not list(tmp_path.glob("*.jpg")) + + +@pytest.mark.parametrize( + "config,error", + [ + ("interval = abc", "Interval must be in frames (24), seconds (1.5s), or HH:MM:SS."), + ("interval = 0", "Interval must be greater than 0."), + ("min-images = x", "x is not a valid integer."), + ], +) +def test_cli_save_images_interval_config_file_load_errors(tmp_path: Path, config: str, error: str): config_path = tmp_path.joinpath("config.cfg") config_path.write_text(f"[save-images]\n{config}\n") with pytest.raises(ConfigLoadFailure) as ex: @@ -903,27 +836,6 @@ def test_cli_save_images_interval_config_file_rejects_invalid_values( assert any(error in message for _, message in ex.value.init_log) -def test_cli_save_images_interval_config_file_rejects_num_images(tmp_path: Path): - """`num-images` and `interval` are mutually exclusive in the config file.""" - config_path = tmp_path.joinpath("config.cfg") - config_path.write_text("[save-images]\nnum-images = 3\ninterval = 1s\n") - exit_code, output = invoke_cli( - [ - "-c", - str(config_path), - "-i", - DEFAULT_VIDEO_PATH, - "-o", - str(tmp_path), - *_TIME_AND_DETECTOR, - "save-images", - ] - ) - assert exit_code != 0 - assert "Only one of num-images or interval can be set in the config file." in output - assert not list(tmp_path.glob("*.jpg")) - - def test_cli_save_html(tmp_path: Path): """Test `save-html` command.""" base_command = "-i {VIDEO} -s {STATS} time {TIME} {DETECTOR} {COMMAND}" diff --git a/website/pages/changelog.md b/website/pages/changelog.md index 0950b177..2ab909e9 100644 --- a/website/pages/changelog.md +++ b/website/pages/changelog.md @@ -5,7 +5,7 @@ ### Unreleased - - [feature] Add `-i`/`--interval` to `save-images` (`interval` in the config file, and `interval`/`min_images`/`max_images` in `save_images()`) to save images based on the length of each scene: one image per `DURATION`, with an optional minimum and maximum number of images per scene, e.g. `save-images -i 2s 1 10` [#280](https://github.com/Breakthrough/PySceneDetect/issues/280) + - [feature] Add `-i`/`--interval` to `save-images` to save images based on the length of each scene: one image per `DURATION`, with `--min-images`/`--max-images` to limit the number of images per scene, e.g. `save-images -i 2s --max-images 10` (`interval`/`min-images`/`max-images` in the config file, and `interval`/`min_images`/`max_images` in `save_images()`) [#280](https://github.com/Breakthrough/PySceneDetect/issues/280) - [bugfix] `$VIDEO_NAME` is now derived from the path component of URL inputs, ignoring the query string and fragment (e.g. `https://host/clip.mp4?token=1.2` -> `clip`). URLs with no path use the host. Previously the query string leaked into output filenames, which fails on Windows. [#197](https://github.com/Breakthrough/PySceneDetect/issues/197) - [bugfix] OTIO exports use the full source duration when provided, including from the CLI, instead of declaring a fixed 1,980-frame duration. The available range remains unspecified when the duration is unknown. Scene source ranges are unchanged. [#580](https://github.com/Breakthrough/PySceneDetect/pull/580) - [bugfix] The OpenCV backend keeps `position` on the last frame after reaching the end of a VFR video, so the last scene no longer ends early (or before it starts) [#561](https://github.com/Breakthrough/PySceneDetect/issues/561) From 3946f3ff8dc1e49217ec8ef706dd5f5decd8c9ab Mon Sep 17 00:00:00 2001 From: Rober <124747128+Rober1208@users.noreply.github.com> Date: Sat, 24 Oct 2026 18:18:58 +0800 Subject: [PATCH 4/4] [cli] Use TimecodeValue and RangeValue for save-images interval options Remove ImageIntervalValue. interval is now a TimecodeValue in the config file, and the command checks that it is greater than 0. min-images and max-images are now RangeValues, so the config file parser checks their ranges, and RangeValue accepts max_val=None for no upper bound. Co-Authored-By: Claude Opus 5.5 --- scenedetect/_cli/__init__.py | 32 +++++++++++-------- scenedetect/_cli/config.py | 59 ++++++++++++------------------------ tests/test_cli.py | 16 +++++----- 3 files changed, 47 insertions(+), 60 deletions(-) diff --git a/scenedetect/_cli/__init__.py b/scenedetect/_cli/__init__.py index 73ac84be..ae23241d 100644 --- a/scenedetect/_cli/__init__.py +++ b/scenedetect/_cli/__init__.py @@ -34,11 +34,12 @@ CONFIG_MAP, DEFAULT_JPG_QUALITY, DEFAULT_WEBP_QUALITY, - ImageIntervalValue, RangeValue, + TimecodeValue, ) from scenedetect._cli.context import USER_CONFIG, CliContext, check_split_video_requirements from scenedetect.backends import AVAILABLE_BACKENDS +from scenedetect.common import FrameTimecode from scenedetect.detectors import ( AdaptiveDetector, ContentDetector, @@ -1453,7 +1454,7 @@ def split_video_command( "--min-images", metavar="N", default=None, - type=click.IntRange(min=1), + type=_click_range("save-images", "min-images"), help="Minimum number of images per scene. Can only be used with -i/--interval.{}".format( USER_CONFIG.get_help_string("save-images", "min-images") ), @@ -1462,7 +1463,7 @@ def split_video_command( "--max-images", metavar="N", default=None, - type=click.IntRange(min=0), + type=_click_range("save-images", "max-images"), help="Maximum number of images per scene, or 0 for no limit. Can only be used with -i/--interval.{}".format( USER_CONFIG.get_help_string("save-images", "max-images") ), @@ -1619,9 +1620,14 @@ def save_images_command( ) if interval is not None: try: - ImageIntervalValue(interval) + TimecodeValue(interval) except ValueError as ex: - raise click.BadParameter(str(ex), param_hint="-i/--interval") from ex + raise click.BadParameter( + "Interval must be in frames (24), seconds (1.5s), or HH:MM:SS.", + param_hint="-i/--interval", + ) from ex + if FrameTimecode(timecode=interval, fps=ctx.video_stream.frame_rate).seconds <= 0: + raise click.BadParameter("Interval must be greater than 0.", param_hint="-i/--interval") elif num_images is None: if not ctx.config.is_default("save-images", "interval") and not ctx.config.is_default( "save-images", "num-images" @@ -1630,9 +1636,16 @@ def save_images_command( "Only one of num-images or interval can be set in the config file.", param_hint="save-images", ) - interval = ctx.config.get_value("save-images", "interval") - if interval is None: + if ctx.config.is_default("save-images", "interval"): num_images = ctx.config.get_value("save-images", "num-images") + else: + interval_value: str = ctx.config.get_value("save-images", "interval") + if FrameTimecode(timecode=interval_value, fps=ctx.video_stream.frame_rate).seconds <= 0: + raise click.BadParameter( + "interval in the config file must be greater than 0.", + param_hint="save-images", + ) + interval = interval_value # --min-images/--max-images only apply with an interval (from the command line or the config # file). Without one, they are an error on the command line, and ignored in the config file. if interval is None: @@ -1645,11 +1658,6 @@ def save_images_command( else: min_images_value: int = ctx.config.get_value("save-images", "min-images", min_images) max_images_value: int = ctx.config.get_value("save-images", "max-images", max_images) - # Click checks the range of values from the command line, but not from the config file. - if min_images_value < 1: - raise click.BadParameter( - "min-images in the config file must be at least 1.", param_hint="save-images" - ) if max_images_value != 0 and max_images_value < min_images_value: raise click.BadParameter( "max-images must be 0 (no limit) or at least min-images.", diff --git a/scenedetect/_cli/config.py b/scenedetect/_cli/config.py index 779de30b..0e3ef9b9 100644 --- a/scenedetect/_cli/config.py +++ b/scenedetect/_cli/config.py @@ -96,45 +96,17 @@ def from_config(config_value: str, default: "TimecodeValue") -> "TimecodeValue": ) from ex -class ImageIntervalValue(ValidatedValue): - """Validator for the save-images interval: a timecode greater than 0, or None if unset. - - Stores value in original representation, like `TimecodeValue`.""" - - def __init__(self, value: str | None = None): - if value is not None: - try: - seconds = FrameTimecode(timecode=value, fps=100.0).seconds - except ValueError as ex: - raise ValueError( - "Interval must be in frames (24), seconds (1.5s), or HH:MM:SS." - ) from ex - if seconds <= 0: - raise ValueError("Interval must be greater than 0.") - self._value = value - - @property - def value(self) -> str | None: - return self._value - - @staticmethod - def from_config(config_value: str, default: "ImageIntervalValue") -> "ImageIntervalValue": - try: - return ImageIntervalValue(config_value) - except ValueError as ex: - raise OptionParseFailure(f"{ex}") from ex - - class RangeValue(ValidatedValue): - """Validator for int/float ranges. `min_val` and `max_val` are inclusive.""" + """Validator for int/float ranges. `min_val` and `max_val` are inclusive. A `max_val` of None + means there is no upper bound.""" def __init__( self, value: int | float, min_val: int | float, - max_val: int | float, + max_val: int | float | None, ): - if value < min_val or value > max_val: + if value < min_val or (max_val is not None and value > max_val): # min and max are inclusive. raise ValueError() self._value = value @@ -151,16 +123,20 @@ def min_val(self) -> int | float: return self._min_val @property - def max_val(self) -> int | float: - """Maximum value of the range.""" + def max_val(self) -> int | float | None: + """Maximum value of the range, or None if there is no upper bound.""" return self._max_val @property def click_range(self) -> "click.IntRange | click.FloatRange": """A `click` parameter type matching this range's bounds and value type.""" if isinstance(self._value, int): - return click.IntRange(int(self._min_val), int(self._max_val)) - return click.FloatRange(float(self._min_val), float(self._max_val)) + return click.IntRange( + int(self._min_val), None if self._max_val is None else int(self._max_val) + ) + return click.FloatRange( + float(self._min_val), None if self._max_val is None else float(self._max_val) + ) @staticmethod def from_config(config_value: str, default: "RangeValue") -> "RangeValue": @@ -171,6 +147,11 @@ def from_config(config_value: str, default: "RangeValue") -> "RangeValue": max_val=default.max_val, ) except ValueError as ex: + if default.max_val is None: + value_type = "an integer" if isinstance(default.value, int) else "a number" + raise OptionParseFailure( + f"Value must be {value_type} of at least {default.min_val}." + ) from ex raise OptionParseFailure( f"Value must be between {default.min_val} and {default.max_val}." ) from ex @@ -471,9 +452,9 @@ class FcpFormat(Enum): "format": "jpeg", "frame-margin": TimecodeValue(1), "height": 0, - "interval": ImageIntervalValue(), - "max-images": 0, - "min-images": 1, + "interval": TimecodeValue(0), # Placeholder, as there is no interval by default + "max-images": RangeValue(0, min_val=0, max_val=None), + "min-images": RangeValue(1, min_val=1, max_val=None), "num-images": 3, "output": None, "quality": RangeValue(_PLACEHOLDER, min_val=0, max_val=100), diff --git a/tests/test_cli.py b/tests/test_cli.py index cdb737f0..50106f53 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -798,17 +798,13 @@ def test_cli_save_images_interval_config_file( ["--max-images", "2"], "--min-images and --max-images can only be used with -i/--interval.", ), - ("interval = 1s\nmin-images = 0", [], "min-images in the config file must be at least 1."), ( "interval = 1s\nmin-images = 3\nmax-images = 2", [], "max-images must be 0 (no limit) or at least min-images.", ), - ( - "interval = 1s\nmax-images = -1", - [], - "max-images must be 0 (no limit) or at least min-images.", - ), + ("interval = 0", [], "interval in the config file must be greater than 0."), + ("interval = 0s", [], "interval in the config file must be greater than 0."), ], ) def test_cli_save_images_interval_config_file_rejects_invalid_values( @@ -823,9 +819,11 @@ def test_cli_save_images_interval_config_file_rejects_invalid_values( @pytest.mark.parametrize( "config,error", [ - ("interval = abc", "Interval must be in frames (24), seconds (1.5s), or HH:MM:SS."), - ("interval = 0", "Interval must be greater than 0."), - ("min-images = x", "x is not a valid integer."), + ("interval = abc", "Timecodes must be in seconds (100.0), frames (100), or HH:MM:SS."), + ("min-images = 0", "Value must be an integer of at least 1."), + ("min-images = 1.5", "Value must be an integer of at least 1."), + ("min-images = x", "Value must be an integer of at least 1."), + ("max-images = -1", "Value must be an integer of at least 0."), ], ) def test_cli_save_images_interval_config_file_load_errors(tmp_path: Path, config: str, error: str):