motionlib: add suite of RenderEffect-based MotionEffects#64
Merged
Conversation
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
sdui | d035766 | Commit Preview URL Branch Preview URL |
May 28 2026, 07:53 PM |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lyrics | d035766 | Commit Preview URL Branch Preview URL |
May 28 2026, 07:53 PM |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
motion-lib | d035766 | Commit Preview URL Branch Preview URL |
May 28 2026, 07:52 PM |
Contributor
📝 WalkthroughWalkthroughAdds many new MotionEffect implementations (brightness/contrast, grayscale, invert, sepia, offset, pixelate, vignette, vintage, chain), integrates BlurEffect into PopupLyricsTemplate, adds a new VintageLyricsTemplate and registry entry, and introduces an ml-kit-ext module with subject-segmentation plugin and runtime shader effect. ChangesMotion Effects Library
Lyrics Templates
ML Kit Extension Module
SDUI Integration
Sequence Diagram(s)(omitted — changes are library additions and wiring; no multi-component sequential control flow diagram required) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Summary
This PR adds a comprehensive suite of RenderEffect-based motion effects including Vignette, Pixelate, Grayscale, Sepia, Invert, BrightnessContrast, and Offset effects. The implementation follows Android's RenderEffect API and integrates well with the existing MotionEffect framework.
Critical Issues (Must Fix Before Merge)
- ChainEffect: Non-null assertion crash risk on uninitialized motionView
- VignetteEffect: Potential division by zero crash when view dimensions are 0
Summary
The implementation is solid overall, but the two crash risks identified above must be addressed before merging to ensure stability. Once fixed, this will be a valuable addition to the motion effects library.
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.
| ) : MotionEffect { | ||
| private var _motionView: MotionView? = null | ||
| override var motionView: MotionView | ||
| get() = _motionView!! |
Contributor
There was a problem hiding this comment.
🛑 Crash Risk: Non-null assertion on lateinit property can crash if motionView is accessed before initialization. Add null check before returning.
Suggested change
| get() = _motionView!! | |
| get() = _motionView ?: throw IllegalStateException("motionView must be initialized before use") |
Comment on lines
+63
to
+65
| val shader = RuntimeShader(VIGNETTE_SHADER) | ||
| shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat()) | ||
| shader.setFloatUniform("intensity", intensity) |
Contributor
There was a problem hiding this comment.
🛑 Crash Risk: Division by zero will occur if view width or height is 0 during initial layout, causing the shader to compute incorrect UV coordinates and potentially crash. Add dimension validation before shader creation.
Suggested change
| val shader = RuntimeShader(VIGNETTE_SHADER) | |
| shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat()) | |
| shader.setFloatUniform("intensity", intensity) | |
| if (view.width == 0 || view.height == 0) return motionView | |
| val shader = RuntimeShader(VIGNETTE_SHADER) | |
| shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat()) | |
| shader.setFloatUniform("intensity", intensity) |
![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 several new motion effects using Android's RenderEffect and AGSL shaders, and integrates BlurEffect into PopupLyricsTemplate. Key feedback focuses on performance optimizations, such as caching RuntimeShader instances in PixelateEffect and VignetteEffect to prevent rendering jank. Additionally, multiple effects need to clear their RenderEffect when the frame is below the startFrame to handle backward scrubbing correctly. Finally, ChainEffect should be refactored to properly chain effects using RenderEffect.createChainEffect and to use idiomatic lateinit var properties instead of unsafe nullable casts.
Comment on lines
+42
to
+49
| // Chaining is tricky with the current side-effect based architecture. | ||
| // For now, we just call the inner and outer effects, which will | ||
| // each try to set the RenderEffect on the view, with the last one winning. | ||
| // To properly support chaining, we would need to refactor MotionEffect | ||
| // to return a RenderEffect instead of applying it. | ||
| innerEffect.forFrame(frame) | ||
| outerEffect.forFrame(frame) | ||
|
|
There was a problem hiding this comment.
The current placeholder implementation does not actually chain the effects; the outer effect completely overwrites the inner effect. Since we are on API 31+ (guaranteed by the SDK check), we can retrieve the applied RenderEffect from the view after running each effect and chain them properly using RenderEffect.createChainEffect.
innerEffect.forFrame(frame)
val innerRenderEffect = view.getRenderEffect()
outerEffect.forFrame(frame)
val outerRenderEffect = view.getRenderEffect()
if (innerRenderEffect != null && outerRenderEffect != null) {
view.setRenderEffect(RenderEffect.createChainEffect(outerRenderEffect, innerRenderEffect))
} else if (innerRenderEffect != null) {
view.setRenderEffect(innerRenderEffect)
} else if (outerRenderEffect != null) {
view.setRenderEffect(outerRenderEffect)
} else {
view.setRenderEffect(null)
}
Comment on lines
+38
to
+64
| 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.TIRAMISU) return motionView | ||
|
|
||
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } | ||
|
|
||
| val pixelSize = MotionInterpolator.interpolateForRange( | ||
| interpolator = Interpolators(Easings.LINEAR), | ||
| currentFrame = frame, | ||
| frameRange = Pair(startFrame, endFrame), | ||
| valueRange = Pair(fromPixelSize, toPixelSize), | ||
| ) | ||
|
|
||
| val shader = RuntimeShader(PIXELATE_SHADER) | ||
| shader.setFloatUniform("pixelSize", pixelSize) | ||
|
|
||
| view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content")) | ||
|
|
||
| return motionView | ||
| } |
There was a problem hiding this comment.
Recreating the RuntimeShader on every frame is extremely expensive because it compiles the AGSL shader on each call, causing rendering stutters (jank). We should cache the RuntimeShader instance as a class property and reuse it, only updating the uniforms. Additionally, the render effect should be cleared when frame < startFrame.
private var cachedShader: RuntimeShader? = 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.TIRAMISU) return motionView
if (frame !in startFrame..endFrame) {
view.setRenderEffect(null)
return motionView
}
val pixelSize = MotionInterpolator.interpolateForRange(
interpolator = Interpolators(Easings.LINEAR),
currentFrame = frame,
frameRange = Pair(startFrame, endFrame),
valueRange = Pair(fromPixelSize, toPixelSize),
)
val shader = cachedShader ?: RuntimeShader(PIXELATE_SHADER).also { cachedShader = it }
shader.setFloatUniform("pixelSize", pixelSize)
view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content"))
return motionView
}
Comment on lines
+42
to
+70
| 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.TIRAMISU) return motionView | ||
|
|
||
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } | ||
|
|
||
| val intensity = | ||
| MotionInterpolator.interpolateForRange( | ||
| interpolator = Interpolators(Easings.LINEAR), | ||
| currentFrame = frame, | ||
| frameRange = Pair(startFrame, endFrame), | ||
| valueRange = Pair(fromIntensity, toIntensity), | ||
| ) | ||
|
|
||
| val shader = RuntimeShader(VIGNETTE_SHADER) | ||
| shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat()) | ||
| shader.setFloatUniform("intensity", intensity) | ||
|
|
||
| view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content")) | ||
|
|
||
| return motionView | ||
| } |
There was a problem hiding this comment.
Recreating the RuntimeShader on every frame is extremely expensive because it compiles the AGSL shader on each call, causing rendering stutters (jank). We should cache the RuntimeShader instance as a class property and reuse it, only updating the uniforms. Additionally, the render effect should be cleared when frame < startFrame.
private var cachedShader: RuntimeShader? = 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.TIRAMISU) return motionView
if (frame !in startFrame..endFrame) {
view.setRenderEffect(null)
return motionView
}
val intensity =
MotionInterpolator.interpolateForRange(
interpolator = Interpolators(Easings.LINEAR),
currentFrame = frame,
frameRange = Pair(startFrame, endFrame),
valueRange = Pair(fromIntensity, toIntensity),
)
val shader = cachedShader ?: RuntimeShader(VIGNETTE_SHADER).also { cachedShader = it }
shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat())
shader.setFloatUniform("intensity", intensity)
view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content"))
return motionView
}
Comment on lines
+34
to
+39
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } |
There was a problem hiding this comment.
The render effect is only cleared when frame > endFrame. If the animation is scrubbed backwards or jumps to a frame before startFrame, the effect will remain applied to the view. It should be cleared whenever the frame is outside the active range.
if (frame !in startFrame..endFrame) {
view.setRenderEffect(null)
return motionView
}
Comment on lines
+20
to
+27
| private var _motionView: MotionView? = null | ||
| override var motionView: MotionView | ||
| get() = _motionView!! | ||
| set(value) { | ||
| _motionView = value | ||
| outerEffect.motionView = value | ||
| innerEffect.motionView = value | ||
| } |
There was a problem hiding this comment.
Using a nullable backing property with the double-bang operator (!!) is not idiomatic in Kotlin. Since motionView is guaranteed to be initialized, we can use lateinit var for the backing property to avoid nullability and unsafe casts.
Suggested change
| private var _motionView: MotionView? = null | |
| override var motionView: MotionView | |
| get() = _motionView!! | |
| set(value) { | |
| _motionView = value | |
| outerEffect.motionView = value | |
| innerEffect.motionView = value | |
| } | |
| private lateinit var _motionView: MotionView | |
| override var motionView: MotionView | |
| get() = _motionView | |
| set(value) { | |
| _motionView = value | |
| outerEffect.motionView = value | |
| innerEffect.motionView = value | |
| } |
Comment on lines
+35
to
+40
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } |
There was a problem hiding this comment.
The render effect is only cleared when frame > endFrame. If the animation is scrubbed backwards or jumps to a frame before startFrame, the effect will remain applied to the view. It should be cleared whenever the frame is outside the active range.
if (frame !in startFrame..endFrame) {
view.setRenderEffect(null)
return motionView
}
Comment on lines
+33
to
+42
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| // Keep the final state if needed, or clear it. | ||
| // Typically transitions might want to stay at final state if it's the end of visibility. | ||
| // But for generic effects, we might want to clear them when out of range. | ||
| // BlurEffect clears it, so we follow that pattern. | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } |
There was a problem hiding this comment.
The render effect is only cleared when frame > endFrame. If the animation is scrubbed backwards or jumps to a frame before startFrame, the effect will remain applied to the view. It should be cleared whenever the frame is outside the active range.
if (frame !in startFrame..endFrame) {
view.setRenderEffect(null)
return motionView
}
Comment on lines
+31
to
+36
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } |
There was a problem hiding this comment.
The render effect is only cleared when frame > endFrame. If the animation is scrubbed backwards or jumps to a frame before startFrame, the effect will remain applied to the view. It should be cleared whenever the frame is outside the active range.
if (frame !in startFrame..endFrame) {
view.setRenderEffect(null)
return motionView
}
Comment on lines
+32
to
+37
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } |
There was a problem hiding this comment.
The render effect is only cleared when frame > endFrame. If the animation is scrubbed backwards or jumps to a frame before startFrame, the effect will remain applied to the view. It should be cleared whenever the frame is outside the active range.
if (frame !in startFrame..endFrame) {
view.setRenderEffect(null)
return motionView
}
Comment on lines
+32
to
+37
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView | ||
| } |
There was a problem hiding this comment.
The render effect is only cleared when frame > endFrame. If the animation is scrubbed backwards or jumps to a frame before startFrame, the effect will remain applied to the view. It should be cleared whenever the frame is outside the active range.
if (frame !in startFrame..endFrame) {
view.setRenderEffect(null)
return motionView
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ChainEffect.kt`:
- Around line 42-49: ChainEffect currently calls innerEffect.forFrame(frame)
then outerEffect.forFrame(frame) which means the second call overwrites the
RenderEffect set by the first; update ChainEffect to fail fast by throwing an
UnsupportedOperationException (or rename to SequentialOverwriteEffect) until
proper chaining is implemented: replace the current sequential calls in
ChainEffect.forFrame with code that throws an UnsupportedOperationException
(message: chaining not supported) so callers of ChainEffect learn chaining is
unsupported rather than silently overwritten; reference innerEffect,
outerEffect, forFrame, MotionEffect, and setRenderEffect when making the change.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/PixelateEffect.kt`:
- Around line 51-56: The computed pixelSize from
MotionInterpolator.interpolateForRange can be zero or negative, which breaks the
shader (fragCoord / pixelSize); clamp/validate it immediately after the
interpolation in PixelateEffect (after the pixelSize assignment) by replacing it
with a positive minimum (e.g., max(pixelSize, 1f) or max(pixelSize,
Float.MIN_VALUE)) so downstream shader math uses a safe non-zero value; update
any references to pixelSize in this scope (the PixelateEffect interpolation that
uses fromPixelSize, toPixelSize, startFrame, endFrame, frame, and the
Interpolators(Easings.LINEAR) call) to use the clamped value.
- Around line 58-61: Currently a new RuntimeShader is created every frame in
PixelateEffect.kt; instead make RuntimeShader (constructed with PIXELATE_SHADER)
a cached field on the PixelateEffect instance and reuse it across frames—update
only the uniform via shader.setFloatUniform("pixelSize", pixelSize) each frame;
likewise create the RenderEffect once with
RenderEffect.createRuntimeShaderEffect(cachedShader, "content") and reuse or
recreate it only if the shader instance changes, ensuring you avoid per-frame
allocations of RuntimeShader and RenderEffect.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VignetteEffect.kt`:
- Around line 63-67: The code currently creates a new RuntimeShader every frame;
change VignetteEffect to cache a single RuntimeShader (from VIGNETTE_SHADER) as
a property (and optionally cache the RenderEffect created from it) and only call
shader.setFloatUniform("size", width, height) and
shader.setFloatUniform("intensity", intensity) each frame. Ensure the cached
shader/RenderEffect is created once (e.g., in the VignetteEffect constructor or
an init block), update uniforms when view size or intensity changes, and reuse
the same shader when calling
view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content"))
to avoid re-instantiation on the hot animation path.
- Line 64: Guard against zero-sized views before calling
shader.setFloatUniform("size", ...) in VignetteEffect: check view.width and
view.height and if either is zero, either return/skip setting uniforms or use a
safe non-zero fallback (e.g., 1f) before computing view.width.toFloat() and
view.height.toFloat(); update the code around the shader.setFloatUniform("size",
view.width.toFloat(), view.height.toFloat()) call so the shader never receives a
zero component for size.
🪄 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: c827b2d9-1b16-4693-9772-8ae3069bea83
📒 Files selected for processing (9)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/BrightnessContrastEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ChainEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/GrayscaleEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/InvertEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/OffsetEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/PixelateEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SepiaEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VignetteEffect.kt
Comment on lines
+42
to
+49
| // Chaining is tricky with the current side-effect based architecture. | ||
| // For now, we just call the inner and outer effects, which will | ||
| // each try to set the RenderEffect on the view, with the last one winning. | ||
| // To properly support chaining, we would need to refactor MotionEffect | ||
| // to return a RenderEffect instead of applying it. | ||
| innerEffect.forFrame(frame) | ||
| outerEffect.forFrame(frame) | ||
|
|
Contributor
There was a problem hiding this comment.
ChainEffect does not actually chain effects; it overwrites.
Current logic applies two side-effecting effects sequentially, so only the last setRenderEffect survives. That breaks the class contract for callers expecting composition.
At minimum, fail fast (e.g., throw UnsupportedOperationException) until true chaining is implemented, or rename this class to reflect sequential overwrite semantics.
🤖 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ChainEffect.kt`
around lines 42 - 49, ChainEffect currently calls innerEffect.forFrame(frame)
then outerEffect.forFrame(frame) which means the second call overwrites the
RenderEffect set by the first; update ChainEffect to fail fast by throwing an
UnsupportedOperationException (or rename to SequentialOverwriteEffect) until
proper chaining is implemented: replace the current sequential calls in
ChainEffect.forFrame with code that throws an UnsupportedOperationException
(message: chaining not supported) so callers of ChainEffect learn chaining is
unsupported rather than silently overwritten; reference innerEffect,
outerEffect, forFrame, MotionEffect, and setRenderEffect when making the change.
Comment on lines
+58
to
+61
| val shader = RuntimeShader(PIXELATE_SHADER) | ||
| shader.setFloatUniform("pixelSize", pixelSize) | ||
|
|
||
| view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content")) |
Contributor
There was a problem hiding this comment.
Avoid allocating RuntimeShader every frame.
Creating a new shader on each frame adds avoidable churn on the render path. Cache one shader instance and only update uniforms per frame.
Proposed fix
class PixelateEffect(
@@
) : MotionEffect {
override lateinit var motionView: MotionView
+ private val shader by lazy { RuntimeShader(PIXELATE_SHADER) }
@@
- val shader = RuntimeShader(PIXELATE_SHADER)
shader.setFloatUniform("pixelSize", pixelSize)
view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content"))🤖 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/PixelateEffect.kt`
around lines 58 - 61, Currently a new RuntimeShader is created every frame in
PixelateEffect.kt; instead make RuntimeShader (constructed with PIXELATE_SHADER)
a cached field on the PixelateEffect instance and reuse it across frames—update
only the uniform via shader.setFloatUniform("pixelSize", pixelSize) each frame;
likewise create the RenderEffect once with
RenderEffect.createRuntimeShaderEffect(cachedShader, "content") and reuse or
recreate it only if the shader instance changes, ensuring you avoid per-frame
allocations of RuntimeShader and RenderEffect.
Comment on lines
+63
to
+67
| val shader = RuntimeShader(VIGNETTE_SHADER) | ||
| shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat()) | ||
| shader.setFloatUniform("intensity", intensity) | ||
|
|
||
| view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content")) |
Contributor
There was a problem hiding this comment.
Reuse a cached shader instead of recreating it each frame.
Re-instantiating RuntimeShader on each frame is expensive on a hot animation path. Keep one shader instance and only update uniforms.
Proposed fix
) : MotionEffect {
override lateinit var motionView: MotionView
+ private val shader by lazy { RuntimeShader(VIGNETTE_SHADER) }
@@
- val shader = RuntimeShader(VIGNETTE_SHADER)
shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat())
shader.setFloatUniform("intensity", intensity)📝 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.
Suggested change
| val shader = RuntimeShader(VIGNETTE_SHADER) | |
| shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat()) | |
| shader.setFloatUniform("intensity", intensity) | |
| view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content")) | |
| ) : MotionEffect { | |
| override lateinit var motionView: MotionView | |
| private val shader by lazy { RuntimeShader(VIGNETTE_SHADER) } | |
| // ... other code ... | |
| // In the forFrame method or equivalent: | |
| shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat()) | |
| shader.setFloatUniform("intensity", intensity) | |
| view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content")) |
🤖 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VignetteEffect.kt`
around lines 63 - 67, The code currently creates a new RuntimeShader every
frame; change VignetteEffect to cache a single RuntimeShader (from
VIGNETTE_SHADER) as a property (and optionally cache the RenderEffect created
from it) and only call shader.setFloatUniform("size", width, height) and
shader.setFloatUniform("intensity", intensity) each frame. Ensure the cached
shader/RenderEffect is created once (e.g., in the VignetteEffect constructor or
an init block), update uniforms when view size or intensity changes, and reuse
the same shader when calling
view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content"))
to avoid re-instantiation on the hot animation path.
| ) | ||
|
|
||
| val shader = RuntimeShader(VIGNETTE_SHADER) | ||
| shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat()) |
Contributor
There was a problem hiding this comment.
Guard against zero-sized views before setting shader uniforms.
When width or height is 0, the shader computes fragCoord / size, which can generate invalid output.
Proposed fix
- shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat())
+ if (view.width <= 0 || view.height <= 0) return motionView
+ shader.setFloatUniform("size", view.width.toFloat(), view.height.toFloat())🤖 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VignetteEffect.kt`
at line 64, Guard against zero-sized views before calling
shader.setFloatUniform("size", ...) in VignetteEffect: check view.width and
view.height and if either is zero, either return/skip setting uniforms or use a
safe non-zero fallback (e.g., 1f) before computing view.width.toFloat() and
view.height.toFloat(); update the code around the shader.setFloatUniform("size",
view.width.toFloat(), view.height.toFloat()) call so the shader never receives a
zero component for size.
* motionlib: add Vignette and Pixelate effects using AGSL shaders * motionlib: add Grayscale, Sepia, Invert, and Brightness/Contrast effects using ColorMatrix * motionlib: add OffsetEffect and placeholder ChainEffect implementation * lyrics-maker: apply BlurEffect to background views in PopupLyricsTemplate
tejpratap46
force-pushed
the
feat/render-effects
branch
from
03863d1 to
d035766
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/VintageLyricsTemplate.kt`:
- Around line 53-69: The current loop using lyrics.zipWithNext() in
VintageLyricsTemplate.kt (inside the block that calls wordWriterTextView and
transition with SlideTransition/SlideDirection) omits the final lyric, causing
the last frame (and single-frame cases) to be skipped; update the rendering to
iterate all items (e.g., use forEachIndexed or a for loop over lyrics) and after
processing each pair call wordWriterTextView with current.text/current.frame and
next.frame for timing, then separately handle the final lyric element by calling
wordWriterTextView with its startFrame and an appropriate endFrame (or duration)
so the last segment is rendered; apply the same fix to the second occurrence of
zipWithNext() at the other block (lines referenced in the comment).
In
`@modules/ml-kit-ext/src/main/java/com/tejpratapsingh/motionlib/mlkit/effects/SubjectSegmentationEffect.kt`:
- Around line 38-39: The maskCache currently (in SubjectSegmentationEffect) is
an unbounded mutableMap and can retain one full-size Bitmap per frame; replace
it with a bounded cache (e.g., an LruCache<Int, Bitmap> or a LinkedHashMap with
access-order eviction) keyed the same way as maskCache so entries are evicted
when size exceeds a configured max (bytes or entry count), and ensure evicted
Bitmaps are recycled/closed to free memory; update all uses of maskCache (where
put/replace happens in the class) to use the new cache API and make the cache
access thread-safe if methods like produceMask() or onFrameProcessed() access it
from different threads.
- Around line 60-65: The code only clears the RenderEffect when frame > endFrame
and when maskBitmap is null it leaves the prior effect in place; update the
logic in SubjectSegmentationEffect (the method that checks frame against
startFrame..endFrame and the branch that handles maskBitmap) to always clear the
prior effect via view.setRenderEffect(null) whenever the segmentation is
inactive (i.e., frame not in startFrame..endFrame, including frame < startFrame)
and also when maskBitmap is null, then return motionView; ensure you reference
the existing calls to view.setRenderEffect(null), the startFrame and endFrame
bounds check, and the maskBitmap null-check while making this change.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VintageEffect.kt`:
- Around line 49-53: The code fails to clear the render effect when frame <
startFrame, leaving stale effects; update the out-of-window handling in
VintageEffect (the block that checks frame against startFrame..endFrame) to call
view.setRenderEffect(null) for any frame not in the range (both before
startFrame and after endFrame) before returning motionView so previously applied
effects are always cleared; locate the conditional using startFrame, endFrame,
view.setRenderEffect(null), and motionView and ensure the nulling happens
whenever frame !in startFrame..endFrame.
- Around line 56-62: The interpolated intensity value returned from
MotionInterpolator.interpolateForRange (assigned to intensity in
VintageEffect.kt) must be clamped to a safe range (e.g., 0f..1f) to avoid
invalid/excessive sepia/vignette parameters; after computing intensity, apply a
clamp (Kotlin's coerceIn or equivalent) and use the clamped value for subsequent
processing (replace usages of intensity with the clamped variable) so external
fromIntensity/toIntensity cannot produce out-of-range effects.
🪄 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: d3c618d2-de1c-4c03-97ad-2467441ec339
📒 Files selected for processing (22)
.idea/gradle.xmlgradle/libs.versions.tomlmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/LyricsTemplateRegistry.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/VintageLyricsTemplate.ktmodules/ml-kit-ext/build.gradlemodules/ml-kit-ext/consumer-rules.promodules/ml-kit-ext/proguard-rules.promodules/ml-kit-ext/src/main/java/com/tejpratapsingh/motionlib/mlkit/MLKitImageProcessor.ktmodules/ml-kit-ext/src/main/java/com/tejpratapsingh/motionlib/mlkit/effects/SubjectSegmentationEffect.ktmodules/ml-kit-ext/src/main/java/com/tejpratapsingh/motionlib/mlkit/plugins/SubjectSegmentationPlugin.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/BrightnessContrastEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ChainEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/GrayscaleEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/InvertEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/OffsetEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/PixelateEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SepiaEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VignetteEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VintageEffect.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.ktsettings.gradle
✅ Files skipped from review due to trivial changes (2)
- gradle/libs.versions.toml
- .idea/gradle.xml
🚧 Files skipped from review as they are similar to previous changes (9)
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/OffsetEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SepiaEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/GrayscaleEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/InvertEffect.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/BrightnessContrastEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VignetteEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ChainEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/PixelateEffect.kt
Comment on lines
+53
to
+69
| lyrics.zipWithNext().forEach { (current, next) -> | ||
| wordWriterTextView( | ||
| text = current.text, | ||
| startFrame = current.frame, | ||
| endFrame = next.frame, | ||
| textSizeVariant = MotionTextVariant.H1, | ||
| textColor = "#FFFFFF", | ||
| writingSpeed = 1.0f, | ||
| textView = | ||
| AppCompatTextView(context).apply { | ||
| setPadding(32, 32, 32, 32) | ||
| textAlignment = AppCompatTextView.TEXT_ALIGNMENT_CENTER | ||
| gravity = Gravity.CENTER | ||
| }, | ||
| ) | ||
| transition(SlideTransition(SlideDirection.RIGHT_TO_LEFT), duration = 15) | ||
| } |
Contributor
There was a problem hiding this comment.
zipWithNext() drops the last lyric segment (and all text when only one frame exists).
Both loops miss rendering the final lyric frame because zipWithNext() emits n-1 pairs only.
Suggested fix
- lyrics.zipWithNext().forEach { (current, next) ->
+ lyrics.forEachIndexed { index, current ->
+ val segmentEndFrame = lyrics.getOrNull(index + 1)?.frame ?: endFrame
wordWriterTextView(
text = current.text,
startFrame = current.frame,
- endFrame = next.frame,
+ endFrame = segmentEndFrame,
textSizeVariant = MotionTextVariant.H1,
textColor = "`#FFFFFF`",
writingSpeed = 1.0f,
textView =
AppCompatTextView(context).apply {
setPadding(32, 32, 32, 32)
textAlignment = AppCompatTextView.TEXT_ALIGNMENT_CENTER
gravity = Gravity.CENTER
},
)
transition(SlideTransition(SlideDirection.RIGHT_TO_LEFT), duration = 15)
}
@@
- previewLyrics.zipWithNext().forEach { (current, next) ->
+ previewLyrics.forEachIndexed { index, current ->
+ val segmentEndFrame = previewLyrics.getOrNull(index + 1)?.frame ?: endFrame
wordWriterTextView(
text = current.text,
startFrame = current.frame,
- endFrame = next.frame,
+ endFrame = segmentEndFrame,
textSizeVariant = MotionTextVariant.H1,
textColor = "`#FFFFFF`",
writingSpeed = 1.0f,
textView =
AppCompatTextView(context).apply {
setPadding(32, 32, 32, 32)
textAlignment = AppCompatTextView.TEXT_ALIGNMENT_CENTER
gravity = Gravity.CENTER
},
)
transition(SlideTransition(SlideDirection.RIGHT_TO_LEFT), duration = 15)
}Also applies to: 102-118
🤖 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/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/VintageLyricsTemplate.kt`
around lines 53 - 69, The current loop using lyrics.zipWithNext() in
VintageLyricsTemplate.kt (inside the block that calls wordWriterTextView and
transition with SlideTransition/SlideDirection) omits the final lyric, causing
the last frame (and single-frame cases) to be skipped; update the rendering to
iterate all items (e.g., use forEachIndexed or a for loop over lyrics) and after
processing each pair call wordWriterTextView with current.text/current.frame and
next.frame for timing, then separately handle the final lyric element by calling
wordWriterTextView with its startFrame and an appropriate endFrame (or duration)
so the last segment is rendered; apply the same fix to the second occurrence of
zipWithNext() at the other block (lines referenced in the comment).
Comment on lines
+38
to
+39
| private val maskCache = mutableMapOf<Int, Bitmap>() | ||
|
|
Contributor
There was a problem hiding this comment.
Bound maskCache growth to avoid frame-count-sized bitmap retention.
At Line 38 and Line 69–79, one full-size bitmap can be retained per frame with no eviction. On long timelines this can escalate memory usage and trigger OOMs.
Suggested fix (bounded cache)
+import android.util.LruCache
...
- private val maskCache = mutableMapOf<Int, Bitmap>()
+ private val maskCache = LruCache<Int, Bitmap>(16)
...
- val maskBitmap =
- maskCache[frame] ?: run {
+ val maskBitmap =
+ maskCache.get(frame) ?: run {
...
- maskCache[frame] = createdMask
+ maskCache.put(frame, createdMask)
createdMaskAlso applies to: 68-80
🤖 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/ml-kit-ext/src/main/java/com/tejpratapsingh/motionlib/mlkit/effects/SubjectSegmentationEffect.kt`
around lines 38 - 39, The maskCache currently (in SubjectSegmentationEffect) is
an unbounded mutableMap and can retain one full-size Bitmap per frame; replace
it with a bounded cache (e.g., an LruCache<Int, Bitmap> or a LinkedHashMap with
access-order eviction) keyed the same way as maskCache so entries are evicted
when size exceeds a configured max (bytes or entry count), and ensure evicted
Bitmaps are recycled/closed to free memory; update all uses of maskCache (where
put/replace happens in the class) to use the new cache API and make the cache
access thread-safe if methods like produceMask() or onFrameProcessed() access it
from different threads.
Comment on lines
+60
to
+65
| if (frame !in startFrame..endFrame) { | ||
| // If we are past the end frame, clear the effect | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView |
Contributor
There was a problem hiding this comment.
Clear stale RenderEffect whenever segmentation is inactive.
At Line 60 and Line 62, the effect is cleared only when frame > endFrame. If playback seeks to a frame before startFrame, the previously applied effect can persist. Also at Line 89–95, when maskBitmap is null, the prior effect is not reset.
Suggested fix
- if (frame !in startFrame..endFrame) {
- // If we are past the end frame, clear the effect
- if (frame > endFrame) {
- view.setRenderEffect(null)
- }
+ if (frame !in startFrame..endFrame) {
+ view.setRenderEffect(null)
return motionView
}
...
- if (maskBitmap != null) {
+ if (maskBitmap != null) {
val shader = RuntimeShader(MASK_SHADER)
val maskShader = BitmapShader(maskBitmap, Shader.TileMode.CLAMP, Shader.TileMode.CLAMP)
shader.setInputShader("mask", maskShader)
view.setRenderEffect(RenderEffect.createRuntimeShaderEffect(shader, "content"))
+ } else {
+ view.setRenderEffect(null)
}Also applies to: 89-95
🤖 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/ml-kit-ext/src/main/java/com/tejpratapsingh/motionlib/mlkit/effects/SubjectSegmentationEffect.kt`
around lines 60 - 65, The code only clears the RenderEffect when frame >
endFrame and when maskBitmap is null it leaves the prior effect in place; update
the logic in SubjectSegmentationEffect (the method that checks frame against
startFrame..endFrame and the branch that handles maskBitmap) to always clear the
prior effect via view.setRenderEffect(null) whenever the segmentation is
inactive (i.e., frame not in startFrame..endFrame, including frame < startFrame)
and also when maskBitmap is null, then return motionView; ensure you reference
the existing calls to view.setRenderEffect(null), the startFrame and endFrame
bounds check, and the maskBitmap null-check while making this change.
Comment on lines
+49
to
+53
| if (frame !in startFrame..endFrame) { | ||
| if (frame > endFrame) { | ||
| view.setRenderEffect(null) | ||
| } | ||
| return motionView |
Contributor
There was a problem hiding this comment.
Clear the effect for all out-of-window frames.
At Line 49, frames before startFrame return without clearing any previously applied render effect, so seeking backward can leave stale vintage visuals on screen.
Suggested fix
- if (frame !in startFrame..endFrame) {
- if (frame > endFrame) {
- view.setRenderEffect(null)
- }
- return motionView
- }
+ if (frame !in startFrame..endFrame) {
+ view.setRenderEffect(null)
+ return motionView
+ }📝 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.
Suggested change
| if (frame !in startFrame..endFrame) { | |
| if (frame > endFrame) { | |
| view.setRenderEffect(null) | |
| } | |
| return motionView | |
| if (frame !in startFrame..endFrame) { | |
| view.setRenderEffect(null) | |
| return motionView | |
| } |
🤖 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VintageEffect.kt`
around lines 49 - 53, The code fails to clear the render effect when frame <
startFrame, leaving stale effects; update the out-of-window handling in
VintageEffect (the block that checks frame against startFrame..endFrame) to call
view.setRenderEffect(null) for any frame not in the range (both before
startFrame and after endFrame) before returning motionView so previously applied
effects are always cleared; locate the conditional using startFrame, endFrame,
view.setRenderEffect(null), and motionView and ensure the nulling happens
whenever frame !in startFrame..endFrame.
Comment on lines
+56
to
+62
| val intensity = | ||
| MotionInterpolator.interpolateForRange( | ||
| interpolator = Interpolators(Easings.LINEAR), | ||
| currentFrame = frame, | ||
| frameRange = Pair(startFrame, endFrame), | ||
| valueRange = Pair(fromIntensity, toIntensity), | ||
| ) |
Contributor
There was a problem hiding this comment.
Clamp interpolated intensity to a safe range.
fromIntensity/toIntensity are externally configurable; clamping prevents invalid/excessive values from producing unstable sepia/vignette output.
Suggested fix
- val intensity =
+ val intensity =
MotionInterpolator.interpolateForRange(
interpolator = Interpolators(Easings.LINEAR),
currentFrame = frame,
frameRange = Pair(startFrame, endFrame),
valueRange = Pair(fromIntensity, toIntensity),
- )
+ ).coerceIn(0f, 1f)📝 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.
Suggested change
| val intensity = | |
| MotionInterpolator.interpolateForRange( | |
| interpolator = Interpolators(Easings.LINEAR), | |
| currentFrame = frame, | |
| frameRange = Pair(startFrame, endFrame), | |
| valueRange = Pair(fromIntensity, toIntensity), | |
| ) | |
| val intensity = | |
| MotionInterpolator.interpolateForRange( | |
| interpolator = Interpolators(Easings.LINEAR), | |
| currentFrame = frame, | |
| frameRange = Pair(startFrame, endFrame), | |
| valueRange = Pair(fromIntensity, toIntensity), | |
| ).coerceIn(0f, 1f) |
🤖 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VintageEffect.kt`
around lines 56 - 62, The interpolated intensity value returned from
MotionInterpolator.interpolateForRange (assigned to intensity in
VintageEffect.kt) must be clamped to a safe range (e.g., 0f..1f) to avoid
invalid/excessive sepia/vignette parameters; after computing intensity, apply a
clamp (Kotlin's coerceIn or equivalent) and use the clamped value for subsequent
processing (replace usages of intensity with the clamped variable) so external
fromIntensity/toIntensity cannot produce out-of-range effects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores