fix: create plugins using media3 lib#73
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
motion-lib | 6fbcddd | Commit Preview URL Branch Preview URL |
Jun 14 2026, 06:41 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
sdui | 6fbcddd | Commit Preview URL Branch Preview URL |
Jun 14 2026, 06:41 PM |
📝 WalkthroughWalkthroughA new Changesmedia3-motion-ext module
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lyrics | 6fbcddd | Commit Preview URL Branch Preview URL |
Jun 14 2026, 06:42 PM |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds media3 library integration for motion effects. Critical issues identified that must be fixed before merge:
Critical Fixes Required:
- ArrayIndexOutOfBoundsException risk in Media3Utils.toColorMatrix - missing bounds checks before accessing array indices 12-14
- UninitializedPropertyAccessException risk in all effect classes - missing initialization checks for lateinit motionView property
- Performance regressions in plugin classes - repeated object allocations on every frame/apply call
Verification Needed:
The matrix conversion logic in Media3Utils needs validation to ensure the column-major to row-major transformation is mathematically correct, especially the offset scaling by 255f.
Please address the critical crash risks before merging.
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.
| colorMatrixArray[4] = media3Matrix[12] * 255f // e (Offset, normalized to 0-255) | ||
|
|
||
| // Row 1 (Green) | ||
| colorMatrixArray[5] = media3Matrix[1] // m1 | ||
| colorMatrixArray[6] = media3Matrix[5] // m5 | ||
| colorMatrixArray[7] = media3Matrix[9] // m9 | ||
| colorMatrixArray[8] = 0f // i | ||
| colorMatrixArray[9] = media3Matrix[13] * 255f // j | ||
|
|
||
| // Row 2 (Blue) | ||
| colorMatrixArray[10] = media3Matrix[2] // m2 | ||
| colorMatrixArray[11] = media3Matrix[6] // m6 | ||
| colorMatrixArray[12] = media3Matrix[10]// m10 | ||
| colorMatrixArray[13] = 0f // n | ||
| colorMatrixArray[14] = media3Matrix[14] * 255f // o |
There was a problem hiding this comment.
🛑 Logic Error: Missing bounds check before array access will cause ArrayIndexOutOfBoundsException. Add validation to prevent crashes when accessing media3Matrix indices 12, 13, and 14.
| colorMatrixArray[4] = media3Matrix[12] * 255f // e (Offset, normalized to 0-255) | |
| // Row 1 (Green) | |
| colorMatrixArray[5] = media3Matrix[1] // m1 | |
| colorMatrixArray[6] = media3Matrix[5] // m5 | |
| colorMatrixArray[7] = media3Matrix[9] // m9 | |
| colorMatrixArray[8] = 0f // i | |
| colorMatrixArray[9] = media3Matrix[13] * 255f // j | |
| // Row 2 (Blue) | |
| colorMatrixArray[10] = media3Matrix[2] // m2 | |
| colorMatrixArray[11] = media3Matrix[6] // m6 | |
| colorMatrixArray[12] = media3Matrix[10]// m10 | |
| colorMatrixArray[13] = 0f // n | |
| colorMatrixArray[14] = media3Matrix[14] * 255f // o | |
| // Row 0 (Red) | |
| colorMatrixArray[0] = media3Matrix[0] // m0 | |
| colorMatrixArray[1] = media3Matrix[4] // m4 | |
| colorMatrixArray[2] = media3Matrix[8] // m8 | |
| colorMatrixArray[3] = 0f // d (Alpha contribution to Red) | |
| colorMatrixArray[4] = if (media3Matrix.size > 12) media3Matrix[12] * 255f else 0f // e (Offset, normalized to 0-255) | |
| // Row 1 (Green) | |
| colorMatrixArray[5] = media3Matrix[1] // m1 | |
| colorMatrixArray[6] = media3Matrix[5] // m5 | |
| colorMatrixArray[7] = media3Matrix[9] // m9 | |
| colorMatrixArray[8] = 0f // i | |
| colorMatrixArray[9] = if (media3Matrix.size > 13) media3Matrix[13] * 255f else 0f // j | |
| // Row 2 (Blue) | |
| colorMatrixArray[10] = media3Matrix[2] // m2 | |
| colorMatrixArray[11] = media3Matrix[6] // m6 | |
| colorMatrixArray[12] = media3Matrix[10]// m10 | |
| colorMatrixArray[13] = 0f // n | |
| colorMatrixArray[14] = if (media3Matrix.size > 14) media3Matrix[14] * 255f else 0f // o |
| override fun forFrame(frame: Int): MotionView { | ||
| if (motionView !is View) return motionView | ||
| val view = motionView as View |
There was a problem hiding this comment.
🛑 Crash Risk: Missing lateinit initialization check causes UninitializedPropertyAccessException. Add check before accessing motionView property to prevent runtime crashes.
| override fun forFrame(frame: Int): MotionView { | |
| if (motionView !is View) return motionView | |
| val view = motionView as View | |
| override fun forFrame(frame: Int): MotionView { | |
| if (!::motionView.isInitialized || motionView !is View) return motionView | |
| val view = motionView as View |
| override fun forFrame(frame: Int): MotionView { | ||
| if (motionView !is View) return motionView | ||
| val view = motionView as View |
There was a problem hiding this comment.
🛑 Crash Risk: Missing lateinit initialization check causes UninitializedPropertyAccessException. Add check before accessing motionView property to prevent runtime crashes.
| override fun forFrame(frame: Int): MotionView { | |
| if (motionView !is View) return motionView | |
| val view = motionView as View | |
| override fun forFrame(frame: Int): MotionView { | |
| if (!::motionView.isInitialized || motionView !is View) return motionView | |
| val view = motionView as View |
| val brightnessEffect = Brightness(brightness) | ||
| val matrix = brightnessEffect.getMatrix(0L, false) | ||
| val colorMatrix = Media3Utils.toColorMatrix(matrix) |
There was a problem hiding this comment.
🛑 Performance Regression: Creating new Brightness instance and matrix conversion on every frame causes unnecessary allocations. Cache the colorMatrix instance if brightness doesn't change.
| val rgbAdjustment = RgbAdjustment.Builder() | ||
| .setRedScale(redScale) | ||
| .setGreenScale(greenScale) | ||
| .setBlueScale(blueScale) | ||
| .build() | ||
|
|
||
| val matrix = rgbAdjustment.getMatrix(0L, false) | ||
| val colorMatrix = Media3Utils.toColorMatrix(matrix) |
There was a problem hiding this comment.
🛑 Performance Regression: Creating new RgbAdjustment and matrix conversion on every apply call causes unnecessary allocations. Initialize the colorMatrix once in the constructor to avoid repeated overhead.
![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 introduces a new module, media3-motion-ext, which integrates Media3-based effects and plugins (such as brightness, contrast, grayscale, invert, and RGB adjustments) into the motion library. The review feedback highlights several critical issues and optimization opportunities: first, RenderEffect should be cleared whenever a frame is outside the active range to support bidirectional scrubbing correctly; second, constant effects and plugins should cache their filters or effects to avoid high allocation overhead; third, a potential crash when processing HARDWARE bitmaps in plugins must be resolved by falling back to ARGB_8888; and finally, the media3-effect dependency should be declared in the version catalog rather than using string interpolation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } |
There was a problem hiding this comment.
When scrubbing backwards or jumping to a frame before startFrame, the RenderEffect will persist on the view because it is only cleared when frame > endFrame. To support bidirectional playback and scrubbing correctly, clear the RenderEffect whenever the frame is outside the active range. Additionally, checking if view.renderEffect is not null before clearing avoids unnecessary invalidations.
| if (frame !in startFrame..endFrame) { | |
| if (frame > endFrame) { | |
| view.setRenderEffect(null) | |
| } | |
| return motionView | |
| } | |
| if (frame !in startFrame..endFrame) { | |
| if (view.renderEffect != null) { | |
| view.setRenderEffect(null) | |
| } | |
| return motionView | |
| } |
|
|
||
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView |
There was a problem hiding this comment.
When scrubbing backwards or jumping to a frame before startFrame, the RenderEffect will persist on the view because it is only cleared when frame > endFrame. To support bidirectional playback and scrubbing correctly, clear the RenderEffect whenever the frame is outside the active range. Additionally, checking if view.renderEffect is not null before clearing avoids unnecessary invalidations.
if (frame !in startFrame..endFrame) {
if (view.renderEffect != null) {
view.setRenderEffect(null)
}
return motionView
}| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } |
There was a problem hiding this comment.
When scrubbing backwards or jumping to a frame before startFrame, the RenderEffect will persist on the view because it is only cleared when frame > endFrame. To support bidirectional playback and scrubbing correctly, clear the RenderEffect whenever the frame is outside the active range. Additionally, checking if view.renderEffect is not null before clearing avoids unnecessary invalidations.
| if (frame !in startFrame..endFrame) { | |
| if (frame > endFrame) { | |
| view.setRenderEffect(null) | |
| } | |
| return motionView | |
| } | |
| if (frame !in startFrame..endFrame) { | |
| if (view.renderEffect != null) { | |
| view.setRenderEffect(null) | |
| } | |
| return motionView | |
| } |
| class GrayscaleEffect( | ||
| override val startFrame: Int, | ||
| override val endFrame: Int, | ||
| ) : MotionEffect { | ||
| override lateinit var motionView: MotionView | ||
|
|
||
| override fun forFrame(frame: Int): MotionView { | ||
| if (motionView !is View) return motionView | ||
| val view = motionView as View | ||
|
|
||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.S) return motionView | ||
|
|
||
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } | ||
|
|
||
| val grayscaleFilter = RgbFilter.createGrayscaleFilter() | ||
| val matrix = grayscaleFilter.getMatrix(0L, false) | ||
| val colorMatrix = Media3Utils.toColorMatrix(matrix) | ||
|
|
||
| val colorFilter = ColorMatrixColorFilter(colorMatrix) | ||
| view.setRenderEffect(RenderEffect.createColorFilterEffect(colorFilter)) | ||
|
|
||
| return motionView | ||
| } | ||
| } |
There was a problem hiding this comment.
Since GrayscaleEffect is a constant effect (its matrix does not change per frame), we can cache the RenderEffect instance to avoid allocating multiple objects (such as GrayscaleFilter, FloatArray, ColorMatrix, ColorMatrixColorFilter, and RenderEffect) on every single frame. This significantly improves performance and prevents GC overhead during animations. Additionally, clearing the RenderEffect when the frame is outside the active range ensures correct behavior during bidirectional scrubbing.
class GrayscaleEffect(
override val startFrame: Int,
override val endFrame: Int,
) : MotionEffect {
override lateinit var motionView: MotionView
private var cachedRenderEffect: RenderEffect? = null
override fun forFrame(frame: Int): MotionView {
if (motionView !is View) return motionView
val view = motionView as View
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.S) return motionView
if (frame !in startFrame..endFrame) {
if (view.renderEffect != null) {
view.setRenderEffect(null)
}
return motionView
}
if (cachedRenderEffect == null) {
val grayscaleFilter = RgbFilter.createGrayscaleFilter()
val matrix = grayscaleFilter.getMatrix(0L, false)
val colorMatrix = Media3Utils.toColorMatrix(matrix)
val colorFilter = ColorMatrixColorFilter(colorMatrix)
cachedRenderEffect = RenderEffect.createColorFilterEffect(colorFilter)
}
view.setRenderEffect(cachedRenderEffect)
return motionView
}
}| class InvertEffect( | ||
| override val startFrame: Int, | ||
| override val endFrame: Int, | ||
| ) : MotionEffect { | ||
| override lateinit var motionView: MotionView | ||
|
|
||
| override fun forFrame(frame: Int): MotionView { | ||
| if (motionView !is View) return motionView | ||
| val view = motionView as View | ||
|
|
||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.S) return motionView | ||
|
|
||
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } | ||
|
|
||
| val invertedFilter = RgbFilter.createInvertedFilter() | ||
| val matrix = invertedFilter.getMatrix(0L, false) | ||
| val colorMatrix = Media3Utils.toColorMatrix(matrix) | ||
|
|
||
| val colorFilter = ColorMatrixColorFilter(colorMatrix) | ||
| view.setRenderEffect(RenderEffect.createColorFilterEffect(colorFilter)) | ||
|
|
||
| return motionView | ||
| } | ||
| } |
There was a problem hiding this comment.
Since InvertEffect is a constant effect (its matrix does not change per frame), we can cache the RenderEffect instance to avoid allocating multiple objects (such as InvertedFilter, FloatArray, ColorMatrix, ColorMatrixColorFilter, and RenderEffect) on every single frame. This significantly improves performance and prevents GC overhead during animations. Additionally, clearing the RenderEffect when the frame is outside the active range ensures correct behavior during bidirectional scrubbing.
class InvertEffect(
override val startFrame: Int,
override val endFrame: Int,
) : MotionEffect {
override lateinit var motionView: MotionView
private var cachedRenderEffect: RenderEffect? = null
override fun forFrame(frame: Int): MotionView {
if (motionView !is View) return motionView
val view = motionView as View
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.S) return motionView
if (frame !in startFrame..endFrame) {
if (view.renderEffect != null) {
view.setRenderEffect(null)
}
return motionView
}
if (cachedRenderEffect == null) {
val invertedFilter = RgbFilter.createInvertedFilter()
val matrix = invertedFilter.getMatrix(0L, false)
val colorMatrix = Media3Utils.toColorMatrix(matrix)
val colorFilter = ColorMatrixColorFilter(colorMatrix)
cachedRenderEffect = RenderEffect.createColorFilterEffect(colorFilter)
}
view.setRenderEffect(cachedRenderEffect)
return motionView
}
}| @OptIn(UnstableApi::class) | ||
| class ContrastPlugin(val contrast: Float) : MotionPlugin { | ||
| override fun apply(input: Bitmap): Bitmap { | ||
| val output = Bitmap.createBitmap(input.width, input.height, input.config ?: Bitmap.Config.ARGB_8888) | ||
| val canvas = Canvas(output) | ||
| val paint = Paint() | ||
|
|
||
| val contrastEffect = Contrast(contrast) | ||
| val matrix = contrastEffect.getMatrix(0L, false) | ||
| val colorMatrix = Media3Utils.toColorMatrix(matrix) | ||
|
|
||
| paint.colorFilter = ColorMatrixColorFilter(colorMatrix) | ||
| canvas.drawBitmap(input, 0f, 0f, paint) | ||
|
|
||
| return output | ||
| } | ||
| } |
There was a problem hiding this comment.
If the input bitmap has a HARDWARE configuration (which is common when loaded via modern image loading libraries with hardware acceleration), calling Canvas(output) with a bitmap created using input.config will crash with an IllegalArgumentException. To prevent this, check if the config is HARDWARE and fallback to ARGB_8888. Additionally, since the contrast parameter is constant for a given plugin instance, we can cache the ColorMatrixColorFilter using lazy to avoid recreating it on every apply call.
class ContrastPlugin(val contrast: Float) : MotionPlugin {
private val colorFilter by lazy {
val contrastEffect = Contrast(contrast)
val matrix = contrastEffect.getMatrix(0L, false)
val colorMatrix = Media3Utils.toColorMatrix(matrix)
ColorMatrixColorFilter(colorMatrix)
}
override fun apply(input: Bitmap): Bitmap {
val config = if (input.config == Bitmap.Config.HARDWARE) Bitmap.Config.ARGB_8888 else (input.config ?: Bitmap.Config.ARGB_8888)
val output = Bitmap.createBitmap(input.width, input.height, config)
val canvas = Canvas(output)
val paint = Paint().apply {
colorFilter = this@ContrastPlugin.colorFilter
}
canvas.drawBitmap(input, 0f, 0f, paint)
return output
}
}| class GrayscalePlugin : MotionPlugin { | ||
| override fun apply(input: Bitmap): Bitmap { | ||
| val output = Bitmap.createBitmap(input.width, input.height, input.config ?: Bitmap.Config.ARGB_8888) | ||
| val canvas = Canvas(output) | ||
| val paint = Paint() | ||
|
|
||
| val grayscaleFilter = RgbFilter.createGrayscaleFilter() | ||
| val matrix = grayscaleFilter.getMatrix(0L, false) | ||
| val colorMatrix = Media3Utils.toColorMatrix(matrix) | ||
|
|
||
| paint.colorFilter = ColorMatrixColorFilter(colorMatrix) | ||
| canvas.drawBitmap(input, 0f, 0f, paint) | ||
|
|
||
| return output | ||
| } | ||
| } |
There was a problem hiding this comment.
If the input bitmap has a HARDWARE configuration (which is common when loaded via modern image loading libraries with hardware acceleration), calling Canvas(output) with a bitmap created using input.config will crash with an IllegalArgumentException. To prevent this, check if the config is HARDWARE and fallback to ARGB_8888. Additionally, since the grayscale filter is constant, we can cache the ColorMatrixColorFilter using lazy to avoid recreating it on every apply call.
class GrayscalePlugin : MotionPlugin {
private val colorFilter by lazy {
val grayscaleFilter = RgbFilter.createGrayscaleFilter()
val matrix = grayscaleFilter.getMatrix(0L, false)
val colorMatrix = Media3Utils.toColorMatrix(matrix)
ColorMatrixColorFilter(colorMatrix)
}
override fun apply(input: Bitmap): Bitmap {
val config = if (input.config == Bitmap.Config.HARDWARE) Bitmap.Config.ARGB_8888 else (input.config ?: Bitmap.Config.ARGB_8888)
val output = Bitmap.createBitmap(input.width, input.height, config)
val canvas = Canvas(output)
val paint = Paint().apply {
colorFilter = this@GrayscalePlugin.colorFilter
}
canvas.drawBitmap(input, 0f, 0f, paint)
return output
}
}| class InvertPlugin : MotionPlugin { | ||
| override fun apply(input: Bitmap): Bitmap { | ||
| val output = Bitmap.createBitmap(input.width, input.height, input.config ?: Bitmap.Config.ARGB_8888) | ||
| val canvas = Canvas(output) | ||
| val paint = Paint() | ||
|
|
||
| val invertedFilter = RgbFilter.createInvertedFilter() | ||
| val matrix = invertedFilter.getMatrix(0L, false) | ||
| val colorMatrix = Media3Utils.toColorMatrix(matrix) | ||
|
|
||
| paint.colorFilter = ColorMatrixColorFilter(colorMatrix) | ||
| canvas.drawBitmap(input, 0f, 0f, paint) | ||
|
|
||
| return output | ||
| } | ||
| } |
There was a problem hiding this comment.
If the input bitmap has a HARDWARE configuration (which is common when loaded via modern image loading libraries with hardware acceleration), calling Canvas(output) with a bitmap created using input.config will crash with an IllegalArgumentException. To prevent this, check if the config is HARDWARE and fallback to ARGB_8888. Additionally, since the inverted filter is constant, we can cache the ColorMatrixColorFilter using lazy to avoid recreating it on every apply call.
class InvertPlugin : MotionPlugin {
private val colorFilter by lazy {
val invertedFilter = RgbFilter.createInvertedFilter()
val matrix = invertedFilter.getMatrix(0L, false)
val colorMatrix = Media3Utils.toColorMatrix(matrix)
ColorMatrixColorFilter(colorMatrix)
}
override fun apply(input: Bitmap): Bitmap {
val config = if (input.config == Bitmap.Config.HARDWARE) Bitmap.Config.ARGB_8888 else (input.config ?: Bitmap.Config.ARGB_8888)
val output = Bitmap.createBitmap(input.width, input.height, config)
val canvas = Canvas(output)
val paint = Paint().apply {
colorFilter = this@InvertPlugin.colorFilter
}
canvas.drawBitmap(input, 0f, 0f, paint)
return output
}
}| class RgbPlugin( | ||
| val redScale: Float = 1f, | ||
| val greenScale: Float = 1f, | ||
| val blueScale: Float = 1f, | ||
| ) : MotionPlugin { | ||
| override fun apply(input: Bitmap): Bitmap { | ||
| val output = Bitmap.createBitmap(input.width, input.height, input.config ?: Bitmap.Config.ARGB_8888) | ||
| val canvas = Canvas(output) | ||
| val paint = Paint() | ||
|
|
||
| val rgbAdjustment = RgbAdjustment.Builder() | ||
| .setRedScale(redScale) | ||
| .setGreenScale(greenScale) | ||
| .setBlueScale(blueScale) | ||
| .build() | ||
|
|
||
| val matrix = rgbAdjustment.getMatrix(0L, false) | ||
| val colorMatrix = Media3Utils.toColorMatrix(matrix) | ||
|
|
||
| paint.colorFilter = ColorMatrixColorFilter(colorMatrix) | ||
| canvas.drawBitmap(input, 0f, 0f, paint) | ||
|
|
||
| return output | ||
| } | ||
| } |
There was a problem hiding this comment.
If the input bitmap has a HARDWARE configuration (which is common when loaded via modern image loading libraries with hardware acceleration), calling Canvas(output) with a bitmap created using input.config will crash with an IllegalArgumentException. To prevent this, check if the config is HARDWARE and fallback to ARGB_8888. Additionally, since the RGB scaling parameters are constant for a given plugin instance, we can cache the ColorMatrixColorFilter using lazy to avoid recreating it on every apply call.
class RgbPlugin(
val redScale: Float = 1f,
val greenScale: Float = 1f,
val blueScale: Float = 1f,
) : MotionPlugin {
private val colorFilter by lazy {
val rgbAdjustment = RgbAdjustment.Builder()
.setRedScale(redScale)
.setGreenScale(greenScale)
.setBlueScale(blueScale)
.build()
val matrix = rgbAdjustment.getMatrix(0L, false)
val colorMatrix = Media3Utils.toColorMatrix(matrix)
ColorMatrixColorFilter(colorMatrix)
}
override fun apply(input: Bitmap): Bitmap {
val config = if (input.config == Bitmap.Config.HARDWARE) Bitmap.Config.ARGB_8888 else (input.config ?: Bitmap.Config.ARGB_8888)
val output = Bitmap.createBitmap(input.width, input.height, config)
val canvas = Canvas(output)
val paint = Paint().apply {
colorFilter = this@RgbPlugin.colorFilter
}
canvas.drawBitmap(input, 0f, 0f, paint)
return output
}
}| // Assuming we add media3-effect to libs.versions.toml if missing, | ||
| // but based on research it should be available or we can use the same version as common | ||
| implementation "androidx.media3:media3-effect:${libs.versions.media3Version.get()}" |
There was a problem hiding this comment.
Instead of using string interpolation with libs.versions.media3Version.get(), it is more idiomatic and cleaner to define the media3-effect dependency in your libs.versions.toml version catalog and reference it directly as a type-safe accessor.
implementation libs.androidx.media3.effect
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
modules/media3-motion-ext/src/test/java/com/tejpratapsingh/motionlib/media3/Media3ExtensionTest.kt (1)
23-53: Replace tautological interface checks with behavior assertions.From lines 23–53,
assertTrue(x is MotionEffect/MotionPlugin)checks are guaranteed by the explicit interface implementation in class declarations (: MotionEffect,: MotionPlugin), providing no regression protection. Test actual behavior instead: verify frame boundary handling (startFrame/endFrame constraints) for effects, and bitmap transformation correctness for plugins.🤖 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/media3-motion-ext/src/test/java/com/tejpratapsingh/motionlib/media3/Media3ExtensionTest.kt` around lines 23 - 53, Replace the tautological interface checks in the testEffectsImplementation() and testPluginsImplementation() methods with behavior assertions that test actual functionality. For the effect objects (BrightnessEffect, ContrastEffect, GrayscaleEffect, InvertEffect, RgbEffect), verify frame boundary handling by asserting constraints on startFrame and endFrame values. For the plugin objects (BrightnessPlugin, ContrastPlugin, GrayscalePlugin, InvertPlugin, RgbPlugin), test bitmap transformation correctness by verifying that the plugins properly modify image properties or output. Remove the trivial isinstance checks (assertTrue(x is MotionEffect/MotionPlugin)) and add meaningful assertions that exercise the actual behavior of each effect and plugin.
🤖 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/media3-motion-ext/build.gradle`:
- Line 50: The media3-effect dependency uses direct string interpolation with
version variables instead of referencing the version catalog like the other
media3 artifacts (media3-common and media3-exoplayer). Add a new entry for
media3-effect in the gradle/libs.versions.toml file under the [libraries]
section following the same pattern as the other androidx.media3 entries, then
update the implementation dependency from the interpolated version string to use
the catalog reference format (libs.androidx.media3.effect) to maintain
consistency across all media3 dependencies.
In
`@modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/BrightnessEffect.kt`:
- Line 56: Effect implementations are overwriting each other because they call
setRenderEffect() which replaces any existing effect instead of composing them.
In BrightnessEffect.kt at line 56, ContrastEffect.kt at line 56, and
GrayscaleEffect.kt at line 42, replace the direct setRenderEffect() call with a
composition strategy that chains effects together. Retrieve the existing
RenderEffect from the view (if any), use RenderEffect.createChainEffect() to
compose the new effect with the existing one, and then apply the composed result
via setRenderEffect(). This ensures multiple effects on the same view are all
visible in the final render.
In
`@modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/BrightnessPlugin.kt`:
- Line 20: The elvis operator in the Bitmap.createBitmap call at
BrightnessPlugin.kt line 20 only protects against null configs but allows
HARDWARE config to propagate, which causes a crash when creating a Canvas since
hardware bitmaps are immutable. Replace the condition `input.config ?:
Bitmap.Config.ARGB_8888` with an explicit check that filters out HARDWARE config
by verifying that the input config is not null and not equal to
Bitmap.Config.HARDWARE before using it; otherwise default to
Bitmap.Config.ARGB_8888. Apply this same fix at ContrastPlugin.kt line 20,
GrayscalePlugin.kt line 19, InvertPlugin.kt line 19, and RgbPlugin.kt line 23.
---
Nitpick comments:
In
`@modules/media3-motion-ext/src/test/java/com/tejpratapsingh/motionlib/media3/Media3ExtensionTest.kt`:
- Around line 23-53: Replace the tautological interface checks in the
testEffectsImplementation() and testPluginsImplementation() methods with
behavior assertions that test actual functionality. For the effect objects
(BrightnessEffect, ContrastEffect, GrayscaleEffect, InvertEffect, RgbEffect),
verify frame boundary handling by asserting constraints on startFrame and
endFrame values. For the plugin objects (BrightnessPlugin, ContrastPlugin,
GrayscalePlugin, InvertPlugin, RgbPlugin), test bitmap transformation
correctness by verifying that the plugins properly modify image properties or
output. Remove the trivial isinstance checks (assertTrue(x is
MotionEffect/MotionPlugin)) and add meaningful assertions that exercise the
actual behavior of each effect and plugin.
🪄 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: 4c4aed41-8d1b-4d48-ad38-4970ed810e32
📒 Files selected for processing (16)
.idea/deploymentTargetSelector.xml.idea/gradle.xmlmodules/media3-motion-ext/build.gradlemodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/Media3Utils.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/BrightnessEffect.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/ContrastEffect.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/GrayscaleEffect.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/InvertEffect.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/RgbEffect.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/BrightnessPlugin.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/ContrastPlugin.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/GrayscalePlugin.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/InvertPlugin.ktmodules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/RgbPlugin.ktmodules/media3-motion-ext/src/test/java/com/tejpratapsingh/motionlib/media3/Media3ExtensionTest.ktsettings.gradle
| implementation libs.androidx.media3.exoplayer | ||
| // Assuming we add media3-effect to libs.versions.toml if missing, | ||
| // but based on research it should be available or we can use the same version as common | ||
| implementation "androidx.media3:media3-effect:${libs.versions.media3Version.get()}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify media3-effect artifact availability at version 1.10.0
curl -s "https://repo1.maven.org/maven2/androidx/media3/media3-effect/1.10.0/" | grep -q "media3-effect-1.10.0.aar" && echo "✓ media3-effect:1.10.0 exists" || echo "✗ media3-effect:1.10.0 not found"
# Check if media3-effect is in the version catalog
rg -n "media3.effect|media3-effect" gradle/libs.versions.tomlRepository: tejpratap46/AndroidVideoMotion
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Find available versions of media3-effect in Maven Central
echo "=== Checking available media3-effect versions ==="
curl -s "https://repo1.maven.org/maven2/androidx/media3/media3-effect/" | grep -oP '">[\d.]+/<' | tr -d '">' | sort -V | tail -10
# Check the gradle/libs.versions.toml file
echo -e "\n=== Checking gradle/libs.versions.toml for media3 references ==="
rg -A 5 "media3" gradle/libs.versions.toml || echo "No media3 references found"
# Read the build.gradle file around line 50
echo -e "\n=== Checking build.gradle around line 50 ==="
sed -n '40,60p' modules/media3-motion-ext/build.gradleRepository: tejpratap46/AndroidVideoMotion
Length of output: 1570
🏁 Script executed:
#!/bin/bash
# Better verification of media3-effect artifact existence using Maven metadata
echo "=== Checking media3-effect in Maven Central ==="
curl -s "https://repo1.maven.org/maven2/androidx/media3/media3-effect/maven-metadata.xml" | grep -oP '<version>\K[^<]+' | tail -5
# Also check specifically for 1.10.0
echo -e "\n=== Checking for media3-effect 1.10.0 directly ==="
curl -s -I "https://repo1.maven.org/maven2/androidx/media3/media3-effect/1.10.0/media3-effect-1.10.0.aar" | head -1
# Check what other media3 artifacts are available at that version
echo -e "\n=== Other media3 artifacts at version 1.10.0 ==="
curl -s "https://repo1.maven.org/maven2/androidx/media3/" | grep -oP '>media3-[^/<]+' | sort -u | head -10Repository: tejpratap46/AndroidVideoMotion
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Verify the artifact truly exists with Python for more reliable checking
python3 << 'PY'
import urllib.request
import urllib.error
url = "https://repo1.maven.org/maven2/androidx/media3/media3-effect/1.10.0/media3-effect-1.10.0.aar"
try:
response = urllib.request.urlopen(url, timeout=5)
print(f"✓ media3-effect:1.10.0 exists (HTTP {response.status})")
except urllib.error.HTTPError as e:
print(f"✗ media3-effect:1.10.0 not found (HTTP {e.code})")
except Exception as e:
print(f"? Could not verify: {e}")
PY
# Also verify other media3 artifacts at same version to confirm pattern
echo -e "\n=== Verifying media3-common at 1.10.0 ==="
python3 << 'PY'
import urllib.request
url = "https://repo1.maven.org/maven2/androidx/media3/media3-common/1.10.0/media3-common-1.10.0.aar"
try:
response = urllib.request.urlopen(url, timeout=5)
print(f"✓ media3-common:1.10.0 exists")
except:
print(f"✗ media3-common:1.10.0 not found")
PYRepository: tejpratap46/AndroidVideoMotion
Length of output: 304
Add media3-effect to the version catalog for consistency.
Line 50 uses string interpolation to construct the dependency, while lines 46-47 reference the version catalog (libs.androidx.media3.common, libs.androidx.media3.exoplayer). Add media3-effect to gradle/libs.versions.toml under [libraries] and update the dependency to use the catalog reference instead, matching the pattern for other media3 artifacts.
🤖 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/media3-motion-ext/build.gradle` at line 50, The media3-effect
dependency uses direct string interpolation with version variables instead of
referencing the version catalog like the other media3 artifacts (media3-common
and media3-exoplayer). Add a new entry for media3-effect in the
gradle/libs.versions.toml file under the [libraries] section following the same
pattern as the other androidx.media3 entries, then update the implementation
dependency from the interpolated version string to use the catalog reference
format (libs.androidx.media3.effect) to maintain consistency across all media3
dependencies.
| val colorMatrix = Media3Utils.toColorMatrix(matrix) | ||
|
|
||
| val colorFilter = ColorMatrixColorFilter(colorMatrix) | ||
| view.setRenderEffect(RenderEffect.createColorFilterEffect(colorFilter)) |
There was a problem hiding this comment.
All effect classes overwrite each other via setRenderEffect.
All five effect implementations call view.setRenderEffect(...) which replaces any existing RenderEffect rather than composing. When multiple effects target the same view in a single frame (e.g., brightness + contrast), only the last-applied effect is visible.
modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/BrightnessEffect.kt#L56-L56: replacesetRenderEffect(createColorFilterEffect(...))with a composition strategy usingRenderEffect.createChainEffect().modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/ContrastEffect.kt#L56-L56: same composition fix needed.modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/GrayscaleEffect.kt#L42-L42: same composition fix needed.- (Also applies to
InvertEffectandRgbEffectimplementations not shown in this review batch.)
📍 Affects 3 files
modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/BrightnessEffect.kt#L56-L56(this comment)modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/ContrastEffect.kt#L56-L56modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/GrayscaleEffect.kt#L42-L42
🤖 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/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/effects/BrightnessEffect.kt`
at line 56, Effect implementations are overwriting each other because they call
setRenderEffect() which replaces any existing effect instead of composing them.
In BrightnessEffect.kt at line 56, ContrastEffect.kt at line 56, and
GrayscaleEffect.kt at line 42, replace the direct setRenderEffect() call with a
composition strategy that chains effects together. Retrieve the existing
RenderEffect from the view (if any), use RenderEffect.createChainEffect() to
compose the new effect with the existing one, and then apply the composed result
via setRenderEffect(). This ensures multiple effects on the same view are all
visible in the final render.
| @OptIn(UnstableApi::class) | ||
| class BrightnessPlugin(val brightness: Float) : MotionPlugin { | ||
| override fun apply(input: Bitmap): Bitmap { | ||
| val output = Bitmap.createBitmap(input.width, input.height, input.config ?: Bitmap.Config.ARGB_8888) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all plugin sites using input.config directly in Bitmap.createBitmap
rg -n "Bitmap\\.createBitmap\\(input\\.width, input\\.height, input\\.config \\?: Bitmap\\.Config\\.ARGB_8888\\)" \
modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/*.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 1140
🏁 Script executed:
#!/bin/bash
# Check BrightnessPlugin implementation to see how output bitmap is used
cat -n modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/BrightnessPlugin.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 1519
🌐 Web query:
Bitmap.Config.HARDWARE Android Canvas drawing compatibility
💡 Result:
In Android, Bitmap.Config.HARDWARE creates bitmaps that are stored exclusively in graphics memory [1][2]. These bitmaps are inherently immutable [1][3] and are intended strictly for display purposes, such as being drawn to a screen [1]. Compatibility with Canvas drawing is subject to strict limitations: 1. Software Rendering Restriction: You cannot draw a HARDWARE bitmap onto a Canvas that is not hardware-accelerated [4][5][6]. Attempting to do so will result in an IllegalStateException with the message "Software rendering doesn't support hardware bitmaps" [7][8][5]. 2. Immutable Property: Because HARDWARE bitmaps are immutable, they cannot be used as the backing store for a Canvas [9]. You cannot instantiate a new Canvas using a HARDWARE bitmap (e.g., new Canvas(hardwareBitmap)) because that operation requires the target bitmap to be mutable [9][3]. 3. Common Workarounds: - If you need to perform software-based drawing or manipulation, you must copy the HARDWARE bitmap into a software-backed format first, such as Bitmap.Config.ARGB_8888, using the copy method: hardwareBitmap.copy(Bitmap.Config.ARGB_8888, false) [10]. - If you are experiencing this crash in a view context (e.g., inside onDraw), ensure the View is not forced into software rendering mode using setLayerType(View.LAYER_TYPE_SOFTWARE, null) or an activity-level flag, unless you have specifically converted the bitmap to a software-compatible format [5]. - For applications using image loading libraries (like Glide or Coil), configure them to disallow hardware bitmaps if you intend to perform further transformations on the retrieved images [7][11]. In summary, HARDWARE bitmaps are optimized for read-only display tasks and are not compatible with general Canvas drawing or offscreen rendering workflows [1][9].
Citations:
- 1: https://developer.android.com/reference/android/graphics/Bitmap.Config
- 2: https://developer.android.com/reference/kotlin/android/graphics/Bitmap.Config
- 3: https://developer.android.com/reference/android/graphics/Bitmap
- 4: https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/graphics/java/android/graphics/BaseCanvas.java
- 5: https://stackoverflow.com/questions/63899262/canvas-draw-bitmap-crash-on-android-10-but-not-on-android-8
- 6: https://android.googlesource.com/platform/frameworks/base/+/079f03f/graphics/java/android/graphics/BaseCanvas.java
- 7: https://stackoverflow.com/questions/47765750/android-o-dragshadowbuilder-throws-illegalstateexception-software-rendering-do
- 8: https://stackoverflow.com/questions/72826447/bitmap-draw-canvas-software-rendering-doesnt-support-hardware-bitmaps
- 9: https://stackoverflow.com/questions/79060984/android-drawing-on-canvas-from-another-canvas-bad-performance
- 10: https://stackoverflow.com/questions/64574453/how-to-convert-a-hardware-bitmap-into-to-a-software-bitmap-in-android
- 11: https://stackoverflow.com/questions/78199376/jetpack-compose-java-lang-illegalstateexception-software-rendering-doesnt-su
Prevent Bitmap.Config.HARDWARE from being used as the output bitmap.
At line 20, the elvis operator input.config ?: Bitmap.Config.ARGB_8888 only protects against null configs. If the input bitmap has Bitmap.Config.HARDWARE, it propagates directly to the output, which then fails when creating a Canvas on line 21. Hardware bitmaps are immutable and incompatible with Canvas drawing—this will crash with "Software rendering doesn't support hardware bitmaps."
Replace input.config with an explicit check that filters out HARDWARE config:
Suggested fix
- val output = Bitmap.createBitmap(input.width, input.height, input.config ?: Bitmap.Config.ARGB_8888)
+ val safeConfig = when (input.config) {
+ null, Bitmap.Config.HARDWARE -> Bitmap.Config.ARGB_8888
+ else -> input.config
+ }
+ val output = Bitmap.createBitmap(input.width, input.height, safeConfig)Apply the same fix at:
modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/ContrastPlugin.ktLine 20modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/GrayscalePlugin.ktLine 19modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/InvertPlugin.ktLine 19modules/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/RgbPlugin.ktLine 23
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val output = Bitmap.createBitmap(input.width, input.height, input.config ?: Bitmap.Config.ARGB_8888) | |
| val safeConfig = when (input.config) { | |
| null, Bitmap.Config.HARDWARE -> Bitmap.Config.ARGB_8888 | |
| else -> input.config | |
| } | |
| val output = Bitmap.createBitmap(input.width, input.height, safeConfig) |
🤖 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/media3-motion-ext/src/main/java/com/tejpratapsingh/motionlib/media3/plugins/BrightnessPlugin.kt`
at line 20, The elvis operator in the Bitmap.createBitmap call at
BrightnessPlugin.kt line 20 only protects against null configs but allows
HARDWARE config to propagate, which causes a crash when creating a Canvas since
hardware bitmaps are immutable. Replace the condition `input.config ?:
Bitmap.Config.ARGB_8888` with an explicit check that filters out HARDWARE config
by verifying that the input config is not null and not equal to
Bitmap.Config.HARDWARE before using it; otherwise default to
Bitmap.Config.ARGB_8888. Apply this same fix at ContrastPlugin.kt line 20,
GrayscalePlugin.kt line 19, InvertPlugin.kt line 19, and RgbPlugin.kt line 23.
1 participant
Closes #72
Summary by CodeRabbit