Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR adds OpenGL ES rendering support to the DotLottie Android library, including new GL-backed UI components (View + Compose) and the required JNI/C++ plumbing to render into GL framebuffers and HardwareBuffer-backed FBOs.
Changes:
- Add JNI/C++ support for setting an OpenGL render target and creating/destroying FBOs backed by
AHardwareBuffer. - Introduce new GL rendering front-ends: a
GLSurfaceViewwidget and Compose composables (with an API 31+ HardwareBuffer path and a fallback GLSurfaceView path). - Refactor event polling/dispatch + Compose pointer input into shared utilities.
Reviewed changes
Copilot reviewed 16 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| dotlottie/src/main/kotlin/com/lottiefiles/dotlottie/core/jni/DotLottiePlayer.kt | Adds new JNI entry points for GL target + HardwareBuffer FBO lifecycle. |
| dotlottie/src/main/kotlin/com/dotlottie/dlplayer/DotLottiePlayer.kt | Adds setGlTarget wrapper API on the Kotlin player. |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/widget/DotLottieGLAnimation.kt | New GLSurfaceView-based widget driving the player on the GL thread. |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/util/GlHardwareRenderer.kt | New dedicated-thread EGL + HardwareBuffer-backed FBO renderer producing hardware Bitmaps. |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/util/DotLottieEventDispatcher.kt | Centralizes player/state machine/internal event dispatch helpers. |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/drawable/DotLottieDrawable.kt | Switches drawable to use centralized event polling/dispatch helper. |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/compose/ui/DotLottieGLSurfaceAnimation.kt | New Compose wrapper around the GLSurfaceView widget fallback path. |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/compose/ui/DotLottieGLAnimationHardwareBuffer.kt | New API 31+ Compose-native HardwareBuffer rendering path. |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/compose/ui/DotLottieGLAnimation.kt | Public Compose entry that selects HardwareBuffer vs GLSurfaceView fallback based on API. |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/compose/ui/DotLottieCommon.kt | Shared Compose helpers (config builder, pointer input, event polling bridge). |
| dotlottie/src/main/java/com/lottiefiles/dotlottie/core/compose/ui/DotLottieAnimation.kt | Refactors to use shared pointer input and shared event polling helper. |
| dotlottie/src/main/cpp/version.txt | Updates embedded dlplayer version. |
| dotlottie/src/main/cpp/jni_bridge.cpp | Implements new JNI methods for GL target + HardwareBuffer FBO creation/destruction + glFinish. |
| dotlottie/src/main/cpp/dotlottie_player.h | Removes web/emscripten-related declarations from the header. |
| dotlottie/src/main/cpp/CMakeLists.txt | Links against EGL and GLESv2. |
| dotlottie/build.gradle.kts | Bumps library version to 0.14.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package com.lottiefiles.dotlottie.core.jni | ||
|
|
||
| import android.graphics.Bitmap | ||
| import android.hardware.HardwareBuffer |
There was a problem hiding this comment.
Referencing android.hardware.HardwareBuffer in this class (import + native method signature) can crash on devices < API 26 during class loading/verification (minSdk is 21). Please move the HardwareBuffer-dependent JNI APIs into a separate @RequiresApi(26+) class/source set (or use reflection/Any + runtime checks) so the core JNI class can load on all supported Android versions.
| import android.hardware.HardwareBuffer |
| // Update render mode based on playback state | ||
| val shouldAnimate = player.isPlaying() || stateMachineIsActive | ||
| post { | ||
| renderMode = if (shouldAnimate) RENDERMODE_CONTINUOUSLY else RENDERMODE_WHEN_DIRTY | ||
| } |
There was a problem hiding this comment.
onDrawFrame() posts a Runnable every frame to update renderMode. This can flood the main thread message queue and cause jank. Consider updating renderMode only when the desired mode changes (track previous value), or switch renderMode in response to state transitions (play/pause/stop/state machine start/stop) instead of every frame.
| if (result != null && result.size == 4) { | ||
| fboIds[i] = result[0] | ||
| texIds[i] = result[1] | ||
| eglImagePtrs[i] = (result[2].toLong() shl 32) or |
There was a problem hiding this comment.
eglImagePtrs is reconstructed from two Ints, but shifting result[2].toLong() can sign-extend if the high 32 bits have the sign bit set, corrupting the pointer. Mask both halves to 0xFFFFFFFFL before shifting/ORing to reliably rebuild the 64-bit value.
| eglImagePtrs[i] = (result[2].toLong() shl 32) or | |
| eglImagePtrs[i] = ((result[2].toLong() and 0xFFFFFFFFL) shl 32) or |
| AHardwareBuffer *nativeBuffer = fromHwBuffer(env, hwBuffer); | ||
| if (!nativeBuffer) { | ||
| LOGE("AHardwareBuffer_fromHardwareBuffer returned null"); | ||
| return nullptr; | ||
| } | ||
|
|
||
| EGLClientBuffer clientBuffer = getNativeClientBuffer(nativeBuffer); | ||
| if (!clientBuffer) { | ||
| LOGE("eglGetNativeClientBufferANDROID returned null"); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
AHardwareBuffer_fromHardwareBuffer() returns a native AHardwareBuffer with an acquired reference, but this function never calls AHardwareBuffer_release() on any path. This will leak native buffer references. Add AHardwareBuffer_release(nativeBuffer) once the EGLImage is created (and also on failure paths).
| eglImagePtrs[i] = (result[2].toLong() shl 32) or | ||
| (result[3].toLong() and 0xFFFFFFFFL) | ||
| } else { | ||
| Log.e(TAG, "Failed to create FBO from HardwareBuffer[$i], result=$result") |
There was a problem hiding this comment.
On failure (result == null/size != 4) this returns early without cleaning up any FBO/texture/EGLImage/HardwareBuffer resources created in earlier loop iterations. Please ensure partial allocations are cleaned up (e.g., call destroyBuffers() before returning, or track and free resources for indices < i).
| Log.e(TAG, "Failed to create FBO from HardwareBuffer[$i], result=$result") | |
| Log.e(TAG, "Failed to create FBO from HardwareBuffer[$i], result=$result") | |
| destroyBuffers() |
| val hwBitmap = Bitmap.wrapHardwareBuffer( | ||
| frontBuffer, | ||
| ColorSpace.get(ColorSpace.Named.SRGB) | ||
| ) | ||
| if (hwBitmap == null) { |
There was a problem hiding this comment.
renderFrame() creates a new Bitmap wrapper via Bitmap.wrapHardwareBuffer() on every tick. This can create significant object churn/GC pressure. Consider caching one Bitmap per HardwareBuffer (2 total) and reusing them, or creating the wrapper once per buffer when buffers are created and reusing it across frames.
| if (dlPlayer != null && dlConfig != null) { | ||
| callback(dlPlayer!!, dlConfig!!) | ||
| } | ||
| // Also set for future recreation | ||
| playerCreatedCallback = callback | ||
| } | ||
|
|
||
| private var playerCreatedCallback: ((DotLottiePlayer, Config) -> Unit)? = null | ||
|
|
There was a problem hiding this comment.
playerCreatedCallback is stored in setOnPlayerCreated but never invoked when the player is (re)created. As a result, Compose/controller wiring via setOnPlayerCreated will not run. Invoke playerCreatedCallback from initPlayerOnGlThread() (after dlPlayer/dlConfig are set) and also after recreations in onSurfaceCreated().
| if (dlPlayer != null && dlConfig != null) { | |
| callback(dlPlayer!!, dlConfig!!) | |
| } | |
| // Also set for future recreation | |
| playerCreatedCallback = callback | |
| } | |
| private var playerCreatedCallback: ((DotLottiePlayer, Config) -> Unit)? = null | |
| val player = dlPlayer | |
| val config = dlConfig | |
| if (player != null && config != null) { | |
| callback(player, config) | |
| } | |
| } |
| // Set up frame callback to receive hardware bitmaps from the renderer | ||
| remember { | ||
| renderer.setFrameCallback(object : GlHardwareRenderer.FrameCallback { | ||
| override fun onFrame(newBitmap: Bitmap) { |
There was a problem hiding this comment.
onFrame() overwrites the previous Bitmap reference without recycling/clearing it. With frequent frames, this can retain many hardware-backed Bitmap wrappers until GC and increase memory pressure. Consider recycling the old Bitmap when replacing it (or reuse/caching strategy) and ensure disposal also clears the last bitmap reference.
| override fun onFrame(newBitmap: Bitmap) { | |
| override fun onFrame(newBitmap: Bitmap) { | |
| val previousBitmap = bitmap | |
| if (previousBitmap !== null && previousBitmap !== newBitmap && !previousBitmap.isRecycled) { | |
| previousBitmap.recycle() | |
| } |
| // Load content + resize when data or layout changes | ||
| LaunchedEffect(animationData, layoutSize) { | ||
| val data = animationData?.getOrNull() ?: return@LaunchedEffect | ||
| val size = layoutSize ?: return@LaunchedEffect | ||
|
|
||
| renderer.resize(size.width.toInt(), size.height.toInt()) | ||
| renderer.loadContent(data) | ||
|
|
||
| // Load state machine AFTER content — both post to the GL handler in order, | ||
| // so stateMachineLoad runs after loadContent completes on the GL thread. | ||
| if (!initialStateMachineId.isNullOrEmpty()) { | ||
| renderer.stateMachineLoad(initialStateMachineId) | ||
| renderer.stateMachineStart() | ||
| } | ||
|
|
||
| drawDstRect.set(0f, 0f, size.width, size.height) | ||
| } | ||
|
|
||
| // Handle resize from layout changes | ||
| LaunchedEffect(layoutSize) { | ||
| val size = layoutSize ?: return@LaunchedEffect | ||
| renderer.resize(size.width.toInt(), size.height.toInt()) | ||
| drawDstRect.set(0f, 0f, size.width, size.height) |
There was a problem hiding this comment.
This LaunchedEffect(layoutSize) duplicates resize/drawDstRect work already performed in the LaunchedEffect(animationData, layoutSize) above, causing redundant resize calls and extra GL-thread work. Consider consolidating into a single effect to avoid duplicate posts.
| // Load content + resize when data or layout changes | |
| LaunchedEffect(animationData, layoutSize) { | |
| val data = animationData?.getOrNull() ?: return@LaunchedEffect | |
| val size = layoutSize ?: return@LaunchedEffect | |
| renderer.resize(size.width.toInt(), size.height.toInt()) | |
| renderer.loadContent(data) | |
| // Load state machine AFTER content — both post to the GL handler in order, | |
| // so stateMachineLoad runs after loadContent completes on the GL thread. | |
| if (!initialStateMachineId.isNullOrEmpty()) { | |
| renderer.stateMachineLoad(initialStateMachineId) | |
| renderer.stateMachineStart() | |
| } | |
| drawDstRect.set(0f, 0f, size.width, size.height) | |
| } | |
| // Handle resize from layout changes | |
| LaunchedEffect(layoutSize) { | |
| val size = layoutSize ?: return@LaunchedEffect | |
| renderer.resize(size.width.toInt(), size.height.toInt()) | |
| drawDstRect.set(0f, 0f, size.width, size.height) | |
| // Load content + handle resize when data or layout changes | |
| LaunchedEffect(animationData, layoutSize) { | |
| val size = layoutSize | |
| // Always resize and update draw rect when layout size is available | |
| if (size != null) { | |
| renderer.resize(size.width.toInt(), size.height.toInt()) | |
| drawDstRect.set(0f, 0f, size.width, size.height) | |
| } | |
| val data = animationData?.getOrNull() | |
| // Load content only when both data and size are available | |
| if (data != null && size != null) { | |
| renderer.loadContent(data) | |
| // Load state machine AFTER content — both post to the GL handler in order, | |
| // so stateMachineLoad runs after loadContent completes on the GL thread. | |
| if (!initialStateMachineId.isNullOrEmpty()) { | |
| renderer.stateMachineLoad(initialStateMachineId) | |
| renderer.stateMachineStart() | |
| } | |
| } |
| if (marker != null) widget.setMarker(marker) | ||
| widget.setLayout(layout.fit, Pair(layout.align[0], layout.align[1])) | ||
| if (themeId != null) widget.setTheme(themeId) |
There was a problem hiding this comment.
When marker/themeId become null, this effect does not clear the previously applied marker/theme (setMarker is only called when marker != null, setTheme only when themeId != null). This can leave stale state after recomposition. Please reset to defaults when values become null (e.g., setMarker("") and resetTheme()).
| if (marker != null) widget.setMarker(marker) | |
| widget.setLayout(layout.fit, Pair(layout.align[0], layout.align[1])) | |
| if (themeId != null) widget.setTheme(themeId) | |
| if (marker != null) { | |
| widget.setMarker(marker) | |
| } else { | |
| widget.setMarker("") | |
| } | |
| widget.setLayout(layout.fit, Pair(layout.align[0], layout.align[1])) | |
| if (themeId != null) { | |
| widget.setTheme(themeId) | |
| } else { | |
| widget.resetTheme() | |
| } |
feat: added open gl es support