feat(cloudy): add an OpenGL ES backend for mirage optics below API 33 - #152
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAndroid rendering now selects AGSL, GLES, or ColorGrade by API level. The change adds GLSL ES translation and readback, asynchronous backdrop blits, configurable fallbacks, Skiko parity updates, rendering validation, benchmarks, and emulator screenshot CI. ChangesMirage rendering pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Modifier
participant MirageProgramCache
participant MirageBackendProgram
participant MirageBackdropNode
participant GlEnv
Modifier->>MirageProgramCache: resolve stages and backend band
MirageProgramCache->>MirageBackendProgram: obtain backend program
MirageBackendProgram-->>MirageBackdropNode: select effect, color filter, or blit
MirageBackdropNode->>GlEnv: render GLES bitmap asynchronously
GlEnv-->>MirageBackdropNode: return transformed bitmap
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
7f616a0 to
7659b27
Compare
Snapshot diff report✅ 8 screenshots verified, no changes.
Full screenshot report (artifact) ScreenshotsCommitted reference screenshots each spec verifies against (current expected state).
Device renderingActual pixels captured on the emulator per API band (API 30 = GLES mirage + legacy blur, API 34 = AGSL + RenderEffect). Render evidence, not a golden diff.
|
b75cbac to
a2c24ce
Compare
9da7346 to
2f61797
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/GlEnv.kt`:
- Around line 95-116: Update render and the GL execution path around
renderOnGlThread so the 2-second limit cannot be presented as a hard bound for
non-suspending block(), glFinish(), or eglSwapBuffers() calls. Ensure a
driver-level hang cannot permanently pin the single glDispatcher thread or block
later renders; retain the existing timeout behavior for suspension points such
as ready.await() and preserve RuntimeException degradation semantics.
In
`@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropNode.kt`:
- Around line 104-110: Update the sky-swap branch in MirageBackdropNode so it
calls glesBackdrop.release() alongside backdropSnapshot.dispose() after removing
the old overlay and adding the new one, ensuring both cached backdrops are
cleared when the sky changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8113f920-6d4e-495e-b585-5c18f1bfdc18
⛔ Files ignored due to path filters (1)
cloudy/api/cloudy.klib.apiis excluded by!cloudy/api/**/*.api
📒 Files selected for processing (30)
.github/workflows/screenshot-test.ymlcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/GlProgramMatchTest.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/GlesRoundtripBenchmark.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/MirageBandScreenshotTest.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/MirageModifier.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/GlEnv.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/GlProgram.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/MirageBackendProgram.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/MirageNode.android.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/MirageModifier.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/BackdropClearBlurMachine.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/CompiledProgram.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropNode.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackend.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackendBand.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageColorGrade.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageCompiler.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageFilterChain.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageGlesBackdrop.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageGlslEs.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageNode.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MiragePreamble.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageProgramCache.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageUniformBinding.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageBackendBandTest.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageCompilerTest.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageGlslEsTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/MirageColorGradeRasterTest.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/MirageModifier.skiko.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/internal/MirageBackendProgram.skiko.kt
…, or color-grade by SDK level
…olor-filter/blit seam
…r-draw color matrix
… pipeline on API 29-32
… cannot exhaust the buffer pool
…stead of a blocking handler thread
… color-matrix oracle
…sing summary artifact
… to break the PixelCopy RenderNode cycle
…ot-backed backdrop
…alize its contrast oracle
…ver cannot hang captures
aa995fb to
e7fae19
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/MirageBandScreenshotTest.kt (1)
324-346: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bulk pixel access for test speed.
applyDuotone,meanAbsDiff,horizontalContrast, andmeanLuminanceall iterate with per-pixelgetPixel/setPixel. On a 3× density emulator the 160 dp card is ~480×480 (230K pixels); eachgetPixelis a JNI transition.meanAbsDiffis also called inside thecaptureCardUntilDiffersconvergence loop. Switching togetPixels/setPixelsbulk arrays would cut JNI overhead significantly and speed up the suite, especially on SwiftShader emulators.♻️ Example: bulk-pixel rewrite for
meanAbsDiffandapplyDuotoneprivate fun meanAbsDiff(a: Bitmap, b: Bitmap): Double { if (a.width != b.width || a.height != b.height) return Double.MAX_VALUE - var sum = 0L - var n = 0 - for (y in 0 until a.height) { - for (x in 0 until a.width) { - val pa = a.getPixel(x, y) - val pb = b.getPixel(x, y) - sum += abs(Color.red(pa) - Color.red(pb)).toLong() - sum += abs(Color.green(pa) - Color.green(pb)).toLong() - sum += abs(Color.blue(pa) - Color.blue(pb)).toLong() - n += 3 - } - } - return sum.toDouble() / n + val wa = IntArray(a.width * a.height) + val wb = IntArray(b.width * b.height) + a.getPixels(wa, 0, a.width, 0, 0, a.width, a.height) + b.getPixels(wb, 0, b.width, 0, 0, b.width, b.height) + var sum = 0L + for (i in wa.indices) { + sum += abs(Color.red(wa[i]) - Color.red(wb[i])).toLong() + sum += abs(Color.green(wa[i]) - Color.green(wb[i])).toLong() + sum += abs(Color.blue(wa[i]) - Color.blue(wb[i])).toLong() + } + return sum.toDouble() / (wa.size * 3) }val m = duotoneMatrix(shadow, highlight, amount) val out = Bitmap.createBitmap(src.width, src.height, Bitmap.Config.ARGB_8888) - for (y in 0 until src.height) { - for (x in 0 until src.width) { - val p = src.getPixel(x, y) - val r = Color.red(p).toFloat() - val g = Color.green(p).toFloat() - val b = Color.blue(p).toFloat() - val a = Color.alpha(p).toFloat() - fun ch(row: Int) = - (m[row] * r + m[row + 1] * g + m[row + 2] * b + m[row + 3] * a + m[row + 4]) - .coerceIn(0f, 255f).toInt() - out.setPixel(x, y, Color.argb(ch(15), ch(0), ch(5), ch(10))) - } - } + val px = IntArray(src.width * src.height) + src.getPixels(px, 0, src.width, 0, 0, src.width, src.height) + for (i in px.indices) { + val p = px[i] + val r = Color.red(p).toFloat() + val g = Color.green(p).toFloat() + val b = Color.blue(p).toFloat() + val a = Color.alpha(p).toFloat() + fun ch(row: Int) = + (m[row] * r + m[row + 1] * g + m[row + 2] * b + m[row + 3] * a + m[row + 4]) + .coerceIn(0f, 255f).toInt() + px[i] = Color.argb(ch(15), ch(0), ch(5), ch(10)) + } + out.setPixels(px, 0, src.width, 0, 0, src.width, src.height) return outAlso applies to: 348-363, 366-391
🤖 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 `@cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/MirageBandScreenshotTest.kt` around lines 324 - 346, Replace per-pixel Bitmap getPixel/setPixel calls with bulk getPixels/setPixels arrays in applyDuotone, meanAbsDiff, horizontalContrast, and meanLuminance. Preserve each method’s existing pixel calculations, channel handling, bitmap dimensions, and return values while reducing JNI calls, especially for the captureCardUntilDiffers convergence loop.
🤖 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
`@cloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/MirageBackendProgram.android.kt`:
- Around line 92-95: Update createBackendProgram and the related AGSL,
AndroidUniformSink, RenderEffect, and ShaderBrush call sites to apply explicit
Build.VERSION.SDK_INT >= 33 guards or delegate to helpers annotated and gated
with `@RequiresApi`(33). Preserve the existing lower-band fallback and ensure
every path remains functional down to API 23 without eagerly referencing API
33-only operations.
---
Nitpick comments:
In
`@cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/MirageBandScreenshotTest.kt`:
- Around line 324-346: Replace per-pixel Bitmap getPixel/setPixel calls with
bulk getPixels/setPixels arrays in applyDuotone, meanAbsDiff,
horizontalContrast, and meanLuminance. Preserve each method’s existing pixel
calculations, channel handling, bitmap dimensions, and return values while
reducing JNI calls, especially for the captureCardUntilDiffers convergence loop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf7f3bea-86f9-4dd1-9046-47fa53557d16
⛔ Files ignored due to path filters (1)
cloudy/api/cloudy.klib.apiis excluded by!cloudy/api/**/*.api
📒 Files selected for processing (30)
.github/workflows/screenshot-test.ymlcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/GlProgramMatchTest.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/GlesRoundtripBenchmark.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/MirageBandScreenshotTest.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/MirageModifier.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/GlEnv.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/GlProgram.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/MirageBackendProgram.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/MirageNode.android.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/MirageModifier.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/BackdropClearBlurMachine.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/CompiledProgram.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropNode.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackend.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackendBand.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageColorGrade.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageCompiler.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageFilterChain.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageGlesBackdrop.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageGlslEs.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageNode.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MiragePreamble.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageProgramCache.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageUniformBinding.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageBackendBandTest.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageCompilerTest.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageGlslEsTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/MirageColorGradeRasterTest.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/MirageModifier.skiko.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/internal/MirageBackendProgram.skiko.kt
✅ Files skipped from review due to trivial changes (2)
- cloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageCompilerTest.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/BackdropClearBlurMachine.kt
🚧 Files skipped from review as they are similar to previous changes (26)
- cloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageBackendBandTest.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageNode.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackendBand.kt
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/MirageNode.android.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MiragePreamble.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageFilterChain.kt
- cloudy/src/skikoMain/kotlin/com/skydoves/cloudy/MirageModifier.skiko.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageUniformBinding.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageGlesBackdrop.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageColorGrade.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackend.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageCompiler.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/CompiledProgram.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageProgramCache.kt
- cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/MirageColorGradeRasterTest.kt
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/GlProgram.kt
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/GlEnv.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropNode.kt
- cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/GlesRoundtripBenchmark.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/MirageModifier.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageGlslEs.kt
- cloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageGlslEsTest.kt
- cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/GlProgramMatchTest.kt
- .github/workflows/screenshot-test.yml
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/MirageModifier.android.kt
- cloudy/src/skikoMain/kotlin/com/skydoves/cloudy/internal/MirageBackendProgram.skiko.kt


















Summary
Mirage optics currently only render on Android API 33+ (AGSL
RuntimeShader) and the skiko targets (SKSL). Below API 33,createBackendProgram()returnsnulland every optic is a draw-time no-op. This PR fills that gap with an SDK-banded backend ladder:RuntimeShader(unchanged).HardwareBuffer/ImageReaderand drawn. Backdrop optics only.ColorMatrix; lens optics no-op unless the caller supplies a fallback.Public API is unchanged except for one opt-in parameter:
Modifier.mirage(..., fallback = MirageFallback.Content(modifier)), which draws a caller-chosen stand-in when the plan can't render on the device. The default (MirageFallback.None) preserves the historic behavior.How it works
The band is chosen once per program build from
Build.VERSION.SDK_INT; the rest of the pipeline branches on a sealedFilterApplication(Effectfor RenderEffect,ColorFilterfor the affine grade,Blitfor the GLES readback). The GLSL ES translator is a mechanical pass over the assembled AGSL: type tokens (half/floatN→float/vecN),content.eval(px)→ asampleContentsampler helper,layout(color)→ plain vec4, and an entry-point rewrite. The one load-bearing detail is the Y-flip:gl_FragCoordis bottom-left while the kernels assume top-left.The GLES output path uses
ImageReader.getSurface()as the EGL window surface rather thanglEGLImageTargetTexture2DOES(which has no Java binding).cloudystays pure Kotlin/KMP, with no NDK.Scope and limits
contentVersioncache key, so the async capture/cache model doesn't apply; they stay a no-op (or draw the fallback).Modifier.liquidGlassis a separate path and is unaffected. It still degrades to its non-shader fallback below API 33.Also fixed: mirage backdrop capture crash
Building the band screenshot tests surfaced a pre-existing crash on
main: capturing a tree that containsModifier.mirage(sky = ...)(PixelCopy,captureToImage, or anything that walks the composed layer tree) hit the issue-112 cyclic-RenderNode recursion and killed the RenderThread, because the mirage backdrop sampled the sky through a livedrawLayerback-edge. The blur backdrop was already fixed for this by sampling through a rasterized snapshot; this PR ports the same fix to the mirage backdrop (BackdropClearBlurMachinemoved to commonMain and reused). The GLES blit input stays live, since it is recorded into an offscreen layer that never reaches the on-screen tree.Verification
Built green across androidMain, desktop, iosArm64/iosSimArm64/wasmJs/macosArm64, the Android host tests, and the ABI check (no new public ABI beyond the experimental
MirageFallback).The Duotone color matrix is proven bit-exact against the kernel formula in a desktop raster test (max per-channel error ~2.8e-16 over 20k samples).
The GLES path is validated on a physical Adreno 840 (API 36), which is the only way to exercise the real vendor
HardwareBuffer/copy()readback contract that an emulator's host-backed translator can't. Each GLES optic is rendered offscreen and compared pixel-for-pixel against its AGSL reference on the same device:Both are effectively bit-close, so the translation (Y-flip, coordinate frame, and fp16→highp precision) holds on real hardware.
A microbenchmark (
GlesRoundtripBenchmark) measures the fullGlEnv.render()round-trip (upload, FBO render,glFinish,wrapHardwareBuffer, and the ARGB_8888 readback copy) for Duotone and Chromatic at card and full-screen sizes. It callsGlProgramdirectly, so it needs no band override. The numbers are a floor for the measuring device, not a target-device budget: the readback copy andglFinishstall dominate and are memory-bandwidth / throughput bound, so an actual API 29–32 device (older mid-range GPU, several times less bandwidth) will be slower. It is meant for same-device before/after regression, and must run on real hardware. An emulator's SwiftShader path distorts it.The round-trip runs on a single-thread coroutine dispatcher rather than a
HandlerThreadwith a blocking latch. The callers (MirageGlesBackdrop) are already coroutines, sorendernow suspends on that dispatcher instead of blocking aDispatchers.Defaultpool thread on aCountDownLatch; the one thread is the EGL affinity anchor, so every GL call stays insidewithContext(glDispatcher). The readback also drains theImageReaderon entry, so a timed-out frame can no longer exhaust the buffer pool (acquireLatestImagethrowsIllegalStateExceptionwhenmaxImagesare held without aclose, which the round-trip would otherwise swallow into a permanent no-op).An instrumented band screenshot test (
MirageBandScreenshotTest) renders the fullModifier.mirage/Modifier.cloudybackdrop pipeline on device and checks it per band: Duotone against a pure-Kotlin color-matrix oracle, Chromatic as non-passthrough, and blur as stripe-contrast reduction against its radius-0 capture. The same test exercises whichever band the device's SDK selects; it passed on API 27 (color grade), API 30 (GLES), and API 34 (AGSL) emulators. A CI matrix runs it on API 30 and 34 emulators and uploads the captures, which the screenshot comment workflow posts as a per-band gallery on the PR (the comment-side half lives onmainvia #154, sinceworkflow_runexecutes the default-branch workflow file).Follow-ups (not in this PR)
Summary by CodeRabbit
MirageFallbacksupport tomirage, letting you provide alternative content when a mirage plan can’t render on the current device (Android and Skiko).