mirror of
https://github.com/nestriness/nestri.git
synced 2026-09-24 19:42:24 +03:00
1e81a8f92d81c7a0f9c33379255d22f67b154823
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 KAAL1 (Bingus)
|
200bc9c75f |
fix(nescapture): stop defaulting unreadable formats to BGRA (#315)
Stacked on #313, which this depends on. Found while checking whether the 10-bit HDR path actually works now that a client can obtain an HDR swapchain (see #314). **It does** — verified end to end rather than from the format list: a client requesting `A2B10G10R10` + `HDR10_ST2084` produces ``` pix_fmt=yuv420p10le color_range=pc color_space=bt2020nc color_transfer=smpte2084 color_primaries=bt2020 ``` which is a correctly tagged HDR10 stream, and the first pixel-level HDR check here with 10-bit rather than 8-bit input. Three ways it could have gone wrong instead. ## Unrecognised formats defaulted to BGRA `vk_format_to_input_format` returned `BGRA` for anything it did not know, which reads a packed 10-bit or FP16 buffer as eight-bit channels. It now returns `None`, and the encode loop drops those frames with one log line per format. A stalled stream is a complaint. A stream at full frame rate carrying nonsense is not, and that is the failure this area keeps producing. ## Bit depth and input format had drifted apart They were two separate matches on the same `VkFormat`. `A2R10G10B10` counted as ten-bit in one and had no entry in the other, so it fell back to eight-bit BGRA — the encoder configured for ten bits while the converter read eight. Depth now derives from the input format, so that disagreement is unrepresentable. `A2R10G10B10` stays unmapped deliberately: a WSI layer offers it as one of its HDR pairs and the compositor dmabuf list advertises it, but the converter has no red-first 10-bit input, so there is nothing correct to map it to. ## The CPU fallback could not read either HDR format It read four bytes per pixel for every format and encoded eight-bit regardless, so a packed 10-bit buffer became garbage and an FP16 one was half an image of misread floats. It now refuses what it cannot read. ## Recorded, not fixed: the colour space we see is not always the one requested A FIXME at the point the value is read. A WSI layer rewrites `imageColorSpace` to `SRGB_NONLINEAR` before calling down — deliberately, since it carries the real colour space to the compositor out of band. We sit below it, so we read the rewrite. Measured, all three lines from one run: ``` [Gamescope WSI] ... colorspace: VK_COLOR_SPACE_HDR10_ST2084_EXT swapchain created — format=A2B10G10R10 colorspace=SRGB_NONLINEAR (re)init encoder: H265 Yuv420 Ten Bt709 → P010 ``` Ten-bit right, BT.709 wrong: PQ samples encoded and tagged as SDR. The same client *without* the layer gives `Ten Bt2020` and an smpte2084 stream, so this is specific to the layer path — which is the path Proton titles take. The fix cannot be local; the true colour space only exists in the compositor, which does receive it, so it needs a channel from there. Layer ordering is not a fix — we do not control it, and the non-layer path still needs the Vulkan value. Left out of this PR as a design change rather than a bug fix. ## Verification - 4 new tests, 12 total, all passing. - No new clippy warnings (diffed against the base branch). - 10-bit HDR path: unchanged, still `Ten Bt2020` / smpte2084. - SDR path: `verify-chain.sh` passes, 835 frames, 8-bit BGRA, brightness agreement 0.57. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR makes Vulkan format handling fail safely instead of interpreting unsupported swapchain buffers as BGRA. - Maps supported Vulkan formats to explicit converter inputs and derives bit depth from that mapping. - Drops unsupported GPU frames with rate-limited logging. - Rejects unsupported HDR formats in the eight-bit CPU fallback. - Documents the color-space limitation caused by rewritten WSI metadata. - Adds tests covering unsupported, eight-bit, 10-bit, and FP16 formats. <h3>Confidence Score: 5/5</h3> The PR appears safe to merge, with no new actionable issues introduced since the previous review. The changes since the previous review are empty, the sole previous finding was manually resolved after Greptile conceded it based on the stacked PR dependency, and the full PR introduces no confirmed rule violations or remaining correctness failures. <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | apps/nescapture/src/encode.rs | Replaces unsafe BGRA fallback behavior with explicit format validation, consistent bit-depth derivation, guarded CPU fallback, and focused tests. | | apps/nescapture/src/swapchain.rs | Documents the known WSI color-space rewrite limitation at the point where swapchain metadata is recorded. | <h3>Flowchart</h3> ```mermaid %%{init: {'theme': 'neutral'}}%% flowchart TD A[Captured Vulkan frame] --> B{Known converter input?} B -->|No| C[Log format change and drop frame] B -->|Yes| D[Derive input format and bit depth] D --> E{DMA-BUF path available?} E -->|Yes| F[GPU color conversion and encoding] E -->|No| G{Eight-bit RGBA or BGRA?} G -->|Yes| H[CPU conversion and encoding] G -->|No| I[Return recoverable error] ``` <sub>Reviews (3): Last reviewed commit: ["docs(nescapture): the colour-space note ..."](https://github.com/nestrilabs/nestri/commit/7b05908e0a94739aa7ee5bd7fe5764a0a2b95071) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=60377562)</sub> **Context used:** - Knowledge Base — [Vulkan capture layer](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/capture-layer.md) <!-- /greptile_comment --> |
||
 
|
3389e6065f |
fix(nescapture): tag encoded streams full-range to match the samples written (#313)
## The bug `nescapture` sets the colour converter full-range unconditionally, but the video usability information carried pixelforge's **default limited-range flag**. A compliant decoder then expanded 16–235 out of samples that already covered 0–255 — darkening midtones and clipping both ends. pixelforge keeps two separate flags for this, one on the converter and one on the colour description, and its own documentation says they must agree. Only the first was being set. The two lines are about forty apart, each is correct on its own, and the comment above the second states the right intent while the call below it does the opposite: ```rust // GPU framebuffer captures are always full-range — use BT.709 full-range // so the decoder doesn't apply limited-range expansion. enc_cfg = enc_cfg.with_color_description(ColorDescription::bt709()); // ^ this constructor is limited-range ``` ## Evidence Measured on a Radeon RX 9060 XT, comparing the encoded result against the compositor's own readback of the same frames: | ground truth = `51` | before | after | |---|---|---| | flat background, decoded | **`38`** | `49–51` | | mean luma, capture path vs readback | **10.41 apart** | **0.55 apart** | | luma histogram intersection | **0.090** | **0.913** | | declared `color_range` | `tv` | `pc` | **The encoded luma is byte-identical before and after** — `Y = 51.00`, standard deviation `0.00` on both runs. Only the tag changed, which is what identifies this as a signalling bug rather than a conversion one, and why nothing short of a comparison against ground truth could see it: the stream was valid, the frame rate was right, the picture was recognisable, and every liveness check passed. The HDR arm (`bt2020_pq`) carried the same defect and is fixed the same way, but **has not been run** — no 10-bit verification here. ## `scripts/verify-chain.sh` Runs a Vulkan workload under `nescope` with the layer active and compares the encoded output against `nescope-shot`'s readback of the same frames. Two paths that share almost no code see the same content, so disagreement localises the fault; a single path cannot tell a correct frame from a plausible-looking wrong one. **Confirmed it fails when this change is reverted** — both the tag check and the brightness-agreement check fire. One note on its thresholds, since it is easy to get backwards: the not-blank check is a low absolute floor plus a comparison against the readback's own structure, rather than a fixed number. A fixed number was tried first and was wrong in the worst way — the **broken** build scored 20.49 on it and the **fixed** build 17.74, because the range defect stretched contrast and that reads as more detail. How much structure a correct frame carries depends on what the workload drew, so the only stable reference is ground truth measured in the same run. ## Not covered `vkcube` rather than a real workload; 720p, H.264, 8-bit; one card, one driver. XWayland, HUD detection and real swapchain formats are untouched. <!-- greptile_comment --> <h3>Greptile Summary</h3> The PR aligns encoded-stream color metadata with the full-range samples produced by nescapture and updates the CPU fallback to BT.709 full-range conversion. - Updates pixelforge and configures matching converter color space, range, and SDR reference white. - Corrects Vulkan color-space mapping and adds regression tests for SDR, HDR, and CPU fallback behavior. - Adds SDR capture-chain and HDR comparison verification scripts. <h3>Confidence Score: 5/5</h3> The PR appears safe to merge. No blocking failure remains. <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | apps/nescapture/src/encode.rs | Aligns GPU and CPU conversion output with encoded color metadata and adds focused regression coverage. | | apps/nescapture/scripts/verify-chain.sh | Adds an end-to-end SDR verifier using a static corner patch to avoid the previously reported temporal mismatch. | | apps/nescapture/scripts/verify-hdr.sh | Adds an HDR comparison harness for inspecting conversion behavior across builds. | | apps/nescapture/Cargo.toml | Advances pixelforge to the revision providing the required color-conversion configuration. | | Cargo.lock | Records the pixelforge update and resulting transitive dependency refresh. | <sub>Reviews (5): Last reviewed commit: ["test(nescapture): check the HDR conversi..."](https://github.com/nestrilabs/nestri/commit/2f9773c4b7ded7774b9396648de26423afe926fa) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=60147076)</sub> **Context used:** - Knowledge Base — [Vulkan capture layer](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/capture-layer.md) <!-- /greptile_comment --> --------- Co-authored-by: DatCaptainHorse <DatCaptainHorse@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
 WanjohiandClaude Opus 5
|
bf2c9632f5 |
fix(build): nescapture's release profile was being ignored
`apps/nescapture/Cargo.toml` carried `[profile.release]` with `opt-level = 3` and `lto = "thin"`. Cargo only reads `[profile.*]` from the workspace root and warns about a member that writes one, so `cargo build --release --workspace` — which is what build/Dockerfile runs — ignored it. The Vulkan capture layer that ends up in the guest image was built at the default release profile, and the warning saying so scrolled past on every build. `opt-level = 3` is already the release default, so `lto = "thin"` is the only part that was actually lost. It moves to the workspace root, where Cargo reads it, and now applies to all five members — the same rule as `[workspace.dependencies]` right above it: what must not differ between members is stated once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
DatCaptainHorse
|
d9cdf60039 | fix: wrong neshub version, formatting | ||
|
Wanjohi
|
6ea241c910 |
docs: the guest READMEs described code that is gone
All three came across with their standalone repos and none had been touched since the transport changed. nescapture claimed to packetize with Reed-Solomon FEC and stream RTP/UDP to a Moonlight client. It sends encoded frames to neshub over a Unix socket. packetizer.rs, control.rs and shard_batch.rs do not exist, and neither does ARCHITECTURE.md. Nine documented environment variables are not read by anything -- NESCAPTURE_RTP_HOST was listed as *required* -- and five that are read were undocumented, including the one that says where frames go. Someone following that quick start would have set a required variable that does nothing and got no output. Documented the three sockets, since nescapture binds one and connects to two and that was written down nowhere. neswire documented --rtp-addr and --channels against a gstreamer pipeline. It has --ipc-path, --channels, --packet-duration-ms and --bitrate-per-channel, and hub-stub exists precisely so it can be tested without a hub. Kept the reason hub-stub decodes rather than counts bytes: Opus codes silence at ~3 kbps, so a dead sink looks alive on a meter. nescope was mostly right. It called the capture layer "vkcapture", listed seven of fifteen modules and five of nine flags, and its TODO list was three items that neshub and nescapture now do. Added compositor mode, which is the shape a real session uses and was undocumented. All three licence lines were "TBD" or "See project repository". |
||
|
Wanjohi
|
40b80d4b14 |
refactor: the hub is neshub everywhere
Six comments across nescope, neswire and nescapture still named nestri-guest-hub, plus one still naming nestri-protocol. Deferred from the import commits so the rename would read as one change rather than six unexplained edits inside otherwise-verbatim trees. Comments only. Realigned an ASCII box in encode.rs that the shorter name knocked crooked. |
||
|
WanjohiandClaude Opus 5
|
6164e0c636 |
feat(nescapture): open the capture layer
A Vulkan implicit layer that captures frames from inside the workload's own process and encodes them on the GPU they were drawn on. Fourth and last of this batch, imported as a tree from `nestrilabs/nescapture` on the same terms. Filed under `apps/` rather than `crates/` despite building a cdylib. The rule here is what a thing *is*, not what it compiles to: this is a finished artefact that gets installed into an image beside its layer manifest, not a library another crate in this tree depends on. `crates/` is for the latter, and putting this there would make the distinction useless the first time someone looked. Wired to the workspace, `nesprotocol` by path. Its description named the transport component; that reads better as what it actually is — where the frames go — so it says that instead. Whole workspace builds and tests: 21 across four members. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |