Canvas renderer and exporter #574
Conversation
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@roprgm is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| const loop = (time: number) => { | ||
| if (previousTimeRef.current !== undefined) { | ||
| const deltaTime = time - previousTimeRef.current; | ||
| callback(deltaTime); |
There was a problem hiding this comment.
The callback receives deltaTime but the canvas preview expects the callback to receive no arguments, causing incorrect time calculations in the renderer.
View Details
Analysis
The useRafLoop hook calls the callback with deltaTime (the difference between current and previous frame times), but the render callback in canvas-preview-panel.tsx expects no arguments and gets the current time from usePlaybackStore.getState().currentTime.
This mismatch means the render function signature doesn't match what useRafLoop provides. The callback should either:
-
Receive the actual
requestAnimationFrametime instead of deltaTime:callback(time); // instead of callback(deltaTime)
-
Or the canvas preview should adapt to use deltaTime for frame calculations
Currently, the render function ignores the deltaTime parameter entirely and fetches time independently, which could lead to timing inconsistencies between the RAF loop and the playback store state.
| setSize(width: number, height: number) { | ||
| this.canvas = new OffscreenCanvas(width, height); | ||
| this.width = width; | ||
| this.height = height; | ||
| } |
There was a problem hiding this comment.
The setSize method creates a new OffscreenCanvas but doesn't update the context reference, causing the renderer to use a stale context from the old canvas.
View Details
📝 Patch Details
diff --git a/apps/web/src/lib/renderer/scene-renderer.ts b/apps/web/src/lib/renderer/scene-renderer.ts
index e4047c1..05b9759 100644
--- a/apps/web/src/lib/renderer/scene-renderer.ts
+++ b/apps/web/src/lib/renderer/scene-renderer.ts
@@ -34,6 +34,12 @@ export class SceneRenderer {
this.canvas = new OffscreenCanvas(width, height);
this.width = width;
this.height = height;
+
+ const context = this.canvas.getContext("2d");
+ if (!context) {
+ throw new Error("Failed to get canvas context");
+ }
+ this.context = context;
}
private clear() {
Analysis
In the setSize method, a new OffscreenCanvas is created and assigned to this.canvas, but the this.context property still references the rendering context from the previous canvas. This means any subsequent rendering operations will draw to the old canvas instead of the new one, causing rendering to fail or produce unexpected results.
The method needs to get the context from the new canvas:
setSize(width: number, height: number) {
this.canvas = new OffscreenCanvas(width, height);
this.width = width;
this.height = height;
const context = this.canvas.getContext("2d");
if (!context) {
throw new Error("Failed to get canvas context");
}
this.context = context;
}Without this fix, the clear() method and all rendering operations will fail because they're trying to draw on a detached context.
| import { useProjectStore } from "@/stores/project-store"; | ||
| import { buildScene } from "@/lib/renderer/build-scene"; | ||
|
|
||
| // TODO: get preview size in a better way |
There was a problem hiding this comment.
TODO comment indicates unfinished work for getting preview size in a better way.
View Details
📝 Patch Details
diff --git a/apps/web/src/components/editor/renderer/canvas-preview-panel.tsx b/apps/web/src/components/editor/renderer/canvas-preview-panel.tsx
index 79b932c..1a0502d 100644
--- a/apps/web/src/components/editor/renderer/canvas-preview-panel.tsx
+++ b/apps/web/src/components/editor/renderer/canvas-preview-panel.tsx
@@ -8,15 +8,14 @@ import { useMediaStore } from "@/stores/media-store";
import { usePlaybackStore } from "@/stores/playback-store";
import { useRendererStore } from "@/stores/renderer-store";
import { useTimelineStore } from "@/stores/timeline-store";
-import { useProjectStore } from "@/stores/project-store";
+import { useProjectStore, DEFAULT_CANVAS_SIZE } from "@/stores/project-store";
import { buildScene } from "@/lib/renderer/build-scene";
-// TODO: get preview size in a better way
function usePreviewSize() {
const { activeProject } = useProjectStore();
return {
- width: activeProject?.canvasSize?.width || 600,
- height: activeProject?.canvasSize?.height || 320,
+ width: activeProject?.canvasSize?.width || DEFAULT_CANVAS_SIZE.width,
+ height: activeProject?.canvasSize?.height || DEFAULT_CANVAS_SIZE.height,
};
}
Analysis
The code contains a TODO comment indicating that the current method of getting preview size needs improvement. This suggests the current implementation may not be robust or may have limitations that could affect functionality.
The current implementation falls back to hardcoded values (600x320) if the active project doesn't have canvas size defined, which may not be appropriate for all use cases and could lead to incorrect rendering dimensions.
Action needed: Address the TODO by implementing a more robust preview size determination method that handles edge cases properly.
| ) { | ||
| await this.render(node, frame); | ||
|
|
||
| const ctx = canvas.getContext("2d")!; |
There was a problem hiding this comment.
| const ctx = canvas.getContext("2d")!; | |
| const ctx = canvas.getContext("2d"); |
Contradictory logic: using non-null assertion operator but then checking for null in the same flow.
View Details
Analysis
The renderToCanvas method uses the non-null assertion operator (!) on line 56 to tell TypeScript that canvas.getContext("2d") will never return null, but then immediately checks if the result is null on lines 58-60. This is contradictory logic that creates confusion.
The non-null assertion bypasses TypeScript's null checking, so the subsequent null check will never be reached if the assertion is correct, or will cause a runtime error if the assertion is wrong.
Fix: Either remove the non-null assertion and keep the null check, or remove the null check if you're confident the context will always exist:
const ctx = canvas.getContext("2d");
if (!ctx) {
throw new Error("Failed to get canvas context");
}| return new SceneRenderer({ | ||
| width, | ||
| height, | ||
| fps: 30, // TODO: get fps from project |
There was a problem hiding this comment.
TODO comment indicates FPS should be retrieved from project settings instead of being hardcoded.
View Details
📝 Patch Details
diff --git a/apps/web/src/components/editor/renderer/canvas-preview-panel.tsx b/apps/web/src/components/editor/renderer/canvas-preview-panel.tsx
index 79b932c..6ec4d5e 100644
--- a/apps/web/src/components/editor/renderer/canvas-preview-panel.tsx
+++ b/apps/web/src/components/editor/renderer/canvas-preview-panel.tsx
@@ -20,6 +20,11 @@ function usePreviewSize() {
};
}
+function useProjectFps() {
+ const { activeProject } = useProjectStore();
+ return activeProject?.fps || 30;
+}
+
function RendererSceneController() {
const setScene = useRendererStore((s) => s.setScene);
@@ -53,14 +58,15 @@ function PreviewCanvas() {
const renderingRef = useRef(false);
const { width, height } = usePreviewSize();
+ const fps = useProjectFps();
const renderer = useMemo(() => {
return new SceneRenderer({
width,
height,
- fps: 30, // TODO: get fps from project
+ fps,
});
- }, [width, height]);
+ }, [width, height, fps]);
const scene = useRendererStore((s) => s.scene);
Analysis
The renderer is initialized with a hardcoded FPS value of 30, but there's a TODO comment indicating this should be retrieved from the project settings. This could lead to mismatched frame rates between the preview and the actual project configuration.
Hardcoding the FPS could cause issues where the preview renders at a different frame rate than intended, potentially affecting timing calculations and the accuracy of the preview relative to the final export.
Action needed: Address the TODO by implementing proper FPS retrieval from project settings to ensure consistency between preview and export.
1 participant
Description
New renderer based on canvas. Used for video export using Mediabunny.
TODO: Clean code
TODO: Export audio using AudioContext.
Fixes #564
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Screenshots (if applicable)
Export dialog:
Export flow example:
https://github.com/user-attachments/assets/7033caba-6191-42fa-a4fd-9d43a75d5852
Checklist:
Additional context
Add any other context about the pull request here.