Repository navigation
perf(wgc): encode the helper frame directly and publish it with one copy - #530
Conversation
|
The direct-input path can encode pixels with the wrong timestamp: The scratch fallback also holds |
|
Thanks, both were real. Fixed in two commits: Pixels and timestamps (2e36f46): images that alias the shared texture now carry the helper's frame record. While holding the keyed mutex, the encoder device reads which frame the texture actually holds. A newer frame than the claimed one replaces the image's timestamps, and a frame that device already converted is skipped instead of encoded again. The cursor is kept per encoder device, so sessions sharing one capture still each encode every frame once. The AMF device cancels its reserved input surface for a skipped frame. Scratch fallback (ce80754): the delivery thread now waits for the shared keyed mutex without holding the D3D context lock. Once it has the mutex, it takes the context only if it's free; if the callback is mid-copy, it hands the texture back and waits for the context instead, so neither side holds one lock while waiting for the other. The frame stays in the delivering state throughout, so the callback can't publish a newer frame ahead of it. Builds with |
|
This landed in vibeshine canary first: Nonary/vibeshine#347 (merge bb02a93). Both of your points are fixed there, and review turned up one more case: a PyroWave client joining a capture that had already switched to direct input. That capture now goes back to the copy path. The Vibepollo port is built and going through the same review and stream tests. Once it passes I'll point this PR at canary and push it here. |
With WGC capture the helper already publishes every frame into a shared D3D11 texture, but the host copied it into a pooled image before the encoder converted it, costing a full-frame GPU copy per frame. Add wgc_direct_encoder_input (default on, latched per capture session). When it is active, each pool image aliases the helper's shared texture through its own duplicated NT handle, opened once per image on the encoder device. The capture thread only claims the newest frame id; the encoder takes the shared keyed mutex on its own device, reads the frame record under it, and converts the texture in place. Switching the option off restores the copy path. The encoder pairs the actual converted pixels with their frame id and timestamp: the helper may publish a newer frame between the claim and the lock, so the conversion reports the id and timestamp it really read through converted_frame_t, and the encode loops use those instead of the claimed ones. A per-encoder-device cursor skips a frame that was already converted, so the same pixels are never converted or encoded twice; the AMF device cancels its reserved input surface when it skips. A restarted helper bumps the frame record generation, which restarts the ids. Sources that need a stable image of their own keep the copy path: constant frame rate capture, which re-sends its cached image on a timeout that the cursor would drop as a repeat, and PyroWave, which pins and reads the capture image's own texture and keyed mutex. Constant frame rate is latched with the input mode, so changing the capture option cannot leave a direct pool with nothing to re-send. PyroWave runs on the shared capture, so any live PyroWave consumer is part of the shared capture's configuration: a PyroWave client joining a capture that already latched direct mode restarts it onto the copy path through the existing demand check (a PyroWave client leaving does not restart it). Software encoding and DXGI capture are unaffected. A direct image whose frame record cannot be read is skipped like a repeat, without touching the frame cursor, and logged at most once a second. That only happens if the helper died mid-publish, and the IPC session's broken-pipe handling restarts capture in that case. The timestamp choice and the skip of an already-converted frame are one policy shared by both encode loops, and only frames actually converted are counted as such. Vibepollo stamps frames before converting them, so both encode loops now replace capture_timestamp and host_processing_timestamp with the converted frame's stamps before the pacing stamp is derived from them (the async loop's frame_variation smoothing then starts from the converted frame). capture_pacing_timestamp stays the claim time, since it anchors the capture loop's pacing rather than the frame. The demand check keeps Vibepollo's pacing context argument. A repeated direct frame is skipped, which must not delay a requested keyframe until the next capture. The async loop carries the request into the next iteration (retry_idr), which encodes the last input as the IDR without waiting; the sync loop encodes the last input as the IDR in the same pass, unstamped like the no-new-frame path. The decision is video::policy::retry_idr_after_repeat. This is the Vibepollo port of Nonary/vibeshine#347 and the follow-up fix. Signed-off-by: Ramazan Kara <github@r-kara.de>
…ng up The helper copied every WGC frame into a scratch texture and then, on its delivery thread, from the scratch texture into the shared texture the host reads, so each frame cost two GPU copies even when nothing was queued. When no older frame is queued or mid-delivery, the capture callback now copies the WGC frame straight into the shared texture, as long as the helper's D3D context is free and the keyed mutex can be acquired without waiting. Otherwise the frame takes the scratch handoff as before, so a newer frame is never published ahead of an older one. The helper counts direct publications in its periodic diagnostics. The scratch fallback no longer holds the D3D context while it waits for the host. The delivery thread used to hold the context lock for up to 200 ms waiting on the shared keyed mutex, and the capture callback needs that lock for its scratch copy, so encoder backpressure could stall FrameArrived. It now waits for the keyed mutex alone and then takes the context only if it is free; if the callback is mid-copy, it hands the texture back and waits for the context instead. Neither side holds one lock while waiting on the other, and the frame stays in the delivering state throughout, which keeps the callback from publishing a newer frame ahead of it. This is the Vibepollo port of Nonary/vibeshine#347. Signed-off-by: Ramazan Kara <github@r-kara.de>
ce80754 to
65c032f
Compare
Propagate backend RFI fallback into the async loop's pending IDR state, reusing Nonary#348's retry handling for repeated direct inputs. Preserve successful invalidation and process every queued feedback range. Add an extracted-production CPU regression covering 36 NVENC/AMF recovery scenarios, with a failing unchanged-canary negative control. Prepared with OpenAI Codex on behalf of the repository owner. Full Windows build and live GPU streaming validation remain unrun. Vibepollo port of Nonary/vibeshine#349. (cherry picked from commit 73a00e8084194894a9da8ef999ebc7f95d828935) Signed-off-by: Ramazan Kara <github@r-kara.de>
6a1e9ca to
3bfb3f8
Compare
Dot will migrate commits from vibeshine to Vibepollo automatically on schedule. And anything added to Vibepollo will also be cherry-picked back to Vibeshine as well if you forget to sync the two. |
Nonary
left a comment
Nonary
left a comment
There was a problem hiding this comment.
Independent round 1 — changes_requested, one conditional P2 finding (R1-530-01, inline). No other actionable defect found.
Reviewed head 3bfb3f82ac1aeac49b73733bd69a5e251ab3bdc8 against canary 12292b2e80617211feb1c01de6d4221f876434e5; both re-fetched unchanged immediately before posting. Session: 01a166de-c3ee-747b-bac5-3c92118e30f1.
Used the installed UltraCode workflow with supported gpt-6-luna / high: 12 independent helpers, six bounded claim assessments each, totaling 72 assessments (36 confirmer + 36 denier), at most six concurrent. Each of nine claims received four independent confirmer/denier pairs. All four pairs independently confirmed the conditional PyroWave join failure after tracing the consumer’s actual failure path.
Read the complete 27-file PR diff and surrounding scheduling/IPC, per-session device construction, AMF reserve/cancel/cache, reinit lifetime, helper publication locks, timestamp replacement/smoothing, and explicit/RFI IDR retries. Rejected the other candidate concerns after source tracing; the two canary-only file changes do not overlap these paths.
Current-head CI run 39894072212 is successful, including Arch and Windows build/package jobs. This review ran no local tests or live Pollo stream. The previously reported CPU fixtures are prior evidence, not rerun here; earlier Shine AMD measurements do not validate this Pollo head or the experimental join race.
Submitted as COMMENT only. Existing non-draft status is unchanged; no edits, pushes, merges, releases, or readiness changes.
| const auto pacing = capture_pacing_for_display(); | ||
| if (const auto demand = capture_config_for_consumers(capture_ctxs, pacing); | ||
| demand && !same_capture_pacing(producer_config, *demand, pacing)) { | ||
| if (const auto demand = capture_config_for_consumers(capture_ctxs, pacing); demand && !same_capture_demand(producer_config, *demand, pacing)) { |
There was a problem hiding this comment.
WITHDRAWN: R1-530-01 is a false positive. WGC inherits is_event_driven_capture() == false, so pending PyroWave joins are admitted after the current frame is broadcast and cause immediate reinit. The prerequisite for the proposed failure is unreachable. See the evidence-backed correction. Original text is preserved below for the audit trail.
[P2] R1-530-01 — Restart before publishing a direct image to a joining PyroWave consumer
When an event-driven WGC capture is already in direct mode, the callback admits a pending PyroWave consumer and raises the current image to it at lines 3364–3388, before this demand check requests reinitialization. A PyroWave worker already waiting in images->pop() can consume that image; its loop checks reinit only before the pop (video.cpp:6478–6516).
The direct alias clears capture_mutex (display_wgc.cpp:476–484). With SUNSHINE_EXPERIMENTAL_PYROWAVE_D3D12=ON, PyroWave passes that null mutex to stage_source, which rejects it (pyrowave_d3d12_conversion.cpp:337–346, 744–746). That error propagates through the core and encoder to encode_run_pyrowave, which returns false and permanently ends the joining stream (video.cpp:6513–6516, 6632–6634), so the subsequent copy-mode restart cannot recover it.
This is conditional on the experimental build option, which defaults OFF; I am not claiming this fatal path in the default build. Check aggregate demand and enter reinit before distributing this old direct image to the incompatible joining consumer. A join/publication interleaving regression would cover the gap in the current demand-policy tests.
There was a problem hiding this comment.
Correction: R1-530-01 is withdrawn as a false positive. I incorrectly assumed Windows WGC used the event-driven admission branch.
At this exact head, display_t::is_event_driven_capture() returns false. The complete Windows inheritance chain is display_wgc_ipc_vram_t → display_vram_t → display_base_t → display_t, with no override (declarations); the factory constructs that concrete WGC class.
Consequently, the callback broadcasts the current image to existing contexts at line 3387, then admits pending joins at 3397–3400, then detects PyroWave demand and requests reinit at 3404–3409. The joining consumer receives no old direct-alias image. The experimental D3D12 option changes encoder conversion, not this display virtual method.
I independently traced the hierarchy/factory and replayed the extracted callback’s control flow in memory: actual false → zero direct images to the joiner and reinit; forced true mutation → one incompatible image. This was a translated control-flow replay, not a compiled C++ or GPU test. All four original confirmer/denier pairs also rechecked the missing reachability premise and withdrew their support.
The downstream null-mutex rejection exists in isolation, but this proposed join cannot reach it. No correction to the PR code is required for R1-530-01. This explicitly supersedes the original finding; amended round-1 verdict: clear, with no remaining actionable findings.
Nonary
left a comment
Nonary
left a comment
There was a problem hiding this comment.
Amended independent round 1 — clear. No remaining actionable findings. This supersedes my earlier round-1 disposition.
R1-530-01 is explicitly withdrawn with source evidence. I incorrectly assumed WGC used the event-driven callback branch. Its concrete class inherits is_event_driven_capture() == false; therefore the callback broadcasts to existing contexts before admitting pending PyroWave joins, then requests reinit for the changed demand. The joining consumer receives no old direct-alias frame. Experimental D3D12 conversion does not alter this classification.
Reviewed head 3bfb3f82ac1aeac49b73733bd69a5e251ab3bdc8 against canary 12292b2e80617211feb1c01de6d4221f876434e5; re-fetched unchanged before this amendment. Session 01a166de-c3ee-747b-bac5-3c92118e30f1.
Original installed UltraCode allocation: 72 bounded claim assessments across 12 gpt-6-luna/high helpers, at most six concurrent, four confirmer/denier pairs per claim. This reassessment used eight follow-ups with the original four C6 pairs; all independently rejected the unreachable premise. I also traced the full WGC hierarchy/factory and ran an in-memory source-derived callback control-flow replay (actual false: zero direct frames to the joiner; forced true mutation: one).
Current-head CI remains successful. Limits: no local compiled C++ fixture, GPU test, or live Pollo stream; prior fixture results and earlier Shine measurements remain prior evidence. Posted as COMMENT; the existing non-draft PR status and source code are unchanged.
Nonary
left a comment
Nonary
left a comment
There was a problem hiding this comment.
Independent final review — clear. No unresolved actionable code findings.
Session 01a166f6-036c-73f9-aa76-fd30894fb0f3, distinct from round-one session 01a166de-c3ee-747b-bac5-3c92118e30f1. Reviewed head 3bfb3f82ac1aeac49b73733bd69a5e251ab3bdc8 against current canary 12292b2e80617211feb1c01de6d4221f876434e5; both re-fetched unchanged immediately before submission. PR merge base is e686f72e336494078131784db4f43670c063f49f. The two canary-only changed files do not overlap these capture/encoder paths.
Used the actual installed UltraCode skill with native gpt-6-luna / high helpers: 72 completed bounded claim assessments, 36 confirmer + 36 denier, across 12 independent helpers, six assessments each and at most six concurrent. Each of the nine claims below received four independent confirmer/denier pairs. Follow-ups clarified those existing assessments; no additional reviewers were launched. I independently read the complete 27-file current diff, relevant surrounding code, upstream #347/#348/#349 changes, and adjudicated the candidate concerns. The exact-head tree is non-truncated; its only AGENTS.md is under docs/linux, outside this change's scope. Contributing instructions were also read.
Dispositions:
- C1, metadata/pixels — clear. Helper publication and encoder record reads use the same keyed mutex. A lost-wakeup candidate is rejected: wait_for_frame checks the metadata predicate before and after waiting, so draining a raced event cannot conceal a newer frame.
- C2, duplicate cursor — clear. Cursor state is per encoder device, includes record generation, and does not advance on unreadable metadata. A subsequent conversion error exits the affected session; no continuing-session frame-loss path was established.
- C3, AMF input/lifetime — clear. A repeat returns before drawing and cancels only its prepared reservation. Retained input remains available for cached IDRs; in-flight input is protected by slot ownership/generations. IDR state clears only after accepted input (AMF reservation/cancel).
- C4, helper locking/publication — clear. Delivery hands back the keyed mutex before waiting for context; pending/delivering frames prevent overtaking. The reserved-scratch candidate required overlapping/reentrant callbacks from one frame pool. No such path was established: the handler processes synchronously on the documented internal worker thread. This is a source/API assessment, not runtime concurrency proof.
- C5, demand/restart and R1-530-01 — rejected/withdrawn as a false positive. Independently verified the default false virtual method, WGC hierarchy/factory with no override, and callback ordering: broadcast existing consumers, admit pending joins, then demand-triggered reinit. The PyroWave joiner receives no old direct alias; experimental D3D12 does not alter classification. This supports the round-one correction and amended clearance.
- C6, both encode paths/timing — clear. Converted frame/host stamps replace claims before Pollo's asynchronous pacing smoothing; synchronous encoding also uses converted stamps. Capture pacing retains claim time.
- C7, explicit/RFI recovery — clear. Both loops preserve explicit IDRs through repeats; asynchronous RFI failure joins that retry state. All encoder entries advertising RFI also select parallel/asynchronous encoding, so the synchronous loop's lack of an RFI event consumer omits no advertised RFI path.
- C8, compatibility/resources — clear. Disabled mode, wgcc, software and PyroWave retain their applicable copy paths. Per-image duplicated handles, texture references and independently mapped frame records account for queued-image/reinit lifetime.
- C9, integration — clear. Config/parser/defaults/schema/locales/docs and focused test registration are coherent. Shine #347/#348/#349 behavior and Pollo timestamp/pacing adaptations are represented.
Freshly verified CI run 39894072212: successful for this head and PR merge ref 2f3194edd3002e36a50b4f9006e38105d0d742bf, including Arch, Windows build/package and native-AMF lifecycle steps. The general Windows test-suite step was skipped. The coordinator's 36 RFI, 64 component, 26 demand/175 pacing and AMF fixture results remain prior evidence; I did not rerun them.
Limits: no build, compiled fixture, GPU/device-loss test, or live Pollo stream run in this review. Earlier Shine measurements do not establish live behavior of this Pollo head. Local sibling-checkout Git reads were denied and stopped. Automatic approval review rejected local workflow-note creation because this assignment prohibits edits; no file creation was retried, and source review continued through supported GitHub reads.
Submitted as exact-head COMMENT only under the bugfix-review workflow. Existing non-draft state is unchanged. No source edits, pushes, merge, release or readiness/status change; final cloud check and merge remain with the parent coordinator.
|
Merged into canary after the two independent reviews and final integration checks. Thanks for the rework and for preserving the recovery-fix attribution. The late-PyroWave-join finding was withdrawn after checking the actual WGC callback ordering; no change was needed for that hypothetical path. The revised head passed the CPU recovery/capture tests and Windows/Arch CI. Live Pollo GPU-stream validation remains separate, and this merge is not yet a published nightly. |
You mentioned Vibepollo #348 (issue, not PR) instead of Vibeshine #348. |
co-written by Claude Opus 5.5
Vibepollo port of Nonary/vibeshine#347 and #348 (merged), plus your fix from Nonary/vibeshine#349, cherry-picked with your authorship. Rebased onto canary; this replaces the earlier commits on this PR and targets canary instead of vibe-test. It waits for your review before merging.
The Arch Linux check fails on a canary bug outside this PR:
packaging/linux/vibepollo-session-broker.c:1565still callsexec_user_servicewith 5 arguments after decf791 added two. Canary's own run skipped the Arch job.What it does
wgc_direct_encoder_input, default on, fixed for each capture session). Each pool image aliases the helper's shared texture through its own duplicated NT handle. The encoder takes the keyed mutex on its own device, so the host's snapshot copy and one keyed-mutex hand-off per frame are gone. Turning the option off restores the copy path.direct_published.Your review points
capture_timestampandhost_processing_timestampbefore the pacing smoothing;capture_pacing_timestampstays the claim time._d3d_context_mutex, and publication order holds.Also covered
wgccand PyroWave keep the copy path. A PyroWave client joining a running direct capture restarts it onto the copy path.Tested
-DBUILD_WERROR=ON, and each commit compiled on its own. It passed WGC 29/29, video capture source 14/14, video policy 20/20 and web unit 98/98, and had an Opus review against the merged vibeshine change (no blockers).test_component_video_policyandregression_video_rfi_retry.