fix: improve performance on windows and macos by passing canvas direclty to VideoFrame()#448
Open
theopfr wants to merge 3 commits intosiddharthvaddem:mainfrom
Open
fix: improve performance on windows and macos by passing canvas direclty to VideoFrame()#448theopfr wants to merge 3 commits intosiddharthvaddem:mainfrom
VideoFrame()#448theopfr wants to merge 3 commits intosiddharthvaddem:mainfrom
Conversation
…d not necessary for other OS' line win or darwin
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughFrameRenderer and exporter code now accept a runtime Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
theopfr
changed the title
VideoFrame()VideoFrame()
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/exporter/videoExporter.ts (1)
114-115: nit: threadplatformintoFrameRendererinstead of doing a second IPC hop.
VideoExporteralready resolves the platform here, andFrameRenderer.initialize()fetches it again at Lines 191-192. not a blocker, just kinda cleaner to passplatform/isLinuxdown in the renderer config so the export path owns the Electron-specific check in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/videoExporter.ts` around lines 114 - 115, VideoExporter currently calls window.electronAPI.getPlatform() and then FrameRenderer.initialize() re-queries the platform; instead, thread the resolved platform (or a boolean like isLinux) from VideoExporter into the FrameRenderer configuration so Electron-specific logic is centralized. Update the call site in VideoExporter where platform is awaited to include that value in the FrameRenderer config/constructor, and remove the redundant IPC call inside FrameRenderer.initialize() (or guard it to use the provided platform when present); reference VideoExporter, FrameRenderer.initialize, platform, and isLinux when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 191-195: The renderer still does a GPU→CPU readback on non-Linux
because readbackVideoCanvas() (which calls gl.readPixels()) is invoked
unconditionally; update the rendering path to reuse the existing isLinux flag
(from the compositeCtx creation) to gate readbackVideoCanvas() so it only runs
on Linux, and on non-Linux draw this.app.canvas directly into the
compositeCanvas/decorative canvas instead of performing gl.readPixels(); locate
and modify the call site that invokes readbackVideoCanvas() and ensure the new
branch uses the existing isLinux boolean to choose between readbackVideoCanvas()
and drawing this.app.canvas, leaving readbackVideoCanvas() implementation
unchanged.
---
Nitpick comments:
In `@src/lib/exporter/videoExporter.ts`:
- Around line 114-115: VideoExporter currently calls
window.electronAPI.getPlatform() and then FrameRenderer.initialize() re-queries
the platform; instead, thread the resolved platform (or a boolean like isLinux)
from VideoExporter into the FrameRenderer configuration so Electron-specific
logic is centralized. Update the call site in VideoExporter where platform is
awaited to include that value in the FrameRenderer config/constructor, and
remove the redundant IPC call inside FrameRenderer.initialize() (or guard it to
use the provided platform when present); reference VideoExporter,
FrameRenderer.initialize, platform, and isLinux when making these changes.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a87bc0f-d9d6-4733-b8ee-4477d788efcd
📒 Files selected for processing (2)
src/lib/exporter/frameRenderer.tssrc/lib/exporter/videoExporter.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/exporter/videoExporter.ts (2)
533-538: kinda inconsistent: platform detection methodsheads up -
getEncoderPreferences()still usesnavigator.userAgentto detect Windows, while the new frame export logic useswindow.electronAPI.getPlatform(). might be worth harmonizing these at some point for consistency. not blocking since both approaches work, just a bit of tech debt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/videoExporter.ts` around lines 533 - 538, getEncoderPreferences currently detects Windows via navigator.userAgent which is inconsistent with the rest of the frame export logic that uses window.electronAPI.getPlatform(); update getEncoderPreferences to call window.electronAPI.getPlatform() when available (fall back to navigator.userAgent or a default) and base the Windows check on that result so the platform detection is unified across the codebase, keeping the existing return order for ["prefer-software","prefer-hardware"] on Windows and the opposite otherwise.
114-115: same deal:getPlatform()outside try-catchsame issue as gifExporter - if this throws, it won't be caught. consistency is nice but both files could be slightly more defensive here.
suggested fix
private async exportWithEncoderPreference( encoderPreference: HardwareAcceleration, ): Promise<ExportResult> { let webcamFrameQueue: AsyncVideoFrameQueue | null = null; let stopWebcamDecode = false; let webcamDecodeError: Error | null = null; let webcamDecodePromise: Promise<void> | null = null; let webcamDecoder: StreamingVideoDecoder | null = null; this.cleanup(); this.cancelled = false; this.fatalEncoderError = null; - const platform = await window.electronAPI.getPlatform(); - try { + const platform = await window.electronAPI.getPlatform(); + const streamingDecoder = new StreamingVideoDecoder();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/videoExporter.ts` around lines 114 - 115, The call to window.electronAPI.getPlatform() is currently outside any try-catch, so if it throws the exception won't be handled; move or re-invoke this call inside the existing try-catch block that surrounds the export logic (or add a small try-catch specifically around the const platform = await window.electronAPI.getPlatform() line) so errors from getPlatform() are caught and logged/handled consistently with the rest of the export flow (reference the const platform assignment and the surrounding export routine in videoExporter.ts).src/lib/exporter/gifExporter.ts (1)
118-120: lowkey risky:getPlatform()called outside try-catchif
window.electronAPIis undefined or the call throws for some reason, the error won't be caught by the try block starting at line 121. probably fine in practice since this is an electron app, but moving it inside the try block would be a bit more defensive.suggested fix
async export(): Promise<ExportResult> { let webcamFrameQueue: AsyncVideoFrameQueue | null = null; - - const platform = await window.electronAPI.getPlatform(); - try { + const platform = await window.electronAPI.getPlatform(); + this.cleanup();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/gifExporter.ts` around lines 118 - 120, Move the call to window.electronAPI.getPlatform() inside the existing try/catch so failures are handled; specifically, update the code that assigns const platform = await window.electronAPI.getPlatform() to be executed within the try block (or wrap that single call in its own try/catch) so any thrown error from window.electronAPI or getPlatform is caught by the existing error handling in the gifExporter logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/exporter/gifExporter.ts`:
- Around line 118-120: Move the call to window.electronAPI.getPlatform() inside
the existing try/catch so failures are handled; specifically, update the code
that assigns const platform = await window.electronAPI.getPlatform() to be
executed within the try block (or wrap that single call in its own try/catch) so
any thrown error from window.electronAPI or getPlatform is caught by the
existing error handling in the gifExporter logic.
In `@src/lib/exporter/videoExporter.ts`:
- Around line 533-538: getEncoderPreferences currently detects Windows via
navigator.userAgent which is inconsistent with the rest of the frame export
logic that uses window.electronAPI.getPlatform(); update getEncoderPreferences
to call window.electronAPI.getPlatform() when available (fall back to
navigator.userAgent or a default) and base the Windows check on that result so
the platform detection is unified across the codebase, keeping the existing
return order for ["prefer-software","prefer-hardware"] on Windows and the
opposite otherwise.
- Around line 114-115: The call to window.electronAPI.getPlatform() is currently
outside any try-catch, so if it throws the exception won't be handled; move or
re-invoke this call inside the existing try-catch block that surrounds the
export logic (or add a small try-catch specifically around the const platform =
await window.electronAPI.getPlatform() line) so errors from getPlatform() are
caught and logged/handled consistently with the rest of the export flow
(reference the const platform assignment and the surrounding export routine in
videoExporter.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0e6c0c4-e546-462a-b20f-2c3f03b3dbd6
📒 Files selected for processing (3)
src/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/exporter/frameRenderer.ts
…back also for canvas composition for non-linux
theopfr
force-pushed
the
fix/cpu-readback-only-for-linux
branch
from
bd090f0 to
3445810
Compare
Owner
|
@theopfr can you take a look at the ci failures |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/platformUtils.ts (1)
14-21:⚠️ Potential issue | 🟡 Minorfallback doesn't detect linux, which could cause the readback workaround to be skipped
if
electronAPI.getPlatform()fails (e.g., during browser-based dev/testing on linux), the fallback only checks for macOS and defaults to"win32". this means linux users in fallback mode would hit the directVideoFrame(canvas)path, potentially triggering the GPU shared-image bug the PR is trying to work around.might want to add linux detection here too:
🛠️ suggested fix
} catch (error) { console.warn("Failed to get platform from Electron, falling back to navigator:", error); // Fallback for development/testing let fallbackPlatform = "win32"; - if (typeof navigator !== "undefined" && /Mac|iPhone|iPad|iPod/.test(navigator.platform)) { + if (typeof navigator !== "undefined") { + if (/Linux/.test(navigator.platform)) { + fallbackPlatform = "linux"; + } else if (/Mac|iPhone|iPad|iPod/.test(navigator.platform)) { fallbackPlatform = "darwin"; + } } cachedPlatform = fallbackPlatform; return fallbackPlatform; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/platformUtils.ts` around lines 14 - 21, The fallback logic in the getPlatform fallback block (where cachedPlatform is set) only detects macOS and otherwise defaults to "win32", which misses Linux; update that block in the function that calls electronAPI.getPlatform() (referencing cachedPlatform and the existing navigator check) to also detect Linux (e.g., check navigator.platform or navigator.userAgent for "Linux" or "X11") and set fallbackPlatform to "linux" when matched so Linux clients use the correct readback workaround instead of defaulting to "win32".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/videoExporter.ts`:
- Around line 248-263: The code assumes a non-null 2D context by calling
canvas.getContext("2d")! before using getImageData which can throw if
readbackVideoCanvas() returned a WebGL canvas; update the block around
canvas.getContext("2d") in videoExporter.ts to check for a valid 2D context
(e.g., const canvasCtx = canvas.getContext("2d"); if (!canvasCtx) throw new
Error("Expected 2D canvas context for video readback")) before calling
getImageData and creating the VideoFrame, or alternatively ensure the canvas
returned by readbackVideoCanvas() is 2D-compatible earlier (reference
readbackVideoCanvas(), getContext("2d"), and the VideoFrame creation block).
---
Outside diff comments:
In `@src/utils/platformUtils.ts`:
- Around line 14-21: The fallback logic in the getPlatform fallback block (where
cachedPlatform is set) only detects macOS and otherwise defaults to "win32",
which misses Linux; update that block in the function that calls
electronAPI.getPlatform() (referencing cachedPlatform and the existing navigator
check) to also detect Linux (e.g., check navigator.platform or
navigator.userAgent for "Linux" or "X11") and set fallbackPlatform to "linux"
when matched so Linux clients use the correct readback workaround instead of
defaulting to "win32".
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85c7de26-0106-40e5-8c3f-d377cb32a2c5
📒 Files selected for processing (3)
src/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.tssrc/utils/platformUtils.ts
Comment on lines
+248
to
+263
| if (platform === "linux") { | ||
| const canvasCtx = canvas.getContext("2d")!; | ||
| const imageData = canvasCtx.getImageData(0, 0, canvas.width, canvas.height); | ||
| exportFrame = new VideoFrame(imageData.data.buffer, { | ||
| format: "RGBA", | ||
| codedWidth: canvas.width, | ||
| codedHeight: canvas.height, | ||
| timestamp, | ||
| duration: frameDuration, | ||
| colorSpace: { | ||
| primaries: "bt709", | ||
| transfer: "iec61966-2-1", | ||
| matrix: "rgb", | ||
| fullRange: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how FrameRenderer acquires/uses the canvas context
ast-grep --pattern $'getContext($$$)'Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
# Find FrameRenderer class definition
rg "class FrameRenderer" -A 5Repository: siddharthvaddem/openscreen
Length of output: 548
🏁 Script executed:
# Search for readbackVideoCanvas function mentioned in the summary
rg "readbackVideoCanvas" -B 2 -A 10Repository: siddharthvaddem/openscreen
Length of output: 1810
🏁 Script executed:
# Also look for where canvas context is initialized in videoExporter
rg "getContext" -B 2 -A 2Repository: siddharthvaddem/openscreen
Length of output: 3212
non-null assertion on getContext("2d") is risky
the canvas from readbackVideoCanvas() could be a WebGL canvas if the raster canvas fails to initialize or isn't available. in that case, getContext("2d") returns null, and the ! blows up on the next line when calling getImageData().
even though FrameRenderer throws if rasterCtx initialization fails, the fallback to glCanvas in readbackVideoCanvas() means the 2D context isn't guaranteed. either:
- add a guard:
const canvasCtx = canvas.getContext("2d"); if (!canvasCtx) throw new Error(...) - or ensure the canvas is always 2D-compatible before this point
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/exporter/videoExporter.ts` around lines 248 - 263, The code assumes a
non-null 2D context by calling canvas.getContext("2d")! before using
getImageData which can throw if readbackVideoCanvas() returned a WebGL canvas;
update the block around canvas.getContext("2d") in videoExporter.ts to check for
a valid 2D context (e.g., const canvasCtx = canvas.getContext("2d"); if
(!canvasCtx) throw new Error("Expected 2D canvas context for video readback"))
before calling getImageData and creating the VideoFrame, or alternatively ensure
the canvas returned by readbackVideoCanvas() is 2D-compatible earlier (reference
readbackVideoCanvas(), getContext("2d"), and the VideoFrame creation block).
There was a problem hiding this comment.
@theopfr, fair point — I misread the canvas source. The canvas here comes from renderer.getCanvas() which is the composite output canvas (always 2D), not the result of readbackVideoCanvas(). The non-null assertion concern doesn't apply. My bad!
🧠 Learnings used
Learnt from: theodorpeifer
Repo: siddharthvaddem/openscreen PR: 414
File: src/lib/exporter/streamingDecoder.ts:249-251
Timestamp: 2026-04-14T20:54:11.170Z
Learning: In `src/lib/exporter/streamingDecoder.ts` (TypeScript), `getExportMetrics()` intentionally computes `totalFrames` with a slight over-estimate (using `Math.ceil` with `EPSILON_SEC` subtracted per segment). This is a deliberate design trade-off: since metrics are computed before rendering starts, perfect prediction is impossible. A slightly high `totalFrames` (progress bar stops just below 100%) is acceptable and preferred over under-counting (progress bar exceeds 100%, the original bug).
Learnt from: zoharbabin
Repo: siddharthvaddem/openscreen PR: 415
File: src/components/video-editor/KalturaBrowseDialog.tsx:102-106
Timestamp: 2026-04-11T01:41:30.163Z
Learning: In `src/components/video-editor/KalturaBrowseDialog.tsx` and `electron/windows.ts`, the `createKalturaBrowseWindow` function creates a BrowserWindow with the full preload/electronAPI bridge attached. A remote Kaltura CDN ESM module is dynamically imported (`await import(loaderUrl)`) in this window. The author is aware of the security concern and has noted a future hardening plan to migrate to a `<webview>` tag or sandboxed BrowserWindow with no preload. `contextIsolation: true` is in place but does not prevent remote code from accessing `window.electronAPI`.
Learnt from: michTheBrandofficial
Repo: siddharthvaddem/openscreen PR: 450
File: src/components/launch/SourceSelector.tsx:68-68
Timestamp: 2026-04-15T13:59:58.679Z
Learning: In this codebase, Tailwind is configured such that `duration-*` classes (e.g., `duration-500`) produce both `transition-duration` and `animation-duration` rules. As a result, `duration-*` used together with `animate-*` utilities (e.g., `animate-spin`) is effective here and sets the animation duration (e.g., to 500ms). Do not flag occurrences of `duration-*` alongside `animate-*` as ineffective.
…so works in the browser
theopfr
force-pushed
the
fix/cpu-readback-only-for-linux
branch
from
9ae6247 to
9712eea
Compare
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
2 participants
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.
Pull Request Template
Description
Because of a potential bug on some Linux systems we were not able to pass the canvas directly to
VideoFrame(...). There was a workaround implemented to read the raw pixels from the canvas usinggetImageData()for every single frame which required a CPU readback and reduces performance. After this PR, this workaround is only used on Linux systems, to allow better performance at least for other OS'.Motivation
Rendering can take quite long. The current CPU readback workaround makes it take even longer and is not necessary for win and darwin systems.
Here is a quick benchmark I did (duration in ms):
Info about the video used:
So for this video about a 30% speed up. I've seen similar speed ups with other videos as well. Additionally, there is this
willReadFrequentlyattribute tocanvas.getContext. Since for Linux we do read frequently from the canvas, I set it there to betrue.Type of Change
Related Issue(s)
Screenshots / Video
Testing
You can time how long a video takes to export with and without the PR. A way to do that is to drop in this snippet:
... where
this.exportWithEncoderPreferenceis currently being called and check the console (CTRL+SHIFT+I).Checklist
Thank you for contributing!
Summary by CodeRabbit
Performance Improvements
Bug Fixes