Repository navigation
Literally rewriting basically everything because this was a bad idea on an architectural level - #4
spacedouut wants to merge 6 commits into
Conversation
…synthetic fallback Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 18.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 11 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Title check |  |
The title indicates a broad rewrite, but it uses informal and vague wording and does not identify the main technical changes, such as the pipeline registry, native capture pipelines, or removal of syn… | Replace the title with a concise, specific summary of the primary architectural change, such as "Rewrite capture architecture with native pipeline registry and KMS capture". |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 18.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 11 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Title check
-
Explanation
The title indicates a broad rewrite, but it uses informal and vague wording and does not identify the main technical changes, such as the pipeline registry, native capture pipelines, or removal of synthetic fallback.
Resolution
Replace the title with a concise, specific summary of the primary architectural change, such as "Rewrite capture architecture with native pipeline registry and KMS capture".
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- 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.
Comment @coderabbitai help to get the list of available commands.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
👀 2 findings need your review
Devin fixed 8 of 10 findings on f1e9c89. Click a finding below to jump to its comment.
For your review (2)
Fixed by Devin (8)
- Windows desktop handles leak across streams
- First display blocks all Windows capture
- Page flips can end KMS streams
- Automatic capture chooses unusable NVIDIA pipeline
- Linear capture emits transparent desktop pixels
- Default macOS frame rate reaches ScreenCaptureKit as zero
- Suballocated linear scanouts fail to map
- Oversized frame rate terminates capture daemon
View all findings in Devin Review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…GB, aligned mmap, fps clamp, first-frame timeout, rotated mutter monitors Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…size Closing the sckit stream from another thread while grab sat in NextFrame freed the handle under it and the frame wait never woke, so stop-stream hung forever. grab now waits in 100 ms slices and checks a stop flag; the stream is only closed on the stream thread after grab returns. Display ids are indexes like the Linux pipelines (main display first), so display 0 works. Width/height come from the CoreGraphics display mode in pixels and the stream asks for that size, since ScreenCaptureKit's display size is in points and would downscale on Retina. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| if *listenTCP != "" { | ||
| tl, err := net.Listen("tcp", *listenTCP) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| log.Printf("tcp: %s", *listen) | ||
| } | ||
|
|
||
| var stream pipelines.FrameStream | ||
| var streamMu sync.Mutex | ||
|
|
||
| serveConn := func(conn net.Conn) { | ||
| defer conn.Close() | ||
| dec := json.NewDecoder(conn) | ||
| enc := json.NewEncoder(conn) | ||
| for { | ||
| var req struct { | ||
| Type string `json:"type"` | ||
| DisplayID uint32 `json:"display_id,omitempty"` | ||
| FPS int `json:"fps,omitempty"` | ||
| } | ||
| if err := dec.Decode(&req); err != nil { | ||
| return | ||
| } | ||
| switch req.Type { | ||
| case "list-displays": | ||
| displays, err := pipeline.ListDisplays(ctx) | ||
| if err != nil { | ||
| enc.Encode(map[string]string{"error": err.Error()}) | ||
| continue | ||
| } | ||
| enc.Encode(map[string]any{"type": "displays", "displays": displays}) | ||
|
|
||
| case "start-stream": | ||
| if req.FPS == 0 { | ||
| req.FPS = 60 | ||
| } | ||
| streamMu.Lock() | ||
| if stream != nil { | ||
| stream.Close() | ||
| } | ||
| s, err := pipeline.StartStream(ctx, req.DisplayID, req.FPS) | ||
| if err != nil { | ||
| streamMu.Unlock() | ||
| enc.Encode(map[string]string{"error": err.Error()}) | ||
| continue | ||
| } | ||
| stream = s | ||
| os.Remove("/tmp/captured-media.socket") | ||
| ml, err := net.Listen("unix", "/tmp/captured-media.socket") | ||
| if err != nil { | ||
| stream.Close() | ||
| stream = nil | ||
| streamMu.Unlock() | ||
| enc.Encode(map[string]string{"error": err.Error()}) | ||
| continue | ||
| } | ||
| go func() { | ||
| <-ctx.Done() | ||
| ml.Close() | ||
| }() | ||
| go func() { | ||
| mc, err := ml.Accept() | ||
| ml.Close() | ||
| if err != nil { | ||
| return | ||
| } | ||
| defer mc.Close() | ||
| for f := range s.Frames() { | ||
| buf := make([]byte, 8+len(f.Data)) | ||
| binary.BigEndian.PutUint32(buf[0:4], uint32(f.Width)) | ||
| binary.BigEndian.PutUint32(buf[4:8], uint32(f.Height)) | ||
| copy(buf[8:], f.Data) | ||
| if _, err := mc.Write(buf); err != nil { | ||
| return | ||
| } | ||
| } | ||
| }() | ||
| streamMu.Unlock() | ||
| enc.Encode(map[string]any{ | ||
| "type": "stream-started", | ||
| "socket": "/tmp/captured-media.socket", | ||
| "format": "bgra", | ||
| }) | ||
|
|
||
| case "stop-stream": | ||
| streamMu.Lock() | ||
| if stream != nil { | ||
| stream.Close() | ||
| stream = nil | ||
| } | ||
| // os.Remove("/tmp/captured-media.socket") - do NOT fucking do this | ||
| streamMu.Unlock() | ||
| enc.Encode(map[string]string{"type": "stream-stopped"}) | ||
|
|
||
| case "info": | ||
| formats := pipeline.SupportedFormats() | ||
| s := make([]string, len(formats)) | ||
| for i, f := range formats { | ||
| s[i] = string(f) | ||
| } | ||
| enc.Encode(map[string]any{ | ||
| "type": "info", | ||
| "formats": s, | ||
| "version": "0.1.0", | ||
| }) | ||
|
|
||
| default: | ||
| enc.Encode(map[string]string{"error": fmt.Sprintf("unknown type: %s", req.Type)}) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| accept := func(l net.Listener) { | ||
| for { | ||
| conn, err := l.Accept() | ||
| if err != nil { | ||
| return | ||
| } | ||
| go serveConn(conn) | ||
| } | ||
| } | ||
|
|
||
| go accept(ul) | ||
| if tl != nil { | ||
| go accept(tl) | ||
| log.Printf("control: tcp %s", tl.Addr()) | ||
| go s.accept(tl) |
There was a problem hiding this comment.
🟥 Remote clients can control desktop capture
With --listen :9090, unauthenticated clients can send start-stream and stop-stream. They can replace or interrupt the active capture without accessing the local media socket.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed: --listen gives control to anyone who can reach the port, with no auth. It's opt-in (off by default). Whether to bind it to loopback only or drop it is a design call I've passed to @spacedouut, so I'm leaving it unchanged until then.
| os.Remove(*mediaPath) | ||
| ml, err := net.Listen("unix", *mediaPath) |
There was a problem hiding this comment.
🟥 Other local users can read desktop frames
A local user can connect to the media socket before the agent and receive raw frames. net.Listen creates it without restrictive permissions or peer checks.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed: both sockets are created with default permissions, so other local users can connect to them. The proposed fix is chmod 0600 after net.Listen. I've asked @spacedouut whether the agent ever runs as a different user, which would break that, before changing it.
|
Ran
capbench's CPU line reads What was broken in
Frames are the real screen, the right way up, with the cursor. Colour swatches read back exactly: (255,0,0), (0,255,0), (0,0,255), (255,255,0), (0,0,0), white 255. The static frame vs On a static screen sckit re-delivers the last buffer at about 57/s, so a static stream keeps running at full rate (227/228 identical in the static run) instead of going quiet. The daemon grabs about 57 fps (grab_avg ~17.6 ms), but capbench only receives 35-45. The Screen Recording: nothing to grant. This VM already has it granted to its agent process, and binaries started from its shell inherit it. Static: Moving: |
Enumerates every attached output with friendly name, desktop position and DisplayConfig refresh rate. AcquireNextFrame drives the stream (paced=false); on WAIT_TIMEOUT the last staged frame is re-sent, and ACCESS_LOST recreates the duplication. Frames are copied to a staging texture, mapped and packed to tight BGRA honoring RowPitch and output rotation. Sockets default to %TEMP% on Windows. capbench takes the media socket from the start-stream reply and reads CPU with GetProcessTimes on Windows. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Added the Windows Tested on Windows Server 2022, 1 output (IddSampleDriver, 1600x1200@60).
On the static run 285 of 286 frames were identical to the one before, so those are timeout re-sends. On the motion run it was 54 of 287, which is Chrome dropping frames on this GPU-less box. The first static run only got 39.7 fps (p50 24.4 ms). That's because AcquireNextFrame's timeout rounds to the 15.6 ms system tick. The grabber now holds Dumps converted to PNG are the real desktop and the colors are right:
Not tested here: ACCESS_LOST recovery (I didn't want to trigger UAC or the lock screen on this box), rotated outputs, and multiple monitors. |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
spacedouut
changed the title
AKA:
capturedv1.Summary
Rewrites captured from scratch. The control and media protocol stays wire-compatible with the agent (
src/backend/captured.go), so the agent needs no changes.Layout
main.go: the daemon (control socket, media socket, flags).pipelines/<source>_<capturer>_<os>.go: one pipeline per file.kms_nvidia_linux.gopipewire_mutter_linux.gokms_linear_linux.goscreencapturekit_darwin.gopipeline.go: interface and priority registry.stream.go: pacing, buffer reuse, and a one-frame mailbox that drops stale frames.drm_linux.goandegl_linux.go: native bindings. Everything is still loaded through purego, so there's no cgo.Behavior changes
kmspath silently streamed a gradient on NVIDIA because the scanout buffer is tiled (modifier0x300000000606014).kms_nvidiapipeline. It imports the scanout dma-buf into EGL (GBM platform) with its modifier, attaches it to an FBO and reads it back withglReadPixels, so the GPU does the de-tiling. Like Sunshine's KMS capture, it needsCAP_SYS_ADMIN.DP-1) and position. Before, pipewire listed a switched-off monitor with a bogus mode, and starting it failed withMonitor not active.videorate, so it stops padding with duplicate frames. Frames are now damage-driven, with a 1 s keepalive.statscommand;width,heightandpipelineinstream-started;pipelineandpipelinesininfo.--pipeline auto|<name>, and--list, which shows every pipeline, whether it works on this machine, and its displays.Measured on observatory (RTX 2070S, GNOME Wayland, DP-1 2560×1440@120), with the new
cmd/capbench:kms(synthetic gradient, not real frames)pipewirekms_nvidia@60kms_nvidia@120kms_linear(virtio 1280×800)A dumped
kms_nvidiaframe shows the real desktop, the right way up, with correct colors.Not done or not tested
pipelines/windowswas an empty file.GOOS=darwin go vetbut hasn't been run.kms_nvidiadoesn't capture the cursor, which is on its own hardware plane.Link to Devin session: https://app.devin.ai/sessions/c5b4a4a36d4e4d6d8a6901ae9001ab84
Open in Devin Desktop: https://app.devin.ai/desktop/session/c5b4a4a36d4e4d6d8a6901ae9001ab84?variant=devin
Requested by: @spacedouut
Summary by CodeRabbit