[kixelated]

Quest: quest/m1/video-readback.md (finished and deleted here).

Problem

A consumer that needs a rendered picture's raw RGBA, with no encoder behind it (such as a Linux virtual camera), could only reach moq-video's GPU readback through render::export::Exporter, whose setup probes for an encoder. The alternative was copying the staging-buffer pool, row unpadding, and mapping lifecycle out of the crate.

Approach

• Moved the private readback out of render/export.rs into a public render::readback module: Readback::new(device, queue, size), Readback::copy(&texture) -> Option<Pending> (non-blocking, None once Readback::DEPTH copies are in flight), and Pending::wait() -> convert::Rgba (tightly packed rows).
• One implementation: Exporter's CPU route now copies through Readback and converts its Rgba to I420, as before. Exporter::DEPTH is defined as Readback::DEPTH.
• The size, format (Rgba8Unorm), and usage check is shared. Readback asks only for COPY_SRC; Exporter still also asks for TEXTURE_BINDING, which its GPU routes sample through.
• Tests: render::readback::tests checks the round trip byte for byte at a width whose rows need padding (60 px), dropping past the depth, and refusing a mismatched or non-COPY_SRC texture. The exporter keeps a test that its readback route hands back I420. just rs render-sw now runs render::readback::tests as well.

Impact

• Added moq_video::render::readback::{Readback, Pending} (behind the render feature). Additive only.
• Exporter::copy's mismatch error text now says "cannot read" instead of "cannot export".
• No wire impact. No MIGRATE.md note: since moq-dev/moq#8024 only the release cut edits it, and this is additive.

Choices made without asking

• Module and names: render::readback::{Readback, Pending}, mirroring render::export::{Exporter, Pending}, as the quest recommended.
• Pending::wait returns the existing convert::Rgba (size plus packed bytes) rather than a bare Vec<u8>, so the picture carries its size; into_data() gives the Vec.
• Only Rgba8Unorm is accepted, matching Exporter.

Alternatives

• Keep Readback inside render::export and make it public there: rejected, since it has nothing to do with exporting to an encoder.

Follow-ups

• None.

Validation

• just rs render-sw (Mesa lavapipe): all 13 GPU tests pass, including the new render::readback::tests::*.
• cargo clippy -p moq-video --features render --all-targets -D warnings: clean.
• just check: passes (11918 tests).

Review

Head 10384d6: review exempt, a conflict-free merge of origin/main (GitHub had reported the branch as conflicting).

Codex gpt-6.1-sol, medium effort, round 1 on d217f853af46587487627ef9936d2adcc157b013: no findings.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 21 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d55ebe86-b7a6-4dcb-8701-bc18292ba743

Commits

Reviewing files that changed from the base of the PR and between f2f9bc9 and 10384d6.

Files selected for processing: 8

• quest/m1/README.md
• quest/m1/video-readback.md
• rs/justfile
• rs/moq-video/src/convert.rs
• rs/moq-video/src/render/export.rs
• rs/moq-video/src/render/export/vulkan.rs
• rs/moq-video/src/render/mod.rs
• rs/moq-video/src/render/readback.rs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed head: d217f85.

No actionable findings in this change.

Direction: extracting the existing staging-buffer pool into render::readback fits the encoder-free RGBA consumer without introducing a second implementation. The exporter still performs its RGBA-to-I420 conversion after the shared readback; its stricter texture-usage requirements remain intact. I checked the pool's pending-handle ownership and reuse, mapping-error cleanup, padded-row removal, the renderer's texture contract, and the updated test selection.

Validation limits: this was a GitHub-only source review. I did not execute the GPU tests, Clippy, or the workspace checks, and the PR's reported local results were not independently reproduced. GitHub's Check was still in progress when inspected. This comment is not an approval or a merge-readiness determination.

[pullfrog]

✅ No new issues found.

Reviewed changes

• render::readback extracted — the private staging-buffer Readback/Staged pair moved out of render/export.rs into a public render::readback::{Readback, Pending}; Readback::new now rejects a zero size and returns Result, and Pending::wait returns convert::Rgba (built by the new Rgba::new) so the picture carries its size.
• One readback path — Exporter's Route::Readback slot owns a Readback, Exporter::DEPTH is Readback::DEPTH, and the size/format/usage check is shared as readback::check, which changes Exporter::copy's refusal text to "cannot read".
• Tests and runner — render::readback::tests covers a byte-exact round trip at 60 px (padded staging rows), the depth drop, and refusal of a mismatched or non-COPY_SRC texture; the exporter keeps an I420 route test; the nightly rs render-sw recipe now selects the new tests.
• Quest retired — quest/m1/video-readback.md and its m1 README line are deleted per the completion convention.

I checked the parts a move like this usually breaks: the staging pool accounting (free/allocated, including the failed-mapping decrement) and the map_async → poll(Wait{submission_index}) → get_mapped_range → unmap lifecycle are unchanged; Exporter::DEPTH stays 3 for the platform GPU routes that also size their pools from it; the nextest filter still selects the pre-existing render::export::tests::* and render::export::nv12::tests::* cases; and the new tests are reachable from a real --run-ignored caller, so no sh/gh/test-inventory.allow entry is needed. The additive module needs no MIGRATE.md note under the current rule, which supersedes the deleted quest's suggestion to add one.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}