mirror of
https://github.com/nestriness/nestri.git
synced 2026-09-19 17:25:19 +03:00
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
..."](7b05908e0a)
| [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 -->
This commit is contained in:
@@ -68,16 +68,27 @@ const VK_COLOR_SPACE_BT2020_LINEAR_EXT: u32 = colorspace(ash::vk::ColorSpaceKHR:
|
|||||||
const VK_COLOR_SPACE_DOLBYVISION_EXT: u32 = colorspace(ash::vk::ColorSpaceKHR::DOLBYVISION_EXT);
|
const VK_COLOR_SPACE_DOLBYVISION_EXT: u32 = colorspace(ash::vk::ColorSpaceKHR::DOLBYVISION_EXT);
|
||||||
const VK_COLOR_SPACE_HDR10_HLG_EXT: u32 = colorspace(ash::vk::ColorSpaceKHR::HDR10_HLG_EXT);
|
const VK_COLOR_SPACE_HDR10_HLG_EXT: u32 = colorspace(ash::vk::ColorSpaceKHR::HDR10_HLG_EXT);
|
||||||
|
|
||||||
|
/// The converter input format for a swapchain's `VkFormat`, or `None` when
|
||||||
|
/// there is no correct one.
|
||||||
|
///
|
||||||
|
/// `None` rather than a default on purpose. This used to fall back to BGRA,
|
||||||
|
/// which reads a packed 10-bit or FP16 buffer as eight-bit channels and
|
||||||
|
/// produces a stream that arrives at the right size and frame rate carrying
|
||||||
|
/// nonsense — the failure nobody notices. Refusing the frame is louder.
|
||||||
|
///
|
||||||
|
/// `A2R10G10B10_UNORM_PACK32` (58) is the notable absence, and it is reachable:
|
||||||
|
/// a WSI layer offers it as one of its HDR pairs and the compositor's dmabuf
|
||||||
|
/// list advertises it too. The converter has no red-first 10-bit input, so
|
||||||
|
/// there is nothing correct to map it to.
|
||||||
pub fn vk_format_to_input_format(vk_format: u32) -> Option<InputFormat> {
|
pub fn vk_format_to_input_format(vk_format: u32) -> Option<InputFormat> {
|
||||||
match vk_format {
|
match vk_format {
|
||||||
44..=50 => Some(InputFormat::BGRA),
|
44..=50 => Some(InputFormat::BGRA),
|
||||||
37..=43 => Some(InputFormat::RGBA),
|
37..=43 => Some(InputFormat::RGBA),
|
||||||
|
// VK_FORMAT_A2B10G10R10_UNORM_PACK32
|
||||||
64 => Some(InputFormat::ABGR2101010),
|
64 => Some(InputFormat::ABGR2101010),
|
||||||
|
// VK_FORMAT_R16G16B16A16_SFLOAT
|
||||||
97 => Some(InputFormat::RGBA16F),
|
97 => Some(InputFormat::RGBA16F),
|
||||||
other => {
|
_ => None,
|
||||||
log::warn!("unsupported VkFormat {other} for color conversion — defaulting to BGRA");
|
|
||||||
Some(InputFormat::BGRA)
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -114,9 +125,17 @@ pub fn sdr_reference_white_nits(color_space: ColorSpace) -> f32 {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn vk_format_to_bit_depth(vk_format: u32) -> EncodeBitDepth {
|
/// Bit depth implied by a converter input format.
|
||||||
match vk_format {
|
///
|
||||||
64 | 58 | 97 => EncodeBitDepth::Ten,
|
/// Taken from the input format rather than matched against the `VkFormat` a
|
||||||
|
/// second time. The two matches had drifted: `A2R10G10B10` counted as ten-bit
|
||||||
|
/// here while the input-format mapping above had no entry for it and fell back
|
||||||
|
/// to eight-bit BGRA, so the encoder was configured for ten-bit while the
|
||||||
|
/// converter read the buffer as eight. Deriving one from the other makes that
|
||||||
|
/// particular disagreement unrepresentable.
|
||||||
|
pub fn input_format_bit_depth(input_fmt: InputFormat) -> EncodeBitDepth {
|
||||||
|
match input_fmt {
|
||||||
|
InputFormat::ABGR2101010 | InputFormat::RGBA16F => EncodeBitDepth::Ten,
|
||||||
_ => EncodeBitDepth::Eight,
|
_ => EncodeBitDepth::Eight,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -583,6 +602,9 @@ fn encoder_thread(
|
|||||||
let ctx = cfg.ctx;
|
let ctx = cfg.ctx;
|
||||||
|
|
||||||
let mut encoder_state: Option<PerFrameEncoder> = None;
|
let mut encoder_state: Option<PerFrameEncoder> = None;
|
||||||
|
// Last VkFormat we refused, so the error is logged on change rather than
|
||||||
|
// once per frame.
|
||||||
|
let mut unsupported_format: Option<u32> = None;
|
||||||
|
|
||||||
let mut dmabuf_importer = match DmaBufImporter::new(ctx.clone()) {
|
let mut dmabuf_importer = match DmaBufImporter::new(ctx.clone()) {
|
||||||
Ok(i) => Some(i),
|
Ok(i) => Some(i),
|
||||||
@@ -648,16 +670,29 @@ fn encoder_thread(
|
|||||||
Err(mpsc::RecvTimeoutError::Disconnected) => break,
|
Err(mpsc::RecvTimeoutError::Disconnected) => break,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
let Some(input_fmt) = vk_format_to_input_format(raw.vk_format) else {
|
||||||
|
// Drop the frame rather than encode it wrongly. Logged once per
|
||||||
|
// format so a persistent mismatch says so without filling the log
|
||||||
|
// sixty times a second.
|
||||||
|
if unsupported_format.replace(raw.vk_format) != Some(raw.vk_format) {
|
||||||
|
log::error!(
|
||||||
|
"VkFormat {} has no colour-conversion input format — dropping frames. \
|
||||||
|
The stream will stall rather than carry wrong colour.",
|
||||||
|
raw.vk_format
|
||||||
|
);
|
||||||
|
}
|
||||||
|
continue;
|
||||||
|
};
|
||||||
|
|
||||||
let bit_depth = if let Some(ov) = cfg.wanted_depth_override {
|
let bit_depth = if let Some(ov) = cfg.wanted_depth_override {
|
||||||
ov
|
ov
|
||||||
} else {
|
} else {
|
||||||
match wanted_depth.as_deref() {
|
match wanted_depth.as_deref() {
|
||||||
Ok("10") => EncodeBitDepth::Ten,
|
Ok("10") => EncodeBitDepth::Ten,
|
||||||
Ok("8") => EncodeBitDepth::Eight,
|
Ok("8") => EncodeBitDepth::Eight,
|
||||||
_ => vk_format_to_bit_depth(raw.vk_format),
|
_ => input_format_bit_depth(input_fmt),
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
let input_fmt = vk_format_to_input_format(raw.vk_format).unwrap_or(InputFormat::BGRA);
|
|
||||||
let color_space = vk_colorspace_to_color_space(raw.vk_colorspace);
|
let color_space = vk_colorspace_to_color_space(raw.vk_colorspace);
|
||||||
let out_fmt = output_format(cfg.pixel_format, bit_depth);
|
let out_fmt = output_format(cfg.pixel_format, bit_depth);
|
||||||
|
|
||||||
@@ -919,6 +954,22 @@ fn cpu_encode_frame(
|
|||||||
) -> Result<EncodeFuture> {
|
) -> Result<EncodeFuture> {
|
||||||
use pixelforge::{EncodeBitDepth, InputImage};
|
use pixelforge::{EncodeBitDepth, InputImage};
|
||||||
|
|
||||||
|
// This path reads four bytes per pixel and encodes eight-bit, so it can
|
||||||
|
// only handle the eight-bit formats. A packed 10-bit buffer would be read
|
||||||
|
// as eight-bit channels and an FP16 one is twice the size with float
|
||||||
|
// samples; both produce a plausible-looking stream of nonsense. Refuse
|
||||||
|
// instead -- an error here is recoverable, a corrupt stream is not
|
||||||
|
// noticeable.
|
||||||
|
if !matches!(
|
||||||
|
vk_format_to_input_format(vk_format),
|
||||||
|
Some(InputFormat::BGRA | InputFormat::RGBA | InputFormat::BGRx | InputFormat::RGBx)
|
||||||
|
) {
|
||||||
|
anyhow::bail!(
|
||||||
|
"CPU encode fallback cannot read VkFormat {vk_format}; it handles \
|
||||||
|
eight-bit RGBA/BGRA only"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
let yuv = bgra_to_yuv420(pixels, width, height, vk_format);
|
let yuv = bgra_to_yuv420(pixels, width, height, vk_format);
|
||||||
|
|
||||||
let mut input_image = InputImage::new(
|
let mut input_image = InputImage::new(
|
||||||
@@ -1169,6 +1220,68 @@ mod tests {
|
|||||||
/// The colour space values were once written out by hand and two were wrong,
|
/// The colour space values were once written out by hand and two were wrong,
|
||||||
/// which routed every HDR swapchain into the SDR arm silently. Deriving them
|
/// which routed every HDR swapchain into the SDR arm silently. Deriving them
|
||||||
/// from `ash` is the fix; this pins the behaviour that depended on them.
|
/// from `ash` is the fix; this pins the behaviour that depended on them.
|
||||||
|
#[test]
|
||||||
|
fn a2r10g10b10_has_no_input_format() {
|
||||||
|
// 58 is offered by a WSI layer's HDR pairs and by the compositor's
|
||||||
|
// dmabuf list, and the converter has no red-first 10-bit input. It has
|
||||||
|
// to come back None: the old fallback read it as eight-bit BGRA.
|
||||||
|
assert_eq!(vk_format_to_input_format(58), None);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn unmapped_formats_are_refused_rather_than_defaulted() {
|
||||||
|
// A format nobody has taught the converter about must not quietly
|
||||||
|
// become BGRA. Picked from the depth/stencil range, which no swapchain
|
||||||
|
// uses, so this stays true as colour formats get added.
|
||||||
|
for vk_format in [124u32, 125, 126, 129] {
|
||||||
|
assert_eq!(
|
||||||
|
vk_format_to_input_format(vk_format),
|
||||||
|
None,
|
||||||
|
"VkFormat {vk_format} should be refused, not defaulted"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn bit_depth_agrees_with_the_input_format() {
|
||||||
|
// The two used to be separate matches on VkFormat and had drifted.
|
||||||
|
// Ten-bit in means ten-bit out, eight means eight, for every format
|
||||||
|
// the converter accepts.
|
||||||
|
let ten = [64u32, 97];
|
||||||
|
let eight = [37u32, 43, 44, 50];
|
||||||
|
for f in ten {
|
||||||
|
let fmt = vk_format_to_input_format(f).expect("mapped");
|
||||||
|
assert_eq!(
|
||||||
|
input_format_bit_depth(fmt),
|
||||||
|
EncodeBitDepth::Ten,
|
||||||
|
"VkFormat {f} is a ten-bit format"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
for f in eight {
|
||||||
|
let fmt = vk_format_to_input_format(f).expect("mapped");
|
||||||
|
assert_eq!(
|
||||||
|
input_format_bit_depth(fmt),
|
||||||
|
EncodeBitDepth::Eight,
|
||||||
|
"VkFormat {f} is an eight-bit format"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn hdr_formats_map_to_their_converter_inputs() {
|
||||||
|
// The two pairs a WSI layer injects that we can actually consume.
|
||||||
|
assert_eq!(
|
||||||
|
vk_format_to_input_format(64),
|
||||||
|
Some(InputFormat::ABGR2101010),
|
||||||
|
"A2B10G10R10_UNORM_PACK32 carries HDR10 PQ"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
vk_format_to_input_format(97),
|
||||||
|
Some(InputFormat::RGBA16F),
|
||||||
|
"R16G16B16A16_SFLOAT carries scRGB linear"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn hdr_colour_spaces_select_the_hdr_arm() {
|
fn hdr_colour_spaces_select_the_hdr_arm() {
|
||||||
for cs in [Cs::HDR10_ST2084_EXT, Cs::DOLBYVISION_EXT, Cs::HDR10_HLG_EXT] {
|
for cs in [Cs::HDR10_ST2084_EXT, Cs::DOLBYVISION_EXT, Cs::HDR10_HLG_EXT] {
|
||||||
|
|||||||
@@ -50,6 +50,31 @@ pub unsafe extern "system" fn vkCreateSwapchainKHR(
|
|||||||
*ds.swapchain.lock().unwrap() = Some(unsafe { *p_swapchain });
|
*ds.swapchain.lock().unwrap() = Some(unsafe { *p_swapchain });
|
||||||
*ds.swapchain_format.lock().unwrap() = ci.image_format;
|
*ds.swapchain_format.lock().unwrap() = ci.image_format;
|
||||||
*ds.swapchain_extent.lock().unwrap() = ci.image_extent;
|
*ds.swapchain_extent.lock().unwrap() = ci.image_extent;
|
||||||
|
// The colour space the layer below us was asked for, which is the one the
|
||||||
|
// game asked for in every configuration we ship.
|
||||||
|
//
|
||||||
|
// The exception is worth knowing about, because it is silent. A WSI layer
|
||||||
|
// of the gamescope kind rewrites `imageColorSpace` to SRGB_NONLINEAR before
|
||||||
|
// calling down -- deliberately, since it carries the real colour space to
|
||||||
|
// the compositor out of band instead. We sit below such a layer, so we
|
||||||
|
// would read the rewrite. Measured, all three lines from one run of a
|
||||||
|
// client requesting HDR10 PQ:
|
||||||
|
//
|
||||||
|
// [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, at
|
||||||
|
// full frame rate, decoding cleanly.
|
||||||
|
//
|
||||||
|
// This is not a bug to fix here. That route predates Wayland colour
|
||||||
|
// management and the compositor no longer enables it -- HDR comes from
|
||||||
|
// `wp_color_manager_v1` on a Wayland surface, where this value is correct
|
||||||
|
// and the same client yields Bt2020 and an smpte2084 stream. It is recorded
|
||||||
|
// because it is the reason the route stays off: enabling it would trade no
|
||||||
|
// HDR for wrong HDR. Anyone re-enabling it has to give this process a
|
||||||
|
// channel to the compositor first, since the true colour space exists only
|
||||||
|
// there.
|
||||||
ds.swapchain_colorspace
|
ds.swapchain_colorspace
|
||||||
.store(ci.image_color_space.as_raw() as u32, Ordering::Relaxed);
|
.store(ci.image_color_space.as_raw() as u32, Ordering::Relaxed);
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user