diff --git a/modules/api/video.py b/modules/api/video.py index 2b34357d2..ff2345214 100644 --- a/modules/api/video.py +++ b/modules/api/video.py @@ -38,7 +38,7 @@ class ReqVideo(BaseModel): mp4_interpolate: int = Field(default=0, ge=0, le=10, title="Interpolation", description="RIFE interpolation passes between frames") mp4_codec: str = Field(default="libx264", title="Codec", description="Video codec; none skips video encoding") mp4_ext: str = Field(default="mp4", title="Container", description="Container extension; the muxer is inferred from it") - mp4_opt: str = Field(default="crf:16", title="Codec options", description="Encoder options as comma-separated key:value pairs") + mp4_opt: str = Field(default="crf=16", title="Codec options", description="Encoder options as key=value pairs separated by : or ,") mp4_video: bool = Field(default=True, title="Save video", description="Write the video container to disk") mp4_frames: bool = Field(default=False, title="Save frames", description="Write individual frame images to disk") mp4_sf: bool = Field(default=False, title="Save safetensors", description="Write raw frames as a safetensors file") diff --git a/modules/framepack/framepack_api.py b/modules/framepack/framepack_api.py index 99efdcfb1..9cc2ace1a 100644 --- a/modules/framepack/framepack_api.py +++ b/modules/framepack/framepack_api.py @@ -34,7 +34,7 @@ class ReqFramepack(BaseModel): mp4_video: bool | None = Field(default=True, title="Save Video", description="Save video") mp4_frames: bool | None = Field(default=False, title="Save Frames", description="Save frames for the video") mp4_thumb: bool | None = Field(default=True, title="Save Thumbnail", description="Save thumbnail for the video") - mp4_opt: str | None = Field(default="crf:16", title="Options", description="Options for the video codec") + mp4_opt: str | None = Field(default="crf=16", title="Options", description="Options for the video codec") mp4_ext: str | None = Field(default="mp4", title="Format", description="Format for the video") mp4_interpolate: int | None = Field(default=0, title="Interpolation", description="Interpolation for the video") attention: str | None = Field(default="Default", title="Attention", description="Attention type for the model") diff --git a/modules/postprocess/seedvr_model.py b/modules/postprocess/seedvr_model.py index 27f8e775b..6fc51531c 100644 --- a/modules/postprocess/seedvr_model.py +++ b/modules/postprocess/seedvr_model.py @@ -248,7 +248,7 @@ class UpscalerSeedVR(Upscaler): log.error(f'Upscaler: name="SeedVR2" video="{video_path}" {e}') return None, None, None - def create_video(self, tensor: torch.Tensor, audio, codec: str = 'libx264', codec_opt: str = 'crf:16', interpolate: int = 0): + def create_video(self, tensor: torch.Tensor, audio, codec: str = 'libx264', codec_opt: str = 'crf=16', interpolate: int = 0): t0 = time.time() from modules.video_models.video_save import save_video pixels = tensor.permute(3, 0, 1, 2).unsqueeze(0) # from (t, h, w, c) to (n, c, t, h, w) @@ -281,7 +281,7 @@ class UpscalerSeedVR(Upscaler): offload: bool = True, interpolate: int = 1, codec: str = 'libx264', - codec_opt: str = 'crf:16', + codec_opt: str = 'crf=16', vae_memory: float = 0.5, vae_tile_encode: bool = True, vae_tile_decode: bool = True, diff --git a/modules/video_models/video_codecs.py b/modules/video_models/video_codecs.py index 2e07d1eb4..856e7a90b 100644 --- a/modules/video_models/video_codecs.py +++ b/modules/video_models/video_codecs.py @@ -26,14 +26,14 @@ codecs_config = { 'desc': 'Legacy open video format for WebM.', 'ext': 'webm', 'allowed_exts': ['webm', 'mkv'], - 'options': 'crf=10:b:v=0', + 'options': 'crf=10:b=0', }, 'libvpx-vp9': { 'name': 'VP9 Video', 'desc': 'Royalty-free web video format.', 'ext': 'webm', 'allowed_exts': ['webm', 'mkv'], - 'options': 'crf=23:b:v=0', + 'options': 'crf=23:b=0', }, 'libsvtav1': { 'name': 'AV1 (SVT-AV1)', @@ -47,14 +47,14 @@ codecs_config = { 'desc': 'Legacy format for old media players.', 'ext': 'mp4', 'allowed_exts': ['mp4', 'avi', 'mkv'], - 'options': 'qscale:v=3', + 'options': 'qmin=3:qmax=3', }, 'mjpeg': { 'name': 'Motion JPEG', 'desc': 'Sequence of JPEG frames; minimal CPU load.', 'ext': 'avi', 'allowed_exts': ['avi', 'mov', 'mkv'], - 'options': 'qscale:v=3', + 'options': 'qmin=3:qmax=3', }, # --- Hardware Accelerated: NVIDIA NVENC --- diff --git a/modules/video_models/video_run.py b/modules/video_models/video_run.py index cd52c950e..453e6738a 100644 --- a/modules/video_models/video_run.py +++ b/modules/video_models/video_run.py @@ -110,7 +110,7 @@ def run(selected: models_def.Model, *, mp4_interpolate: int = 0, mp4_codec: str = 'libx264', mp4_ext: str = 'mp4', - mp4_opt: str = 'crf:16', + mp4_opt: str = 'crf=16', mp4_video: bool = True, mp4_frames: bool = False, mp4_sf: bool = False, diff --git a/scripts/postprocessing_seedvr.py b/scripts/postprocessing_seedvr.py index 0e47eb268..29868cd0c 100644 --- a/scripts/postprocessing_seedvr.py +++ b/scripts/postprocessing_seedvr.py @@ -44,7 +44,7 @@ class ScriptSeedVR(scripts_postprocessing.ScriptPostprocessing): from modules.ui_common import create_refresh_button seedvr_codec = gr.Dropdown(label="Video codec", choices=['none', 'libx264'], value='libx264', type='value') create_refresh_button(seedvr_codec, get_codecs, elem_id="video_mp4_codec_refresh") - seedvr_codec_opt = gr.Textbox(label="Video options", value='crf:16', elem_id="video_mp4_opt") + seedvr_codec_opt = gr.Textbox(label="Video options", value='crf=16', elem_id="video_mp4_opt") return { "seedvr_enabled": seedvr_enabled, diff --git a/test/test-video-codecs.py b/test/test-video-codecs.py new file mode 100644 index 000000000..3d03581b8 --- /dev/null +++ b/test/test-video-codecs.py @@ -0,0 +1,222 @@ +#!/usr/bin/env python +""" +Offline unit tests for the video encoder option strings in modules.video_models. + +``parse_options`` treats ``:`` and ``,`` as separators and ``=`` as the only assignment, so a +segment without ``=`` becomes a valueless flag set to ``'1'``. That makes ffmpeg command line +spellings parse into something legal but wrong: ``crf:16`` yields ``{'crf': '1', '16': '1'}`` and +``crf=23:b:v=0`` pins the generic bitrate option to 1 bit per second. Neither raises, so the only +way to catch it is to assert the shape of the strings themselves. + +Covers: + +- ``parse_options`` semantics for both separators, the flag form, and the dict passthrough +- every preset in ``codecs_config`` reaching the encoder as the keys it was written with +- the shipped ``mp4_opt`` default parsing to the value it names + +No running server required. Nothing is encoded. + +Usage: + python test/test-video-codecs.py +""" + +import os +import sys + +script_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +sys.path.insert(0, script_dir) +os.chdir(script_dir) + +os.environ['SD_INSTALL_QUIET'] = '1' + +# Bootstrap cmd_args before any module that pulls in shared.py. +import modules.cmd_args # pylint: disable=wrong-import-position +import installer # pylint: disable=wrong-import-position +orig_argv = sys.argv +sys.argv = [sys.argv[0]] +try: + modules.cmd_args.parse_args() +finally: + sys.argv = orig_argv +installer.add_args(modules.cmd_args.parser) +modules.cmd_args.parsed, _ = modules.cmd_args.parser.parse_known_args([]) + +from modules.errors import log # pylint: disable=wrong-import-position +from modules.video_models.video_codecs import codecs_config # pylint: disable=wrong-import-position +from modules.video_models.video_save import parse_options # pylint: disable=wrong-import-position + + +results: dict[str, dict] = {} + + +def category(name: str): + if name not in results: + results[name] = {'passed': 0, 'failed': 0, 'tests': []} + return name + + +def record(cat: str, passed: bool, name: str, detail: str = ''): + status = 'PASS' if passed else 'FAIL' + results[cat]['passed' if passed else 'failed'] += 1 + results[cat]['tests'].append((status, name)) + msg = f' {status}: {name}' + if detail: + msg += f' ({detail})' + if passed: + log.info(msg) + else: + log.error(msg) + + +def run_test(cat: str, fn): + name = fn.__name__ + try: + ok = fn() + if ok is False: + record(cat, False, name) + else: + record(cat, True, name) + except AssertionError as e: + record(cat, False, name, str(e)) + except Exception as e: # pylint: disable=broad-except + record(cat, False, name, f'exception: {e}') + + +# ============================================================ +# parse_options semantics +# ============================================================ + +def test_assignment_uses_equals(): + assert parse_options('crf=16') == {'crf': '16'} + + +def test_colon_separates_pairs(): + assert parse_options('crf=18:preset=medium') == {'crf': '18', 'preset': 'medium'} + + +def test_comma_separates_pairs(): + assert parse_options('crf=18,preset=medium') == {'crf': '18', 'preset': 'medium'} + + +def test_bare_item_becomes_a_flag(): + assert parse_options('fastseek') == {'fastseek': '1'} + + +def test_whitespace_is_stripped(): + assert parse_options(' crf = 18 : preset = medium ') == {'crf': '18', 'preset': 'medium'} + + +def test_empty_and_dict_inputs(): + assert parse_options('') == {} + assert parse_options(' ') == {} + assert parse_options(None) == {} + assert parse_options({'crf': '16'}) == {'crf': '16'} + + +def test_ffmpeg_cli_spelling_does_not_assign(): + # the failure this suite exists for: legal parse, wrong meaning, no exception + assert parse_options('crf:16') != {'crf': '16'} + + +# ============================================================ +# shipped presets +# ============================================================ + +def test_every_preset_segment_assigns(): + broken = [] + for name, cfg in codecs_config.items(): + options = cfg.get('options', '') + if not options: + continue + for segment in options.replace(',', ':').split(':'): + if segment.strip() and '=' not in segment: + broken.append(f'{name}="{options}" segment="{segment}"') + assert not broken, f'segments parse as flags instead of assignments: {broken}' + + +def test_no_preset_key_is_numeric(): + numeric = [] + for name, cfg in codecs_config.items(): + for key in parse_options(cfg.get('options', '')): + if key.isdigit(): + numeric.append(f'{name}: {key}') + assert not numeric, f'a value was parsed as a key: {numeric}' + + +def test_no_preset_sets_a_degenerate_bitrate(): + # b is a generic AVCodecContext option, so a stray b=1 caps the encoder at 1 bit per second + bad = [] + for name, cfg in codecs_config.items(): + bitrate = parse_options(cfg.get('options', '')).get('b', None) + if bitrate is not None and bitrate != '0': + bad.append(f'{name}: b={bitrate}') + assert not bad, f'bitrate pinned to a nonzero value: {bad}' + + +def test_every_preset_keeps_its_key_count(): + for name, cfg in codecs_config.items(): + options = cfg.get('options', '') + if not options: + continue + segments = [s for s in options.replace(',', ':').split(':') if s.strip()] + parsed = parse_options(options) + assert len(parsed) == len(segments), f'{name}="{options}" parsed to {parsed}' + + +# ============================================================ +# shipped defaults +# ============================================================ + +def test_run_default_names_its_own_value(): + import inspect + from modules.video_models import video_run + default = inspect.signature(video_run.run).parameters['mp4_opt'].default + assert parse_options(default) == {'crf': '16'}, f'default "{default}" parsed to {parse_options(default)}' + + +def run_all(): + log.warning('=== Video codec options ===') + cat = category('parser') + for fn in [ + test_assignment_uses_equals, + test_colon_separates_pairs, + test_comma_separates_pairs, + test_bare_item_becomes_a_flag, + test_whitespace_is_stripped, + test_empty_and_dict_inputs, + test_ffmpeg_cli_spelling_does_not_assign, + ]: + run_test(cat, fn) + cat = category('presets') + for fn in [ + test_every_preset_segment_assigns, + test_no_preset_key_is_numeric, + test_no_preset_sets_a_degenerate_bitrate, + test_every_preset_keeps_its_key_count, + ]: + run_test(cat, fn) + cat = category('defaults') + for fn in [ + test_run_default_names_its_own_value, + ]: + run_test(cat, fn) + + log.warning('=== Results ===') + total_passed = 0 + total_failed = 0 + for cat_name, info in results.items(): + ok = info['failed'] == 0 + status = 'PASS' if ok else 'FAIL' + log.info(f" {cat_name}: {info['passed']} passed, {info['failed']} failed [{status}]") + total_passed += info['passed'] + total_failed += info['failed'] + log.warning(f'Total: {total_passed} passed, {total_failed} failed') + return total_failed == 0 + + +if __name__ == '__main__': + import time + t0 = time.time() + ok = run_all() + log.warning(f'Total time: {time.time() - t0:.2f}s') + sys.exit(0 if ok else 1)