perf: parallelize face detection during reframing - #137
Conversation
Face sampling in _track_and_crop was the dominant cost of the reframing stage, and it was inference-bound rather than decode-bound: on a 60s 1080p clip, YuNet accounted for ~83% of the time against ~17% for decode. Detections are independent per frame and OpenCV releases the GIL inside YuNet, so frames are now decoded serially (cheap, and it keeps frame indices exact) and detected on a small thread pool. Each worker owns its own cv2.FaceDetectorYN and walks its own stride of the batch: the detector carries per-instance input-size state and is not thread-safe, and striding keeps the assignment deterministic. Batches are bounded by bytes rather than frame count so a 4K source does not hold a whole clip of decoded frames in memory. Measured on a 60s 1080p clip: the stage goes 3281ms -> 1179ms (2.8x), with detection itself 2718ms -> 594ms (4.6x). Output is bit-identical, verified across 620 real decoded frames at 4, 8 and 12 workers. The pool is sized by workload as well as by cores. Each extra detector costs ~21ms to construct against ~4.5ms per detection, so on a short clip an unconditional pool is a net loss — a 2s clip measured 216ms -> 313ms before the cap was added. Below 32 frames per worker it now stays serial. Also downscale to 320px and drop audio before the scene-detect filter in count_scene_cuts, which decoded every split-screen clip at full resolution with audio. The scene score is a whole-frame statistic, so this yields the same cuts for a fraction of the decode: 240ms -> 110ms on a 12s 1080p clip, with scene scores within 0.005 of the full-resolution values. Adds two diagnostics behind existing conventions: a timed() context manager that emits stage timings at debug level (silent unless PODCLI_LOG_VERBOSE), and PODCLI_CROP_DUMP, which writes each clip's computed camera path as JSON so framing decisions can be diffed as text across a change. Crop decisions were previously the least testable part of the pipeline — unit tests cover the helper math, and the e2e render uses a synthetic video with no faces. PODCLI_FACE_WORKERS=1 forces serial detection; it changes speed only, never the result, so it is the first thing to reach for when bisecting a framing regression.
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe render pipeline adds stage timing logs, downscaled scene-cut detection, batched threaded face detection, optional crop-path JSON diagnostics, configuration documentation, and deterministic regression tests. ChangesRender pipeline updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClipGenerator
participant TrackAndCrop
participant ThreadPoolExecutor
participant CropPathDump
ClipGenerator->>TrackAndCrop: process decoded frame batches
TrackAndCrop->>ThreadPoolExecutor: run deterministic face detection
ThreadPoolExecutor-->>TrackAndCrop: return ordered detections
TrackAndCrop->>CropPathDump: write crop metadata when configured
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/services/video_processor.py (1)
1689-1703: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDump misses two paths it is meant to guard.
Two blind spots in the harness:
- It runs before
_simplify_keyframes()(Line 1713), so the path that actually reaches FFmpeg isn't what gets dumped — a regression in simplification or_build_cam_exprwon't show up in a diff.- The mixed-layout branch returns at Lines 1400/1448/1472/1517, so mixed-layout clips never dump at all.
Moving the call after simplification (and adding one in the mixed-layout branch, or dumping
runs/crop_xthere) would close both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/video_processor.py` around lines 1689 - 1703, The crop-path dump currently captures keyframes before _simplify_keyframes(), and mixed-layout early-return paths bypass it. Move the dump in the standard pipeline to after _simplify_keyframes() so it reflects the path passed to FFmpeg, and add equivalent dumping to the mixed-layout branch before each relevant return, using its computed runs or crop_x values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/utils/log.py`:
- Around line 62-64: Update the `timed` function’s `log_event` call to remove
reserved keys (`category`, `message`, `level`, `stage`, and `ms`) from the
merged `fields` and `extra` payload before expansion. Ensure the `finally`
logging path cannot raise a duplicate-key `TypeError` that masks an exception
from the timed block.
---
Nitpick comments:
In `@backend/services/video_processor.py`:
- Around line 1689-1703: The crop-path dump currently captures keyframes before
_simplify_keyframes(), and mixed-layout early-return paths bypass it. Move the
dump in the standard pipeline to after _simplify_keyframes() so it reflects the
path passed to FFmpeg, and add equivalent dumping to the mixed-layout branch
before each relevant return, using its computed runs or crop_x values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d38bc49b-cb99-49c9-8174-7bfacaae7b6d
📒 Files selected for processing (7)
backend/services/clip_generator.pybackend/services/local_reframe.pybackend/services/video_processor.pybackend/utils/log.pydocs/configuration.mdtests/test_crop_path_golden.pytests/test_local_reframe.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_log_timed.py`:
- Around line 13-22: Update both test_reserved_field_does_not_mask_exception and
test_reserved_extra_field_does_not_mask_exception to capture the raised
ValueError with assertRaises and assert that its message remains "original",
verifying the context manager does not replace the original exception.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41323d55-fc6f-466c-a36d-28b5f126e2c5
📒 Files selected for processing (2)
backend/utils/log.pytests/test_log_timed.py
1 participant
Face sampling in
_track_and_cropdominates the reframing stage, and it turned out to be inference-bound, not decode-bound. On a 60s 1080p clip YuNet is ~83% of the stage against ~17% for decode — so the obvious "decode fewer pixels" optimization would have targeted the wrong 17%.Detections are independent per frame and OpenCV releases the GIL inside YuNet, so frames are now decoded serially (cheap, and it keeps frame indices exact) and detected on a small thread pool.
Results
60s 1080p clip, single shared scale:
Decode is untouched — it was never the bottleneck.
By clip length (same scale):
Correctness
Output is bit-identical — only the wall clock moves. Verified across 620 real decoded 1080p frames at 4, 8 and 12 workers (
identical=True,order_ok=True).Two properties make that safe:
cv2.FaceDetectorYNcarries per-instance input-size state and is not thread-safe.iis always handled by detectori % n.Batches are bounded by bytes rather than frame count, so a 4K source does not hold a whole clip of decoded frames in memory.
Pool sizing
The pool is sized by workload as well as by cores. Each extra detector costs ~21ms to construct against ~4.5ms per detection, so an unconditional pool makes short clips slower — a 2s clip measured 216ms → 313ms before this cap. Below 32 frames per worker it now stays serial, which is the 2s row above.
Also included
Scene detection, 2.2x.
count_scene_cutsdecoded every split-screen clip at full resolution with audio. Now-an -snplusscale=320:-2ahead of the filter: 240ms → 110ms on a 12s 1080p clip, identical cut counts, scene scores within 0.005 of full-resolution values.Two diagnostics. A
timed()context manager emitting stage timings at debug level (silent unlessPODCLI_LOG_VERBOSE), andPODCLI_CROP_DUMP, which writes each clip's computed camera path as JSON so framing decisions diff as text. Crop decisions were the least testable part of the pipeline — unit tests cover the helper math, and the e2e render uses a synthetic video with no faces in it.Escape hatch.
PODCLI_FACE_WORKERS=1forces serial detection. It changes speed only, never the result, so it is the first thing to reach for when bisecting a framing regression.Testing
scripts/e2e_render.pypasses all 11 checks onbrandedandkaraoketests/test_crop_path_golden.pyandtests/test_local_reframe.py— the latter had no coverage at alltest_ai_fallback::test_find_cli_falls_back_to_shell_lookup, which fails identically on a clean checkout and is environment-dependentNot covered
An end-to-end crop-path diff on a real episode. Synthetic video has no faces, so
_track_and_cropreturns before the dump runs. The unit-level proof of identical detections is strong, but confirming the full camera path on real footage needs a real video:Decode is now 48% of the remaining stage time, so overlapping it with inference is the next available win — deliberately left out here, since it is the first change that could actually reorder work.
Summary by CodeRabbit
Performance
Diagnostics
Documentation
Reliability