Repository navigation
feat(video,platform): declare explicit capture pixel formats - #5568
luanweslley77 wants to merge 15 commits into
Conversation
9e6fe0f to
4f25ed1
Compare
Build artifactsNo artifacts matched the configured patterns in workflow run 33539186965. |
3c8ab99 to
29e068c
Compare
|
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
Reviewed head 29e068ce9f98dea70d2ed2ce6226232598bdf4cb against master at 9f7daa5e5a804b8eb09586d39b00e3e9bc176200. The explicit format mappings and software conversions look sound, but the newly advertised NV12 format exposes two capture-path defects, and the build-deps pin must point to a tagged release.
Validation: git diff --check and clang-format --dry-run --Werror passed for the changed native files. Three relevant PR tests passed in a focused C++/gtest harness using the unchanged PR conversion methods and the FFmpeg binaries from v2026.1004.34232. An additional test comparing nonzero colors across all five packed 10-bit formats with padded input strides also passed. A synthetic test of the unchanged PipeWire on_process callback passed for contiguous NV12 but failed for separate Y/UV planes: only 76,800 of the required 115,200 bytes were copied for a 320x240 frame. These are focused tests, not a complete Sunshine build or a Linux hardware streaming run.
Merge status: GitHub confirms conflicts with the target branch. Please rebase and resolve the conflicts before merging.
Hosted verification is limited: the native workflow runs report failure with no jobs or retrievable logs, the screenshot publisher fails because no CI screenshots were produced, and the failed GH-Pages documentation build's logs were inaccessible. I am not attributing those workflow statuses to these code changes. Sonar analysis and the main documentation status passed.
29e068c to
fe9cab1
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fe9cab1 to
05659b2
Compare
094abf0 to
05e70c9
Compare
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
Re-reviewed 05e70c96c46efacdbc0f903202a41bbb21a6739f, including the full PR diff, changes since the earlier review, and the live master tip 1582fffb47eaf80c91dfa282d7346fb7cd6ceb09.
The three earlier findings are addressed: NV12 negotiation is filtered by converter capability, separate Y/UV planes are repacked, and build-deps now uses the published v2026.1004.34232 commit. The explicit format support still adds behavior absent from current master. Two new P2 findings remain: some rejected NV12 frames bypass the new recovery accounting, and the color-pattern regression uses uninitialized colorimetry.
Validation: whitespace and clang-format checks passed. A focused C++/gtest harness using unchanged PR conversion methods, headers, and the tagged Darwin ARM64 FFmpeg binaries ran 43 relevant PR tests: 41 passed, the new color-pattern comparison failed, and the SPA-dependent format-map test skipped. Initializing and applying the test colorspace as suggested produced 42 passes and the same one skip. Separate tests of the extracted NV12 copy methods reproduced both missing drop-accounting cases; adding the suggested calls made all three callback checks pass. These are focused tests, not a full Sunshine build or Linux capture/GL/CUDA execution.
Merge status: GitHub confirms conflicts. Please rebase onto current master and resolve them before merging, preserving the recent PipeWire teardown and capture-device fixes.
Hosted verification: native workflows await maintainer approval. Sonar's failed gate reports three mutable-global declarations in src/video.h that are unchanged on the target branch; I am not treating them as introduced findings. The main documentation check passed, while the GH-Pages documentation check failed and its logs were inaccessible.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
| if (!luma_stride) { | ||
| return false; | ||
| } | ||
| const auto luma_region = resolve_memptr_region(static_cast<const std::uint8_t *>(luma_data.data), luma_data.maxsize, luma_data.chunk->offset, luma_data.chunk->size, luma_data.chunk->flags); | ||
| if (!luma_region) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[P2] Account for NV12 luma failures before returning. A negative/missing luma stride or a corrupted/out-of-bounds luma chunk returns here without calling report_drop(). on_process() then just queues the buffer and returns, so a producer that keeps sending these unusable NV12 frames never advances drop_burst or sets stream_error. Capture keeps timing out and showing a stale/black frame instead of taking the new two-second reinit/abort path. The packed-frame path already accounts for equivalent failures. Please account for both early exits and add callback-level coverage; the existing helper tests do not exercise this wiring.
The following change passed isolated tests of these copy methods for invalid luma stride, corrupted luma, and missing chroma; Linux runtime capture was not run.
| if (!luma_stride) { | |
| return false; | |
| } | |
| const auto luma_region = resolve_memptr_region(static_cast<const std::uint8_t *>(luma_data.data), luma_data.maxsize, luma_data.chunk->offset, luma_data.chunk->size, luma_data.chunk->flags); | |
| if (!luma_region) { | |
| return false; | |
| } | |
| if (!luma_stride) { | |
| report_drop(d); | |
| return false; | |
| } | |
| const auto luma_region = resolve_memptr_region(static_cast<const std::uint8_t *>(luma_data.data), luma_data.maxsize, luma_data.chunk->offset, luma_data.chunk->size, luma_data.chunk->flags); | |
| if (!luma_region) { | |
| report_drop(d); | |
| return false; | |
| } |
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
There was a problem hiding this comment.
Fixed, thanks. All NV12 rejections go through one helper now, so the luma failures count as drops too and the abandonment path actually runs. I added tests that call the copy helpers directly. They fail on the previous code because stream_error never gets set. While testing this I also hit a deadlock (fill_img holds frame_mutex and report_drop re-locked it), and fixed that as well.
| frame.release(); | ||
| ASSERT_NE(device.frame, nullptr); | ||
| ASSERT_NE(device.frame->data[0], nullptr); | ||
| ASSERT_NE(device.frame->data[2], nullptr); |
There was a problem hiding this comment.
[P2] Initialize and apply the colorspace in the conversion test setup. These tests default-initialize the device, leaving encode_device_t::colorspace indeterminate. Switching the input format calls apply_colorspace(), which reads that state; the initial BGR0 reference also needs the same matrix/range as later formats. With the published Darwin ARM64 FFmpeg, SoftwareEncoderConversion.ComparesEquivalentColorPatterns fails its comparisons in the unchanged test harness. Initializing the colorimetry and applying it before the first conversion makes all 42 executable focused tests pass (one SPA-dependent test remains skipped), so the test does not depend on stack contents or compare different color ranges.
This suggestion was tested in the focused harness, not a full platform build.
| ASSERT_NE(device.frame->data[2], nullptr); | |
| ASSERT_NE(device.frame->data[2], nullptr); | |
| device.colorspace = {video::colorspace_e::rec601, false, static_cast<unsigned>(av_pix_fmt_desc_get(format)->comp[0].depth)}; | |
| device.apply_colorspace(); |
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
There was a problem hiding this comment.
Fixed as suggested. setup_conversion sets rec601/limited/depth and applies it before the first conversion, so the reference and the later conversions use the same matrix and range. Skipping the apply reproduces the mismatch, so the test does not depend on the environment anymore.
d07f46a to
d21bb7c
Compare
Add the shared pix_fmt_e header, the img_t pixel format field, and the helpers that describe or validate a declared capture layout, keeping the historical 4-byte BGR0 fallback for undeclared captures.
The CUDA translation unit derives its captured image from the host layout instead of duplicating the field declarations.
Desktop capture formats are translated to the declared capture format, and unsupported HDR formats stay undeclared so conversion fails closed.
The DXGI backends stamp the mapped pixel format on each captured image instead of relying on the converter to infer it.
The macOS backends map the negotiated Core Video format to the declared capture format instead of exposing an undeclared image.
KMS output and cursor capture declare the format described by the DRM fourcc, and unmapped 10-bit or foreign layouts remain undeclared so conversion fails closed.
wlroots images carry the format produced by the texture download instead of leaving the converter to infer it from the pixel pitch.
6d72620 to
01296ff
Compare
Map the SPA formats explicitly, advertise only formats the selected encoder path can consume, validate and pack the memory buffers, and abandon a stream that keeps producing invalid frames.
Map declared capture formats to FFmpeg, keep the historical fallback only for undeclared captures, support P010, copy aspect-ratio padding on the chroma grid, and fail closed for inconsistent layouts.
Classify the actual visual layout, reject unsupported visuals at init, take ownership of the XImage, recover from transient grabs, and declare XFixes cursors as BGRA.
Upload padded frames with GL_UNPACK_ROW_LENGTH, restore the unpack state, and reject capture formats, pixel pitches or row strides that the RAM converters cannot represent.
Tests can start an isolated X server, probe a live socket instead of stale lock files, and restore the previous DISPLAY environment.
X11 capture has integration coverage for the declared BGR0 layout, content changes, and rejection of an unsupported 16-bit visual.
The EGL upload path runs against a real GL context, including padded rows, unpack-state restoration, and fail-closed layouts.
A capture is pushed through the software converter, encoded as H.264, decoded, and checked against the captured pixels.
01296ff to
1c02f40
Compare
|
This branch has not been deployed
3 participants
Description
Capture backends now declare the negotiated pixel format on
platf::img_t(pix_fmt_e pixel_format), and the RAM-path consumers (software encoder via libswscale, VAAPI/CUDA RAM) respect it instead of guessing frompixel_pitch. This continues the direction of #5495, which made the pitch information explicit.With this, 10-bit HDR captures (PipeWire
ARGB_2101010/ABGR_2101010, KMSBGRA1010102/RGBA1010102) reach the software encoder and the VAAPI/CUDA RAM converters correctly instead of being misread as 8-bit BGR0 with wrong colors. It also unblocks software HEVC Main10, which the encoder already advertises (AV_PIX_FMT_YUV420P10) but could never receive. Declared formats that a converter cannot represent now fail loudly instead of producing corrupt frames. The 10-bit shift formats are mapped unconditionally and therefore require the FFmpeg patch from build-deps #762, which the last commit pins inthird-party/build-deps.The last commit bumps
third-party/build-depsto include the FFmpeg patch that registersBGRA1010102/RGBA1010102(LizardByte/build-deps PR: LizardByte/build-deps#762). Without it, the branch still works but falls back to the previous behavior for those two formats.Screenshot
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage
See our AI usage policy.