From 2225288cefd3e65ba9f3f023f5dc7eb0b45e81b5 Mon Sep 17 00:00:00 2001 From: Ahmed Hindy Date: Thu, 11 Jun 2026 20:11:56 +0300 Subject: [PATCH] Fix CLI one-sided range and resolution options --- src/renderkit/api/processor.py | 9 ++- src/renderkit/cli/batch.py | 4 +- src/renderkit/cli/conversion.py | 20 ++++++ src/renderkit/cli/main.py | 14 ++-- src/renderkit/core/config.py | 4 +- tests/test_api.py | 43 +++++++++++++ tests/test_batch.py | 31 +++++++++ tests/test_cli.py | 111 ++++++++++++++++++++++++++++++++ tests/test_converter.py | 13 ++++ 9 files changed, 235 insertions(+), 14 deletions(-) create mode 100644 tests/test_api.py diff --git a/src/renderkit/api/processor.py b/src/renderkit/api/processor.py index 91469c3..e3d6717 100644 --- a/src/renderkit/api/processor.py +++ b/src/renderkit/api/processor.py @@ -82,15 +82,14 @@ def convert_exr_sequence_to_mp4( if layer is not None: config.with_layer(layer) + if (width is None) != (height is None): + raise ValueError("width and height must be used together") + if width is not None and height is not None: config.with_resolution(width, height) - if start_frame is not None and end_frame is not None: + if start_frame is not None or end_frame is not None: config.with_frame_range(start_frame, end_frame) - elif start_frame is not None: - config.with_frame_range(start_frame, start_frame) - elif end_frame is not None: - config.with_frame_range(0, end_frame) if contact_sheet: config.with_contact_sheet(True, contact_sheet_config) diff --git a/src/renderkit/cli/batch.py b/src/renderkit/cli/batch.py index 035761d..7f5c1ca 100644 --- a/src/renderkit/cli/batch.py +++ b/src/renderkit/cli/batch.py @@ -9,7 +9,7 @@ import click from renderkit.api.processor import RenderKit -from renderkit.cli.conversion import base_conversion_config_builder +from renderkit.cli.conversion import base_conversion_config_builder, validate_paired_resolution from renderkit.core.batch import ( BatchManifestRecord, BatchSequence, @@ -156,6 +156,8 @@ def batch_convert( no_progress: bool, ) -> None: """Recursively convert discovered frame sequences under ROOT to review MP4s.""" + validate_paired_resolution(width, height) + root = root.resolve() if not root.is_dir(): click.echo(f"Error: root directory does not exist: {root}", err=True) diff --git a/src/renderkit/cli/conversion.py b/src/renderkit/cli/conversion.py index 6bcd173..20aea64 100644 --- a/src/renderkit/cli/conversion.py +++ b/src/renderkit/cli/conversion.py @@ -3,6 +3,8 @@ from pathlib import Path from typing import Optional +import click + from renderkit.core.config import ConversionConfigBuilder from renderkit.processing.color_space import ColorSpacePreset @@ -14,6 +16,12 @@ } +def validate_paired_resolution(width: Optional[int], height: Optional[int]) -> None: + """Require CLI resolution overrides to include both dimensions.""" + if (width is None) != (height is None): + raise click.UsageError("--width and --height must be used together.") + + def base_conversion_config_builder( input_pattern: str, output_path: str | Path, @@ -27,6 +35,8 @@ def base_conversion_config_builder( layer: Optional[str], ) -> ConversionConfigBuilder: """Build shared conversion settings used by CLI conversion commands.""" + validate_paired_resolution(width, height) + config_builder = ( ConversionConfigBuilder() .with_input_pattern(input_pattern) @@ -47,3 +57,13 @@ def base_conversion_config_builder( config_builder.with_resolution(width, height) return config_builder + + +def apply_frame_range( + config_builder: ConversionConfigBuilder, + start_frame: Optional[int], + end_frame: Optional[int], +) -> None: + """Apply an optional open-ended frame range to a conversion builder.""" + if start_frame is not None or end_frame is not None: + config_builder.with_frame_range(start_frame, end_frame) diff --git a/src/renderkit/cli/main.py b/src/renderkit/cli/main.py index 758ba71..bae6770 100644 --- a/src/renderkit/cli/main.py +++ b/src/renderkit/cli/main.py @@ -10,7 +10,11 @@ from renderkit import __version__ from renderkit.api.processor import RenderKit from renderkit.cli.batch import batch_convert -from renderkit.cli.conversion import base_conversion_config_builder +from renderkit.cli.conversion import ( + apply_frame_range, + base_conversion_config_builder, + validate_paired_resolution, +) from renderkit.core.config import ( BurnInConfig, BurnInElement, @@ -304,6 +308,7 @@ def convert_exr_sequence( renderkit convert-exr-sequence render.%04d.exr output.mp4 --fps 24 --start-frame 100 --end-frame 200 """ output_path_obj = Path(output_path) + validate_paired_resolution(width, height) # Check if output exists if output_path_obj.exists() and not overwrite: @@ -336,12 +341,7 @@ def convert_exr_sequence( ) config_builder.with_contact_sheet(True, cs_config) - if start_frame is not None and end_frame is not None: - config_builder.with_frame_range(start_frame, end_frame) - elif start_frame is not None: - config_builder.with_frame_range(start_frame, start_frame) - elif end_frame is not None: - config_builder.with_frame_range(0, end_frame) + apply_frame_range(config_builder, start_frame, end_frame) burnin_elements = [] font_size = 20 diff --git a/src/renderkit/core/config.py b/src/renderkit/core/config.py index 5ffe14b..fb853fb 100644 --- a/src/renderkit/core/config.py +++ b/src/renderkit/core/config.py @@ -262,7 +262,9 @@ def with_layer(self, layer: str) -> "ConversionConfigBuilder": self._layer = layer return self - def with_frame_range(self, start: int, end: int) -> "ConversionConfigBuilder": + def with_frame_range( + self, start: Optional[int], end: Optional[int] + ) -> "ConversionConfigBuilder": """Set the frame range.""" self._start_frame = start self._end_frame = end diff --git a/tests/test_api.py b/tests/test_api.py new file mode 100644 index 0000000..f39ab21 --- /dev/null +++ b/tests/test_api.py @@ -0,0 +1,43 @@ +"""Tests for the public RenderKit API.""" + +import pytest + +from renderkit.api import processor as processor_module +from renderkit.api.processor import RenderKit + + +def test_api_rejects_one_sided_resolution() -> None: + """The public API should not silently ignore one resize dimension.""" + processor = RenderKit.__new__(RenderKit) + + with pytest.raises(ValueError, match="width and height must be used together"): + processor.convert_exr_sequence_to_mp4( + "render.%04d.exr", + "output.mp4", + width=1920, + ) + + +def test_api_preserves_one_sided_frame_range(monkeypatch) -> None: + """A single API range bound should remain open-ended.""" + captured_configs = [] + + class FakeSequenceConverter: + def __init__(self, config) -> None: + captured_configs.append(config) + + def convert(self, show_progress=None) -> None: + pass + + monkeypatch.setattr(processor_module, "SequenceConverter", FakeSequenceConverter) + processor = RenderKit.__new__(RenderKit) + + processor.convert_exr_sequence_to_mp4( + "render.%04d.exr", + "output.mp4", + start_frame=1001, + ) + + assert len(captured_configs) == 1 + assert captured_configs[0].start_frame == 1001 + assert captured_configs[0].end_frame is None diff --git a/tests/test_batch.py b/tests/test_batch.py index 61807f2..3b5ef8e 100644 --- a/tests/test_batch.py +++ b/tests/test_batch.py @@ -203,3 +203,34 @@ def convert_with_config(self, config, show_progress=None) -> None: tmp_path / "_review_mp4s" / "render.mp4", tmp_path / "_review_mp4s" / "render_2.mp4", ] + + +def test_batch_convert_rejects_one_sided_resolution(monkeypatch, tmp_path: Path) -> None: + """Batch conversion should use the same paired resize validation.""" + (tmp_path / "render.0001.exr").touch() + calls = [] + + class FakeRenderKit: + def convert_with_config(self, config, show_progress=None) -> None: + calls.append(config) + + monkeypatch.setattr(cli_main, "ensure_ffmpeg_env", lambda: None) + monkeypatch.setattr(cli_main, "setup_logging", lambda: None) + monkeypatch.setattr(cli_batch, "RenderKit", FakeRenderKit) + + result = CliRunner().invoke( + cli_main.main, + [ + "batch-convert", + str(tmp_path), + "--fps", + "24", + "--width", + "1920", + ], + ) + + assert result.exit_code == 2 + assert "--width and --height must be used together" in result.output + assert calls == [] + assert not (tmp_path / "_review_mp4s").exists() diff --git a/tests/test_cli.py b/tests/test_cli.py index 50f21ce..72cf7a2 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2,6 +2,7 @@ from pathlib import Path +import pytest from click.testing import CliRunner from renderkit.cli import main as cli_main @@ -172,6 +173,116 @@ def convert_with_config(self, config, show_progress=None) -> None: assert show_progress is None +@pytest.mark.parametrize( + ("option", "value", "missing_option"), + [ + ("--width", "1920", "--height"), + ("--height", "1080", "--width"), + ], +) +def test_convert_rejects_one_sided_resolution( + monkeypatch, tmp_path: Path, option: str, value: str, missing_option: str +) -> None: + """Lone resize dimensions should fail instead of being silently ignored.""" + calls = [] + + class FakeRenderKit: + def convert_with_config(self, config, show_progress=None) -> None: + calls.append(config) + + monkeypatch.setattr(cli_main, "ensure_ffmpeg_env", lambda: None) + monkeypatch.setattr(cli_main, "setup_logging", lambda: None) + monkeypatch.setattr(cli_main, "RenderKit", FakeRenderKit) + + result = CliRunner().invoke( + cli_main.main, + [ + "convert-exr-sequence", + "render.%04d.exr", + str(tmp_path / "output.mp4"), + "--fps", + "24", + option, + value, + ], + ) + + assert result.exit_code == 2 + assert "--width and --height must be used together" in result.output + assert missing_option in result.output + assert calls == [] + + +def test_convert_resolution_validation_runs_before_output_exists_check( + monkeypatch, tmp_path: Path +) -> None: + """Invalid resize options should report usage before runtime output checks.""" + output_path = tmp_path / "output.mp4" + output_path.touch() + + monkeypatch.setattr(cli_main, "ensure_ffmpeg_env", lambda: None) + monkeypatch.setattr(cli_main, "setup_logging", lambda: None) + + result = CliRunner().invoke( + cli_main.main, + [ + "convert-exr-sequence", + "render.%04d.exr", + str(output_path), + "--fps", + "24", + "--width", + "1920", + ], + ) + + assert result.exit_code == 2 + assert "--width and --height must be used together" in result.output + assert "Use --overwrite" not in result.output + + +@pytest.mark.parametrize( + ("option", "value", "expected_range"), + [ + ("--start-frame", "1001", (1001, None)), + ("--end-frame", "1008", (None, 1008)), + ], +) +def test_convert_preserves_one_sided_frame_range( + monkeypatch, + tmp_path: Path, + option: str, + value: str, + expected_range: tuple[int | None, int | None], +) -> None: + """A single range bound should stay open-ended for sequence filtering.""" + captured_ranges = [] + + class FakeRenderKit: + def convert_with_config(self, config, show_progress=None) -> None: + captured_ranges.append((config.start_frame, config.end_frame)) + + monkeypatch.setattr(cli_main, "ensure_ffmpeg_env", lambda: None) + monkeypatch.setattr(cli_main, "setup_logging", lambda: None) + monkeypatch.setattr(cli_main, "RenderKit", FakeRenderKit) + + result = CliRunner().invoke( + cli_main.main, + [ + "convert-exr-sequence", + "render.%04d.exr", + str(tmp_path / "output.mp4"), + "--fps", + "24", + option, + value, + ], + ) + + assert result.exit_code == 0, result.output + assert captured_ranges == [expected_range] + + def test_contact_sheet_command_uses_still_writer(monkeypatch, tmp_path: Path) -> None: """Verify the still contact sheet command wires CLI options into the writer.""" calls = [] diff --git a/tests/test_converter.py b/tests/test_converter.py index af01bc1..ad3df90 100644 --- a/tests/test_converter.py +++ b/tests/test_converter.py @@ -141,6 +141,19 @@ def test_builder_with_frame_range(self) -> None: assert config.start_frame == 100 assert config.end_frame == 200 + def test_builder_with_open_ended_frame_range(self) -> None: + """Test builder with an open-ended frame range.""" + config = ( + ConversionConfigBuilder() + .with_input_pattern("render.%04d.exr") + .with_output_path("output.mp4") + .with_frame_range(100, None) + .build() + ) + + assert config.start_frame == 100 + assert config.end_frame is None + def test_builder_with_prefetch_workers(self) -> None: """Test builder with prefetch workers.""" config = (