feat(moq-video): render decoded frames on the GPU, and carry color spaces end to end - #2552
Merged
Merged
Conversation
Adds a `render` role module behind a non-default `render` feature: a wgpu pipeline that converts a decoded frame to RGB and hands back a texture the caller presents. This is the egress half of the zero-copy story #2467 started, and the consumer `Surface::PixelBuffer` has been missing since decoded frames began staying GPU-resident. A hardware-decoded frame is imported by aliasing the decoder's IOSurface as a Metal texture through CVMetalTextureCache, adopted via wgpu's hal seam. The CoreVideo wrapper rides in the texture's drop callback, since it is what holds the pixel buffer open: releasing it early lets the decoder's pool recycle the surface while a submitted draw is still sampling it. Anything the fast path does not recognize falls back to a plane upload, and an import path that keeps failing retires itself. I420 now records the color space it was converted with rather than leaving it to be guessed downstream. The crate's RGB conversions all emit BT.601 limited at every resolution, so inferring BT.709 from a frame's size (what a player does with an untagged stream) skewed saturated colors above 576 lines: red rendered as (255, 24, 0). Passthrough paths still infer, since nothing names the space those samples are in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
WalkthroughAdds color-space and range metadata to I420 frames, including inference for frame size and macOS NV12 formats. Introduces an optional 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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}
Contributor
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 `@rs/moq-video/src/frame.rs`:
- Around line 527-537: Update the packed-frame construction around pack and
from_yuyv so from_yuyv passes None for the color metadata, while RGB-derived
callers continue passing Some(Color::Bt601Limited). Preserve the input
color/range for BT.709 and full-range V4L2 YUYV frames, and add a regression
test verifying I420::color() takes precedence over resolution-based inference
and prevents incorrect conversion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: add985e2-8a33-434b-85a3-d0abe58fa3dc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
rs/moq-video/Cargo.tomlrs/moq-video/src/capture/channel.rsrs/moq-video/src/color.rsrs/moq-video/src/error.rsrs/moq-video/src/frame.rsrs/moq-video/src/lib.rsrs/moq-video/src/render/color.rsrs/moq-video/src/render/metal.rsrs/moq-video/src/render/mod.rsrs/moq-video/src/render/renderer.rsrs/moq-video/src/render/shader.wgslrs/moq-video/src/render/source.rsrs/moq-video/src/size.rs
The Linux CI job failed on dead code that a macOS build cannot see: an item is
only dead where its consumers are cfg'd or feature-gated out, and on macOS
those consumers all compile.
- `Layout::Nv12` and its pipeline are constructed only by the Metal importer,
so they follow it. The shader still declares the entry point everywhere, so
naga keeps validating it on every platform.
- `Color::with_range` serves surfaces whose pixel format names their range,
which today is only CoreVideo's NV12.
- `Color::{weights, limited}` derive the shader's matrix and had no business on
the shared enum: they move into the render module, which is the only caller
and is itself behind a non-default feature.
Verified across {macOS, Linux} x {default, all-features}; the Linux half runs
in a Fedora container, confirmed to reproduce the original failure before the
fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
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 `@rs/moq-video/src/color.rs`:
- Around line 46-51: Update the documentation near the matrix range method to
remove the speculative sentence about widening for future platform import paths,
keeping only the description of current range-specific behavior and the existing
NV12 case.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 25e5704d-f0c4-4ed6-b00a-b791b2e29f2d
📒 Files selected for processing (4)
rs/moq-video/src/color.rsrs/moq-video/src/render/color.rsrs/moq-video/src/render/renderer.rsrs/moq-video/src/render/source.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rs/moq-video/src/render/color.rs
- rs/moq-video/src/render/source.rs
- rs/moq-video/src/render/renderer.rs
…uced `pack` hardcoded BT.601 limited for all four of its callers, but `from_yuyv` is a 4:2:2 -> 4:2:0 chroma resample with no color conversion: V4L2 hands back samples already in the camera's space. Tagging those BT.601 regressed the capture path, since a 720p camera is usually BT.709 and the resolution heuristic had been getting it right. `pack` now takes the color, so a caller that picks a matrix states it and one that only moves samples leaves it open. Also stop linking `Surface::PixelBuffer` from the render module's docs. The variant is macOS-only, so the link dangles in the docs built for every other platform and fails CI's `-D warnings` rustdoc pass. Regression tests for both, the YUYV one gated to Linux where that path compiles; verified in a Fedora container to fail without each fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- `Color::with_range` no longer speculates about platforms that might gain an import path; it says why it is macOS-only. - The note explaining why `PixelBuffer` is not a doc link is for whoever is tempted to add one, not for a consumer reading docs.rs, so it becomes a plain comment instead of doc prose. - `render::Config::color` still claimed `None` infers from the frame's size. It has taken the frame's own `I420::color` first since that field landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding on #2552. #2553 taught `PixelBuffer` to read the matrix off the buffer's `kCVImageBufferYCbCrMatrixKey` attachment, which VideoToolbox copies out of the stream's VUI, and routed `download_i420` through it. The zero-copy Metal import kept inferring the matrix from the frame's size. So the same decoded buffer was read two different ways depending on which path it took: a 1080p source tagged BT.601 rendered correctly once it fell back to the CPU, and wrongly whenever the import succeeded. Which path a frame takes is supposed to be a question of cost, not of what color it comes out. The import now asks the buffer, falling back to the size guess only for the planar format, which carries no such attachment. Also drop the claim in `Color`'s docs that a decoded frame never knows its own space. That stopped being true when the attachment started being read, and the sentence pointed at `I420::color` rather than the `Surface::color` a caller actually reaches for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kixelated
enabled auto-merge (squash)
July 29, 2026 18:58
#2553 made the RGB conversions convert into `Color::infer`'s space, so the render test asserting they always produce BT.601 no longer held. CI could not catch it: the test needs a GPU adapter and is `#[ignore]`d. The stale assertion was also the whole point of the test. Now that conversion and inference agree by construction, checking one against the other proves nothing, so the test grows a case where they genuinely disagree: a frame converted at SD and scaled past 576 lines keeps its BT.601 samples while its size implies BT.709. Rendering that by size alone reproduces the original (255, 24, 0), confirmed by mutation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kixelated
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First half of the
rendermodule from #2481's Wave 1. Adds arenderrole module to moq-video behind a non-defaultrenderfeature: a wgpu pipeline that converts a decoded frame to RGB and hands back a texture the caller presents.This is the egress half of the zero-copy story #2467 started.
Surface::PixelBufferhas had a producer (capture, VideoToolbox decode) and no consumer since decoded frames began staying GPU-resident; this is the consumer.Summary
render::Renderertakes the application'swgpu::Device/Queueand returns a plainwgpu::Texturefromrender(&Frame). That texture is the whole integration seam, so nothing here depends on windowing or a UI toolkit. The renderer caches its pipeline, plane textures, and output texture across frames.CVMetalTextureCachealiases the decoder's IOSurface planes as Metal textures, adopted through wgpu'shalseam. The CoreVideo wrapper rides in the texture's drop callback, because it is the wrapper (not theMTLTexture) that holds the sourceCVPixelBufferopen. Releasing it when the import returns lets the decoder's pool recycle the surface while a submitted draw is still sampling it: a torn frame under pool pressure and nothing at all when the pool is idle.Surface::into_i420. An import path that keeps failing retires itself after three strikes rather than paying for the attempt at frame rate.I420now records the color space it was converted with, and only a conversion that actually picks a matrix labels its output. The RGB conversions emit BT.601 limited at all resolutions, so inferring BT.709 from a frame's size (what a player does with an untagged stream) applied the wrong inverse matrix above 576 lines and skewed saturated colors: red rendered as(255, 24, 0). Paths that merely move samples around leave the space open for the consumer to infer, which matters for V4L2 YUYV capture, where claiming BT.601 would pin a 720p camera to the wrong matrix instead of letting the heuristic get it right. The macOS download does carry the range through, since the pixel format names it.Deliberately split: the Vulkan DMA-BUF and EGL/GLES importers land with
Surface::DmaBuf, which they are the consumers for. A#[non_exhaustive]variant with neither a producer nor a consumer would be dead public API, so it ships alongside VAAPI/PipeWire in one hardware-validated series.Public API changes
All additive; nothing renamed, removed, or signature-changed. Targets
mainper Branch Targeting.rendercargo feature (off by default) andmoq_video::rendermodule:Renderer,Config, and a re-exportedwgpu(a major bump of which is a breaking change for this crate, same contract as the existingobjc2_core_videore-export).moq_video::Colorenum (#[non_exhaustive]) withColor::infer.I420::color()andI420::with_color().I420::newis unchanged and defaults the color to unknown.Error::Rendervariant on the already-#[non_exhaustive]error enum.Size::validate_nonzeroispub(crate), split out ofvalidateso an RGB render target is not held to I420's even-dimension rule.Surface::color()reports a frame's color space where the crate knows it, andencode::Config::coloroverrides what the encoder writes into the VUI.h264-readeris added as a dev-dependency for the SPS-parsing tests.Default
moq-video,moq-relay, andmoq-clibuilds pull no wgpu; confirmed withcargo tree.Test plan
just checkclean (workspace,--all-features, on currentmain).--ignoredon an M-series Mac. The four GPU tests are#[ignore]because CI has no adapter; the color math they depend on is covered by tests that need no device and do run in CI.{macOS, Linux} x {default features, --all-features}(clippy, rustdoc, and tests), the Linux half in a Fedora container. That matrix exists because an item is only dead where its consumers are compiled out: three helpers here are reachable solely from the macOS import or the non-defaultrenderfeature, so a macOS-only check cannot see them. The container was confirmed to reproduce each failure before its fix rather than being trusted for going green.got [255, 24, 0, 255], expected about [255, 0, 0, 255]) before the fix. The sibling render tests all run at 64x64, which sits below the 576-line threshold where the size guess happens to agree with the conversion, so only that test pins it down.imports_survive_decoder_pool_recyclingrenders 24 frames through a fixed-size pool, dropping each immediately, asserting none fell back to the CPU. It is a race, so passing is evidence rather than proof.the_metal_import_matches_the_cpu_pathasserts the zero-copy output is pixel-identical to the upload path, and thatstrikes == 0so a silent fallback cannot satisfy it.yuyv_capture_keeps_its_color_space_opencovers the V4L2 path; Linux-only, since that is where it compiles.just checkclean, and CI green on Linux, Windows, and macOS.#[ignore]d GPU tests that CI structurally cannot run. Fixed on the combined head, and strengthened: it now also renders a frame converted at SD and scaled past 576 lines, where the samples stay BT.601 while the size implies BT.709. Mutation-checked (renderer ignoring the declared space reproduces the original(255, 24, 0)).Cross-package sync
doc/row. moq-video has no page underdoc/at all today, so this would mean authoring the crate's entire documentation presence rather than extending a page. The module and item docs cover the consumer surface via docs.rs. Worth doing as its own change.Stacked change: the encode side, from #2553
The encode-side half of the same bug landed here rather than separately: #2553 was opened against this branch and merged into it, so this PR now carries both. Squash-merging produces one commit for the pair.
The bug it fixes was pre-existing and worse than the rendering one, because it reaches players we do not control: every RGB-to-I420 conversion was BT.601 at all resolutions, and nothing wrote a VUI color description, so Windows Desktop Duplication, PipeWire, and V4L2 capture above 576 lines shipped BT.601 pixels in an untagged stream that a compliant player decodes as BT.709.
Color::infer's space for the frame's size rather than always BT.601, and label the result. Pixels and label are chosen together, so they cannot drift apart.encode::Config::colorpins the space when frames come from somewhere the crate did not convert. A frame whose declared space disagrees with the encoder's warns once instead of failing, so a live gateway keeps serving rather than dropping the stream.moq-transcodeopens each rung's encoder from its first frame, since a rung scaled below 576 lines still carries the source's space and would otherwise be labeled by its own size.The tests parse the color description back out of the encoded SPS rather than asserting on config, so a backend that drops the VUI (or a driver that ignores the request) fails instead of passing on our own bookkeeping.
Refs #2481, #2467, #2272, #1837.
(Written by Opus 5)