Skip to content

feat: implement video compositor review recommendations#136

Merged
streamer45 merged 1 commit intovideofrom
devin/1773350263-review-recommendations
Mar 13, 2026
Merged

feat: implement video compositor review recommendations#136
streamer45 merged 1 commit intovideofrom
devin/1773350263-review-recommendations

Conversation

@staging-devin-ai-integration
Copy link
Copy Markdown
Contributor

Implement video compositor review recommendations

Summary

Implements all actionable findings from the video compositor & pixel operations code review. Changes span safety hardening, performance improvements, API ergonomics, and benchmark modernisation.

Changes by finding

# Severity Change
1 Critical Add debug assertions for Y-plane raw pointer aliasing in rgba8_to_i420_buf and rgba8_to_nv12_buf
2 Critical Clarify AVX2 _mm256_packus_epi16 cross-lane comments in simd_x86_64.rs
4 Important Shrink ConversionCache entries when resolution decreases (>2× old size)
5 Important Add max-entry bound (64) to FONT_CACHE with non-bundled eviction
7 Important Validate decoded image dimensions against max_canvas_dimension before to_rgba8()
11 Minor Replace fragile sleep(50ms) with recv_timeout in test_identical_frame_caching
12 Minor Derive Copy for Rect and BlitRect, remove unnecessary .clone() calls
14 Nit Remove compositor/pixel_ops/mod.rs re-export shim (all consumers updated to crate::video::pixel_ops)
16 Nit Extract shared bench_utils module for generate_rgba_frame and frame generators
17 Nit Use font.metrics() instead of font.rasterize() for measurement-only calls in measure_text
Migrate compositor_only and pixel_convert benchmarks to criterion

Benchmark results (criterion, 100 samples)

Compositor (1280×720)

Scenario Time/frame
1-layer-rgba 87 µs
2-layer-rgba-pip 110 µs
4-layer-rgba 172 µs
2-layer-i420+rgba 107 µs
2-layer-nv12+rgba 104 µs
2-layer-rgba-rotated 247 µs
2-layer-pip+overlays 144 µs
i420+pip+overlays 137 µs
rgba-to-nv12-output 78 µs

Pixel convert (1280×720)

Conversion Time/frame
rgba8→nv12 78 µs
rgba8→i420 89 µs
nv12→rgba8 108 µs
i420→rgba8 149 µs

Review & Testing Checklist for Human

  • Verify the decode_image_overlay dimension check rejects oversized images (finding chore: proper notice file #7) — construct a test image wider than max_canvas_dimension and confirm the error
  • Check that FONT_CACHE eviction preserves bundled fonts under pressure — send >64 unique font_data_base64 overlays and confirm bundled entries survive
  • Run cargo bench -p streamkit-engine --bench compositor_only and --bench pixel_convert locally to verify criterion produces sensible results on your hardware
  • Confirm Rect being Copy doesn't break any downstream serialisation / TypeScript codegen (the Deserialize / JsonSchema / ts_rs::TS derives are unaffected but worth a spot check)

Notes

  • compositor_pipeline.rs is kept as a custom harness (not criterion) since it's an integration benchmark with validation logic (EBML header, output size checks)
  • The cold-cache benchmark from the original pixel_convert.rs was intentionally not migrated to criterion — its pre-allocated-unique-buffers approach doesn't map well to criterion's iteration model
  • Session requested by Claudio

Devin Session: https://staging.itsdev.in/sessions/832613bfbf1f4ace9294ba1525e568ec

- Add debug assertions for Y-plane raw pointer aliasing in convert.rs
- Clarify AVX2 packus cross-lane comments in simd_x86_64.rs
- Shrink ConversionCache entries when resolution decreases (>2x)
- Add max-entry bound (64) to FONT_CACHE with eviction
- Validate decoded image dimensions against max_canvas_dimension
- Fix fragile sleep-based test in pixel_convert.rs
- Derive Copy for Rect and BlitRect, remove unnecessary clones
- Remove compositor/pixel_ops/mod.rs re-export shim
- Extract shared bench_utils module for generate_rgba_frame
- Use font.metrics() instead of rasterize() for measurement
- Migrate compositor_only and pixel_convert benchmarks to criterion

Signed-off-by: Devin AI <devin@cognition.ai>
Co-Authored-By: Staging-Devin AI <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
@staging-devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 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

@streamer45 streamer45 merged commit c9c6942 into video Mar 13, 2026
@streamer45 streamer45 deleted the devin/1773350263-review-recommendations branch March 13, 2026 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant