diff --git a/SCRATCHPAD.md b/SCRATCHPAD.md index c0ab74d..4864883 100644 --- a/SCRATCHPAD.md +++ b/SCRATCHPAD.md @@ -9,6 +9,8 @@ - The biggest near-term wins are in startup cost, repeated subprocess work, repeated database query patterns, and general repo hygiene. - This list is intentionally optimization-oriented rather than bug-oriented. Some items below also improve correctness or maintainability, but they were selected because they can reduce runtime cost, operator friction, or iteration overhead. - A first modern integration slice now exists under [`tests/integration/subtrack_mapping`](/home/osgw/.local/src/codex/ffx/tests/integration/subtrack_mapping). Remaining test-suite cleanup is now mostly about migrating and shrinking the legacy harness surface under [`tests/legacy`](/home/osgw/.local/src/codex/ffx/tests/legacy). +- The CLI root now lazy-loads heavy runtime dependencies so lightweight commands such as `version`, `help`, `configure_workstation`, and `upgrade` stay import-light. +- Shared CLI defaults for container/output tokens now live outside [`src/ffx/ffx_controller.py`](/home/osgw/.local/src/codex/ffx/src/ffx/ffx_controller.py), and a focused unit test locks in the lazy-import contract. - FFX logger setup now reuses named handlers, and fallback logger access no longer mutates handlers in ordinary constructors and helpers. - The process wrapper now uses `subprocess.run(...)` with centralized command formatting plus stable timeout and missing-command error mapping. - Active ORM controllers now use single-query accessors instead of paired `count()` plus `first()` lookups. @@ -17,8 +19,8 @@ ## Focused Snapshot - Highest-leverage application optimizations: - - Lazy-load CLI command dependencies so lightweight commands do not import most of the app. - Collapse repeated `ffprobe` calls into a single probe result per source file. + - Revisit crop detection cost after the probe path is consolidated. - Highest-leverage repo and workflow optimizations: - Consolidate setup and upgrade tooling to reduce overlapping shell-script responsibilities. @@ -26,16 +28,7 @@ ## Optimization Candidates -1. CLI startup and import cost -- [`src/ffx/cli.py`](/home/osgw/.local/src/codex/ffx/src/ffx/cli.py) imports a large portion of the application at module import time, even for cheap commands such as `version`, `help`, `configure_workstation`, and `upgrade`. -- Optimization: - - Move heavy imports into the commands that actually need them. - - Keep the CLI root importable with only core stdlib and Click dependencies. -- Expected value: - - Faster startup for scripting and tooling commands. - - Less coupling between maintenance commands and the runtime stack. - -2. Media probing does two separate `ffprobe` subprocesses per file +1. Media probing does two separate `ffprobe` subprocesses per file - [`src/ffx/file_properties.py`](/home/osgw/.local/src/codex/ffx/src/ffx/file_properties.py) calls `ffprobe` once for format data and once for stream data. - Optimization: - Use one probe call that requests both format and streams. @@ -44,7 +37,7 @@ - Less subprocess overhead. - Faster inspect and convert flows. -3. Crop detection is always a full extra ffmpeg scan +2. Crop detection is always a full extra ffmpeg scan - [`src/ffx/file_properties.py`](/home/osgw/.local/src/codex/ffx/src/ffx/file_properties.py) runs a dedicated `ffmpeg -vf cropdetect` pass for each file when crop detection is requested. - Optimization: - Cache crop results for repeated runs on the same source. @@ -52,7 +45,7 @@ - Expected value: - Lower latency on repeated experimentation. -4. Tooling overlap and naming drift +3. Tooling overlap and naming drift - There are still overlapping workstation-setup entrypoints across [`tools/configure_workstation.sh`](/home/osgw/.local/src/codex/ffx/tools/configure_workstation.sh), [`tools/setup.sh`](/home/osgw/.local/src/codex/ffx/tools/setup.sh), and newer CLI maintenance commands. - Optimization: - Decide which scripts remain canonical. @@ -62,7 +55,7 @@ - Less operator confusion. - Fewer duplicated procedures to maintain. -5. Placeholder UI surfaces should either ship or disappear +4. Placeholder UI surfaces should either ship or disappear - [`src/ffx/help_screen.py`](/home/osgw/.local/src/codex/ffx/src/ffx/help_screen.py) and [`src/ffx/settings_screen.py`](/home/osgw/.local/src/codex/ffx/src/ffx/settings_screen.py) are placeholders. - Optimization: - Either remove them from the active UI surface or complete them. @@ -71,7 +64,7 @@ - Leaner interface. - Lower UX ambiguity. -6. Large Textual screens repeat configuration and controller loading +5. Large Textual screens repeat configuration and controller loading - Screens such as [`src/ffx/media_details_screen.py`](/home/osgw/.local/src/codex/ffx/src/ffx/media_details_screen.py), [`src/ffx/pattern_details_screen.py`](/home/osgw/.local/src/codex/ffx/src/ffx/pattern_details_screen.py), and [`src/ffx/show_details_screen.py`](/home/osgw/.local/src/codex/ffx/src/ffx/show_details_screen.py) repeat setup patterns and local metadata filtering extraction. - Optimization: - Extract a shared screen base or helper for common config/controller/bootstrap logic. @@ -80,7 +73,7 @@ - Lower maintenance overhead. - Easier UI iteration. -7. Several helper functions are unfinished or dead-weight +6. Several helper functions are unfinished or dead-weight - [`src/ffx/helper.py`](/home/osgw/.local/src/codex/ffx/src/ffx/helper.py) contains `permutateList(...): pass`. - There are many combinator and conversion placeholders across tests and migrations. - Optimization: @@ -90,7 +83,7 @@ - Smaller mental model. - Less time spent re-evaluating inactive paths. -8. Test suite shape is expensive to understand and likely expensive to run +7. Test suite shape is expensive to understand and likely expensive to run - The project still carries a large legacy matrix of combinator files under [`tests/legacy`](/home/osgw/.local/src/codex/ffx/tests/legacy), several placeholder `pass` implementations, and at least one suspicious filename with an embedded space: [`tests/legacy/disposition_combinator_2_3 .py`](/home/osgw/.local/src/codex/ffx/tests/legacy/disposition_combinator_2_3 .py). - A first focused replacement slice now exists in [`tests/integration/subtrack_mapping/test_cli_bundle.py`](/home/osgw/.local/src/codex/ffx/tests/integration/subtrack_mapping/test_cli_bundle.py), so the remaining work is migration and consolidation rather than creating the modern test shape from scratch. - Optimization: @@ -101,7 +94,7 @@ - Faster contributor onboarding. - Easier CI adoption later. -9. Process resource limiting semantics could be clearer +8. Process resource limiting semantics could be clearer - [`src/ffx/process.py`](/home/osgw/.local/src/codex/ffx/src/ffx/process.py) prepends `nice` and `cpulimit` directly when values are set. - Optimization: - Validate and document effective behavior for combined `nice` + `cpulimit`. @@ -110,24 +103,16 @@ - Fewer surprises in production-like runs. - Easier support for user-reported performance behavior. -10. Import-time dependency coupling makes maintenance commands brittle -- Even after recent CLI maintenance additions, the top-level CLI module still imports most application modules before Click dispatch. +9. Regex and string utility cleanup +- [`src/ffx/helper.py`](/home/osgw/.local/src/codex/ffx/src/ffx/helper.py) still has repeated string-replacement churn in filename/TMDB normalization helpers, and regex handling in helpers is easy to regress quietly. - Optimization: - - Push imports for ORM, Textual, TMDB, ffmpeg helpers, and descriptors behind the commands that actually need them. -- Expected value: - - Maintenance commands such as setup and upgrade stay usable when optional runtime dependencies are broken. - - Better separation between media runtime code and maintenance tooling. - -11. Regex and string utility cleanup -- [`src/ffx/helper.py`](/home/osgw/.local/src/codex/ffx/src/ffx/helper.py) still emits a `SyntaxWarning` for `RICH_COLOR_PATTERN`. -- Optimization: - - Convert regex literals to raw strings where appropriate. + - Keep regex literals raw and centralized where appropriate. - Review filename and TMDB substitution helpers for repeated string churn. - Expected value: - Cleaner runtime output. - Less warning noise during dry-run maintenance commands. -12. Database startup always runs schema creation and version checks +10. Database startup always runs schema creation and version checks - [`src/ffx/database.py`](/home/osgw/.local/src/codex/ffx/src/ffx/database.py) runs `Base.metadata.create_all(...)` and version checks every time a DB-backed context is created. - Optimization: - Measure startup cost and consider separating bootstrapping from ordinary command execution. @@ -151,9 +136,9 @@ 1. Triage the list into quick wins, medium refactors, and long-horizon cleanup. 2. Tackle the cheapest high-impact items first: - - regex raw-string warning cleanup, - single-call `ffprobe` refactor. -3. Decide whether maintenance/tooling command imports should be split from media-runtime imports before adding more CLI maintenance surface. + - crop detection sampling or caching pass. +3. Decide which setup and upgrade entrypoints stay canonical before adding more maintenance surface. ## Delete When diff --git a/src/ffx/cli.py b/src/ffx/cli.py index 2d18395..d36f8ad 100755 --- a/src/ffx/cli.py +++ b/src/ffx/cli.py @@ -1,6 +1,9 @@ #! /usr/bin/python3 +from __future__ import annotations + import os, sys, click, time, shutil, subprocess +from typing import TYPE_CHECKING # Allow direct execution via `python src/ffx/cli.py` by preferring the package # root on sys.path. @@ -10,42 +13,24 @@ if __package__ in (None, ''): sys.path = [p for p in sys.path if os.path.abspath(p) != os.path.abspath(script_dir)] sys.path.insert(0, package_root) -from ffx.configuration_controller import ConfigurationController +from ffx.constants import ( + DEFAULT_AC3_BANDWIDTH, + DEFAULT_CONTAINER_EXTENSION, + DEFAULT_CONTAINER_FORMAT, + DEFAULT_DTS_BANDWIDTH, + DEFAULT_STEREO_BANDWIDTH, + DEFAULT_VIDEO_ENCODER_LABEL, + FFMPEG_COMMAND_TOKENS, + SUPPORTED_INPUT_FILE_EXTENSIONS, + VERSION, +) -from ffx.file_properties import FileProperties +if TYPE_CHECKING: + from ffx.media_descriptor import MediaDescriptor + from ffx.track_descriptor import TrackDescriptor -from ffx.ffx_app import FfxApp -from ffx.ffx_controller import FfxController -from ffx.tmdb_controller import TmdbController +LIGHTWEIGHT_COMMANDS = {None, 'version', 'help', 'configure_workstation', 'upgrade'} -from ffx.database import databaseContext - -from ffx.media_descriptor import MediaDescriptor -from ffx.track_descriptor import TrackDescriptor -from ffx.show_descriptor import ShowDescriptor - -from ffx.track_type import TrackType -from ffx.video_encoder import VideoEncoder -from ffx.track_disposition import TrackDisposition -from ffx.track_codec import TrackCodec - -from ffx.process import executeProcess -from ffx.helper import filterFilename, substituteTmdbFilename -from ffx.helper import getEpisodeFileBasename - -from ffx.constants import DEFAULT_STEREO_BANDWIDTH, DEFAULT_AC3_BANDWIDTH, DEFAULT_DTS_BANDWIDTH, DEFAULT_7_1_BANDWIDTH - -from ffx.filter.quality_filter import QualityFilter -from ffx.filter.preset_filter import PresetFilter - -from ffx.filter.crop_filter import CropFilter -from ffx.filter.nlmeans_filter import NlmeansFilter -from ffx.filter.deinterlace_filter import DeinterlaceFilter - -from ffx.constants import VERSION - -from ffx.shifted_season_controller import ShiftedSeasonController -from ffx.logging_utils import configure_ffx_logger @click.group() @@ -58,11 +43,18 @@ def ffx(ctx, database_file, verbose, dry_run): ctx.obj = {} - if ctx.invoked_subcommand in ('configure_workstation', 'upgrade'): + if ctx.resilient_parsing: + return + + if ctx.invoked_subcommand in LIGHTWEIGHT_COMMANDS: ctx.obj['dry_run'] = dry_run ctx.obj['verbosity'] = verbose return + from ffx.configuration_controller import ConfigurationController + from ffx.database import databaseContext + from ffx.logging_utils import configure_ffx_logger + ctx.obj['config'] = ConfigurationController() ctx.obj['database'] = databaseContext(databasePath=database_file @@ -184,6 +176,7 @@ def upgrade(ctx, branch): @click.pass_context @click.argument('filename', nargs=1) def inspect(ctx, filename): + from ffx.ffx_app import FfxApp ctx.obj['command'] = 'inspect' ctx.obj['arguments'] = {} @@ -196,7 +189,7 @@ def inspect(ctx, filename): def getUnmuxSequence(trackDescriptor: TrackDescriptor, sourcePath, targetPrefix, targetDirectory = ''): # executable and input file - commandTokens = FfxController.COMMAND_TOKENS + ['-i', sourcePath] + commandTokens = list(FFMPEG_COMMAND_TOKENS) + ['-i', sourcePath] trackType = trackDescriptor.getType() @@ -237,6 +230,10 @@ def unmux(ctx, subtitles_only, nice, cpu): + from ffx.file_properties import FileProperties + from ffx.process import executeProcess + from ffx.track_disposition import TrackDisposition + from ffx.track_type import TrackType existingSourcePaths = [p for p in paths if os.path.isfile(p)] ctx.obj['logger'].debug(f"\nUnmuxing {len(existingSourcePaths)} files") @@ -307,6 +304,7 @@ def cropdetect(ctx, paths, nice, cpu): + from ffx.file_properties import FileProperties existingSourcePaths = [p for p in paths if os.path.isfile(p)] ctx.obj['logger'].debug(f"\nUnmuxing {len(existingSourcePaths)} files") @@ -333,6 +331,7 @@ def cropdetect(ctx, @click.pass_context def shows(ctx): + from ffx.ffx_app import FfxApp ctx.obj['command'] = 'shows' @@ -341,6 +340,8 @@ def shows(ctx): def checkUniqueDispositions(context, mediaDescriptor: MediaDescriptor): + from ffx.track_disposition import TrackDisposition + from ffx.track_type import TrackType # Check for multiple default or forced dispositions if not set by user input or database requirements # @@ -390,7 +391,7 @@ def checkUniqueDispositions(context, mediaDescriptor: MediaDescriptor): @click.option('-l', '--label', type=str, default='', help='Label to be used as filename prefix') -@click.option('-v', '--video-encoder', type=str, default=FfxController.DEFAULT_VIDEO_ENCODER, help=f"Target video encoder (vp9, av1, h264 or copy)", show_default=True) +@click.option('-v', '--video-encoder', type=str, default=DEFAULT_VIDEO_ENCODER_LABEL, help=f"Target video encoder (vp9, av1, h264 or copy)", show_default=True) @click.option('-q', '--quality', type=str, default="", help=f"Quality settings to be used with VP9/H264 encoder") @click.option('-p', '--preset', type=str, default="", help=f"Quality preset to be used with AV1 encoder") @@ -507,6 +508,20 @@ def convert(ctx, Filename extensions will be changed appropriately. Suffices will we appended to filename in case of multiple created files or if the filename has not changed.""" + from ffx.ffx_controller import FfxController + from ffx.file_properties import FileProperties + from ffx.filter.crop_filter import CropFilter + from ffx.filter.deinterlace_filter import DeinterlaceFilter + from ffx.filter.nlmeans_filter import NlmeansFilter + from ffx.filter.preset_filter import PresetFilter + from ffx.filter.quality_filter import QualityFilter + from ffx.helper import filterFilename, getEpisodeFileBasename, substituteTmdbFilename + from ffx.shifted_season_controller import ShiftedSeasonController + from ffx.show_descriptor import ShowDescriptor + from ffx.tmdb_controller import TmdbController + from ffx.track_codec import TrackCodec + from ffx.track_disposition import TrackDisposition + from ffx.video_encoder import VideoEncoder startTime = time.perf_counter() @@ -519,8 +534,8 @@ def convert(ctx, targetFormat = '' targetExtension = 'mkv' else: - targetFormat = FfxController.DEFAULT_FILE_FORMAT - targetExtension = FfxController.DEFAULT_FILE_EXTENSION + targetFormat = DEFAULT_CONTAINER_FORMAT + targetExtension = DEFAULT_CONTAINER_EXTENSION context['use_tmdb'] = not no_tmdb context['use_pattern'] = not no_pattern @@ -540,7 +555,7 @@ def convert(ctx, context['subtitle_prefix'] = subtitle_prefix - existingSourcePaths = [p for p in paths if os.path.isfile(p) and p.split('.')[-1] in FfxController.INPUT_FILE_EXTENSIONS] + existingSourcePaths = [p for p in paths if os.path.isfile(p) and p.split('.')[-1] in SUPPORTED_INPUT_FILE_EXTENSIONS] # CLI Overrides diff --git a/src/ffx/constants.py b/src/ffx/constants.py index b4f9d87..b1471db 100644 --- a/src/ffx/constants.py +++ b/src/ffx/constants.py @@ -4,6 +4,13 @@ DATABASE_VERSION = 2 DEFAULT_QUALITY = 32 DEFAULT_AV1_PRESET = 5 +DEFAULT_VIDEO_ENCODER_LABEL = "vp9" +DEFAULT_CONTAINER_FORMAT = "webm" +DEFAULT_CONTAINER_EXTENSION = "webm" +SUPPORTED_INPUT_FILE_EXTENSIONS = ("mkv", "mp4", "avi", "flv", "webm") +FFMPEG_COMMAND_TOKENS = ("ffmpeg", "-y") +FFMPEG_NULL_OUTPUT_TOKENS = ("-f", "null", "/dev/null") + DEFAULT_STEREO_BANDWIDTH = "112" DEFAULT_AC3_BANDWIDTH = "256" DEFAULT_DTS_BANDWIDTH = "320" diff --git a/src/ffx/ffx_controller.py b/src/ffx/ffx_controller.py index f3241fc..a4907ff 100644 --- a/src/ffx/ffx_controller.py +++ b/src/ffx/ffx_controller.py @@ -10,7 +10,16 @@ from ffx.track_codec import TrackCodec from ffx.video_encoder import VideoEncoder from ffx.process import executeProcess -from ffx.constants import DEFAULT_cut_start, DEFAULT_cut_length +from ffx.constants import ( + DEFAULT_CONTAINER_EXTENSION, + DEFAULT_CONTAINER_FORMAT, + DEFAULT_VIDEO_ENCODER_LABEL, + DEFAULT_cut_start, + DEFAULT_cut_length, + FFMPEG_COMMAND_TOKENS, + FFMPEG_NULL_OUTPUT_TOKENS, + SUPPORTED_INPUT_FILE_EXTENSIONS, +) from ffx.filter.quality_filter import QualityFilter from ffx.filter.preset_filter import PresetFilter @@ -21,17 +30,17 @@ from ffx.model.pattern import Pattern class FfxController(): - COMMAND_TOKENS = ['ffmpeg', '-y'] - NULL_TOKENS = ['-f', 'null', '/dev/null'] # -f null /dev/null + COMMAND_TOKENS = list(FFMPEG_COMMAND_TOKENS) + NULL_TOKENS = list(FFMPEG_NULL_OUTPUT_TOKENS) # -f null /dev/null TEMP_FILE_NAME = "ffmpeg2pass-0.log" - DEFAULT_VIDEO_ENCODER = VideoEncoder.VP9.label() + DEFAULT_VIDEO_ENCODER = DEFAULT_VIDEO_ENCODER_LABEL - DEFAULT_FILE_FORMAT = 'webm' - DEFAULT_FILE_EXTENSION = 'webm' + DEFAULT_FILE_FORMAT = DEFAULT_CONTAINER_FORMAT + DEFAULT_FILE_EXTENSION = DEFAULT_CONTAINER_EXTENSION - INPUT_FILE_EXTENSIONS = ['mkv', 'mp4', 'avi', 'flv', 'webm'] + INPUT_FILE_EXTENSIONS = list(SUPPORTED_INPUT_FILE_EXTENSIONS) CHANNEL_MAP_5_1 = 'FL-FL|FR-FR|FC-FC|LFE-LFE|SL-BL|SR-BR:5.1' diff --git a/src/ffx/helper.py b/src/ffx/helper.py index 4c40292..cbb8e46 100644 --- a/src/ffx/helper.py +++ b/src/ffx/helper.py @@ -16,7 +16,7 @@ DIFF_REMOVED_KEY = 'removed' DIFF_CHANGED_KEY = 'changed' DIFF_UNCHANGED_KEY = 'unchanged' -RICH_COLOR_PATTERN = '\[[a-z_]+\](.+)\[\/[a-z_]+\]' +RICH_COLOR_PATTERN = '\\[[a-z_]+\\](.+)\\[\\/[a-z_]+\\]' def dictDiff(a : dict, b : dict, ignoreKeys: list = [], removeKeys: list = []): diff --git a/tests/unit/test_cli_lazy_imports.py b/tests/unit/test_cli_lazy_imports.py new file mode 100644 index 0000000..3707219 --- /dev/null +++ b/tests/unit/test_cli_lazy_imports.py @@ -0,0 +1,104 @@ +from __future__ import annotations + +import json +from pathlib import Path +import subprocess +import sys +import textwrap +import unittest + + +REPO_ROOT = Path(__file__).resolve().parents[2] +SRC_ROOT = REPO_ROOT / "src" +HEAVY_MODULES = [ + "ffx.configuration_controller", + "ffx.database", + "ffx.ffx_app", + "ffx.ffx_controller", + "ffx.file_properties", + "ffx.tmdb_controller", +] + + +class CliLazyImportTests(unittest.TestCase): + def run_python(self, code: str) -> dict: + completed = subprocess.run( + [sys.executable, "-c", code], + capture_output=True, + cwd=REPO_ROOT, + text=True, + ) + if completed.returncode != 0: + self.fail( + "Python helper failed\n" + f"STDOUT:\n{completed.stdout}\n" + f"STDERR:\n{completed.stderr}" + ) + return json.loads(completed.stdout) + + def test_importing_cli_keeps_runtime_modules_unloaded(self): + result = self.run_python( + textwrap.dedent( + f""" + import json + import sys + + sys.path.insert(0, {str(SRC_ROOT)!r}) + + import ffx.cli + + print(json.dumps({{ + "modules": {{ + module_name: module_name in sys.modules + for module_name in {HEAVY_MODULES!r} + }}, + }})) + """ + ) + ) + + self.assertTrue( + all(not is_loaded for is_loaded in result["modules"].values()), + result["modules"], + ) + + def test_lightweight_configure_workstation_command_stays_light(self): + result = self.run_python( + textwrap.dedent( + f""" + import json + import sys + from click.testing import CliRunner + + sys.path.insert(0, {str(SRC_ROOT)!r}) + + import ffx.cli + + runner = CliRunner() + invoke_result = runner.invoke( + ffx.cli.ffx, + ["--dry-run", "configure_workstation", "--check"], + ) + if invoke_result.exit_code != 0: + raise SystemExit(invoke_result.output) + + print(json.dumps({{ + "output": invoke_result.output, + "modules": {{ + module_name: module_name in sys.modules + for module_name in {HEAVY_MODULES!r} + }}, + }})) + """ + ) + ) + + self.assertIn("configure_workstation.sh --check", result["output"]) + self.assertTrue( + all(not is_loaded for is_loaded in result["modules"].values()), + result["modules"], + ) + + +if __name__ == "__main__": + unittest.main()