Use Choreographer-based timing in MotionVideoPlayerCompose#63
Use Choreographer-based timing in MotionVideoPlayerCompose#63tejpratap46 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesFrame Playback Timing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The migration from coroutine-based delay to Choreographer-based vsync timing is architecturally sound, but there are critical defects that must be fixed before merge:
Critical Issues Found (5):
- Off-by-one error: Frame boundary check allows accessing invalid frame index (frame N when valid range is 0 to N-1)
- Division by zero: No validation for
fps = 0causing potential ArithmeticException - Race conditions: Unsynchronized access to
isPlayingandcurrentFramefrom callback thread - Resource leak: Callback continues posting itself even after disposal, causing unnecessary work and potential memory leaks
Required Actions:
- Fix the frame boundary condition from
<=to< - Add fps validation to prevent division by zero
- Address thread synchronization for state access in the callback
- Conditionally post the next frame callback only when
isPlayingis true
These issues block merge as they can cause crashes, incorrect behavior, and resource leaks in production.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| accumulatedNanos %= frameDurationNanos | ||
|
|
||
| val nextFrame = currentFrame + framesToAdvance | ||
| if (nextFrame <= totalFrames) { |
There was a problem hiding this comment.
🛑 Logic Error: The condition nextFrame <= totalFrames allows playback to advance beyond the last valid frame index. For a video with N frames (0 to N-1), this condition permits currentFrame to be set to N, which is out of bounds. Change the condition to nextFrame < totalFrames to prevent accessing an invalid frame.
| if (nextFrame <= totalFrames) { | |
| if (nextFrame < totalFrames) { |
|
|
||
| val callback = object : Choreographer.FrameCallback { | ||
| override fun doFrame(frameTimeNanos: Long) { | ||
| if (!isPlaying) return |
There was a problem hiding this comment.
🛑 Crash Risk: Reading isPlaying inside doFrame without synchronization creates a race condition. The Choreographer callback runs on the main thread, but isPlaying can be modified by UI events simultaneously. Capture isPlaying state when registering the callback instead of reading the mutable property, or ensure thread-safe access.
|
|
||
| val nextFrame = currentFrame + framesToAdvance | ||
| if (nextFrame <= totalFrames) { | ||
| currentFrame = nextFrame |
There was a problem hiding this comment.
🛑 Crash Risk: Writing to currentFrame inside the Choreographer callback without synchronization creates a race condition. The callback modifies currentFrame on the main thread while other Compose code may be reading it, potentially causing inconsistent state. Use a thread-safe mechanism or ensure all state updates occur on the same thread with proper synchronization.
| onDispose { } | ||
| } else { | ||
| val choreographer = Choreographer.getInstance() | ||
| val frameDurationNanos = (1_000_000_000L / motionConfig.fps).coerceAtLeast(1L) |
There was a problem hiding this comment.
🛑 Crash Risk: Integer division by motionConfig.fps can throw ArithmeticException if fps is zero. Add validation to prevent division by zero, or ensure fps is always positive before this calculation.
| val frameDurationNanos = (1_000_000_000L / motionConfig.fps).coerceAtLeast(1L) | |
| val frameDurationNanos = if (motionConfig.fps > 0) { | |
| (1_000_000_000L / motionConfig.fps).coerceAtLeast(1L) | |
| } else { | |
| 1_000_000_000L / 30 // Default to 30 fps if invalid | |
| } |
| if (accumulatedNanos >= frameDurationNanos) { | ||
| val framesToAdvance = (accumulatedNanos / frameDurationNanos).toInt().coerceAtLeast(1) | ||
| accumulatedNanos %= frameDurationNanos | ||
|
|
||
| val nextFrame = currentFrame + framesToAdvance | ||
| if (nextFrame <= totalFrames) { | ||
| currentFrame = nextFrame | ||
| } else { | ||
| currentFrame = 0 | ||
| activePlayers.values.forEach { it.stop(); it.release() } | ||
| activePlayers.clear() | ||
| } | ||
| } | ||
| lastFrameTime = currentTime | ||
|
|
||
| choreographer.postFrameCallback(this) |
There was a problem hiding this comment.
🛑 Logic Error: The callback continues to post itself via choreographer.postFrameCallback(this) even after cleanup should occur. When isPlaying becomes false or the DisposableEffect is disposed, the callback remains scheduled. Move the check for !isPlaying before posting the next frame callback to prevent unnecessary callback executions after disposal.
| if (accumulatedNanos >= frameDurationNanos) { | |
| val framesToAdvance = (accumulatedNanos / frameDurationNanos).toInt().coerceAtLeast(1) | |
| accumulatedNanos %= frameDurationNanos | |
| val nextFrame = currentFrame + framesToAdvance | |
| if (nextFrame <= totalFrames) { | |
| currentFrame = nextFrame | |
| } else { | |
| currentFrame = 0 | |
| activePlayers.values.forEach { it.stop(); it.release() } | |
| activePlayers.clear() | |
| } | |
| } | |
| lastFrameTime = currentTime | |
| choreographer.postFrameCallback(this) | |
| if (accumulatedNanos >= frameDurationNanos) { | |
| val framesToAdvance = (accumulatedNanos / frameDurationNanos).toInt().coerceAtLeast(1) | |
| accumulatedNanos %= frameDurationNanos | |
| val nextFrame = currentFrame + framesToAdvance | |
| if (nextFrame <= totalFrames) { | |
| currentFrame = nextFrame | |
| } else { | |
| currentFrame = 0 | |
| activePlayers.values.forEach { it.stop(); it.release() } | |
| activePlayers.clear() | |
| } | |
| } | |
| if (isPlaying) { | |
| choreographer.postFrameCallback(this) | |
| } |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
motion-lib | 83551a7 | Commit Preview URL Branch Preview URL |
May 21 2026, 11:58 AM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
sdui | 83551a7 | Commit Preview URL Branch Preview URL |
May 21 2026, 11:58 AM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lyrics | 83551a7 | Commit Preview URL Branch Preview URL |
May 21 2026, 11:59 AM |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the video playback loop in MotionVideoPlayerCompose.kt to use Choreographer for vsync-aligned frame updates, replacing the previous delay-based coroutine loop. Feedback suggests adopting the more idiomatic withFrameNanos API within a LaunchedEffect to simplify lifecycle handling and improve code readability. Additionally, a safety check was recommended to prevent division by zero when calculating frame duration from the FPS configuration.
|
|
||
| import android.graphics.Bitmap | ||
| import android.media.MediaPlayer | ||
| import android.view.Choreographer |
There was a problem hiding this comment.
The Choreographer import is no longer necessary if you use the more idiomatic withFrameNanos approach for Compose timing.
| DisposableEffect(isPlaying, motionConfig.fps, totalFrames) { | ||
| if (!isPlaying) { | ||
| onDispose { } | ||
| } else { | ||
| val choreographer = Choreographer.getInstance() | ||
| val frameDurationNanos = (1_000_000_000L / motionConfig.fps).coerceAtLeast(1L) | ||
| var lastFrameNanos = 0L | ||
| var accumulatedNanos = 0L | ||
|
|
||
| val callback = object : Choreographer.FrameCallback { | ||
| override fun doFrame(frameTimeNanos: Long) { | ||
| if (!isPlaying) return | ||
|
|
||
| if (lastFrameNanos != 0L) { | ||
| accumulatedNanos += (frameTimeNanos - lastFrameNanos).coerceAtLeast(0L) | ||
| } | ||
| lastFrameNanos = frameTimeNanos | ||
|
|
||
| if (accumulatedNanos >= frameDurationNanos) { | ||
| val framesToAdvance = (accumulatedNanos / frameDurationNanos).toInt().coerceAtLeast(1) | ||
| accumulatedNanos %= frameDurationNanos | ||
|
|
||
| val nextFrame = currentFrame + framesToAdvance | ||
| if (nextFrame <= totalFrames) { | ||
| currentFrame = nextFrame | ||
| } else { | ||
| currentFrame = 0 | ||
| activePlayers.values.forEach { it.stop(); it.release() } | ||
| activePlayers.clear() | ||
| } | ||
| } | ||
| lastFrameTime = currentTime | ||
|
|
||
| choreographer.postFrameCallback(this) | ||
| } | ||
| delay(10) // Smooth check | ||
| } | ||
|
|
||
| choreographer.postFrameCallback(callback) | ||
|
|
||
| onDispose { | ||
| choreographer.removeFrameCallback(callback) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Using DisposableEffect with a manual Choreographer.FrameCallback is less idiomatic in Jetpack Compose. A more robust and cleaner approach is to use LaunchedEffect combined with withFrameNanos. This automatically handles the lifecycle (cancellation when isPlaying changes or the composable leaves the tree) and aligns perfectly with the Compose rendering pipeline.
Additionally, I've added a safety check for motionConfig.fps to prevent a potential division by zero.
LaunchedEffect(isPlaying, motionConfig.fps, totalFrames) {
if (!isPlaying) return@LaunchedEffect
val frameDurationNanos = (1_000_000_000L / motionConfig.fps.coerceAtLeast(1)).toLong()
var lastFrameNanos = 0L
var accumulatedNanos = 0L
while (true) {
withFrameNanos { frameTimeNanos ->
if (lastFrameNanos != 0L) {
accumulatedNanos += (frameTimeNanos - lastFrameNanos).coerceAtLeast(0L)
}
lastFrameNanos = frameTimeNanos
if (accumulatedNanos >= frameDurationNanos) {
val framesToAdvance = (accumulatedNanos / frameDurationNanos).toInt()
accumulatedNanos %= frameDurationNanos
val nextFrame = currentFrame + framesToAdvance
if (nextFrame <= totalFrames) {
currentFrame = nextFrame
} else {
currentFrame = 0
activePlayers.values.forEach { it.stop(); it.release() }
activePlayers.clear()
}
}
}
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`:
- Line 73: The calculation of frameDurationNanos uses (1_000_000_000L /
motionConfig.fps) which throws if motionConfig.fps is 0 and misbehaves for
negatives; fix by coercing motionConfig.fps before the division (e.g., val
safeFps = motionConfig.fps.coerceAtLeast(1)) and then compute frameDurationNanos
= (1_000_000_000L / safeFps).coerceAtLeast(1L) to ensure a safe, non-zero
divisor and still guard the result; update the code around the
frameDurationNanos calculation in MotionVideoPlayerCompose (reference
motionConfig.fps and frameDurationNanos).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 673d17d0-841c-4ef0-88f4-2274219353e4
📒 Files selected for processing (1)
modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt
| onDispose { } | ||
| } else { | ||
| val choreographer = Choreographer.getInstance() | ||
| val frameDurationNanos = (1_000_000_000L / motionConfig.fps).coerceAtLeast(1L) |
There was a problem hiding this comment.
Guard against zero/negative motionConfig.fps.
coerceAtLeast(1L) runs after the division, so if motionConfig.fps is 0 you get ArithmeticException, and a negative fps would yield a negative duration that silently coerces to 1L (effectively advancing one frame per vsync). Coerce the divisor instead.
🛡️ Proposed fix
- val frameDurationNanos = (1_000_000_000L / motionConfig.fps).coerceAtLeast(1L)
+ val safeFps = motionConfig.fps.coerceAtLeast(1)
+ val frameDurationNanos = (1_000_000_000L / safeFps).coerceAtLeast(1L)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`
at line 73, The calculation of frameDurationNanos uses (1_000_000_000L /
motionConfig.fps) which throws if motionConfig.fps is 0 and misbehaves for
negatives; fix by coercing motionConfig.fps before the division (e.g., val
safeFps = motionConfig.fps.coerceAtLeast(1)) and then compute frameDurationNanos
= (1_000_000_000L / safeFps).coerceAtLeast(1L) to ensure a safe, non-zero
divisor and still guard the result; update the code around the
frameDurationNanos calculation in MotionVideoPlayerCompose (reference
motionConfig.fps and frameDurationNanos).
1 participant
Motivation
Description
delay-based playback loop with aChoreographer.FrameCallbackloop inmodules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt.android.view.Choreographerand removed thedelayusage, adding nanosecond-based timing and accumulation to computeframesToAdvancefrommotionConfig.fps(video FPS).DisposableEffectto register the frame callback and toremoveFrameCallbackon dispose to avoid leaks and duplicate callbacks.Testing
./gradlew :modules:motion-video-player:compileDebugKotlinwhich failed due to missing Android SDK configuration (ANDROID_HOMEorlocal.properties sdk.dir), so no successful module compile or CI run could be performed in this environment.Codex Task
Summary by CodeRabbit