fix(recording): record the window Linux users actually pick - #250
Merged
Conversation
Choosing a window on Wayland produced a recording of the whole screen. Two independent defects, both proven on a real GNOME 46 session. The portal restore token pinned the first grant forever. A token is bound to the source it was minted for, so once any monitor had been approved the portal restored that monitor on every later run and stopped raising its picker — and SelectSources has no parameter naming a source, so the app had no way to ask for anything else. GNOME's permission store held ours as source_type=1 (MONITOR, CMN:0x1468), created 1.7s into one full-screen recording and replayed 93ms into a later one. The token is gone, and PersistMode is DoNot so we stop littering that store. SPA_META_VideoCrop was never requested. mutter pins a window stream to its monitor's size — "we cannot set the stream size to the exact size of the window, because windows can be resized, whereas streams cannot" — and reports the window's live rectangle in that meta, but only writes it if the consumer asked. We never asked, so a window arrived as a monitor-sized buffer: measured 1920x982 of content at (0,0) inside a 1920x1080 frame, 98px of black below. The declaration and per-frame read follow OBS and WebRTC; the validation follows WebRTC's stricter posture because the pointer goes straight to swscale. Neither reference handles a mid-recording resize — both let the frame size vary per frame, which an MP4 track cannot. The crop is latched at encoder open, the live origin is followed but clamped inside the buffer, and the first divergence is reported once as `crop-changed`. The encoder is opened from the first frame carrying a usable crop rather than the first frame at all: mutter's rectangle intersection reports success on an empty result and records a frame synchronously from enable(), so committing to frame zero could pin a window recording at monitor size again. Since the portal owns the choice, the in-app picker is gone on Linux: it could not steer the capture and only raised a second portal dialog whose grant was discarded. Both entry points — the HUD and the editor's Rec stage — now ask one shared hook, and the tray names what the portal granted instead of echoing a selection the capture never heard of. The picker also now comes before the countdown. The helper negotiates the portal, reports `source-selected`, and waits for `record` on stdin; the stream is not connected until then, so nothing is captured and WirePlumber cannot suspend an idle node out from under us. Preparing is optional and best-effort — start still negotiates on its own — so every other platform and any future caller is unaffected.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughLinux native recording now opens the portal picker before countdown, supports prepared and cancellable sessions, reports portal-selected source kinds, and handles window crops through PipeWire. The renderer, Electron bridge, UI, tests, translations, and documentation reflect this flow. ChangesLinux portal recording flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Recorder as useScreenRecorder
participant Electron as Electron IPC
participant Helper as Linux native helper
participant Portal as ScreenCast portal
participant PipeWire as PipeWire capture
Recorder->>Electron: prepare Linux recording
Electron->>Helper: start with deferStart
Helper->>Portal: open source picker
Portal-->>Helper: selected source and source kind
Helper-->>Electron: source-selected
Recorder->>Electron: start prepared recording
Electron->>Helper: record
Helper->>PipeWire: connect stream and process cropped frames
PipeWire-->>Electron: stream-started with source kind
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
electron/ipc/handlers.ts (1)
1870-1912: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftTrack the in-flight prepare, or a superseded or cancelled negotiation leaks a live portal session.
preparedLinuxCaptureis assigned only afterwaitUntilSourceSelected()resolves, and that await has no upper bound. While a prepare is negotiating, the slot is stillnull, so two things fail:
- A second
prepare-native-linux-recordingfindsnullat Line 1873, discards nothing, and spawns a second helper. Both pickers appear, and the later assignment at Line 1912 overwrites the earlier one. The overwritten session is never discarded, so a live ScreenCast session and its child process stay alive with the compositor sharing indicator on. Two renderer surfaces can prepare (the HUD and the editor Rec stage both drive this flow).cancel-native-linux-prepareis a no-op while the picker is up, becausediscardPreparedLinuxCapturereturns early onnull. The session then assigns and persists.useScreenRecorder.tsawaits the prepare before cancelling, so the countdown path is covered, but the hook unmount path is not.Record the negotiation itself, and discard the session if that negotiation was superseded or cancelled before it completed.
🛠️ Proposed fix: claim the slot before the unbounded await
let preparedLinuxCapture: { session: LinuxNativeCaptureSession; outputPath: string } | null = null; +/** Identifies the prepare that is still negotiating with the portal. */ +let preparingLinuxCaptureToken: symbol | null = null; /** Tears down a prepared-but-unarmed session, e.g. an abandoned countdown. */ function discardPreparedLinuxCapture(reason: string) { + // Invalidate a negotiation that has not assigned its session yet, so the + // session it is about to produce is discarded instead of stranded. + preparingLinuxCaptureToken = null; if (!preparedLinuxCapture) { return; }discardPreparedLinuxCapture("superseded by a new prepare"); + const token = Symbol("prepare-native-linux-recording"); + preparingLinuxCaptureToken = token; try {await session.start(); // The picker is up now. No timeout: a human is reading a dialog. await session.waitUntilSourceSelected(); + // Superseded or cancelled while the picker was up. + if (preparingLinuxCaptureToken !== token) { + session.discard(); + return { success: false, reason: "cancelled" }; + } + preparingLinuxCaptureToken = null; preparedLinuxCapture = { session, outputPath };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/handlers.ts` around lines 1870 - 1912, Track the in-flight Linux capture negotiation in preparedLinuxCapture before awaiting waitUntilSourceSelected(), so subsequent prepare requests and cancel-native-linux-prepare can discard that session instead of creating or retaining overlapping portal sessions. Update the post-selection assignment to preserve the same tracked session and ensure superseded or cancelled negotiations cleanly discard the session without allowing a later completion to overwrite newer state.electron/native/pipewire-capture/src/main.rs (1)
746-770: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
capture-startedreports the unmasked crop size, not the encoded size.Line 748 re-derives
(width, height)fromframe.crop. The encoder was opened at line 657-658 with those values masked to even. The comment at line 654-656 states that a window rectangle is routinely odd, so the two differ in the common case.capture-startedthen reports a size the MP4 does not have.linuxNativeCaptureSession.tslogs these values atcapture-started, and the app treats that event as "recording started", so the one line that answers "what size is my recording" is off by a pixel.Apply the same mask where the event is built.
🐛 Report the encoded dimensions
let first = !capture.started(); let staged = capture.stage(&frame); - let (width, height) = (frame.crop.width, frame.crop.height); + // The masked values, matching what the encoder was opened + // with: an odd window rect is rounded down for H.264 chroma. + let (width, height) = (frame.crop.width & !1, frame.crop.height & !1); mailbox.recycle(frame.pixels);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/pipewire-capture/src/main.rs` around lines 746 - 770, Update the CaptureStarted event construction in the capture loop to report the encoded dimensions by applying the same even-dimension mask used when opening the encoder, instead of using the raw frame.crop width and height. Keep the existing event flow and encoder configuration unchanged.
🧹 Nitpick comments (3)
electron/native/pipewire-capture/src/events.rs (1)
52-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a serialization test for
SourceSelected.
SourceSelectedis a new wire event and the TypeScript side matches on the exact literal"source-selected"plus the camelCase field names. This module already pinsStreamStartedwithstream_started_is_kebab_case_with_camel_case_fields. A matching test for the new variant would catch a rename or a missingrename_allbefore it reaches the helper protocol.♻️ Proposed test
#[test] fn source_selected_names_the_moment_the_picker_was_answered() { let value = parse_one(&Event::SourceSelected { timestamp_ms: 3, node_id: 42, source_kind: Some("window".to_owned()), position_x: Some(10), position_y: None, }); assert_eq!(value["event"], "source-selected"); assert_eq!(value["nodeId"], 42); assert_eq!(value["sourceKind"], "window"); assert_eq!(value["positionX"], 10); // Absent, not omitted: the TypeScript union accepts null and must keep doing so. assert!(value["positionY"].is_null()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/pipewire-capture/src/events.rs` around lines 52 - 67, Add a serialization test alongside the existing StreamStarted serialization test for Event::SourceSelected, using representative populated and absent optional fields. Assert the serialized event is "source-selected", uses camelCase keys such as nodeId, sourceKind, and positionX, and serializes a None position_y as null.electron/native-bridge/capture/linuxNativeCaptureSession.test.ts (1)
246-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
error-event rejection of a pending source-selection wait.This test covers the process-exit path.
handleEventalso rejectssourceSelectedRejecton anerrorevent (linuxNativeCaptureSession.ts lines 471-473), and that branch has no direct test. A cancelled picker reaches the parent asportal-cancelledon that path, which is the most likely real failure during a prepared session.As per coding guidelines: "Add a test for every new behavior in the same package as the code under test."
♻️ Proposed test
/** * A dismissed picker arrives as an `error` event, not as an exit: the helper * reports `portal-cancelled` and only then goes away. Whoever is waiting on * the picker must learn the cause, not just the exit code. */ it("rejects a pending source selection when the portal reports an error", async () => { const session = newSession(true); await startReady(session); const selecting = session.waitUntilSourceSelected(); helper.emitEvent({ event: "error", code: "portal-cancelled", message: "The screen capture request was cancelled.", }); await expect(selecting).rejects.toThrow("The screen capture request was cancelled."); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native-bridge/capture/linuxNativeCaptureSession.test.ts` around lines 246 - 255, Add a test beside the existing pending source-selection rejection test that starts a ready session, waits via waitUntilSourceSelected(), emits an error event through helper.emitEvent with portal-cancelled and a descriptive message, and verifies the pending promise rejects with that message.Source: Coding guidelines
electron/native/pipewire-capture/src/main.rs (1)
635-641: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTreat an unknown source kind like a window while waiting for the crop.
source_kindis documented as unknown when absent, so the currentexpecting_cropcheck skips the crop wait on older backends. If the first frame is the window before crop metadata arrives, the encoder commits to the full-monitor-size stream for the whole recording. On a monitor stream, a missing crop wait only costsMAX_FRAMES_AWAITING_CROPframes before the full stream is used.♻️ Treat an unknown kind the same as a window while waiting for the crop
- let expecting_crop = - granted_kind == Some(portal::SourceKind::Window) && !frame.has_crop; + // An absent kind is unknown, not "monitor": the same + // wait applies, and a real monitor stream only pays + // MAX_FRAMES_AWAITING_CROP frames before falling back. + let expecting_crop = !frame.has_crop + && !matches!( + granted_kind, + Some(portal::SourceKind::Monitor) + | Some(portal::SourceKind::Virtual) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/pipewire-capture/src/main.rs` around lines 635 - 641, Update the expecting_crop condition in the frame-processing loop to treat an absent or unknown source kind the same as portal::SourceKind::Window when frame.has_crop is false. Preserve the existing MAX_FRAMES_AWAITING_CROP limit, frame recycling, and continue behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 1961-2002: Update the prepared-session path around
LinuxNativeCaptureSession so a START request cannot silently apply audio or
cursor settings that were not used when the session was prepared. Either reject
the prepared session and execute the full start flow when its configuration
differs, or retain and use the prepared configuration for the session and
linuxNativeCaptureCursorMode; also ensure countdown controls cannot change those
affected settings if required by the chosen approach.
- Around line 596-607: Update linuxSourceLabel to resolve all four source labels
through the existing mainT localization function instead of returning hardcoded
English strings. Add the corresponding keys to the appropriate locale namespace,
preserving the current mappings for window, monitor, virtual, and the default
recording case; ensure the localized result continues flowing through
onRecordingStateChange.
In `@electron/native/pipewire-capture/src/capture.rs`:
- Around line 293-300: Update crop_diverged to normalize the live frame crop
width and height to the encoder’s even dimensions before comparing them with
committed_width and committed_height. Preserve resize detection for genuinely
different normalized dimensions, and add a regression test covering an unchanged
odd-sized crop such as 321x241.
- Around line 308-337: Keep the live portal crop as the recording boundary:
update capture.rs:308-337 and the stage/read_origin flow to encode only pixels
within a validated crop, safely scale, pad, drop, or stop when the committed
rectangle exceeds it. Track crop-metadata validity separately from crop
dimensions, marking it valid only after metadata arrival and bounds validation;
update pw_shim.c:617-650, pw_shim.h:88-96, and shim.rs:44-48, 171-177, 248-254
to propagate that state. Update capture.rs:609-634 and coverage handling at
capture.rs:663-681 so missing or shrunken crops cannot report coverage that
permits out-of-window pixels.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 509-512: The usePortalOwnsSource hook returns false before its
asynchronous check completes, and the current code treats this unresolved
pending state identically to a resolved false result. Update the sourceSelected
logic in LaunchWindow.tsx (line 509-512) to expose an unresolved state from the
portal ownership check, then suppress the openSourceSelector call when ownership
is still pending. Similarly update RecStage.tsx (line 145-151) to not treat an
unresolved pending state as a locally selected source, and update RecStage.tsx
(line 193-226) to conditionally hide the interactive source selector UI when
ownership is unresolved. Finally, add a test case in LaunchWindow.test.tsx (line
425-454) that verifies openSourceSelector is not invoked when Record is clicked
before the portal ownership check resolves, ensuring the deferred behavior works
correctly.
In `@src/i18n/locales/it/editor.json`:
- Line 46: Update the Italian translation value for accessibilityAllowAndRetry
to replace the unnatural “accesso all'accessibilità” wording with clear, natural
language asking the user to grant OpenScreen access to accessibility features,
while preserving the rest of the instruction.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 1870-1912: Track the in-flight Linux capture negotiation in
preparedLinuxCapture before awaiting waitUntilSourceSelected(), so subsequent
prepare requests and cancel-native-linux-prepare can discard that session
instead of creating or retaining overlapping portal sessions. Update the
post-selection assignment to preserve the same tracked session and ensure
superseded or cancelled negotiations cleanly discard the session without
allowing a later completion to overwrite newer state.
In `@electron/native/pipewire-capture/src/main.rs`:
- Around line 746-770: Update the CaptureStarted event construction in the
capture loop to report the encoded dimensions by applying the same
even-dimension mask used when opening the encoder, instead of using the raw
frame.crop width and height. Keep the existing event flow and encoder
configuration unchanged.
---
Nitpick comments:
In `@electron/native-bridge/capture/linuxNativeCaptureSession.test.ts`:
- Around line 246-255: Add a test beside the existing pending source-selection
rejection test that starts a ready session, waits via waitUntilSourceSelected(),
emits an error event through helper.emitEvent with portal-cancelled and a
descriptive message, and verifies the pending promise rejects with that message.
In `@electron/native/pipewire-capture/src/events.rs`:
- Around line 52-67: Add a serialization test alongside the existing
StreamStarted serialization test for Event::SourceSelected, using representative
populated and absent optional fields. Assert the serialized event is
"source-selected", uses camelCase keys such as nodeId, sourceKind, and
positionX, and serializes a None position_y as null.
In `@electron/native/pipewire-capture/src/main.rs`:
- Around line 635-641: Update the expecting_crop condition in the
frame-processing loop to treat an absent or unknown source kind the same as
portal::SourceKind::Window when frame.has_crop is false. Preserve the existing
MAX_FRAMES_AWAITING_CROP limit, frame recycling, and continue behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6986443-e7c6-4134-b7d0-0575ea44bd00
📒 Files selected for processing (49)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/native-bridge/capture/linuxNativeCaptureSession.test.tselectron/native-bridge/capture/linuxNativeCaptureSession.tselectron/native-bridge/cursor/recording/pipeWireCursorAccumulator.tselectron/native/pipewire-capture/csrc/pw_shim.celectron/native/pipewire-capture/csrc/pw_shim.helectron/native/pipewire-capture/src/capture.rselectron/native/pipewire-capture/src/encoder.rselectron/native/pipewire-capture/src/events.rselectron/native/pipewire-capture/src/main.rselectron/native/pipewire-capture/src/portal.rselectron/native/pipewire-capture/src/shim.rselectron/preload.tssrc/components/ai-edition/v4/RecStage.tsxsrc/components/launch/LaunchWindow.test.tsxsrc/components/launch/LaunchWindow.tsxsrc/hooks/usePortalOwnsSource.tssrc/hooks/useScreenRecorder.tssrc/i18n/locales/ar/editor.jsonsrc/i18n/locales/ar/launch.jsonsrc/i18n/locales/en/editor.jsonsrc/i18n/locales/en/launch.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/es/launch.jsonsrc/i18n/locales/fr/editor.jsonsrc/i18n/locales/fr/launch.jsonsrc/i18n/locales/it/editor.jsonsrc/i18n/locales/it/launch.jsonsrc/i18n/locales/ja-JP/editor.jsonsrc/i18n/locales/ja-JP/launch.jsonsrc/i18n/locales/ko-KR/editor.jsonsrc/i18n/locales/ko-KR/launch.jsonsrc/i18n/locales/pt-BR/editor.jsonsrc/i18n/locales/pt-BR/launch.jsonsrc/i18n/locales/ru/editor.jsonsrc/i18n/locales/ru/launch.jsonsrc/i18n/locales/tr/editor.jsonsrc/i18n/locales/tr/launch.jsonsrc/i18n/locales/vi/editor.jsonsrc/i18n/locales/vi/launch.jsonsrc/i18n/locales/zh-CN/editor.jsonsrc/i18n/locales/zh-CN/launch.jsonsrc/i18n/locales/zh-TW/editor.jsonsrc/i18n/locales/zh-TW/launch.jsonsrc/lib/nativeLinuxRecording.test.tssrc/lib/nativeLinuxRecording.tstechnical-documentation/architecture/recording.mdtechnical-documentation/testing/manual-e2e-checklist.md
Comment on lines
+308
to
+337
| fn read_origin(&self, frame: &shim::Frame) -> (i32, i32) { | ||
| let max_x = (frame.width - self.committed_width).max(0); | ||
| let max_y = (frame.height - self.committed_height).max(0); | ||
| ( | ||
| frame.crop.x.clamp(0, max_x), | ||
| frame.crop.y.clamp(0, max_y), | ||
| ) | ||
| } | ||
|
|
||
| /// Converts a captured frame into the encoder's staging buffer. Nothing is | ||
| /// written until [`Self::advance`] runs. | ||
| pub fn stage(&mut self, frame: &shim::Frame) -> Result<(), String> { | ||
| let format = pixel_format(frame.video_format)?; | ||
| self.encoder.stage(&frame.pixels, frame.stride, format)?; | ||
|
|
||
| // Address the crop by moving the START of the slice, and hand swscale the | ||
| // frame's OWN stride unchanged. The stride is the distance between rows | ||
| // in the source buffer, which cropping does not alter — WebRTC's memfd | ||
| // path subtracts the x offset from it, which is wrong for any non-zero x | ||
| // and is latent there only because no shipping compositor sets one. | ||
| let (x, y) = self.read_origin(frame); | ||
| let offset = (y as usize) | ||
| .checked_mul(frame.stride) | ||
| .and_then(|rows| rows.checked_add((x as usize) * BYTES_PER_SOURCE_PIXEL)) | ||
| .ok_or_else(|| "crop offset overflows".to_owned())?; | ||
| let pixels = frame | ||
| .pixels | ||
| .get(offset..) | ||
| .ok_or_else(|| format!("crop offset {offset} is past the end of the frame"))?; | ||
|
|
||
| self.encoder.stage(pixels, frame.stride, format)?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -p 'electron/native/pipewire-capture/src/(capture|shim).rs|electron/native/pipewire-capture/csrc/pw_shim(c|h)' .
echo "== git status short =="
git status --short
echo "== capture.rs relevant lines =="
sed -n '1,120p;260,360p;540,690p' electron/native/pipewire-capture/src/capture.rs | cat -n
echo "== shim.rs relevant lines =="
sed -n '1,90p;130,190p;220,270p' electron/native/pipewire-capture/src/shim.rs | cat -n
echo "== pw_shim.c relevant lines =="
sed -n '560,670p' electron/native/pipewire-capture/csrc/pw_shim.c | cat -n
echo "== pw_shim.h relevant lines =="
sed -n '1,130p' electron/native/pipewire-capture/csrc/pw_shim.h | cat -n
echo "== references to crop/has_crop/committed =="
rg -n "crop|has_crop|committed_width|committed_height|read_origin|stage\\(" electron/native/pipewire-capture/src electron/native/pipewire-capture/csrcRepository: getopenscreen/openscreen
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== encoder/staging references =="
fd -p 'electron/native/pipewire-capture/src/.*encoder.*|electron/native/pipewire-capture/src/pix.*|electron/native/pipewire-capture/src/ffmpeg.*' .
echo "== files =='
git ls-files electron/native/pipewire-capture/src | rg 'encoder|pix|ffmpeg|swscale|scaler' || true
echo "== likely lines =="
for f in $(git ls-files electron/native/pipewire-capture/src | rg 'encoder|pix|ffmpeg|swscale|scaler'); do
echo "--- $f ($(wc -l < "$f") lines) ---"
rg -n 'struct|class|fn .*stage|sws_scale|AvFrame|stage|width|height' "$f" | head -200 || true
done
echo "== C capture loop/calling capture from C =="
rg -n "osc_pw_start|on_frame|PipeWire|start_video|video|stage\\(|frame\\.crop|has_crop|committed" electron/native/pipewire-capture/csrc electron/native/pipewire-capture/src/capture.rs
echo "== read-only behavioral probe for read_origin/stage crop size clamp =="
python3 - <<'PY'
def read_origin(frame, committed_width, committed_height):
max_x = max(frame["width"] - committed_width, 0)
max_y = max(frame["height"] - committed_height, 0)
return (max(0, min(frame["crop"]["x"], max_x)), max(0, min(frame["crop"]["y"], max_y)))
committed = (320, 240)
cases = [
{
"name": "window shrinks from 320x240 to 20x10, origin near buffer edge",
"frame": {"width": 400, "height": 300, "stride": 400*4, "crop": {"x": 380, "y": 290, "width": 20, "height": 10}},
},
{
"name": "valid full-frame crop",
"frame": {"width": 1920, "height": 1080, "stride": 1920*4, "crop": {"x": 0, "y": 0, "width": 1920, "height": 1080}},
},
{
"name": "window inside large frame stays same crop",
"frame": {"width": 1920, "height": 1080, "stride": 1920*4, "crop": {"x": 100, "y": 50, "width": 320, "height": 240}},
},
]
for c in cases:
frame = c["frame"]
(x,y) = read_origin(frame, *committed)
print(f"{c['name']}")
print(f" committed {committed} frame {frame['width']}x{frame['height']} crop {frame['crop']}")
print(f" read_origin = ({x}, {y})")
offset = y * frame["stride"] + x * 4
print(f" encoder will receive pixels.slice(offset) with stride {frame['stride']} width {committed[0]} height {committed[1]}")
print(f" offset = {offset} (row {y}, column {x})")
PARepository: getopenscreen/openscreen
Length of output: 348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== encoder.rs outline/relevant =="
rg -n 'struct VideoEncoder|fn stage|swscale|sws_scale|frame|width|height|stride' electron/native/pipewire-capture/src/encoder.rs
sed -n '1,260p' electron/native/pipewire-capture/src/encoder.rs | cat -n
echo "== ffmpeg.rs relevant =="
rg -n 'struct VideoEncoder|VideoEncoder|FFmpeg|AVFrame|sws_scale|encoder|width|height|stride' electron/native/pipewire-capture/src/ffmpeg.rs
sed -n '1,260p' electron/native/pipewire-capture/src/ffmpeg.rs | cat -n
echo "== deterministic staging geometry probe =="
python3 - <<'PY'
def read_origin(frame, committed_width, committed_height):
max_x = max(frame["width"] - committed_width, 0)
max_y = max(frame["height"] - committed_height, 0)
return (max(0, min(frame["crop"]["x"], max_x)), max(0, min(frame["crop"]["y"], max_y)))
committed = (320, 240)
cases = [
{"name": "window shrinks to 20x10 near buffer edge",
"frame": {"width": 400, "height": 300, "stride": 400*4, "crop": {"x": 380, "y": 290, "width": 20, "height": 10}}},
{"name": "valid full-frame crop",
"frame": {"width": 1920, "height": 1080, "stride": 1920*4, "crop": {"x": 0, "y": 0, "width": 1920, "height": 1080}}},
{"name": "valid original window crop",
"frame": {"width": 1920, "height": 1080, "stride": 1920*4, "crop": {"x": 100, "y": 50, "width": 320, "height": 240}}},
]
for c in cases:
frame = c["frame"]
x, y = read_origin(frame, *committed)
offset = y * frame["stride"] + x * 4
encoded_source_size = (x + committed[0], y + committed[1])
print(f"{c['name']}")
print(f" crop_width x height = {frame['crop']['width']}x{frame['crop']['height']}")
print(f" read_origin = ({x}, {y})")
print(f" encoded output width x height = {committed[0]}x{committed[1]}")
print(f" source rectangle from crop origin is x:{x}:w{committed[0]} y:{y}:h{committed[1]}")
print(f" source rectangle bottom-right = {encoded_source_size}")
print(f" outside live crop? {encoded_source_size[0] > field_x + field_w for field_x, field_y, field_w, field_w in [0,0,20,10] if c['name']==cases[0]['name']}")
PYRepository: getopenscreen/openscreen
Length of output: 29077
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External
Reachability path
● Entry
electron/native/pipewire-capture/src/encoder.rs
│
▼
● Sink
electron/native/pipewire-capture/src/capture.rs
Keep the live portal crop as the recording boundary.
A PipeWire window stream is monitor-sized, and SPA_META_VideoCrop is the portal-granted rectangle. Valid full-frame crop metadata is currently indistinguishable from missing or invalid metadata, and a smaller later crop is still encoded at the committed size from its origin. The current offset check only keeps swscale from reading past the buffer; it does not stop adjacent desktop pixels from being staged into the recording.
electron/native/pipewire-capture/src/capture.rs#L308-L337: Reject encoding a committed-size rectangle that extends beyond a valid live crop. If dimensions diverge, scale only crop pixels, add masked/pad pixels, drop the frame, or stop safely.- Set a crop-metadata-valid flag only when the crop actually arrived and passed bounds validation, and keep full-frame valid metadata separate from absence/invalid metadata.
electron/native/pipewire-capture/src/capture.rs#L663-L681: Update coverage to check that a shrunken or missing crop cannot encode pixels outside the window.
📍 Affects 4 files
electron/native/pipewire-capture/src/capture.rs#L308-L337(this comment)electron/native/pipewire-capture/csrc/pw_shim.c#L617-L650electron/native/pipewire-capture/csrc/pw_shim.h#L88-L96electron/native/pipewire-capture/src/shim.rs#L44-L48electron/native/pipewire-capture/src/shim.rs#L171-L177electron/native/pipewire-capture/src/shim.rs#L248-L254electron/native/pipewire-capture/src/capture.rs#L609-L634electron/native/pipewire-capture/src/capture.rs#L663-L681
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/native/pipewire-capture/src/capture.rs` around lines 308 - 337, Keep
the live portal crop as the recording boundary: update capture.rs:308-337 and
the stage/read_origin flow to encode only pixels within a validated crop, safely
scale, pad, drop, or stop when the committed rectangle exceeds it. Track
crop-metadata validity separately from crop dimensions, marking it valid only
after metadata arrival and bounds validation; update pw_shim.c:617-650,
pw_shim.h:88-96, and shim.rs:44-48, 171-177, 248-254 to propagate that state.
Update capture.rs:609-634 and coverage handling at capture.rs:663-681 so missing
or shrunken crops cannot report coverage that permits out-of-window pixels.
Comment on lines
+509
to
+512
| // Linux never detours through the in-app picker: there is nothing for | ||
| // it to select, and waiting for a selection that can never arrive left | ||
| // the record button opening a modal instead of recording. | ||
| const sourceSelected = portalOwnsSource || (sourceSelectedOverride ?? hasSelectedSource); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not treat a pending portal check as Chromium fallback.
usePortalOwnsSource() returns false before its asynchronous check completes. A Linux user can click the in-app selector during that interval. The auto-start path can also open the selector when its source override is false before the portal result arrives.
Expose an unresolved state. Suppress the local selector until ownership is known. Defer or queue recording start until the check resolves.
src/components/launch/LaunchWindow.tsx#L509-L512: do not callopenSourceSelector()while portal ownership is unresolved.src/components/ai-edition/v4/RecStage.tsx#L145-L151: do not label unresolved portal ownership as a locally selected source.src/components/ai-edition/v4/RecStage.tsx#L193-L226: do not render an interactive source selector while ownership is unresolved.src/components/launch/LaunchWindow.test.tsx#L425-L454: add a deferred helper-availability test that clicks Record before resolution and verifies thatopenSourceSelector()is not called.
As per coding guidelines, add a test for every new behavior in the same package.
📍 Affects 3 files
src/components/launch/LaunchWindow.tsx#L509-L512(this comment)src/components/ai-edition/v4/RecStage.tsx#L145-L151src/components/ai-edition/v4/RecStage.tsx#L193-L226src/components/launch/LaunchWindow.test.tsx#L425-L454
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/launch/LaunchWindow.tsx` around lines 509 - 512, The
usePortalOwnsSource hook returns false before its asynchronous check completes,
and the current code treats this unresolved pending state identically to a
resolved false result. Update the sourceSelected logic in LaunchWindow.tsx (line
509-512) to expose an unresolved state from the portal ownership check, then
suppress the openSourceSelector call when ownership is still pending. Similarly
update RecStage.tsx (line 145-151) to not treat an unresolved pending state as a
locally selected source, and update RecStage.tsx (line 193-226) to conditionally
hide the interactive source selector UI when ownership is unresolved. Finally,
add a test case in LaunchWindow.test.tsx (line 425-454) that verifies
openSourceSelector is not invoked when Record is clicked before the portal
ownership check resolves, ensuring the deferred behavior works correctly.
Source: Coding guidelines
| "cameraNotFound": "Fotocamera non trovata.", | ||
| "permissionDenied": "Autorizzazione di registrazione negata. Consenti la registrazione dello schermo.", | ||
| "accessibilityAllowAndRetry": "Consenti l'accesso all'accessibilità per OpenScreen, poi premi di nuovo registra per avviare il conto alla rovescia." | ||
| "accessibilityAllowAndRetry": "Consenti l'accesso all'accessibilità per OpenScreen, poi premi di nuovo registra per avviare il conto alla rovescia.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the Italian accessibility instruction.
Line 46 uses the unnatural phrase accesso all'accessibilità. Replace it with wording that clearly asks the user to grant OpenScreen access to accessibility features.
Proposed wording
- "accessibilityAllowAndRetry": "Consenti l'accesso all'accessibilità per OpenScreen, poi premi di nuovo registra per avviare il conto alla rovescia.",
+ "accessibilityAllowAndRetry": "Consenti a OpenScreen l'accesso alle funzioni di accessibilità, quindi premi di nuovo il pulsante Registra per avviare il conto alla rovescia.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "accessibilityAllowAndRetry": "Consenti l'accesso all'accessibilità per OpenScreen, poi premi di nuovo registra per avviare il conto alla rovescia.", | |
| "accessibilityAllowAndRetry": "Consenti a OpenScreen l'accesso alle funzioni di accessibilità, quindi premi di nuovo il pulsante Registra per avviare il conto alla rovescia.", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/it/editor.json` at line 46, Update the Italian translation
value for accessibilityAllowAndRetry to replace the unnatural “accesso
all'accessibilità” wording with clear, natural language asking the user to grant
OpenScreen access to accessibility features, while preserving the rest of the
instruction.
…path A prepared portal session could be stranded. The slot is only filled after waitUntilSourceSelected(), which waits on a human reading a dialog, so for that whole window it was null — a cancel found nothing to cancel and a second prepare found nothing to supersede. The first session then assigned itself afterwards and stayed alive, holding a ScreenCast grant and the compositor's sharing indicator with nothing recording behind it. The negotiation now carries a token, and a session whose token was invalidated is discarded on arrival. A prepared session ignored the start request. Every capture setting is baked into the helper's spawn arguments and arm() only writes `record`, so a microphone, system-audio or cursor-mode change made during the countdown — the HUD does not lock those controls — was silently dropped. Worse, the cursor mode recorded for the editor came from the start request, so the app could believe one mode while the helper used another, and that flag decides whether the editor draws its own pointer. The prepared session is now rejected when its settings differ, falling through to a full start. crop_diverged compared the raw crop against the rounded committed size, so a window sitting stably at an odd size reported a resize on every frame. The tray label was English in a localized UI. It now resolves through mainT, in the `common` namespace the main process already loads. Also: a Record click landing before the portal-ownership check resolves opened a selector the main process refuses, and was swallowed. The refusal is authoritative and immediate, so it starts the recording instead.
Collaborator
Author
|
Four of the six were real. Addressed in Accepted — a prepared portal session could be stranded. The most serious of the six, and correctly diagnosed. Accepted — a prepared session ignored the start request. Every capture setting is baked into the helper's spawn arguments and Accepted — Accepted — the tray label was English in a localized UI. Partly accepted — pending portal check treated as Chromium fallback. The premise needs one correction: Declined — Italian accessibility wording. The wording is right, but it is not this PR's string: Full suite on the rebased base: 135 files, 1609 passing, 0 failures — the previously-noted |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Choosing a window on Wayland produced a recording of the whole screen. Two independent defects, both proven on a real GNOME 46 / mutter session.
The portal restore token pinned the first grant forever. A token is bound to the source it was minted for, so once any monitor had been approved the portal restored that monitor on every later run and stopped raising its picker — and
SelectSourceshas no parameter naming a source, so the app had no way to ask for anything else. GNOME's permission store held ours decoded assource_type=1(MONITOR,CMN:0x1468— the built-in panel), created 1.7 s into one full-screen recording and replayed 93 ms into a later one. The token is gone, andPersistModeis nowDoNotso we stop accumulating entries in that store.SPA_META_VideoCropwas never requested. mutter pins a window stream to its monitor's size — "we cannot set the stream size to the exact size of the window, because windows can be resized, whereas streams cannot" — and reports the window's live rectangle in that meta, but only writes it if the consumer declared it. We never did, so a window arrived as a monitor-sized buffer: measured 1920×982 of content at (0,0) inside a 1920×1080 frame, 98 px of black below. The declaration and per-frame read follow OBS and WebRTC; validation follows WebRTC's stricter posture, because the pointer goes straight to swscale.Neither reference handles a mid-recording resize — both let the delivered frame size vary per frame, which an MP4 track cannot. The crop is latched at encoder open, the live origin is followed but clamped inside the buffer, and the first divergence is reported once as
crop-changed. The encoder is opened from the first frame carrying a usable crop rather than the first frame at all: mutter's rectangle intersection reports success on an empty result and records a frame synchronously fromenable(), so committing to frame zero could pin a window recording at monitor size again.The in-app picker is gone on Linux. It could not steer the capture, and it raised a second portal dialog — via
desktopCapturer.getSources()— whose grant was discarded. Both entry points (the HUD and the editor's Rec stage) now ask one shared hook, and the tray reports what the portal actually granted instead of echoing a selection the capture never heard of.The picker now comes before the countdown. The helper negotiates the portal, reports
source-selected, and waits forrecordon stdin; the PipeWire stream is not connected until then, so nothing is captured and WirePlumber cannot suspend an idle node out from under us during the count. Preparing is optional and best-effort —startstill negotiates on its own — so other platforms and any future caller are unaffected.Related issue
Refs #
Type of change
Release impact
Desktop impact
Screenshots / video
Before: a maximised window recorded as 1920×1080 with the window in the top 982 px and black beneath — indistinguishable from a broken full-screen capture.
After: the file is the window's own size.
Testing
npm run test— 1604 passing (electron/media/audioPeaks.test.tsfails locally only, on a machine whereelectron/native/bin/linux-x64/ffmpegis a directory of shared libraries; fixed separately infix/ffmpeg-resolve-executable).cargo testinelectron/native/pipewire-capture— 54 passing, including new coverage for the crop, an out-of-bounds shrunken window, and the prepare/arm protocol.npm run lint,npx tsc --noEmit,npx tsc -p tsconfig.test.json --noEmit,npm run docs:check,npm run i18n:check,npx vite build— all clean.Note
CI does not build or test
electron/native/pipewire-capture— therust-linux-compositor-checkjob covers the compositor crate, not this helper. The C and Rust changes here are verified locally only.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Localization