Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/renderkit/api/processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 3 additions & 1 deletion src/renderkit/cli/batch.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
20 changes: 20 additions & 0 deletions src/renderkit/cli/conversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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,
Expand All @@ -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)
Expand All @@ -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)
14 changes: 7 additions & 7 deletions src/renderkit/cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion src/renderkit/core/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
43 changes: 43 additions & 0 deletions tests/test_api.py
Original file line number Diff line number Diff line change
@@ -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:

Check failure on line 29 in tests/test_api.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Add a nested comment explaining why this method is empty, or complete the implementation.

See more on https://sonarcloud.io/project/issues?id=Ahmed-Hindy_renderkit&issues=AZ63rH7f6-8DAyAuIIlC&open=AZ63rH7f6-8DAyAuIIlC&pullRequest=90
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
31 changes: 31 additions & 0 deletions tests/test_batch.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
111 changes: 111 additions & 0 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from pathlib import Path

import pytest
from click.testing import CliRunner

from renderkit.cli import main as cli_main
Expand Down Expand Up @@ -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 = []
Expand Down
13 changes: 13 additions & 0 deletions tests/test_converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand Down
Loading