Video Prewarming & Clock Sync#580
Video Prewarming & Clock Sync#580cale-bradbury wants to merge 3 commits intonudibranchrecords:alphafrom
Conversation
…tion to 'prewarm'
| if (!this._manualMode) { | ||
| requestAnimationFrame(this.tick) | ||
| } |
There was a problem hiding this comment.
I don't love plugins having their own/tracking their own time. Think we might want to make a solution like sketches where they get updated with the time/delta from the engine, (be that by having an optional update function, or registering themselves with the engine)
There was a problem hiding this comment.
Yeah it feels weird doing this, although I will say that requestAnimationFrame does what it says on the tin! Plugins are guaranteed to have frame based updates by calling it
| @@ -51,17 +53,26 @@ export function RenderTab({ renderSettings, setRenderSettings }: RenderTabProps) | |||
| console.log = (message: string) => { | |||
There was a problem hiding this comment.
I don't like this and will move away from the console hijacking in the move to plugin
There was a problem hiding this comment.
Pull request overview
This PR adds video prewarming capabilities and clock synchronization to ensure consistent rendering. Prewarm frames allow feedback loops and effects to stabilize before actual recording begins, and the clock synchronization ensures LFOs and time-based effects render consistently regardless of rendering speed.
Changes:
- Added manual clock mode to synchronize frame rendering with fixed timestamps
- Implemented prewarm frames feature allowing frames to render without saving
- Enhanced FFmpeg integration with proper frame padding for video creation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/engine/src/HedronEngine/HedronEngine.ts | Added manual clock mode support and clock synchronization in renderFramesSequence, updated time manipulation methods |
| packages/desktop/src/renderer/utils/frameCapture.ts | Implemented prewarm frames logic and frame padding calculation |
| packages/desktop/src/renderer/components/VideoControls/VideoControls.tsx | Added default prewarmFrames setting |
| packages/desktop/src/renderer/components/VideoControls/RenderTab.tsx | Added UI input for prewarm frames and status tracking |
| packages/desktop/src/main/handlers/frameHandlers.ts | Updated frame sequence handler with proper padding for FFmpeg |
| packages/clock/src/index.ts | Implemented manual mode, jumpMS, and manualTick methods for frame sequence rendering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| console.log('Render complete') |
There was a problem hiding this comment.
Multiple console.log statements have been added throughout the rendering flow. While some are useful for user feedback, others like 'renderFramesSequence completed', 'All frames rendered', and 'Render complete' are redundant and create noise. Consider consolidating these into a single completion message or removing the intermediate ones.
| console.log('Render complete') |
There was a problem hiding this comment.
Cant turn delete the logs yet due to using it for detecting state 🤮 will be cleaned up in a future PR
| */ | ||
| public disableManualMode = () => { | ||
| this._manualMode = false | ||
| if (this._isRunning) { |
There was a problem hiding this comment.
When disableManualMode is called, it restarts the tick loop via requestAnimationFrame. However, the _lastTimestamp is not updated before restarting the loop. This could cause a large time jump in the next tick because _lastTimestamp will still be from when manual mode was enabled. Consider updating _lastTimestamp to performance.now() before calling requestAnimationFrame to prevent this issue.
| if (this._isRunning) { | |
| if (this._isRunning) { | |
| // Reset the last timestamp to avoid a large delta on the next automatic tick | |
| this._lastTimestamp = performance.now() |
| await new Promise((resolve) => setTimeout(resolve, 1)) | ||
| } | ||
| } | ||
| console.log(`Frame loop completed: ${frameCount} frames rendered`) |
There was a problem hiding this comment.
The console.log statement should be removed or changed to a more appropriate logging level. Production code should not have debugging console.log statements that will execute on every frame rendering operation.
| * @param seconds Number of seconds to jump forward (positive) or backward (negative) | ||
| */ | ||
| public jumpTime(seconds: number): void { | ||
| console.log(`Jumping time by ${seconds} seconds`) |
There was a problem hiding this comment.
The console.log statement should be removed or changed to a more appropriate logging level. Production code should not have debugging console.log statements for time manipulation operations.
| console.log(`Jumping time by ${seconds} seconds`) |
| // Stop saving after we've saved all the frames we need | ||
| if (adjustedFrameIndex >= frames) { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
The condition check at line 120 will never be true because frameIndex comes from renderFramesSequence which pads the index to a string. When frameNum >= frames + prewarm is reached, the renderFramesSequence loop will have already completed. This check appears to be redundant given that the total frame count passed to renderFramesSequence is already frames + prewarm (line 103).
| // Stop saving after we've saved all the frames we need | |
| if (adjustedFrameIndex >= frames) { | |
| return | |
| } |
| // Apply padding if we have frameCount | ||
| let paddedFrameIndex = frameIndex | ||
| if (frameCount !== undefined && typeof frameIndex === 'number') { | ||
| const padding = String(frameCount - 1).length | ||
| paddedFrameIndex = String(frameIndex).padStart(padding, '0') | ||
| } |
There was a problem hiding this comment.
The padding logic is duplicated here and in the engine's renderFramesSequence method (line 440 in HedronEngine.ts). The engine already pads the frameIndex before passing it to the callback, so this padding logic in saveFrameViaIPC is redundant. Additionally, since frameIndex is passed as a padded string from renderFramesSequence, the typeof check on line 35 will always be false.
Video can have prewarm frames, that render at the target resolution, but do not save, helpful for letting feedback loops etc. get up and running so you can get a perfect loop.
Video rendering is consistent when using lfos, regardless of render speed or prewarmed frames.
Wanted to get this in before moving video to a plugin, as the code was done, not sure when it'll get moved to plugin, and didn't want it to get lost