|
| 1 | +# Sharp Frames Application Audit |
| 2 | + |
| 3 | +Date: 2026-07-14 |
| 4 | +Baseline: `develop` at `e712afa` |
| 5 | +Remediation branch: `codex/deep-application-review` |
| 6 | + |
| 7 | +## Scope |
| 8 | + |
| 9 | +This audit covered the direct CLI, Textual UI, extraction and saving pipelines, |
| 10 | +all three selection methods, multi-video and image-directory inputs, sharpness |
| 11 | +analysis, packaging, dependency health, cancellation, and platform support. |
| 12 | + |
| 13 | +The audit combined source inspection with focused behavior probes, synthetic |
| 14 | +FFmpeg runs, package builds, static analysis, and the full test suite. |
| 15 | + |
| 16 | +## Remediation Outcome |
| 17 | + |
| 18 | +All Critical and High findings in this audit are resolved on |
| 19 | +`codex/deep-application-review`. |
| 20 | + |
| 21 | +| Finding | Resolution | |
| 22 | +| --- | --- | |
| 23 | +| C1 | All non-interactive save paths now fail closed on unapproved output conflicts. | |
| 24 | +| C2 | Modern and direct-CLI directory saves transcode to the requested format and deterministically disambiguate colliding stems. | |
| 25 | +| H1–H5 | CLI, TUI, previews, and compatibility entry points share canonical, source-aware selection implementations with exact previews. | |
| 26 | +| H6 | FFmpeg output is continuously drained; completion requires a clean exit; failure, timeout, and cancellation reject partial output. | |
| 27 | +| H7 | Cancellation reaches active extraction and analysis, and temporary directories are cleaned on every tested exit path. | |
| 28 | +| H8 | The declared and tested support floor is Python 3.10. | |
| 29 | +| H9 | Image-only processing skips video-tool checks; video checks require FFmpeg and FFprobe; HDR and platform requirements are documented. | |
| 30 | +| H10 | CLI, TUI, validation, extraction, and documentation use one shared media-format contract. | |
| 31 | +| H11 | FFprobe failures preserve actionable stderr and can no longer be masked by a local import error. | |
| 32 | +| H12 | The maintained suite is green, dependencies no longer install a stale app revision, and CI covers four Python versions on three operating systems. | |
| 33 | + |
| 34 | +Final local verification used clean environments with current dependency |
| 35 | +resolution on Python 3.10, 3.11, 3.12, and 3.13. Each environment passed all |
| 36 | +307 tests. The runtime regressions include real synthetic FFmpeg extraction, |
| 37 | +nonzero partial output, overall timeout, cancellation, temporary-directory |
| 38 | +cleanup, image-only dependency checks, format discovery, and OS-specific |
| 39 | +guidance. The sdist and wheel build cleanly and advertise Python 3.10+ with the |
| 40 | +expected runtime dependencies and license metadata. A final dependency audit |
| 41 | +reported no known vulnerabilities in the resolved environment. |
| 42 | + |
| 43 | +## Baseline Verification |
| 44 | + |
| 45 | +- Wheel build succeeded. |
| 46 | +- A synthetic video completed end-to-end extraction with the expected frame |
| 47 | + count. |
| 48 | +- Color-space tests passed, including real FFmpeg Display P3, BT.2020 SDR, and |
| 49 | + HDR filter execution. |
| 50 | +- The full suite reported 220 passing and 71 failing tests. |
| 51 | +- No GitHub Actions workflow was configured. |
| 52 | +- Two Dependabot findings were open: a medium-severity pytest temporary |
| 53 | + directory issue and a low-severity Pygments ReDoS issue. |
| 54 | + |
| 55 | +## Critical Findings |
| 56 | + |
| 57 | +### C1. Non-interactive saving bypasses overwrite protection |
| 58 | + |
| 59 | +When the Textual path uses `FrameSaver(show_progress=False)`, an existing |
| 60 | +output directory only produces a warning when `force_overwrite` is false. The |
| 61 | +save then proceeds and overwrites matching output names. This contradicts the |
| 62 | +UI choice and creates a direct data-loss risk. |
| 63 | + |
| 64 | +Required outcome: existing output files must remain untouched unless overwrite |
| 65 | +was explicitly authorized. Non-interactive conflicts must fail safely and |
| 66 | +report the conflicting paths. |
| 67 | + |
| 68 | +### C2. Image-directory output can have false extensions and collisions |
| 69 | + |
| 70 | +For directory inputs without resizing, the saver constructs a filename using |
| 71 | +the configured output format but copies the source bytes unchanged. A PNG can |
| 72 | +therefore be written as `image.jpg` while retaining PNG content. Files with the |
| 73 | +same stem but different extensions can map to the same destination and |
| 74 | +overwrite one another. |
| 75 | + |
| 76 | +Required outcome: byte copies preserve their real format, requested format |
| 77 | +changes perform an actual transcode, and every selected input gets a unique, |
| 78 | +deterministic destination. |
| 79 | + |
| 80 | +## High Findings |
| 81 | + |
| 82 | +### H1. Best-N is behaviorally different between CLI and TUI |
| 83 | + |
| 84 | +The Textual `FrameSelector` ranks by sharpness only; its advertised |
| 85 | +distribution weighting is absent. The direct CLI uses a separate segmented |
| 86 | +implementation. Users can receive materially different selections from the |
| 87 | +same frames and parameters depending on entry point. |
| 88 | + |
| 89 | +Required outcome: one canonical algorithm and identical results across CLI, |
| 90 | +TUI, preview, and tests. Sharpness and temporal distribution must be normalized |
| 91 | +to comparable scales before weighting. |
| 92 | + |
| 93 | +### H2. Best-N buffer semantics disagree with the preview |
| 94 | + |
| 95 | +The preview treats `min_buffer` as the number of intervening frames, while |
| 96 | +selection accepts an index distance equal to `min_buffer`. This creates count |
| 97 | +and guarantee mismatches. |
| 98 | + |
| 99 | +Required outcome: define `min_buffer` as intervening frames, require an index |
| 100 | +distance of at least `min_buffer + 1`, and calculate preview counts using the |
| 101 | +same implementation as execution. |
| 102 | + |
| 103 | +### H3. Outlier-removal endpoints contradict each other |
| 104 | + |
| 105 | +At sensitivity 100, the Textual implementation removes every frame, the CLI |
| 106 | +keeps every frame, and the preview estimates that roughly 60 percent remain. |
| 107 | +At sensitivity 0, execution keeps every frame while preview still removes a |
| 108 | +small percentage. |
| 109 | + |
| 110 | +Required outcome: consistent monotonic semantics, with 0 disabling removal and |
| 111 | +100 representing maximum useful aggressiveness without a hard-coded all/none |
| 112 | +special case. |
| 113 | + |
| 114 | +### H4. Outlier preview is not an actual preview |
| 115 | + |
| 116 | +The Textual preview uses a fixed sensitivity-to-removal-rate lookup and ignores |
| 117 | +the scores and window size. It can disagree with execution and show an |
| 118 | +incorrect save count and completion message. |
| 119 | + |
| 120 | +Required outcome: exact preview counts from the canonical selection algorithm. |
| 121 | + |
| 122 | +### H5. Video-directory selection crosses source boundaries |
| 123 | + |
| 124 | +All extracted frames are concatenated. Best-N spacing, batched groups, and |
| 125 | +outlier neighborhoods can cross from one source video to another. Global |
| 126 | +sharpness ranking can also starve videos with lower-resolution or lower-contrast |
| 127 | +content. |
| 128 | + |
| 129 | +Required outcome: group-aware selection. Batches, neighborhoods, and spacing |
| 130 | +reset at source boundaries; Best-N allocates a deterministic and fair quota |
| 131 | +across sources. |
| 132 | + |
| 133 | +### H6. FFmpeg extraction can silently accept incomplete output |
| 134 | + |
| 135 | +The modern extractor pipes stderr without continuously draining it, terminates |
| 136 | +after ten seconds without a newly observed file, may terminate when its frame |
| 137 | +estimate is reached, and accepts any extraction that produced at least ten |
| 138 | +files even if FFmpeg failed or was killed. |
| 139 | + |
| 140 | +Required outcome: continuously consume process output, support explicit |
| 141 | +cancellation and a real overall timeout, wait for a successful FFmpeg exit, |
| 142 | +and never infer success from an arbitrary minimum file count. |
| 143 | + |
| 144 | +### H7. Cancellation does not stop extraction and can leak temporary files |
| 145 | + |
| 146 | +`TUIProcessor.cancel_processing()` only signals sharpness analysis. Active |
| 147 | +FFmpeg extraction continues in a worker thread. Cleanup attempts before |
| 148 | +`current_result` is assigned do not know the temporary directory and therefore |
| 149 | +leak it on cancellation or intermediate errors. |
| 150 | + |
| 151 | +Required outcome: cancellation propagates to the extractor and process, every |
| 152 | +temporary directory has explicit ownership, and cleanup runs on all exit paths. |
| 153 | + |
| 154 | +### H8. Declared Python support is inaccurate |
| 155 | + |
| 156 | +Packaging advertises Python 3.7 and 3.8, while runtime code uses built-in |
| 157 | +generic annotations and executor APIs requiring Python 3.9 behavior. |
| 158 | + |
| 159 | +Required outcome: either restore compatibility or declare and test the real |
| 160 | +minimum. The implemented correction is Python 3.10+ with matching classifiers |
| 161 | +and CI. Python 3.9 is end-of-life, and the security-fixed pytest 9 line also |
| 162 | +requires Python 3.10+. |
| 163 | + |
| 164 | +### H9. Platform dependency checks and documentation disagree with reality |
| 165 | + |
| 166 | +The Textual validation path requires FFmpeg even for image-only input. Modern |
| 167 | +video processing requires FFprobe although metadata describes it as optional. |
| 168 | +HDR conversion requires an FFmpeg build with `zscale`/libzimg but this is not |
| 169 | +documented. |
| 170 | + |
| 171 | +Required outcome: dependency checks are input-type-specific, FFprobe and |
| 172 | +zscale requirements are accurately communicated, and errors include |
| 173 | +platform-appropriate remediation. |
| 174 | + |
| 175 | +### H10. Supported formats differ between entry points |
| 176 | + |
| 177 | +The README, validators, single-file processing, image extractor, and video |
| 178 | +directory scanner use different extension sets. Validated WebP or M4V inputs |
| 179 | +can later be ignored. |
| 180 | + |
| 181 | +Required outcome: shared format constants used by detection, validation, |
| 182 | +extraction, help, and documentation. |
| 183 | + |
| 184 | +### H11. FFprobe errors are masked |
| 185 | + |
| 186 | +`FrameExtractor._get_video_info()` imports `json` inside its try block and |
| 187 | +references it in an exception clause. Failures before that import can surface |
| 188 | +as `UnboundLocalError` instead of the real FFprobe error. |
| 189 | + |
| 190 | +Required outcome: preserve the original failure type and actionable stderr. |
| 191 | + |
| 192 | +### H12. Test and dependency baselines are not release-grade |
| 193 | + |
| 194 | +The suite had 71 failures, many encoding obsolete or contradictory selection |
| 195 | +semantics. `requirements.txt` mixes runtime, documentation, and test packages |
| 196 | +and includes an editable install pinned to an old application commit. |
| 197 | +`tests/requirements.txt` omits Pillow even though tests import it. With no CI, |
| 198 | +these failures cannot prevent regressions. |
| 199 | + |
| 200 | +Required outcome: one truthful green suite, separated dependency manifests, |
| 201 | +and CI across supported Python and operating-system versions. |
| 202 | + |
| 203 | +## Deferred Selection-Quality Roadmap |
| 204 | + |
| 205 | +The following quality improvements are important but are deferred until the |
| 206 | +Critical and High correctness and safety defects above are closed. |
| 207 | + |
| 208 | +### Q1. Normalize analysis resolution |
| 209 | + |
| 210 | +Laplacian variance is strongly resolution-dependent. The same synthetic edge |
| 211 | +scored approximately 2032 at 128 pixels and 508 at 512 pixels under the current |
| 212 | +half-size analysis. Normalize every analysis image to a defined scale before |
| 213 | +comparing focus scores, especially across image directories and source videos. |
| 214 | + |
| 215 | +### Q2. Reduce sensitivity to noise and compression |
| 216 | + |
| 217 | +Noise can score as high-frequency detail. In a focused probe, a blurred image |
| 218 | +scored about 66, while adding noise raised it to about 820. Add light denoising |
| 219 | +and evaluate a combined focus metric such as normalized Laplacian variance plus |
| 220 | +Tenengrad. Validate against a labeled sharp/blurred corpus. |
| 221 | + |
| 222 | +### Q3. Use natural and metadata-aware ordering |
| 223 | + |
| 224 | +Lexicographic ordering places `frame10` before `frame2` and is case-sensitive |
| 225 | +across platforms. Use natural sorting for numbered sequences and consider EXIF |
| 226 | +capture time where appropriate. Temporal methods must operate on the intended |
| 227 | +chronology. |
| 228 | + |
| 229 | +### Q4. Make outlier statistics robust |
| 230 | + |
| 231 | +The global min/max range is distorted by a single extreme noisy or corrupt |
| 232 | +frame. Prefer robust scale estimates such as median/MAD or percentile ranges, |
| 233 | +and consider scene boundaries so legitimate softer scenes are not compared to |
| 234 | +unrelated high-detail scenes. |
| 235 | + |
| 236 | +### Q5. Treat unreadable images explicitly |
| 237 | + |
| 238 | +The modern analyzer converts failed reads into score zero while keeping the |
| 239 | +frame eligible for later saving. Exclude failed inputs from selection, report |
| 240 | +them separately, and expose partial-success information in metadata and UI. |
| 241 | + |
| 242 | +### Q6. Define selection quality metrics |
| 243 | + |
| 244 | +Build a repeatable evaluation corpus covering motion blur, defocus, noise, |
| 245 | +compression, exposure changes, animation, scene cuts, mixed resolution, HDR, |
| 246 | +and multi-camera directories. Track precision/recall for rejected blurry |
| 247 | +frames, coverage uniformity, duplicate rate, and per-source representation. |
| 248 | + |
| 249 | +### Q7. Improve visualization and explainability |
| 250 | + |
| 251 | +The Textual chart only shows the first portion of long inputs. Downsample the |
| 252 | +whole timeline, show source boundaries, and explain why a frame was selected or |
| 253 | +rejected. This makes parameter tuning and regression review substantially more |
| 254 | +trustworthy. |
| 255 | + |
| 256 | +## Lower-Priority Engineering Improvements |
| 257 | + |
| 258 | +- Remove dead and duplicated legacy UI/preview implementations after migration. |
| 259 | +- Replace broad `except`/silent-pass blocks with typed errors and structured |
| 260 | + logging. |
| 261 | +- Avoid import-time FFmpeg subprocess checks, especially for image-only use. |
| 262 | +- Use `opencv-python-headless` as an optional server/CLI installation extra. |
| 263 | +- Add deterministic metadata schemas and atomic metadata writes. |
| 264 | +- Add performance benchmarks for large frame sets and long videos. |
| 265 | +- Add linting, type checking, security scanning, and package smoke tests to CI. |
| 266 | + |
| 267 | +## Exit Criteria for This Remediation |
| 268 | + |
| 269 | +All Critical and High findings are closed only when: |
| 270 | + |
| 271 | +1. Each issue has a regression test that failed before the fix. |
| 272 | +2. CLI and TUI selection results agree for equivalent input and parameters. |
| 273 | +3. All save paths preserve data and honor conflict policy. |
| 274 | +4. Synthetic FFmpeg success, failure, timeout, and cancellation paths pass. |
| 275 | +5. Image-only processing succeeds without FFmpeg. |
| 276 | +6. Supported format and Python-version declarations match tested behavior. |
| 277 | +7. The complete maintained test suite is green. |
| 278 | +8. A final review finds no remaining Critical or High regression. |
0 commit comments