fix: resolve blurry screen recordings and video editor previews#419
fix: resolve blurry screen recordings and video editor previews#419siddharthvaddem merged 4 commits intosiddharthvaddem:mainfrom
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMotion-blur is toggled at runtime inside the Pixi ticker in the video playback component; screen-recorder mime/codec preference now prefers h264 before vp8/vp9/av1. The rest of the diff is reordered named exports across UI components and small build/config include and comment tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 7774a78898
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/VideoPlayback.tsx (1)
67-103:⚠️ Potential issue | 🔴 Criticalcritical: blur props removed but VideoEditor still passes them
VideoPlayback no longer accepts
blurRegions,selectedBlurId,onSelectBlur,onBlurPositionChange,onBlurSizeChange,onBlurDataChange, oronBlurDataCommit—but VideoEditor.tsx is still passing all of these at lines 1811-1817, 1876, 1880-1881, and 1982-1983. this will cause TypeScript errors.either add these props back to VideoPlayback (if blur handling should stay there) or remove all these prop passes from VideoEditor. looks like blur got moved elsewhere though, so you probably want to just strip them all out of the VideoEditor call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoPlayback.tsx` around lines 67 - 103, The VideoPlayback component signature no longer includes blur-related props (blurRegions, selectedBlurId, onSelectBlur, onBlurPositionChange, onBlurSizeChange, onBlurDataChange, onBlurDataCommit), but VideoEditor is still passing them into the <VideoPlayback ... /> call; remove all these blur prop passes from the VideoPlayback invocations in VideoEditor (or alternatively restore the props on VideoPlayback if blur handling should remain there). Locate the <VideoPlayback> usage in VideoEditor and delete each of the following prop assignments: blurRegions, selectedBlurId, onSelectBlur, onBlurPositionChange, onBlurSizeChange, onBlurDataChange, onBlurDataCommit so the props match the VideoPlaybackProps interface.
🧹 Nitpick comments (1)
electron-builder.json5 (1)
1-39: this config overhaul seems unrelated to the PR objectivethe PR description mentions fixing blurry recordings via codec changes (H.264 preference) and PixiJS filter removal. this electron-builder rewrite is... kinda tangential? removing wallpaper resources, changing build targets, and updating appId don't really connect to video blur fixes.
might be worth splitting this into a separate PR to keep the blur fix atomic and reviewable. bundling unrelated config changes makes it harder to bisect if something breaks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron-builder.json5` around lines 1 - 39, This PR mixes unrelated packaging changes with the blur-fix; revert the unrelated edits in the current change and move them to a separate PR: restore the original values for electron-builder config keys modified here (appId, productName, directories.output/buildResources, files list, mac.identity, mac.artifactName/mac.target and arch, mac.category, linux.target/linux.category, win.target/arch) so this PR only includes the H.264 preference and PixiJS filter removal, then create a new PR that contains the full electron-builder.json5 overhaul (the packaging changes) for independent review and testing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron-builder.json5`:
- Around line 13-23: The mac build config currently uses "target": "dir" with
"arch": ["arm64"] and "identity": null which produces only an unpacked app for
arm64 and disables code signing; change the "mac" config to include proper
distributable targets (e.g., "dmg" and/or "zip" in the "target" array instead of
or in addition to "dir"), broaden "arch" to include "x64" or use "universal" so
Intel Macs are supported, and remove or replace "identity": null with the
correct signing identity or configuration for CI signing (or add a conditional
that keeps null only for local testing). Ensure artifactName remains appropriate
for the chosen targets.
- Line 3: The appId value ("appId") was changed from
com.siddharthvaddem.openscreen to com.openscreen.app which will cause OSs to
treat installs as a new app (losing preferences, app data, and permissions);
either revert the change in electron-builder.json5 to the original appId to
preserve continuity or, if this rebrand is intentional, update the
release/migration plan and repository docs to include steps for migrating user
data and re-requesting platform permissions (macOS screen-recording entitlement,
Windows AppUserModelID, etc.) and ensure all build configs and entitlements use
the new appId consistently.
- Around line 9-12: The packaged app no longer includes wallpapers at
process.resourcesPath/assets because extraResources was removed from
electron-builder.json5 and Vite now places public/wallpapers into
dist/wallpapers, causing electron/ipc/handlers.ts (which expects
process.resourcesPath/assets/) to miss them; fix by either restoring
extraResources with an assets mapping so wallpapers are copied to
process.resourcesPath/assets, or update the handler in electron/ipc/handlers.ts
to check process.resourcesPath/dist/wallpapers (or both locations) and/or change
the Vite build output so wallpapers land at the expected assets path; update
only the relevant config (electron-builder.json5 "extraResources") or the
handler path resolution logic to normalize and look for wallpapers under
dist/wallpapers when packaged.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1015-1026: The conditional that applies Pixi filters wrongly
includes showBlurRef, causing CSS background blur to trigger Pixi's offscreen
RenderTexture; change the logic in the block that uses videoContainerRef,
blurFilterRef, motionBlurFilterRef so only motion blur controls Pixi filters
(i.e., use isMotionBlurActive/motionBlurAmountRef and isPlayingRef) and remove
showBlurRef from the condition; ensure that when motion blur is inactive you
clear videoContainerRef.filters (set to null) so Pixi does not intercept native
rendering, or if there is a separate intended video blur effect add a distinct
flag/function (e.g., videoBlurActive) and gate Pixi filter application on that
instead of showBlurRef.
- Around line 1331-1332: The overlay's clientWidth/clientHeight are read
directly during render (containerWidth={overlayRef.current?.clientWidth || 800}
/ containerHeight={overlayRef.current?.clientHeight || 600}), so resizes don't
trigger React re-renders and annotations can be stale; fix by tracking overlay
dimensions in state (e.g., overlayWidth, overlayHeight) and updating them inside
the existing ResizeObserver callback (the one that calls layoutVideoContent())
or a new resize handler, then pass those state values to the annotation overlay
instead of reading overlayRef.current directly; update any references in
VideoPlayback (overlayRef, layoutVideoContent, and the annotation container
props) to use the state so pure resizes force a re-render.
---
Outside diff comments:
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 67-103: The VideoPlayback component signature no longer includes
blur-related props (blurRegions, selectedBlurId, onSelectBlur,
onBlurPositionChange, onBlurSizeChange, onBlurDataChange, onBlurDataCommit), but
VideoEditor is still passing them into the <VideoPlayback ... /> call; remove
all these blur prop passes from the VideoPlayback invocations in VideoEditor (or
alternatively restore the props on VideoPlayback if blur handling should remain
there). Locate the <VideoPlayback> usage in VideoEditor and delete each of the
following prop assignments: blurRegions, selectedBlurId, onSelectBlur,
onBlurPositionChange, onBlurSizeChange, onBlurDataChange, onBlurDataCommit so
the props match the VideoPlaybackProps interface.
---
Nitpick comments:
In `@electron-builder.json5`:
- Around line 1-39: This PR mixes unrelated packaging changes with the blur-fix;
revert the unrelated edits in the current change and move them to a separate PR:
restore the original values for electron-builder config keys modified here
(appId, productName, directories.output/buildResources, files list,
mac.identity, mac.artifactName/mac.target and arch, mac.category,
linux.target/linux.category, win.target/arch) so this PR only includes the H.264
preference and PixiJS filter removal, then create a new PR that contains the
full electron-builder.json5 overhaul (the packaging changes) for independent
review and testing.
🪄 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: c8438d75-24a0-45aa-a8b0-63a61063d6fc
📒 Files selected for processing (3)
electron-builder.json5src/components/video-editor/VideoPlayback.tsxsrc/hooks/useScreenRecorder.ts
rajtiwariee
force-pushed
the
fix/video-blur
branch
from
7774a78 to
90d04c7
Compare
|
Reviewed, this fixes the issue on my Mac as well where the video editor was blurry after screen recording |
|
@rajtiwariee will merge once the linting is fixed |
Will fix the linting issue |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/video-editor/VideoPlayback.tsx (1)
1083-1095: motion blur gating looks good, tiny perf nitthe fix addressed the earlier concern —
showBlurRefis out, and pixi filters are now driven purely byisMotionBlurActive. nice, that should stop the offscreen RenderTexture downscaling path from kicking in when only CSS bg blur is on.one small 2am thought though: this runs every ticker frame and allocates a fresh
[blurFilter, motionBlur]array (or reassignsnull) on every tick, even when the active state hasn't changed. pixi v8 treats thefilterssetter as a state change, so you're lowkey churning the filter pipeline ~60x/sec. not a bug, just wasteful. caching the last applied state and only writing on transitions would be cleaner:♻️ optional: only toggle on state change
+ let lastMotionBlurActive: boolean | null = null; const ticker = () => { // ... existing logic ... const isMotionBlurActive = (motionBlurAmountRef.current || 0) > 0 && isPlayingRef.current; - if ( - isMotionBlurActive && - blurFilterRef.current && - motionBlurFilterRef.current && - videoContainerRef.current - ) { - videoContainerRef.current.filters = [blurFilterRef.current, motionBlurFilterRef.current]; - } else if (videoContainerRef.current) { - videoContainerRef.current.filters = null; - } + if (isMotionBlurActive !== lastMotionBlurActive && videoContainerRef.current) { + if ( + isMotionBlurActive && + blurFilterRef.current && + motionBlurFilterRef.current + ) { + videoContainerRef.current.filters = [ + blurFilterRef.current, + motionBlurFilterRef.current, + ]; + } else { + videoContainerRef.current.filters = null; + } + lastMotionBlurActive = isMotionBlurActive; + } };also heads up — the cleanup at line 859 uses
videoContainer.filters = []while here you're usingnull. both work in pixi v8 but picking one is kinda nicer for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoPlayback.tsx` around lines 1083 - 1095, The ticker currently reassigns videoContainerRef.current.filters every frame (allocating a new array or null) even when state hasn't changed; introduce a cached state (e.g., lastIsMotionBlurActive or lastAppliedFiltersRef) and only set videoContainerRef.current.filters to [blurFilterRef.current, motionBlurFilterRef.current] or null when that cached state differs from the computed isMotionBlurActive to avoid churning Pixi's filter pipeline in the ticker; also normalize the "empty" cleanup value to match the other usage (choose either [] or null and use it consistently where videoContainer.filters is cleared) so cleanup and runtime behavior are consistent.
🤖 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/components/video-editor/VideoPlayback.tsx`:
- Around line 1083-1095: The ticker currently reassigns
videoContainerRef.current.filters every frame (allocating a new array or null)
even when state hasn't changed; introduce a cached state (e.g.,
lastIsMotionBlurActive or lastAppliedFiltersRef) and only set
videoContainerRef.current.filters to [blurFilterRef.current,
motionBlurFilterRef.current] or null when that cached state differs from the
computed isMotionBlurActive to avoid churning Pixi's filter pipeline in the
ticker; also normalize the "empty" cleanup value to match the other usage
(choose either [] or null and use it consistently where videoContainer.filters
is cleared) so cleanup and runtime behavior are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fc4fd8d-0761-4a03-b0c0-d5ff4dbe67f4
📒 Files selected for processing (11)
src/components/ui/accordion.tsxsrc/components/ui/card.tsxsrc/components/ui/dialog.tsxsrc/components/ui/dropdown-menu.tsxsrc/components/ui/popover.tsxsrc/components/ui/select.tsxsrc/components/ui/tabs.tsxsrc/components/ui/tooltip.tsxsrc/components/video-editor/VideoPlayback.tsxtsconfig.node.jsonvite.config.ts
✅ Files skipped from review due to trivial changes (10)
- src/components/ui/accordion.tsx
- src/components/ui/tabs.tsx
- src/components/ui/card.tsx
- tsconfig.node.json
- src/components/ui/tooltip.tsx
- src/components/ui/dropdown-menu.tsx
- src/components/ui/select.tsx
- src/components/ui/dialog.tsx
- src/components/ui/popover.tsx
- vite.config.ts
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/components/video-editor/VideoPlayback.tsx`:
- Around line 1085-1097: The bug is that lastMotionBlurActive is set to
isMotionBlurActive even when filters weren't applied due to missing refs, which
can permanently prevent re-applying them; in the effect surrounding
isMotionBlurActive, only update lastMotionBlurActive after you actually change
videoContainerRef.filters: when entering the "apply" branch, check
blurFilterRef.current and motionBlurFilterRef.current and set filters then set
lastMotionBlurActive = true; in the "clear" branch set filters = null and then
set lastMotionBlurActive = false; alternatively compute the actualApplied =
isMotionBlurActive && blurFilterRef.current && motionBlurFilterRef.current and
assign lastMotionBlurActive = actualApplied so the cached flag reflects the real
filter state (references: lastMotionBlurActive, isMotionBlurActive,
blurFilterRef, motionBlurFilterRef, videoContainerRef).
🪄 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: cd93fa14-3e1c-457b-a288-dc73a465f4ae
📒 Files selected for processing (1)
src/components/video-editor/VideoPlayback.tsx
3 participants
Pull Request Template
Description
This PR resolves two related bugs causing blurry video output during both real-time capture and playback inside the editor. It forces the
MediaRecorderto prioritize hardware-accelerated codecs (H.264) and removes static visual filters in PixiJS that were destroying canvas resolution via off-screen render texture downscaling.Motivation
Users on high-resolution displays (like M-series Macs) were experiencing severe recording lag/blurriness due to AV1 software encoding bottlenecking. Furthermore, even perfectly captured videos appeared pixelated and blurry when scaled down to fit the window during editor playback, rendering screen recordings of code or text illegible.
Type of Change
Related Issue(s)
#381 (comment)
Testing
video/webm;codecs=h264ensuring 60fps captures without dropped frames.Summary by CodeRabbit
Bug Fixes
Chores