Adopt CameraX Feature Group API #456
Conversation
Summary of ChangesHello @Zeronfinity, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the camera's robustness by integrating the new CameraX Feature Combination Query API. The primary goal is to prevent application crashes and ensure a smoother user experience by dynamically checking and enforcing device compatibility for various camera settings. By mapping internal camera features to CameraX's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![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 is an excellent and significant update that adopts the new CameraX Feature Group API. The migration from UseCaseGroup to SessionConfig for single camera sessions, along with the dynamic validation of feature combinations, is well-executed and crucial for preventing runtime failures on various devices. The introduction of FeatureGroupData provides a clean mapping layer, and the asynchronous updating of system constraints is a thoughtful performance consideration. My feedback is focused on improving test coverage for the complex new logic and ensuring adherence to the project's testing style guide.
core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraXCameraSystem.kt
Show resolved
Hide resolved
core/camera/src/test/java/com/google/jetpackcamera/core/camera/FeatureGroupDataTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code Review
This pull request successfully adopts the new CameraX Feature Combination Query API, which is a significant and valuable improvement for ensuring device compatibility and preventing runtime errors with unsupported feature combinations. The refactoring from UseCaseGroup to SessionConfig for single camera sessions is well-executed and aligns with modern CameraX practices. The introduction of FeatureGroupData and the dynamic constraint filtering in CameraXCameraSystem are the core of this change and are logically sound.
My review includes a few critical and high-severity findings. One critical issue is a compilation error in the new filtering logic due to using a suspend function within a non-suspend lambda. Another high-severity issue relates to potential resource leaks from improper CoroutineScope creation within a frequently called helper function. I've also included suggestions for improving code structure and adhering to the project's testing style guide. Addressing these points will enhance the robustness and maintainability of this new implementation.
core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraXCameraSystem.kt
Show resolved
Hide resolved
core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraXCameraSystem.kt
Show resolved
Hide resolved
core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraSession.kt
Show resolved
Hide resolved
core/camera/src/test/java/com/google/jetpackcamera/core/camera/FeatureGroupDataTest.kt
Outdated
Show resolved
Hide resolved
Zeronfinity
force-pushed
the
tahsinm/cxfg-adoption
branch
from
5b9bbda to
43a556c
Compare
Zeronfinity
force-pushed
the
tahsinm/cxfg-adoption
branch
from
43a556c to
70fa0fa
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring to adopt the new CameraX Feature Combination Query API. The changes are extensive, touching on session creation, constraint management, and settings application, but they are well-structured and improve the overall architecture. The migration from UseCaseGroup to SessionConfig for single camera sessions is a great modernization. The introduction of FeatureGroupData and the associated logic to dynamically filter system constraints is a robust solution to prevent crashes from unsupported feature combinations.
I've identified one high-severity issue regarding side effects in a feature-checking function that should be addressed. I've also included a suggestion to enhance the new integration test to make it more robust. Overall, this is excellent work.
core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraXCameraSystem.kt
Outdated
Show resolved
Hide resolved
.../camera/src/androidTest/java/com/google/jetpackcamera/core/camera/CameraXCameraSystemTest.kt
Outdated
Show resolved
Hide resolved
Zeronfinity
force-pushed
the
tahsinm/cxfg-adoption
branch
from
f56b76a to
bde634e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully adopts the new CameraX Feature Group API, which is a significant and complex undertaking. The refactoring from UseCaseGroup to SessionConfig is well-executed, and the new logic for dynamically updating system constraints based on feature combinations is robust. I particularly appreciate the addition of comprehensive unit and integration tests, which ensure the new functionality is reliable. The fix to call setSurfaceProvider on the main thread is also a crucial correction. I have one suggestion to improve the efficiency of the feature support checks.
core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraXCameraSystem.kt
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully adopts the new CameraX Feature Group API, which is a significant and valuable improvement. The changes allow for dynamic validation of feature combinations, preventing runtime crashes on devices with unsupported settings like HDR + 60 FPS. The refactoring from UseCaseGroup to SessionConfig for single camera sessions is well-executed and aligns with modern CameraX practices. The introduction of FeatureGroupData and the related mapping logic is clear and well-tested. The new integration test (setMultipleFeatures_systemConstraintsUpdatedAndFeaturesSetIfSupported) is particularly strong, as it verifies the dynamic constraint updates across various feature permutations. I have a couple of minor suggestions to improve code style and ensure documentation stays in sync with build changes. Overall, this is an excellent contribution that enhances the app's stability and correctness.
core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraSession.kt
Show resolved
Hide resolved
Zeronfinity
force-pushed
the
tahsinm/cxfg-adoption
branch
from
b8feb82 to
c577a74
Compare
| * Does not correspond to a specific [GroupableFeature] object, but inexplicitly groupable as | ||
| * it is equivalent to the base value CameraX feature groups API will use. | ||
| */ | ||
| object InexplicitlyGroupable : FeatureGroupData<Nothing> |
There was a problem hiding this comment.
nit: "Implicit" is more commonly used as an antonym with "Explicit". Could this be changed to "ImplicitlyGroupable"?
| import com.google.jetpackcamera.model.VideoQuality.UHD | ||
| import com.google.jetpackcamera.model.VideoQuality.UNSPECIFIED | ||
|
|
||
| sealed interface FeatureGroupData<out T> { |
There was a problem hiding this comment.
nit: Maybe instead of "FeatureGroupData", this could be named "FeatureGroupability"?
| import com.google.jetpackcamera.model.VideoQuality.UHD | ||
| import com.google.jetpackcamera.model.VideoQuality.UNSPECIFIED | ||
|
|
||
| sealed interface FeatureGroupData<out T> { |
There was a problem hiding this comment.
Can you add a quick kdoc to the top level to explain what this used for?
| OFF -> InexplicitlyGroupable | ||
| ON -> ExplicitlyGroupable(GroupableFeature.PREVIEW_STABILIZATION) | ||
| HIGH_QUALITY -> ExplicitlyGroupable(GroupableFeatures.VIDEO_STABILIZATION) | ||
| AUTO, // AUTO should be resolved to concrete value before this API is called |
There was a problem hiding this comment.
Throw an IllegalStateException rather than returning Ungroupable for AUTO. It's a programming error to try to call toFeatureGroupData without first resolving it to a concrete stabilization mode.
| * This update happens asynchronously after the initial camera session is started to avoid | ||
| * blocking the UI thread. | ||
| */ | ||
| private suspend fun updateSystemConstraintsByFeatureGroups() { |
There was a problem hiding this comment.
Is there any way we can move this to a seaparate file. CameraXCameraSystem is in dire need of refactoring, but I'd prefer not to add so many lines of code if possible.
Eventually I would like to move all of the constraints logic out, which may even help with consistency in the Settings screen.
| captureMode: CaptureMode, | ||
| backgroundDispatcher: CoroutineDispatcher, | ||
| targetFrameRate: Int? = null, | ||
| stabilizationMode: StabilizationMode? = null, | ||
| dynamicRange: DynamicRange? = null, | ||
| videoQuality: VideoQuality? = null | ||
| ): VideoCapture<Recorder>? { | ||
| if (captureMode != CaptureMode.STANDARD && captureMode != CaptureMode.VIDEO_ONLY) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
I suggest removing captureMode from this function and returning a non-nullable VideoCapture<Recorder>. The captureMode check should happen at the call site (similar to createImageUseCase) to avoid unnecessary argument passing and implicit null returns.
| withContext(Dispatchers.Main) { | ||
| setSurfaceProvider { surfaceRequest -> | ||
| surfaceRequests.update { surfaceRequest } | ||
| } |
There was a problem hiding this comment.
I'm assuming this withContext was added so that this can be called from within the featuregroup check? Do we need to set a surface provider just to check for capabilities?
| createSessionConfig( | ||
| cameraConstraints = cameraConstraints, | ||
| initialTransientSettings = transientSettings, | ||
| videoCaptureUseCase = videoCaptureUseCase, | ||
| sessionSettings = sessionSettings, | ||
| sessionScope = CoroutineScope(defaultDispatcher + job) | ||
| ) |
There was a problem hiding this comment.
This seems quite expensive. Any time we want to call supportsGrouping, which happens when we change a setting, we set up and creating an entire pipeline (including use cases, effects, etc.). Maybe it's unavoidable, but do we really need to do this every time we change settings? Are there enough combinations of settings that we couldn't just do this once up front during initialization? Maybe there is, but I would much prefer exploring that approach if it meant we could avoid this type of check whenever we change a setting.
| fun getCurrentSettings(): StateFlow<CameraAppSettings?> | ||
|
|
||
| fun setFlashMode(flashMode: FlashMode) | ||
| suspend fun setFlashMode(flashMode: FlashMode) |
There was a problem hiding this comment.
I'm not sure why we have so many setters here that are suspend, but most (all?) of them don't need to be, at least before this PR. We should be careful turning synchronous functions into asynchronous suspend functions. If one of these setters suspends for long enough, and another setter is called while it's suspending, you can run into a case where your suspend code will be run multiple times in order to ensure consistency (this is how CAS loops such as MutableStateFlow.update work).
Ideally, I would like for these setters to remain synchronous (non-suspend). They atomically update the CameraAppSettings, which also acts as our "prescriptive" UI settings, so it's important that they update quickly. If they have to suspend, we may need an intermediate state for CameraAppSettings that temporarily disables other settings in the UI while we're suspended.
| override fun getCurrentSettings() = currentSettings.asStateFlow() | ||
|
|
||
| override fun setFlashMode(flashMode: FlashMode) { | ||
| override suspend fun setFlashMode(flashMode: FlashMode) { |
There was a problem hiding this comment.
With this change, you're now calling a suspend function (tryApplyDynamicRangeConstraints) within a MutableStateFlow.update{} lambda, which internally uses a CAS loop. This could run multiple times depending on if the CameraAppSettings change during execution of the lambda, which is more likely if it takes longer, such as with a suspend function. Generally, you want to avoid any heavy computation within an update lambda.
2 participants
Description
This PR adopts the new CameraX Feature Group API to validate feature combinations and dynamically update system constraints. This ensures that the app only allows supported combinations of settings (e.g., HDR, FPS, Stabilization), preventing runtime failures on devices that do not support specific combinations (like HDR + 60 FPS on Pixel 9).
Key Changes
1.6.0-SNAPSHOTto access the stable Feature Groups API.FeatureGroupDatato map JCA internal models (DynamicRange,VideoQuality,StabilizationMode, etc.) to CameraX'sGroupableFeature.updateSystemConstraintsByFeatureGroupsinCameraXCameraSystem. This method asynchronously validates the current session settings against the device capabilities and filters out unsupported options for other settings (e.g., if 60 FPS is selected, it might disable HDR if they can't be used together).UseCaseGrouptoSessionConfigfor single camera session creation to align with the new API usage and allow forisSessionConfigSupportedchecks.Bug Fixes & Enhancements
Known Limitations & Future Work
System constraints not updating for multiple updates from setting screen
The system constraints are currently updated only when the camera session is initialized or re-created (e.g., upon returning to the preview screen). This means that if a user sets feature options (like UHD and 60 FPS) directly from the settings menu without navigating back to the preview screen, the system constraints is not re-evaluated.
Consequently, it allows users to select a combination of features that are not mutually supported by the device, which can lead to issues when they eventually navigate back to the preview screen.
TODO: Addressing this limitation requires further architectural refactoring to trigger constraint updates dynamically upon any individual setting change. Or, the constraints repository should be updated with the info of all possible constraints at initialization, rather than only the settings state navigable from the current settings state.
Features incompatible with CameraX Feature Group API
CameraX depends on the Camera2 feature combination query (FCQ) API which guarantees support only a limited set of features. So, the scenarios using features incompatible with the FCQ API do not get much benefits from this PR.
Tests
FeatureGroupDataTest(unit tests) to verify mapping logic between JCA models and CameraXGroupableFeature.setMultipleFeatures_systemConstraintsUpdatedAndFeaturesSetIfSupportedtoCameraXCameraSystemTest(integration test) to verify that the camera system dynamically updates constraints and sets features correctly based on CameraX feature group compatibility.Additional Context
Related Issues