refactor: video-component playback control#1429
Merged
Conversation
Contributor
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactored VideoComponent playback and seeking: moved frame upload to onUpdate, serialized play/seek flows, added activation/end-behavior flags and disposer-based composition subscriptions, improved lifecycle cleanup and manual loop/playbackRate overrides; added demo HTML, index link, and a demo runner script with playback controls and stutter repros. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DemoScript as "Demo Script"
participant Player as "Player / Composition"
participant VideoComp as "VideoComponent"
participant HTMLVideo as "HTMLVideoElement"
participant Renderer as "Renderer / Texture"
User->>DemoScript: load scene / switch
DemoScript->>Player: createPlayer() + loadScene({autoplay:true})
Player->>VideoComp: composition events (goto/play/pause/end)
VideoComp->>VideoComp: set pendingSeekTime / flags
VideoComp->>VideoComp: onUpdate -> processPendingSeek()
VideoComp->>HTMLVideo: performSeek (set currentTime)
HTMLVideo-->>VideoComp: seeked
VideoComp->>Renderer: upload frame (if not videoSeeking)
VideoComp->>HTMLVideo: safePlay() (uses playPromise)
HTMLVideo-->>VideoComp: play resolved/rejected
User->>DemoScript: trigger stutter / change playbackRate
DemoScript->>Player: dispose / recreate (stutter repro)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin-packages/multimedia/src/video/video-component.ts`:
- Around line 395-406: The video can restart after being destroyed because
shouldStartVideo() doesn't check the videoDestroyed flag; update
shouldStartVideo (the method named shouldStartVideo) to immediately return false
if this.videoDestroyed is true, so it never permits startVideo() to run for
destroyed elements, and audit related start logic (where startVideo() is
invoked) to ensure they also respect this.videoDestroyed (the
destroy/mark-destroy code paths around the earlier destroy logic) so no code
path (e.g., after a seek or composition.time change) can relaunch a destroyed
video.
- Around line 263-269: handleGoto currently sets pendingSeekTime then other
branches call performSeek() or safePlay() directly, causing double-seeks and
racey play; instead, only store the post-seek intent in the pending state (e.g.
pendingSeekTime plus a flag like pendingSeekMode or reuse
isWaitingForGotoResult/playTriggered/manualPause) inside handleGoto and the
gotoAndStop/gotoAndPlay paths, remove immediate calls to performSeek() and
safePlay() there, and let processPendingSeek() be the single owner that reads
pendingSeekTime and the intent flag, performs the actual performSeek(), and then
runs safePlay() or enforce pause based on the stored intent; update references
in handleGoto, any gotoAndStop/gotoAndPlay branches, and processPendingSeek() so
no other code triggers a seek directly.
- Around line 462-469: ensureLoopFlag only forces loop=true for
EndBehavior.restart but setLoop(true) leaves video.loop stuck after reset;
update ensureLoopFlag and resetLoop so automatic mode always derives loop from
this.item.endBehavior (respecting this.manualLoop) instead of only setting true:
in ensureLoopFlag (and any analogous logic around 690-692) set this.video!.loop
= (this.item.endBehavior === spec.EndBehavior.restart) when not this.manualLoop,
and make resetLoop restore the same automatic value (i.e., set this.video.loop
to this.item.endBehavior === spec.EndBehavior.restart when manualLoop is false)
so composition-controlled looping is correctly restored.
- Around line 177-189: The videoLoaded flag is currently set once from
this.video.readyState in fromData() and never updated, causing stale state
across later media loads or element swaps; change videoLoaded into a live signal
that updates on media events (e.g., 'loadeddata'/'loadedmetadata' -> true,
'emptied'/'abort'/'error' -> false) and ensure you attach those listeners
whenever this.video is set/changed and remove old listeners on swap; also ensure
fromData() initializes/reset the signal (and any loop/endBehavior logic using
this.video.loop remains after you rebind listeners) so subsequent updates to the
media update videoLoaded correctly.
In `@web-packages/demo/src/video.ts`:
- Around line 528-529: The delayed callbacks (that call gotoAndPlay) close over
the mutable globals currentIndex and player so old timers can affect a new
player instance; fix by either (A) store timeout handles when scheduling those
setTimeouts and clear them before assigning/replacing player (clearTimeout on
stored handles in the disposal path), or (B) capture the local Player instance
in each timeout callback (const local = player) and return early if player !==
local before calling local.gotoAndPlay; apply the chosen pattern consistently to
all delayed gotoAndPlay usages (including the occurrences around currentIndex
and the other delayed calls mentioned).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be3f7b3e-4cf7-4445-8b65-4bf19a669935
📒 Files selected for processing (4)
plugin-packages/multimedia/src/video/video-component.tsweb-packages/demo/html/video.htmlweb-packages/demo/index.htmlweb-packages/demo/src/video.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
plugin-packages/multimedia/src/video/video-component.ts (3)
456-464:⚠️ Potential issue | 🟠 Major
resetLoop()never restores composition-controlledloop = false.Line 461 only forces
true, so aftersetLoop(true)a laterresetLoop()leaves freeze/destroy videos looping forever.♻️ Proposed fix
private ensureLoopFlag (): void { - if (this.manualLoop) { + if (this.manualLoop || !this.video) { return; } - if (this.item.endBehavior === spec.EndBehavior.restart && !this.video!.loop) { - this.video!.loop = true; + const shouldLoop = this.item.endBehavior === spec.EndBehavior.restart; + + if (this.video.loop !== shouldLoop) { + this.video.loop = shouldLoop; } } @@ resetLoop () { this.manualLoop = false; + this.ensureLoopFlag(); }Also applies to: 684-686
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/multimedia/src/video/video-component.ts` around lines 456 - 464, The ensureLoopFlag currently only sets this.video.loop = true when composition requests restart, but resetLoop does not clear composition-controlled looping so videos remain looping after setLoop(true); update the reset logic (the resetLoop method and any duplicate at the other block around the same pattern) to check that this.manualLoop is false and, if so, explicitly set this.video.loop = false to restore composition control; ensure you reference and respect the same conditionals used in ensureLoopFlag (this.manualLoop, this.item.endBehavior === spec.EndBehavior.restart) so manual overrides are preserved while composition-controlled loop is reverted.
389-400:⚠️ Potential issue | 🟠 MajorDestroyed videos can still auto-restart.
Line 390 still ignores
videoDestroyed, so a destroy-ended element can start again on the next update oncecurrentTimehas been reset to0. Please mirror the same guard in the composition-resume path around Lines 293-297 as well, otherwisehandleCompositionPlay()can bypass the fix.🔧 Proposed fix
private handleCompositionPlay (): void { @@ - if (!this.manualPause && !this.videoSeeking && this.video?.paused) { + if (!this.manualPause && !this.videoSeeking && this.video?.paused && !this.videoDestroyed && !this.checkVideoEnded()) { this.safePlay(); } @@ private shouldStartVideo (): boolean { - if (this.playTriggered || this.manualPause || this.checkVideoEnded()) { + if (this.playTriggered || this.manualPause || this.videoDestroyed || this.checkVideoEnded()) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/multimedia/src/video/video-component.ts` around lines 389 - 400, The shouldStartVideo() guard currently omits the videoDestroyed flag so destroyed videos can auto-restart; update shouldStartVideo() to return false if this.videoDestroyed (in addition to playTriggered/manualPause/checkVideoEnded and missing composition) and also add the same videoDestroyed guard to the composition-resume path in handleCompositionPlay() so composition-based resumes cannot run for destroyed elements; locate the checks around the composition resume logic and ensure they short-circuit when this.videoDestroyed is true.
257-263:⚠️ Potential issue | 🟠 Major
gotostill has two competing seek owners.
packages/effects-core/src/composition.ts:495-510emitsgotobeforeresume()/pause(), so Line 262 always queuespendingSeekTimefirst.gotoAndStopthen seeks immediately on Line 275, whilegotoAndPlaycan reach Line 296 before Line 413 runs. That still leaves the double-seek / play-before-seek race in place. Please keep the post-gotointent in pending state and letprocessPendingSeek()own the onlyperformSeek()call.Also applies to: 269-279, 287-297, 406-414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/multimedia/src/video/video-component.ts` around lines 257 - 263, The handleGoto method sets pendingSeekTime and flips play/resume flags immediately, causing two competing seek owners and a race between gotoAndStop/gotoAndPlay and resume/pause; change handleGoto (and analogous blocks around lines with pendingSeekTime assignments) to only record the intended post-goto intent (e.g., set a new enum/flag like pendingGotoIntent = "play"|"stop" and set pendingSeekTime but do NOT call performSeek or trigger immediate seek), clear playTriggered/manualPause appropriately, and ensure processPendingSeek is the single place that reads pendingGotoIntent and pendingSeekTime and calls performSeek once; update gotoAndStop/gotoAndPlay flows to rely on processPendingSeek owning the seek so no immediate performSeek happens elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin-packages/multimedia/src/video/video-component.ts`:
- Around line 520-527: The play() catch block currently treats AbortError as a
render error and swallows other errors; change the error handling in the
promise.catch so that this.playPromise is cleared if (this.playPromise ===
promise) as now, then if error.name === 'AbortError' only set this.playTriggered
= false (don’t add to engine.renderErrors), else add the error to
this.engine.renderErrors (so real rejections like NotAllowedError are reported);
update the logic around playPromise, promise, playTriggered and
engine.renderErrors in the play() method accordingly.
- Around line 558-605: The performSeek method sets videoSeeking true but only
applies the iOS pre-play workaround when isGotoAndStop is true, causing paused
regular seeks (invoked from processPendingSeek and gotoAndPlayStart) to never
fire seeked on iOS and leave videoSeeking stuck; change performSeek so that
whenever the video is paused (wasPlaying === false) you attempt the same
play-then-seek sequence with a .play().then(...).catch(...) fallback before
calling doSeek(), regardless of isGotoAndStop, and keep the existing branch
behavior for when the video was playing (pause first for non-gotoAndStop),
ensuring videoSeeking is always cleared in the seeked handler.
---
Duplicate comments:
In `@plugin-packages/multimedia/src/video/video-component.ts`:
- Around line 456-464: The ensureLoopFlag currently only sets this.video.loop =
true when composition requests restart, but resetLoop does not clear
composition-controlled looping so videos remain looping after setLoop(true);
update the reset logic (the resetLoop method and any duplicate at the other
block around the same pattern) to check that this.manualLoop is false and, if
so, explicitly set this.video.loop = false to restore composition control;
ensure you reference and respect the same conditionals used in ensureLoopFlag
(this.manualLoop, this.item.endBehavior === spec.EndBehavior.restart) so manual
overrides are preserved while composition-controlled loop is reverted.
- Around line 389-400: The shouldStartVideo() guard currently omits the
videoDestroyed flag so destroyed videos can auto-restart; update
shouldStartVideo() to return false if this.videoDestroyed (in addition to
playTriggered/manualPause/checkVideoEnded and missing composition) and also add
the same videoDestroyed guard to the composition-resume path in
handleCompositionPlay() so composition-based resumes cannot run for destroyed
elements; locate the checks around the composition resume logic and ensure they
short-circuit when this.videoDestroyed is true.
- Around line 257-263: The handleGoto method sets pendingSeekTime and flips
play/resume flags immediately, causing two competing seek owners and a race
between gotoAndStop/gotoAndPlay and resume/pause; change handleGoto (and
analogous blocks around lines with pendingSeekTime assignments) to only record
the intended post-goto intent (e.g., set a new enum/flag like pendingGotoIntent
= "play"|"stop" and set pendingSeekTime but do NOT call performSeek or trigger
immediate seek), clear playTriggered/manualPause appropriately, and ensure
processPendingSeek is the single place that reads pendingGotoIntent and
pendingSeekTime and calls performSeek once; update gotoAndStop/gotoAndPlay flows
to rely on processPendingSeek owning the seek so no immediate performSeek
happens elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c341a95a-b324-4e84-b6c1-631f8112fedc
📒 Files selected for processing (1)
plugin-packages/multimedia/src/video/video-component.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
plugin-packages/multimedia/src/video/video-component.ts (3)
586-605:⚠️ Potential issue | 🟠 MajorRegular paused seeks still lack the iOS workaround.
The pre-play workaround for iOS Safari (where
seekeddoesn't fire reliably on paused videos) is only applied whenisGotoAndStop = true. Calls fromprocessPendingSeek()andhandleDestroyBehavior()passisGotoAndStop = false, potentially leavingvideoSeeking = trueindefinitely on iOS Safari.Consider applying the same play-then-seek pattern for all paused seeks, or add a timeout fallback to clear
videoSeeking.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/multimedia/src/video/video-component.ts` around lines 586 - 605, The current iOS play-before-seek workaround is only used when isGotoAndStop is true, leaving calls from processPendingSeek() and handleDestroyBehavior() vulnerable and risking videoSeeking staying true; update the seek logic around doSeek() so that whenever wasPlaying is false on iOS/Safari you call this.video!.play().then(()=>doSeek()).catch(()=>{ doSeek(); }) or, if you prefer, add a timeout fallback that clears videoSeeking after N ms when a seeked event isn’t observed; make this change in the same block that uses isGotoAndStop/doSeek()/wasPlaying and ensure videoSeeking is cleared on both successful seek and the timeout fallback so processPendingSeek(), handleDestroyBehavior(), and other callers are covered.
389-401:⚠️ Potential issue | 🟠 Major
videoDestroyedcheck still missing inshouldStartVideo.After the destroy behavior triggers and seeks to 0, a nonzero
composition.timeon line 400 can satisfy the condition, causingstartVideo()to relaunch a video that should stay terminated.,
🔧 Suggested fix
private shouldStartVideo (): boolean { - if (this.playTriggered || this.manualPause || this.checkVideoEnded()) { + if (this.playTriggered || this.manualPause || this.videoDestroyed || this.checkVideoEnded()) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/multimedia/src/video/video-component.ts` around lines 389 - 401, The shouldStartVideo method can return true after destroy because it doesn't check the videoDestroyed flag; update shouldStartVideo (located on the same class as startVideo and where this.video and this.item.composition are used) to return false when this.videoDestroyed is true — e.g., add a guard checking this.videoDestroyed (alongside this.playTriggered and this.manualPause) or ensure the final return also requires !this.videoDestroyed so that composition.time or this.video.currentTime cannot trigger startVideo after destroy.
520-528:⚠️ Potential issue | 🟠 MajorThe
play()error handling is still inverted.
AbortErroris expected when play is interrupted by pause—not a render error. Real errors likeNotAllowedError(autoplay policy rejection) are swallowed.,
🔧 Suggested fix
.catch(error => { if (this.playPromise === promise) { this.playPromise = null; } if (error.name === 'AbortError') { this.playTriggered = false; - this.engine.renderErrors.add(error); + return; } + this.engine.renderErrors.add(error); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/multimedia/src/video/video-component.ts` around lines 520 - 528, The catch block for the play() promise currently treats AbortError as a render error and swallows other errors; update the logic in the .catch handler (referencing playPromise, promise, playTriggered, and engine.renderErrors) so that you always clear playPromise when promise matches, set playTriggered = false for AbortError but do NOT add AbortError to engine.renderErrors, and for any other error add the error to engine.renderErrors (and optionally rethrow or propagate if desired); ensure the promise-identification check (this.playPromise === promise) still runs before clearing state.
🧹 Nitpick comments (1)
plugin-packages/multimedia/src/video/video-component.ts (1)
628-632: Consider clamping playbackRate to valid range.
playbackRateof 0 or negative values may behave unexpectedly across browsers. Ifengine.speedorcomposition.speedcan ever be 0 or negative, consider clamping to a small positive minimum.💡 Optional clamping
const playbackRate = this.engine.speed * composition.speed; + // Most browsers support 0.25 to 4.0; clamp to avoid unexpected behavior + const clampedRate = Math.max(0.25, Math.min(4.0, playbackRate)); - if (this.video.playbackRate !== playbackRate) { - this.video.playbackRate = playbackRate; + if (this.video.playbackRate !== clampedRate) { + this.video.playbackRate = clampedRate; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/multimedia/src/video/video-component.ts` around lines 628 - 632, The computed playbackRate (const playbackRate = this.engine.speed * composition.speed) should be clamped to a safe positive range before assigning to this.video.playbackRate; introduce a MIN_PLAYBACK_RATE constant (e.g. 0.01–0.1) and optionally a MAX_PLAYBACK_RATE, replace uses of the raw playbackRate with a clamped value (e.g. clamped = Math.max(playbackRate, MIN_PLAYBACK_RATE) and optionally Math.min(clamped, MAX_PLAYBACK_RATE)), and update the comparison/assignment to compare against and set this.video.playbackRate to the clamped value instead of the raw product so 0 or negative speeds are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugin-packages/multimedia/src/video/video-component.ts`:
- Around line 586-605: The current iOS play-before-seek workaround is only used
when isGotoAndStop is true, leaving calls from processPendingSeek() and
handleDestroyBehavior() vulnerable and risking videoSeeking staying true; update
the seek logic around doSeek() so that whenever wasPlaying is false on
iOS/Safari you call this.video!.play().then(()=>doSeek()).catch(()=>{ doSeek();
}) or, if you prefer, add a timeout fallback that clears videoSeeking after N ms
when a seeked event isn’t observed; make this change in the same block that uses
isGotoAndStop/doSeek()/wasPlaying and ensure videoSeeking is cleared on both
successful seek and the timeout fallback so processPendingSeek(),
handleDestroyBehavior(), and other callers are covered.
- Around line 389-401: The shouldStartVideo method can return true after destroy
because it doesn't check the videoDestroyed flag; update shouldStartVideo
(located on the same class as startVideo and where this.video and
this.item.composition are used) to return false when this.videoDestroyed is true
— e.g., add a guard checking this.videoDestroyed (alongside this.playTriggered
and this.manualPause) or ensure the final return also requires
!this.videoDestroyed so that composition.time or this.video.currentTime cannot
trigger startVideo after destroy.
- Around line 520-528: The catch block for the play() promise currently treats
AbortError as a render error and swallows other errors; update the logic in the
.catch handler (referencing playPromise, promise, playTriggered, and
engine.renderErrors) so that you always clear playPromise when promise matches,
set playTriggered = false for AbortError but do NOT add AbortError to
engine.renderErrors, and for any other error add the error to
engine.renderErrors (and optionally rethrow or propagate if desired); ensure the
promise-identification check (this.playPromise === promise) still runs before
clearing state.
---
Nitpick comments:
In `@plugin-packages/multimedia/src/video/video-component.ts`:
- Around line 628-632: The computed playbackRate (const playbackRate =
this.engine.speed * composition.speed) should be clamped to a safe positive
range before assigning to this.video.playbackRate; introduce a MIN_PLAYBACK_RATE
constant (e.g. 0.01–0.1) and optionally a MAX_PLAYBACK_RATE, replace uses of the
raw playbackRate with a clamped value (e.g. clamped = Math.max(playbackRate,
MIN_PLAYBACK_RATE) and optionally Math.min(clamped, MAX_PLAYBACK_RATE)), and
update the comparison/assignment to compare against and set
this.video.playbackRate to the clamped value instead of the raw product so 0 or
negative speeds are avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d9880f4-d911-49f8-bf19-479f75734554
📒 Files selected for processing (1)
plugin-packages/multimedia/src/video/video-component.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
web-packages/demo/src/video.ts (1)
674-763: Extract a helper forVideoComponentlookup.The same
getCompositions() -> getItemByName('video_2') -> getComponent(VideoComponent)sequence is repeated across handlers. A small helper reduces duplication and keeps null-handling consistent.♻️ Suggested refactor
+function getCurrentVideoComponent (): VideoComponent | undefined { + const composition = player?.getCompositions()[0]; + return composition?.getItemByName('video_2')?.getComponent(VideoComponent); +} ... - const compositions = player.getCompositions(); - const composition = compositions[0]; - const videoItem = composition?.getItemByName('video_2'); - const videoComp = videoItem?.getComponent(VideoComponent); - videoComp?.pauseVideo(); + getCurrentVideoComponent()?.pauseVideo();Apply the same pattern to play/speed handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-packages/demo/src/video.ts` around lines 674 - 763, Repeated lookups using player.getCompositions() -> composition.getItemByName('video_2') -> getComponent(VideoComponent) should be extracted to a helper (e.g., getVideoComponent()) that returns the VideoComponent or undefined; implement this helper near the top of the file and replace the duplicate sequences in videoPauseBtn.onclick, videoPlayBtn.onclick, compPauseBtn/compPlayBtn usages (where applicable), and the speed handlers speed05Btn.onclick, speed1Btn.onclick, speed2Btn.onclick to call getVideoComponent() and then invoke pauseVideo(), playVideo(), or setPlaybackRate(...) on the returned component, preserving existing null checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-packages/demo/src/video.ts`:
- Around line 542-557: The async scene switches can race: modify switchVideo
(and the similar playStuckVideo* functions) to use a monotonic request
token/counter (e.g., increment a module-scoped requestId before starting the
async work), capture the current token locally, and after await
player.loadScene(...) verify the token still matches before applying any state
changes (like assigning player, calling updateInfo, or other side effects); if
the token is stale, abort/ignore the completion to prevent older loadScene
resolutions from overwriting newer ones. Ensure the token is incremented
whenever a new scene switch is initiated and used to gate all post-await logic
in switchVideo and the playStuckVideo* handlers.
- Around line 548-549: The demo currently calls player.dispose() right before
reassigning player = new Player({...}), which can throw if the async setup
hasn't finished and player is still null/uninitialized; change each dispose call
to a safe guard (e.g., if (player) player.dispose()) and preserve player as a
nullable variable so the guard is valid, ensuring you update every occurrence
where player.dispose() is called prior to reinitializing (the calls immediately
before new Player({...}) and any similar early-interaction dispose sites).
- Line 530: The code calls document.getElementById('J-container') and passes the
possibly-null container into new Player(...), which can throw at runtime; add an
explicit null check for the variable container after the getElementById call and
before any invocation of new Player(...). If container is null, handle it
gracefully (e.g., throw a clear Error or return early and log an informative
message) so that Player is only constructed with a non-null HTMLElement; update
the code paths that reference container to rely on that guard.
- Line 576: Remove the unnecessary double-casts on stuckVideoJson and
stuckVideoJson2 when calling player.loadScene: the variables already match
Scene.LoadType, so replace calls to player.loadScene(stuckVideoJson as any as
string, ...) and player.loadScene(stuckVideoJson2 as any as string, ...) with
player.loadScene(stuckVideoJson, ...) and player.loadScene(stuckVideoJson2,
...). Update the two invocations around the existing loadScene calls
(referencing the loadScene method on the player and the
stuckVideoJson/stuckVideoJson2 symbols) to remove the casts so TypeScript can
enforce the correct types.
---
Nitpick comments:
In `@web-packages/demo/src/video.ts`:
- Around line 674-763: Repeated lookups using player.getCompositions() ->
composition.getItemByName('video_2') -> getComponent(VideoComponent) should be
extracted to a helper (e.g., getVideoComponent()) that returns the
VideoComponent or undefined; implement this helper near the top of the file and
replace the duplicate sequences in videoPauseBtn.onclick, videoPlayBtn.onclick,
compPauseBtn/compPlayBtn usages (where applicable), and the speed handlers
speed05Btn.onclick, speed1Btn.onclick, speed2Btn.onclick to call
getVideoComponent() and then invoke pauseVideo(), playVideo(), or
setPlaybackRate(...) on the returned component, preserving existing null checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4c99c91-3a9e-409d-b589-0864ab8086d1
📒 Files selected for processing (2)
plugin-packages/multimedia/src/video/video-component.tsweb-packages/demo/src/video.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin-packages/multimedia/src/video/video-component.ts
yiiqii
approved these changes
Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修改说明：
● seek 安全机制：新增 performSeek 方法，seek 前先 pause()，监听 seeked 事件完成后再恢复播放，修复 seek 后视频卡顿问题；增加 videoSeeking 标志位阻止 seek 期间上传旧帧；通过 pendingSeekTime 合并同一帧内的多次 seek 请求
● play/pause 竞态重构：用 playPromise 引用串行化替代 isPlayLoading + pendingPause 标志位，已有 play promise 时跳过重复调用，pause 时若有 pending promise 则在 then 中补一次 pause
● 事件监听管理：改为 eventDisposers 数组收集 composition.on() 返回的 dispose 函数，销毁时统一遍历调用；新增 end 事件监听
● 播放速率与循环控制：新增 manualPlaybackRate / manualLoop 标志位区分手动和自动模式，新增 resetPlaybackRate() / resetLoop() 恢复自动控制；自动模式下每帧同步 engine.speed * composition.speed
● 结束行为处理拆分：从 onUpdate 中拆分为 shouldFreezeVideo、freezeVideo、handleDestroyBehavior、ensureLoopFlag、detectCompositionRestart、handleCompositionEnd 等独立方法
● goto 场景支持：通过 isWaitingForGotoResult 标志位和后续 play/pause 事件时序区分 gotoAndPlay 和 gotoAndStop
● 生命周期简化：onEnable / onDisable 不再直接操作播放状态，仅设置 isVideoActive 标志位；帧上传从 render 移至 onUpdate 末尾，受 videoSeeking 保护
● 其他：新增 manualPause 区分用户手动暂停和系统暂停；新增 videoLoaded 检查跳过未加载完成的视频；threshold 改为静态常量 0.01；
Summary by CodeRabbit
New Features
Improvements