fix(useVideoTexture): clean up video element and cache on unmount#2714
fix(useVideoTexture): clean up video element and cache on unmount#2714mohamedtahaguelzim wants to merge 2 commits intopmndrs:masterfrom
Conversation
The video element created by useVideoTexture was never cleaned up when the component unmounted. Because suspend-react cached the element, it persisted and continued playing in the background, leaking resources. On cleanup, the effect now pauses the video, clears its source, disposes the texture, and evicts the suspend-react cache entry. Closes pmndrs#2712
|
@mohamedtahaguelzim is attempting to deploy a commit to the Poimandres Team on Vercel. A member of the Team first needs to authorize it. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
travisbreaks
left a comment
There was a problem hiding this comment.
Nice fix. The root cause analysis is solid, and the cleanup pattern aligns with what WebcamVideoTexture and ScreenVideoTexture already do. A few observations:
The fix looks correct for the single-consumer case. The cleanup order (pause, remove src, load, destroy HLS, dispose texture, clear cache) is the right sequence. Removing the src attribute and calling load() releases any ongoing network fetch, which matters for large video files. Good.
Cache key alignment is correct. suspend([srcOrSrcObject]) and clear([srcOrSrcObject]) use the same key shape, so the eviction will hit the right entry.
One concern: shared cache entries across multiple consumers. If two components both call useVideoTexture("same-video.mp4"), they share the same suspend-react cache entry (keyed by [srcOrSrcObject]). When the first component unmounts, the cleanup will pause the video, strip the src, dispose the texture, and evict the cache. The second component now holds a reference to a dead texture with a stopped, src-less video element. This is probably an uncommon pattern, but worth noting. A ref-counting approach (only clear/dispose when the last consumer unmounts) would handle this, though that might be out of scope for this PR. Worth a comment in the code or docs at minimum?
Double dispose when using the VideoTexture component. The VideoTexture component wrapper (same file, around line 115) has its own cleanup:
useEffect(() => {
return () => void texture.dispose()
}, [texture])With this PR, useVideoTexture now also calls texture.dispose() in its cleanup. When someone uses the <VideoTexture> component (which calls useVideoTexture internally), dispose fires twice on unmount. Three.js handles this gracefully (the dispose event just fires again), so it is not a bug, but it is redundant. Might be worth removing the dispose from the VideoTexture component wrapper since the hook now owns that responsibility.
Adding srcOrSrcObject to the dependency array is the right call. Without it, swapping the video source would not trigger cleanup of the old video element.
Minor edge case: unmount during initial load. If the component unmounts while still suspended (before the unsuspend event fires), the useEffect cleanup never runs because the component never fully mounted. The video element created inside the Promise callback and the pending cache entry would both leak. This is a pre-existing issue, not introduced by this PR, but worth a follow-up issue if someone wants to pursue it.
Overall this is a clean, well-scoped fix. The shared-consumer edge case and the double-dispose in the component wrapper are the main items I would flag.
- Remove redundant texture.dispose() from VideoTexture component since the hook now owns disposal - Clear video.srcObject for proper MediaStream release - Add comment noting single-consumer assumption
|
Thanks for the review. Pushed a follow up commit addressing the feedback. |
travisbreaks
left a comment
There was a problem hiding this comment.
Follow-up commit looks good. The double-dispose is properly resolved by centralizing cleanup in the hook, and the srcObject = null addition is the right call for MediaStream cleanup.
Two items for consideration (neither blocks approval):
-
Shared cache: The inline comment documenting the single-consumer assumption is honest, but consumers won't see it. Consider a
console.warnin dev mode ifclear()is called on an entry with active consumers, or a note in the JSDoc/docs. Video walls (multiple meshes, same source) are a real use case that will silently break. -
Unmount during suspension: You mentioned filing a follow-up issue for the case where unmount happens while
suspend-reactis still suspended. Worth tracking before this merges so it doesn't get lost.
Overall this is a solid fix for the dominant single-consumer case. The cleanup sequence matches WebcamVideoTexture and ScreenVideoTexture patterns in the codebase.
Why
Resolves #2712
useVideoTextureusessuspend-reactto cache the<video>element, but never cleans it up on unmount. The cached video persists and continues playing in the background, leaking resources.What
Added proper cleanup to the
useEffectinuseVideoTexture:load()to release network resourcessuspend-reactcache entry viaclear([srcOrSrcObject])This follows the same pattern already used by
WebcamVideoTextureandScreenVideoTexturein this codebase.Reproduction repo: https://github.com/mohamedtahaguelzim/drei-video-bug
Checklist