Some fixes identified in another PR #556
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUpdates publisher keyframe trigger to include the initial frame and make GOP boundary check inclusive. Adjusts watch-side imports to mark Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Src as VideoSource
participant Pub as Publisher(Video)
participant Enc as Encoder
Note over Pub,Enc: Keyframe decision updated (initial frame + inclusive GOP boundary)
Src->>Pub: frame(timestamp)
Pub->>Pub: if !group || groupTs + GOP_US <= frame.ts ⇒ keyframe=true
Pub->>Enc: encode(frame, { keyframe })
Enc-->>Pub: encoded chunk
sequenceDiagram
autonumber
participant Stream as VideoStream
participant Ren as Renderer
participant Canvas as Canvas2D
Stream->>Ren: provide video frame (optional)
Ren->>Canvas: save()
alt no frame
Ren->>Canvas: fillRect(black, 0,0,w,h)
else frame available
Ren->>Ren: if displayWidth/Height changed ⇒ resize canvas
opt horizontal flip
Ren->>Canvas: transform(scale(-1,1) + translate(-canvas.width,0))
end
Ren->>Canvas: fillRect(black, 0,0,w,h)
Ren->>Canvas: drawImage(frame)
end
Ren->>Canvas: restore()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
|
Claude finished @kixelated's task —— View job Code Review CompleteReviewed the 3 changed files in this "misc fix" PR for bugs and security issues: js/hang/src/publish/video/index.ts - Added better frame cleanup in the signal setter Issues Found🚨 Security Issue: Line 32 in 🐛 Potential Bug: Line 112 in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/hang/src/publish/video/index.ts (1)
164-166: Guard for undefined timestamps to avoid never-rotating GOPsSome
VideoFramesources can yieldframe.timestamp === undefined. In that case, the time-based check always evaluates false, and the GOP may never rotate after the initial group. Consider a count-based fallback keyed to the track framerate.Suggested change in-place:
-// Force a keyframe if this is the first frame (no group yet), or GOP elapsed. -const keyFrame = !group || groupTimestamp + GOP_DURATION_US <= frame.timestamp; +// Force a keyframe if this is the first frame (no group yet), or GOP elapsed by time or frame count. +const ts = frame.timestamp; +const elapsedByTime = ts !== undefined && (groupTimestamp + GOP_DURATION_US) <= ts; +const keyFrame = !group || elapsedByTime || framesSinceKey >= Math.max(1, Math.round(settings.frameRate * (GOP_DURATION_US / 1_000_000))); +if (keyFrame) framesSinceKey = 0; else framesSinceKey++;And define the counter near the loop setup:
// just before the while-loop let framesSinceKey = 0;If you prefer not to add a counter, minimally assert timestamps are present to fail fast during unexpected sources:
-const keyFrame = !group || groupTimestamp + GOP_DURATION_US <= frame.timestamp; +if (frame.timestamp === undefined) { + console.warn("VideoFrame has no timestamp; falling back to keyFrame=true to ensure GOP rotation"); +} +const keyFrame = !group || frame.timestamp === undefined || (groupTimestamp + GOP_DURATION_US) <= frame.timestamp;js/hang/src/watch/video/renderer.ts (2)
132-134: Nit: translate then scale for slightly clearer transform mathFunctionally equivalent, but applying
translate(w, 0)beforescale(-1, 1)avoids negative coordinates in intermediate space, which some readers find easier to reason about.-ctx.scale(-1, 1); -ctx.translate(-ctx.canvas.width, 0); +ctx.translate(ctx.canvas.width, 0); +ctx.scale(-1, 1);
139-142: Optional: throttle rendering when no frame is availableRight now we fill black every RAF tick when no frame is present. Consider pausing the RAF (or rendering at a lower cadence) until a new frame arrives to save CPU on long gaps.
Potential approach:
- Only schedule RAF when
this.source.frameis defined; re-schedule onframesignal changes.- Or, keep RAF but early-return unless a frame is present and only re-fill black when the canvas size changes.
Happy to sketch a minimal change if you want to pursue this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
js/hang/src/publish/video/index.ts(1 hunks)js/hang/src/watch/video/index.ts(1 hunks)js/hang/src/watch/video/renderer.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/watch/video/index.tsjs/hang/src/watch/video/renderer.tsjs/hang/src/publish/video/index.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/watch/video/index.tsjs/hang/src/watch/video/renderer.tsjs/hang/src/publish/video/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (3)
js/hang/src/watch/video/index.ts (1)
2-2: Type-only Getter import is correct and reduces runtime surfaceGood cleanup. Getter is used purely as a type, so importing it with
typeavoids an unnecessary runtime import and helps tree-shaking.js/hang/src/publish/video/index.ts (1)
164-166: Inclusive GOP boundary + first-frame keyframe: correct fixUsing
!group || groupTimestamp + GOP_DURATION_US <= frame.timestampensures the initial frame is a keyframe and prevents the off-by-one at exact GOP boundaries.js/hang/src/watch/video/renderer.ts (1)
117-138: Smarter resizing + single save/restore: solid rendering cleanup
- Resize only when dimensions change (uses
displayWidth/Height) to avoid needless context resets.- One
save()/restore()around all per-draw state simplifies correctness, especially with transforms.- Unified
drawImagepath keeps the render loop lean.Looks good.
It might be the cause of screen tearing.
|
Claude finished @kixelated's task —— View job Code Review for Misc Fixes PRReviewing 3 changed files for bugs and security issues:
Review Results👍 Overall Assessment: The changes look good - they fix keyframe timing logic, improve rendering efficiency, and clean up imports. No major bugs or security issues found. Minor Issues Found:
Positive Changes:
|
Untested
Summary by CodeRabbit
Bug Fixes
Performance
Chores