Refactor JCA to extract OpenGL/EGL effects into dedicated :core📷effects module#527
Refactor JCA to extract OpenGL/EGL effects into dedicated :core📷effects module#527davidjiagoogle wants to merge 8 commits into
Conversation
…ts module and dynamically inject it via Hilt
There was a problem hiding this comment.
Code Review
This pull request modularizes the camera effects by introducing a new :core:camera:effects module and decoupling SingleSurfaceForcingEffect from the core camera session. It defines a SingleStreamEffectProvider interface and injects its implementation using Hilt multibindings. The review feedback highlights a potential compilation error if the effects module is excluded, which can be resolved using @Multibinds. Additionally, suggestions were made to adhere to the repository style guide by restricting visibility to internal, adding KDoc documentation, and adjusting the Hilt component scope.
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.
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
temcguir
left a comment
There was a problem hiding this comment.
The overall direction of moving effects into a dedicated module and injecting them to allow for "lite" builds is solid. However, there are a few architectural issues with the current implementation that prevent it from being truly extensible for arbitrary custom effects.
Most notably, the PR description states that this uses Hilt @IntoSet multi-binding, but the code actually relies on @BindsOptionalOf to inject a single Optional<SingleStreamEffectProvider>. This single-instance injection prevents multiple effects from coexisting and forces developers to completely overwrite the module if they want to inject their own custom effect.
To resolve this, I've left a few inline comments outlining an approach that aligns with our existing LowLightBoost and ImagePostProcessor implementations:
- Generalize the interface: Rename
SingleStreamEffectProviderto a genericCameraEffectProvider. - Use a Map Registry: Replace
CameraSessionOptionalModulewith a@Multibindsmodule inside:data:camerato provide aMap<CameraEffectFeatureKey, Provider<CameraEffectProvider>>. This guarantees the Map is always present (even if empty in lite builds) and completely eliminates the need forOptionalinjection. - Drive via Settings: Treat the active effect as a configurable UI state by adding it to
CameraAppSettings(andSessionSettings), rather than hardcoding it instartCamera.CameraSessioncan then simply use the setting state to look up the correct provider from the injected Map registry.
| /** | ||
| * Provider for the camera effect that forces a single surface for use cases. | ||
| */ | ||
| interface SingleStreamEffectProvider { |
There was a problem hiding this comment.
Since the goal is to establish a pattern where users can inject their own arbitrary effects, the name SingleStreamEffectProvider is a bit too tied to our internal "single stream" effect. Let's rename this to something generic like CameraEffectProvider so it reads like a proper API for custom effects.
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| interface CameraSessionOptionalModule { | ||
| @BindsOptionalOf |
There was a problem hiding this comment.
The PR description mentions using a Hilt @IntoSet multi-binding, but it looks like we're actually using @BindsOptionalOf to inject a single java.util.Optional here.
Because this only injects a single instance, if developers want to inject their own custom effect (like a color filter or AR effect), they would have to completely override and replace our EffectsModule. It prevents multiple effects from coexisting.
To truly allow arbitrary effects without adding Hilt dependencies to :core:camera, we should match the Map-based registry pattern we already use for LowLightBoost and ImagePostProcessor:
- Create a
CameraEffectFeatureKeyinterface. - Delete this
CameraSessionOptionalModuleand replace it with a new module right here in:data:camera(e.g.,CameraEffectModule) that uses@Multibindsto provide an empty Map or Set ofMap.Entry<CameraEffectFeatureKey, Provider<CameraEffectProvider>>by default. - Have the
:core:camera:effectsmodule use@IntoSetto contribute its specific implementation to that Map. - Update
CameraModuleandCameraXCameraSystemto simply accept aMap<CameraEffectFeatureKey, Provider<CameraEffectProvider>>.
Because the @Multibinds module inside :data:camera guarantees the Map will always exist (even if it's empty when :core:camera:effects is excluded from a lite build), we completely eliminate the need for Optional injection and cleanly support any number of custom effects.
| val imagePostProcessors: | ||
| Map<ImagePostProcessorFeatureKey, @JvmSuppressWildcards Provider<ImagePostProcessor>> | ||
| Map<ImagePostProcessorFeatureKey, @JvmSuppressWildcards Provider<ImagePostProcessor>>, | ||
| private val singleStreamEffectProvider: SingleStreamEffectProvider? |
There was a problem hiding this comment.
Following up on the injection comment in CameraSessionOptionalModule, this parameter should be updated to accept the Map pattern:
cameraEffectProviders: Map<CameraEffectFeatureKey, @JvmSuppressWildcards Provider<CameraEffectProvider>>
Having this map acts as a registry of available effects, which elegantly allows us to treat the active effect as a configurable setting rather than hardcoding it here.
To complete this architecture, we should:
- Add an
activeCameraEffect: CameraEffectFeatureKey?field toCameraAppSettings(andSessionSettings). - Have
CameraSessionchecksessionSettings.activeCameraEffect, look up the matching provider in this injected map, and apply only the selected effect to theUseCaseGroup.
This gives us a clean, unidirectional data flow where the UI can change the effect via the SettingsRepository, and the camera session automatically rebuilds with the correct effect from the registry. Note: It's totally fine to keep this map separate from lowLightBoostEffectProvider, since Low Light Boost requires complex UI lifecycle callbacks that generic effects don't need.
This PR modularizes the OpenGL/EGL surface-processing and single-stream effects in the Jetpack Camera App (JCA), separating them into a dedicated
:core:camera:effectsmodule.Key Changes:
SingleStreamEffectProviderin:core:cameraas the injection contract.effects/package hierarchy (includingSingleSurfaceForcingEffect,CopyingSurfaceProcessor, shaders, and GL helper classes) from:core:camerato:core:camera:effects.libs.androidx.graphics.core(OpenGL/EGL wrapper library) to:core:camera:effects'sbuild.gradle.kts.SingleStreamEffectProviderImplusing Hilt@IntoSetmulti-binding.CameraXCameraSystemnow injectsSet<SingleStreamEffectProvider>and passes the effect provider toCameraSessiondynamically.:core:camera:effectsdependency, preventing theandroidx.graphics:graphics-corelibrary (~15KB) from being compiled and bundled into the production APK.Tried out removing effects module in the app layer in the david/splitEffectsTryout branch