fix: resolve green MP4 exports on CachyOS/Arch Linux (Wayland)#330
fix: resolve green MP4 exports on CachyOS/Arch Linux (Wayland)#330maxbailey wants to merge 1 commit intosiddharthvaddem:mainfrom
Conversation
On Linux/Wayland the implicit GPU-to-2D texture-sharing path used by drawImage(webglCanvas) fails silently (EGL/Ozone), producing green frames. Use explicit gl.readPixels to copy from GPU to CPU memory, bypassing that path.
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b5ad5064e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!this.compositeCanvas || !this.compositeCtx || !this.app) return; | ||
|
|
||
| const videoCanvas = this.app.canvas as HTMLCanvasElement; | ||
| const videoCanvas = this.readbackVideoCanvas(); |
There was a problem hiding this comment.
compositeWithShadows now always uses readbackVideoCanvas(), which in turn calls gl.readPixels(...) on every frame when WebGL is available. That introduces a synchronous GPU→CPU transfer for all exports, not just Linux/Wayland, and can heavily degrade throughput on common paths (especially high-resolution exports). Since this commit is a platform-specific workaround, the readback path should be conditional/opt-in instead of unconditional.
Useful? React with 👍 / 👎.
|
|
||
| const w = glCanvas.width; | ||
| const h = glCanvas.height; | ||
| const buf = new Uint8Array(w * h * 4); |
There was a problem hiding this comment.
This allocates a fresh Uint8Array sized w*h*4 for every frame, which creates very large transient allocations during export (about 32MB per frame at 4K). Over long videos this causes substantial GC pressure and memory spikes, slowing or destabilizing exports. Keep a persistent pixel buffer (and related ImageData) keyed by dimensions to avoid per-frame heap churn.
Useful? React with 👍 / 👎.
![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
🤖 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 704-719: The current path in compositeWithShadows()
unconditionally calls readbackVideoCanvas() which always does a gl.readPixels()
into a freshly allocated Uint8Array and temp buffer then flips rows and
putImageData, causing synchronous stalls and per-frame allocations; change this
so the default path uses the existing drawImage() branch and only uses the
readbackVideoCanvas() code when a platform/workaround flag (detect affected
Wayland/Linux cases) is set, and when readback is enabled reuse buffers across
frames by promoting buf and the row temp buffer to reusable properties (cache a
Uint8Array for pixel data and a row-sized temp Uint8Array) instead of allocating
them inside readbackVideoCanvas(), and avoid repeated ImageData allocations by
reusing a single ImageData / Uint8ClampedArray view where possible before
calling this.rasterCtx.putImageData; keep references to compositeWithShadows()
and readbackVideoCanvas() to locate the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e8c45f0-1a34-4572-83fa-b2fb6c7b55f8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/lib/exporter/frameRenderer.ts
| const buf = new Uint8Array(w * h * 4); | ||
| gl.readPixels(0, 0, w, h, gl.RGBA, gl.UNSIGNED_BYTE, buf); | ||
|
|
||
| // readPixels returns rows bottom-to-top; flip vertically | ||
| const rowSize = w * 4; | ||
| const temp = new Uint8Array(rowSize); | ||
| for (let top = 0, bot = h - 1; top < bot; top++, bot--) { | ||
| const tOff = top * rowSize; | ||
| const bOff = bot * rowSize; | ||
| temp.set(buf.subarray(tOff, tOff + rowSize)); | ||
| buf.copyWithin(tOff, bOff, bOff + rowSize); | ||
| buf.set(temp, bOff); | ||
| } | ||
|
|
||
| const imageData = new ImageData(new Uint8ClampedArray(buf.buffer), w, h); | ||
| this.rasterCtx.putImageData(imageData, 0, 0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
sed -n '700,750p' src/lib/exporter/frameRenderer.tsRepository: siddharthvaddem/openscreen
Length of output: 1611
🏁 Script executed:
rg -n "readbackVideoCanvas" src/lib/exporter/frameRenderer.tsRepository: siddharthvaddem/openscreen
Length of output: 179
🏁 Script executed:
rg -n "drawImage" src/lib/exporter/frameRenderer.ts -B 2 -A 2Repository: siddharthvaddem/openscreen
Length of output: 1359
🏁 Script executed:
rg -n "compositeWithShadows" src/lib/exporter/frameRenderer.ts -B 3 -A 3Repository: siddharthvaddem/openscreen
Length of output: 542
🏁 Script executed:
sed -n '680,800p' src/lib/exporter/frameRenderer.tsRepository: siddharthvaddem/openscreen
Length of output: 4173
Don't make CPU readback the default export path.
compositeWithShadows() is called unconditionally on every frame (line 410), and within it line 727 unconditionally routes through readbackVideoCanvas(), which allocates and flips a full RGBA buffer on each call. This turns a Linux/Wayland workaround (documented at lines 688–691) into the global path, introducing a synchronous gl.readPixels() stall plus ~8 MiB/frame at 1080p and ~33 MiB/frame at 4K, with no buffer reuse. Keep the old drawImage() path as the default and only enable readback for affected environments, reusing buffers once that path is active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/exporter/frameRenderer.ts` around lines 704 - 719, The current path
in compositeWithShadows() unconditionally calls readbackVideoCanvas() which
always does a gl.readPixels() into a freshly allocated Uint8Array and temp
buffer then flips rows and putImageData, causing synchronous stalls and
per-frame allocations; change this so the default path uses the existing
drawImage() branch and only uses the readbackVideoCanvas() code when a
platform/workaround flag (detect affected Wayland/Linux cases) is set, and when
readback is enabled reuse buffers across frames by promoting buf and the row
temp buffer to reusable properties (cache a Uint8Array for pixel data and a
row-sized temp Uint8Array) instead of allocating them inside
readbackVideoCanvas(), and avoid repeated ImageData allocations by reusing a
single ImageData / Uint8ClampedArray view where possible before calling
this.rasterCtx.putImageData; keep references to compositeWithShadows() and
readbackVideoCanvas() to locate the change.
1 participant
Description
On Linux/Wayland (specifically CachyOS/Arch with KDE), exported MP4 videos are solid green despite audio working correctly. This fix uses explicit
gl.readPixels()to copy pixel data from the WebGL framebuffer to CPU memory, bypassing the broken shared-image path entirely.Motivation
PR #281 resolved green MP4 exports on Ubuntu by reading raw pixels at the
VideoFrameconstruction step. However, on CachyOS/Arch Linux with Wayland/KDE, the corruption occurs one layer deeper inFrameRenderer.compositeWithShadows(), wherectx.drawImage(webglCanvas, ...)copies the PixiJS WebGL canvas onto the 2D composite canvas. By the time theVideoFramefix reads pixels, the composite canvas already contains corrupted data.This PR fixes the compositing layer by reading back the WebGL framebuffer directly via
gl.readPixels(), which always performs an explicit GPU-to-CPU copy.Type of Change
Related Issue(s)
#264
Checklist
Summary by CodeRabbit