Skip to content

Use Choreographer-based timing in MotionVideoPlayerCompose #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tejpratap46 wants to merge 1 commit into from
Closed

Use Choreographer-based timing in MotionVideoPlayerCompose #63

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 40 additions & 24 deletions ...er/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.tejpratapsingh.motionlib.ui.custom.video

import android.graphics.Bitmap
import android.media.MediaPlayer
import android.view.Choreographer

@gemini-code-assist gemini-code-assist Bot May 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The Choreographer import is no longer necessary if you use the more idiomatic withFrameNanos approach for Compose timing.

import androidx.compose.foundation.Image
import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Arrangement
Expand Down Expand Up @@ -40,7 +41,6 @@ import com.tejpratapsingh.motionlib.core.MotionAudio
import com.tejpratapsingh.motionlib.core.MotionConfig
import com.tejpratapsingh.motionlib.core.motion.MotionVideoProducer
import com.tejpratapsingh.motionlib.core.provideCurrentConfig
import kotlinx.coroutines.delay
import java.util.Locale

@Composable
Expand All @@ -64,31 +64,47 @@ fun MotionVideoPlayerCompose(
previewBitmap = motionVideoProducer.motionComposerView.getViewBitmap()
}

// Playback loop
LaunchedEffect(isPlaying) {
if (isPlaying) {
val frameDurationMs = (1000.0 / motionConfig.fps).toLong()
var lastFrameTime = System.currentTimeMillis()

while (isPlaying) {
val currentTime = System.currentTimeMillis()
val elapsed = currentTime - lastFrameTime

if (elapsed >= frameDurationMs) {
val framesToAdvance = (elapsed / frameDurationMs).toInt().coerceAtLeast(1)
val nextFrame = currentFrame + framesToAdvance

if (nextFrame <= totalFrames) {
currentFrame = nextFrame
} else {
// Loop
currentFrame = 0
activePlayers.values.forEach { it.stop(); it.release() }
activePlayers.clear()
// Playback loop driven by display vsync; advances by video fps time budget.
DisposableEffect(isPlaying, motionConfig.fps, totalFrames) {
if (!isPlaying) {
onDispose { }
} else {
val choreographer = Choreographer.getInstance()
val frameDurationNanos = (1_000_000_000L / motionConfig.fps).coerceAtLeast(1L)

@amazon-q-developer amazon-q-developer Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 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.

Suggested change
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
}

@coderabbitai coderabbitai Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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).

var lastFrameNanos = 0L
var accumulatedNanos = 0L

val callback = object : Choreographer.FrameCallback {
override fun doFrame(frameTimeNanos: Long) {
if (!isPlaying) return

@amazon-q-developer amazon-q-developer Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 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.


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) {

@amazon-q-developer amazon-q-developer Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 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.

Suggested change
if (nextFrame <= totalFrames) {
if (nextFrame < totalFrames) {

currentFrame = nextFrame

@amazon-q-developer amazon-q-developer Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 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.

} else {
currentFrame = 0
activePlayers.values.forEach { it.stop(); it.release() }
activePlayers.clear()
}
}
lastFrameTime = currentTime

choreographer.postFrameCallback(this)
Comment on lines +86 to +100

@amazon-q-developer amazon-q-developer Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 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.

Suggested change
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)
}

}
delay(10) // Smooth check
}

choreographer.postFrameCallback(callback)

onDispose {
choreographer.removeFrameCallback(callback)
}
}
}
Comment on lines +68 to 110

@gemini-code-assist gemini-code-assist Bot May 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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()
                    }
                }
            }
        }
    }

Expand Down
Loading