refactor: video branch code de-duplication and simplification

[staging-devin-ai-integration]

refactor: video branch code de-duplication and simplification

Summary

Addresses high and medium priority findings from a code review of the video feature branch. The goal is reducing duplication, loosening coupling, and improving module organization — no behavioral changes intended.

1. Extract codec_forward_loop into shared codec_utils module (~400 lines removed)

• Moved the generic async select-loop (result forwarding, shutdown handling, input-completion draining) from vp9.rs into crate::codec_utils.
• Refactored Opus decoder, Opus encoder, PixelConvertNode, and both VP9 nodes to use the shared helper instead of hand-rolling identical loops.
• The Opus decoder now constructs Packet::Audio(AudioFrame) inside the blocking task so it can share the same codec_forward_loop<T=Packet> signature.

2. Move pixel_ops out of compositor (coupling fix)

• Relocated video/compositor/pixel_ops/ → video/pixel_ops/ as a sibling module. The compositor and pixel_convert both import from the new location.
• Introduced BlitRect in blit.rs to replace the dependency on compositor::config::Rect, so pixel_ops has zero imports from the compositor.
• Gated behind #[cfg(any(feature = "video", feature = "compositor"))] so the module is available both when the umbrella video feature is enabled and when compositor is enabled standalone (since compositor does not depend on video).

3. Split compositor tests (~1050 lines moved)

• Extracted inline tests from compositor/mod.rs into compositor/tests.rs via #[path = "tests.rs"] mod tests;.

4. Consolidate VideoFrame constructor validation

• Extracted fn validate_layout(...) to deduplicate identical validation logic between from_pooled_with_layout and from_arc_with_layout.

Skipped items (analyzed, decided not worth the abstraction):

• FrameCache extraction: The two caching patterns (compositor's per-layer ConversionCache vs pixel_convert's single-slot cache) are structurally different enough that a shared abstraction adds complexity without real benefit.
• Unify text rendering: overlay.rs already delegates to the shared blit_text_wrapped/measure_text_wrapped helpers. The only separate path is colorbars' YUV-plane-direct writing, which is inherently different from RGBA blitting.

Review & Testing Checklist for Human

• Opus decoder behavioral change: The decoder now builds AudioFrame inside spawn_blocking and sends Result<Packet, String> (was Result<PooledSamples, String> + separate metadata). Empty decode results are now continued inside the blocking task instead of being filtered in the forward loop. Verify this doesn't change observable behavior (metadata propagation, empty-frame handling, timing of packet construction).
• BlitRect ↔ Rect conversion in kernel.rs: The compositor now converts its config Rect to BlitRect before calling blit functions. Verify the field mapping (x, y, width, height) is correct and no call sites were missed.
• codec_forward_loop generic correctness across all 5 callers: VP9 dec/enc pass domain objects with a to_packet closure; Opus dec/enc and PixelConvert pass Packet directly (identity-ish to_packet). Verify the type signatures and closure semantics are correct for each.
• Feature gate: pixel_ops is gated on any(feature = "video", feature = "compositor"). Confirm no build issues when enabling individual sub-features (e.g. --features compositor alone, or --features vp9 alone).

Suggested test plan: just lint && just test should be sufficient — all changes are structural refactoring. If relevant benchmarks exist in crates/engine/benches/, running them before/after would confirm no performance regression from the module moves.

Notes

• Requested by: @streamer45
• Link to Devin session

---

[staging-devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[staging-devin-ai-integration]

Devin Review found 1 new potential issue.

View 7 additional findings in Devin Review.

Debug

Playground

[staging-devin-ai-integration]

🔴 Feature gate mismatch: pixel_ops gated on video but consumed by compositor-gated modules

Before this PR, the pixel_ops module lived inside video::compositor::pixel_ops and was always available when the compositor feature was enabled. The PR moves it to video::pixel_ops and gates it on #[cfg(feature = "video")] (crates/nodes/src/video/mod.rs:40-41). However, the compositor feature does NOT depend on video — the dependency is the other direction (video = ["vp9", "colorbars", "compositor"] at crates/nodes/Cargo.toml:132). This means enabling just compositor (without video) breaks two modules:

1. The compositor shim at crates/nodes/src/video/compositor/pixel_ops/mod.rs:12 does pub use crate::video::pixel_ops::*; — the target module doesn't exist.
2. pixel_convert (crates/nodes/src/video/pixel_convert.rs:39) imports from crate::video::pixel_ops — same issue.

CI likely only builds with default features (which include video), so this regression won't be caught automatically. The fix is to gate pixel_ops on a feature that compositor implies — e.g. #[cfg(any(feature = "video", feature = "compositor"))] — or add pixel_ops as a separate internal feature that compositor depends on.

Suggested change

	#[cfg(feature = "video")]
	pub mod pixel_ops;
	#[cfg(any(feature = "video", feature = "compositor"))]
	pub mod pixel_ops;

---

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground