Create Micro Image widget - 1/3 #2954
Conversation
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the application's image capture and face detection functionality. A new activity, MicroImageActivity, is added to support micro image captures, with its corresponding layout defined in micro_image_widget.xml. Updates in the Android manifest include a new meta-data entry for ML Kit’s face detection dependencies and an activity declaration for MicroImageActivity. The build script now includes new dependencies for the AndroidX Camera libraries and ML Kit face detection. New resource entries in XML files provide additional styling, string resources, and color definitions for the user interface components. Additionally, support for micro image capture is integrated into the existing form entry flow by extending constants, modifying response handling in FormEntryActivity, and creating a new widget class MicroImageWidget. Enhancements in MediaUtil and FaceCaptureView facilitate image cropping, compression, and processing, while access modifiers in ImageWidget have been adjusted for better subclass integration. Sequence Diagram(s)sequenceDiagram
participant U as User
participant MIW as MicroImageWidget
participant MIA as MicroImageActivity
participant CX as CameraX Provider
participant ML as ML Kit Face Detection
participant FCV as FaceCaptureView
participant MU as MediaUtil
U->>MIW: Initiates image capture
MIW->>MIA: Launch MicroImageActivity (Intent)
MIA->>CX: Request camera provider
CX-->>MIA: Provide camera provider
MIA->>CX: Bind preview & image analyzer
CX->>ML: Process image frames
ML-->>CX: Return detection results
CX->>MIA: Forward frame analysis
MIA->>FCV: Update face capture view
FCV-->>MIA: Notify image stabilization
MIA->>MU: Process image (crop & compress)
MU-->>MIA: Return processed image
MIA->>MIW: Deliver image result
MIW->>U: Display captured image
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
app/src/org/commcare/fragments/MicroImageActivity.java (3)
52-55: Consider adding user guards for critical UI references.
InonCreate, the cameraView and faceCaptureView references are used soon after retrieving them viafindViewById. If these views are crucial for functionality, you may want to add non-null checks or logs for better robustness in the event that layout inflation fails or a future layout change might remove these views.
66-71: Handle incompatible camera hardware gracefully.
When callingstartCamera()insideonCreate(), anyExecutionExceptionorInterruptedExceptionleads to an error message and returns. However, consider whether you also need a user-friendly fallback or an alternate path if the device lacks camera support or lacks the required ML capabilities, so that the user isn’t left in a dead-end activity.
127-148: Close the FaceDetector to free resources.
While theimageProxyis correctly closed, theFaceDetectormight also need explicit shutdown when you’re done (e.g., inonDestroy()) to release underlying resources. This is particularly helpful if the activity runs for an extended time or if multiple detectors could be used.app/src/org/commcare/views/FaceCaptureView.java (4)
48-60: Handle orientation changes dynamically.
Currently,imageWidthandimageHeightare set based on the device’s orientation at initialization. If the user rotates the device after launch, the sizes might no longer match the reality. Consider re-initializing or recalculating these inonConfigurationChangedor ensuring you lock the orientation.
74-100: Avoid redrawing background overlay on every orientation pass.
initCameraViewalways recreates a large bitmap and draws a face capture overlay. If the view size changes frequently (e.g., orientation changes or split-screen mode), it might be more efficient to recalculate only when strictly necessary, or to move this logic into a dedicatedonDrawoverride.
129-138: Notify listeners only upon actual face transitions.
Currently, every timeupdateFaceis called with a non-blank or blank face,postInvalidate()is triggered. Depending on the detection frequency, this can cause frequent UI updates. If performance becomes an issue on less capable devices, you might consider a threshold-based approach (e.g., only re-draw if face position changed meaningfully).
213-219: Optionally handle multiple faces.
InupdateFace, the code processes the first detected face. If future needs require tracking multiple faces simultaneously, you might expand logic to loop over all faces. Clarify in doc comments that only a single face is tracked if that’s by design.app/src/org/commcare/views/widgets/MicroImageWidget.java (1)
80-101: Address the TODO comment and improve bitmap configuration handling.The scaling logic is correct, but consider:
- Moving the scaling logic to a utility class for reuse
- Adding null checks for input bitmap
- Using more efficient bitmap configuration options
Would you like me to generate a refactored implementation in a utility class?
app/src/org/commcare/utils/MediaUtil.java (2)
45-45: Consider adjusting the quality reduction factor.The constant
IMAGE_QUALIY_REDUCTION_FACTOR = 10might be too aggressive for quality reduction. A smaller value like 5 would provide more granular control over compression.- private static final int IMAGE_QUALIY_REDUCTION_FACTOR = 10; + private static final int IMAGE_QUALIY_REDUCTION_FACTOR = 5;
538-543: Simplify the validation logic.The validation logic can be simplified for better readability.
private static boolean validateCropArea(Bitmap bitmap, Rect cropArea) { - if (bitmap.getHeight() >= cropArea.top && bitmap.getHeight() >= cropArea.bottom && bitmap.getWidth() >= cropArea.left && bitmap.getWidth() >= cropArea.right){ - return true; - } - return false; + return bitmap.getHeight() >= cropArea.bottom && + bitmap.getWidth() >= cropArea.right && + cropArea.top >= 0 && + cropArea.left >= 0; }app/res/layout/micro_image_widget.xml (1)
20-28: Consider adding state handling attributes.The FaceCaptureView should handle state changes and configuration changes gracefully.
<org.commcare.views.FaceCaptureView android:id="@+id/face_overlay" android:layout_width="match_parent" android:layout_height="0dp" + android:saveEnabled="true" app:background_color="@color/cc_neutral_bg_tr" app:face_capture_area_delimiter_color="@color/white" app:face_marker_color="@color/cc_attention_positive_color" app:countdown_text_size="@dimen/font_size_large" android:layout_weight="1"/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/AndroidManifest.xml(2 hunks)app/build.gradle(1 hunks)app/res/layout/micro_image_widget.xml(1 hunks)app/res/values/attrs.xml(1 hunks)app/res/values/colors.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/FormEntryActivity.java(1 hunks)app/src/org/commcare/activities/components/FormEntryConstants.java(1 hunks)app/src/org/commcare/fragments/MicroImageActivity.java(1 hunks)app/src/org/commcare/utils/MediaUtil.java(4 hunks)app/src/org/commcare/views/FaceCaptureView.java(1 hunks)app/src/org/commcare/views/widgets/ImageWidget.java(4 hunks)app/src/org/commcare/views/widgets/MediaWidget.java(1 hunks)app/src/org/commcare/views/widgets/MicroImageWidget.java(1 hunks)app/src/org/commcare/views/widgets/WidgetFactory.java(1 hunks)build.gradle(1 hunks)
🔇 Additional comments (20)
app/src/org/commcare/fragments/MicroImageActivity.java (1)
174-185: Validate cropping area before saving.
InonImageStabilizedListener, you assumeinputImageis valid and the cropping coordinates are within bounds. Consider verifying that the rectangle does not exceed the bitmap dimensions (especially if rotation or scaling edge cases occur). This reduces the risk of runtime errors.app/src/org/commcare/activities/components/FormEntryConstants.java (1)
24-24: No concerns; the constant is well-scoped.
The addition ofMICRO_IMAGE_CAPTUREaligns with the existing pattern of request codes. Looks good.app/src/org/commcare/views/widgets/MicroImageWidget.java (4)
24-27: LGTM! Constants are well-defined.The size limits (72px dimensions, 2KB file size) are appropriate for micro images.
29-36: LGTM! Constructor is properly implemented.The constructor correctly initializes the widget, hides unnecessary UI elements, and handles existing image data.
38-43: LGTM! Image capture is properly implemented.The method correctly launches the micro image capture activity and handles the form entry state.
65-78: LGTM! Answer handling is properly implemented.The methods correctly handle the binary data and cleanup.
app/src/org/commcare/views/widgets/WidgetFactory.java (2)
54-56: LGTM! Widget factory integration is correct.The new case properly handles the micro image control type.
54-56: LGTM!The new case for handling
CONTROL_MICRO_IMAGEis correctly positioned and properly instantiates theMicroImageWidgetwith the required parameters.app/src/org/commcare/views/widgets/MediaWidget.java (2)
359-361: LGTM! File existence check prevents potential crashes.The added check ensures the media file exists before attempting to play it.
359-361: LGTM!The added file existence check improves error handling by preventing potential crashes when attempting to play non-existent media files.
app/src/org/commcare/activities/FormEntryActivity.java (1)
361-362: LGTM! Micro image capture handling added.The micro image capture is correctly integrated into the existing image capture flow.
build.gradle (1)
33-33:✅ Verification successful
Verify CameraX version compatibility.
The specified CameraX version 1.2.3 should be verified for compatibility with the minimum supported Android API level.
🌐 Web query:
What is the minimum Android API level required for CameraX version 1.2.3?💡 Result:
CameraX version 1.2.3 requires a minimum Android API level of 21 (Android 5.0 Lollipop)[5][7]. This applies to all core CameraX functionality, including preview, image capture, and analysis use cases.
The requirement for API 21 ensures compatibility with approximately 90% of active Android devices[1][3]. While newer versions of Android may include additional optimizations or features, CameraX maintains backward compatibility through abstraction layers and automated device testing[1][5].
Citations:
- 1: https://proandroiddev.com/android-camerax-preview-analyze-capture-1b3f403a9395
- 2: https://www.youtube.com/watch?v=XUN6mUQiDpg
- 3: https://apilevels.com
- 4: https://developer.android.com/jetpack/androidx/releases/camera
- 5: https://developer.android.com/codelabs/camerax-getting-started
- 6: https://issuetracker.google.com/294126265
- 7: https://android-developers.googleblog.com/2022/08/camerax-12-is-now-in-beta.html
- 8: https://www.youtube.com/watch?v=12_iKwGIP64
- 9: Document new Android minimum requirements in the README (Java version / minSdk / compileSdk) juliansteenbakker/mobile_scanner#922
- 10: https://www.delasign.com/blog/android-studio-jetpack-compose-kotlin-camerax-composable/
Action: Ensure your project's minSdkVersion is set to API level 21 or higher.
The
cameraX_versionis set to'1.2.3'inbuild.gradle(line 33), and as confirmed, CameraX v1.2.3 requires a minimum Android API level of 21. Please verify that your project's build configuration (e.g., in the module-levelbuild.gradle) specifies aminSdkVersionof at least 21 to maintain compatibility.app/res/values/attrs.xml (1)
56-61: New FaceCaptureView Styleable Declaration Added
The new<declare-styleable name="FaceCaptureView">with its four attributes (background_color,face_capture_area_delimiter_color,face_marker_color, andcountdown_text_size) is clear and well defined. Ensure that these attribute names match the naming conventions used elsewhere in the project for consistency.app/res/values/colors.xml (1)
89-89: Addition of New Color cc_neutral_bg_tr
A new color resource<color name="cc_neutral_bg_tr">#80D6D6D4</color>has been added to expand the palette. This color—with its transparency setting—appears to be designed for use in overlays or subtle backgrounds. Verify that its visual integration with UI components (especially in the Micro Image Widget) meets design expectations.app/build.gradle (1)
69-73: Integration of New CameraX and ML Kit Dependencies
New dependencies for the AndroidX Camera libraries (camera-view, camera-core, camera-camera2, camera-lifecycle) and ML Kit face detection (com.google.android.gms:play-services-mlkit-face-detection:17.1.0) have been added. They’re correctly parameterized using thecameraX_versionvariable. Please ensure that thecameraX_versionis defined appropriately elsewhere and that these versions are compatible with the rest of the codebase.app/AndroidManifest.xml (2)
142-145: Adding ML Kit Face Detection Meta-data
The new<meta-data>element forcom.google.mlkit.vision.DEPENDENCIESwith a value of"face"is appropriately added. This configuration informs ML Kit which dependencies to load for face detection. Confirm that this value aligns with the ML Kit implementation expectations in your app.
324-327: New MicroImageActivity Declaration
A new<activity>fororg.commcare.fragments.MicroImageActivityis introduced withandroid:windowSoftInputMode="adjustResize". This ensures that the activity’s UI adapts correctly when the soft keyboard appears. Verify that the package name and declared behavior match the implementation of the Micro Image Widget, and that any corresponding resources (layouts, strings, etc.) are properly configured.app/res/values/strings.xml (1)
464-467: New String Resources for Micro Image Functionality
Four new string resources related to the micro image capture feature have been added:
micro_image_activity_title: "Capture Face Picture"camera_start_failed: "Camera failed to start"face_detection_mode_failed: "Face detection mode failed"micro_image_cropping_failed: "Image cropping failed, make sure the target is in the capture area"These resources follow the project’s naming conventions and support clear user feedback in cases of error.
app/src/org/commcare/views/widgets/ImageWidget.java (2)
67-67: LGTM!The access modifier changes from private to protected enable proper inheritance and extension by subclasses like
MicroImageWidget.Also applies to: 71-71, 78-78, 247-247, 273-273
212-212: LGTM!Moving the discard button visibility update inside the conditional block ensures proper UI state management, showing the button only when an image exists.
| @Override | ||
| public void setBinaryData(Object binaryPath) { | ||
| if (mBinaryName != null) { | ||
| deleteMedia(); | ||
| } | ||
|
|
||
| File f = new File(binaryPath.toString()); | ||
| Bitmap originalImage = BitmapFactory.decodeFile(binaryPath.toString()); | ||
|
|
||
| Bitmap scaledDownBitmap = scaleImage(originalImage, IMAGE_DIMEN_SCALED_MAX_PX, IMAGE_DIMEN_SCALED_MAX_PX); | ||
| byte[] compressedBitmapByteArray = MediaUtil.compressBitmapToTargetSize(scaledDownBitmap, MICRO_IMAGE_MAX_SIZE_BYTES); | ||
|
|
||
| try { | ||
| mBinary = Base64.encodeToString(compressedBitmapByteArray, Base64.DEFAULT); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| mBinaryName = f.getName(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and resource cleanup for bitmap operations.
The bitmap operations could lead to OutOfMemoryError in low memory conditions. Consider:
- Adding try-catch blocks for OOM errors
- Explicitly recycling bitmap resources
Apply this diff to improve error handling and resource cleanup:
@Override
public void setBinaryData(Object binaryPath) {
if (mBinaryName != null) {
deleteMedia();
}
File f = new File(binaryPath.toString());
- Bitmap originalImage = BitmapFactory.decodeFile(binaryPath.toString());
+ Bitmap originalImage = null;
+ Bitmap scaledDownBitmap = null;
+ try {
+ originalImage = BitmapFactory.decodeFile(binaryPath.toString());
+ if (originalImage == null) {
+ throw new RuntimeException("Failed to decode image file");
+ }
+ scaledDownBitmap = scaleImage(originalImage, IMAGE_DIMEN_SCALED_MAX_PX, IMAGE_DIMEN_SCALED_MAX_PX);
+ byte[] compressedBitmapByteArray = MediaUtil.compressBitmapToTargetSize(scaledDownBitmap, MICRO_IMAGE_MAX_SIZE_BYTES);
+ mBinary = Base64.encodeToString(compressedBitmapByteArray, Base64.DEFAULT);
+ } catch (OutOfMemoryError e) {
+ throw new RuntimeException("Insufficient memory to process image", e);
+ } finally {
+ if (originalImage != null) {
+ originalImage.recycle();
+ }
+ if (scaledDownBitmap != null) {
+ scaledDownBitmap.recycle();
+ }
+ }
- Bitmap scaledDownBitmap = scaleImage(originalImage, IMAGE_DIMEN_SCALED_MAX_PX, IMAGE_DIMEN_SCALED_MAX_PX);
- byte[] compressedBitmapByteArray = MediaUtil.compressBitmapToTargetSize(scaledDownBitmap, MICRO_IMAGE_MAX_SIZE_BYTES);
- try {
- mBinary = Base64.encodeToString(compressedBitmapByteArray, Base64.DEFAULT);
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
mBinaryName = f.getName();
}📝 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.
| @Override | |
| public void setBinaryData(Object binaryPath) { | |
| if (mBinaryName != null) { | |
| deleteMedia(); | |
| } | |
| File f = new File(binaryPath.toString()); | |
| Bitmap originalImage = BitmapFactory.decodeFile(binaryPath.toString()); | |
| Bitmap scaledDownBitmap = scaleImage(originalImage, IMAGE_DIMEN_SCALED_MAX_PX, IMAGE_DIMEN_SCALED_MAX_PX); | |
| byte[] compressedBitmapByteArray = MediaUtil.compressBitmapToTargetSize(scaledDownBitmap, MICRO_IMAGE_MAX_SIZE_BYTES); | |
| try { | |
| mBinary = Base64.encodeToString(compressedBitmapByteArray, Base64.DEFAULT); | |
| } catch (Exception e) { | |
| throw new RuntimeException(e); | |
| } | |
| mBinaryName = f.getName(); | |
| } | |
| @Override | |
| public void setBinaryData(Object binaryPath) { | |
| if (mBinaryName != null) { | |
| deleteMedia(); | |
| } | |
| File f = new File(binaryPath.toString()); | |
| Bitmap originalImage = null; | |
| Bitmap scaledDownBitmap = null; | |
| try { | |
| originalImage = BitmapFactory.decodeFile(binaryPath.toString()); | |
| if (originalImage == null) { | |
| throw new RuntimeException("Failed to decode image file"); | |
| } | |
| scaledDownBitmap = scaleImage(originalImage, IMAGE_DIMEN_SCALED_MAX_PX, IMAGE_DIMEN_SCALED_MAX_PX); | |
| byte[] compressedBitmapByteArray = MediaUtil.compressBitmapToTargetSize(scaledDownBitmap, MICRO_IMAGE_MAX_SIZE_BYTES); | |
| mBinary = Base64.encodeToString(compressedBitmapByteArray, Base64.DEFAULT); | |
| } catch (OutOfMemoryError e) { | |
| throw new RuntimeException("Insufficient memory to process image", e); | |
| } finally { | |
| if (originalImage != null) { | |
| originalImage.recycle(); | |
| } | |
| if (scaledDownBitmap != null) { | |
| scaledDownBitmap.recycle(); | |
| } | |
| } | |
| mBinaryName = f.getName(); | |
| } |
| /** | ||
| * Crops an image according to a given area and saves the resulting image | ||
| */ | ||
| public static void cropAndSaveImage(Bitmap bitmap, Rect cropArea, File imageFile) { | ||
| if (!validateCropArea(bitmap, cropArea)) { | ||
| throw new RuntimeException("Cropping failed due to invalid area!"); | ||
| } | ||
|
|
||
| Bitmap croppedBitmap = Bitmap.createBitmap(bitmap, cropArea.left, cropArea.top, | ||
| cropArea.right - cropArea.left, cropArea.bottom - cropArea.top); | ||
|
|
||
| try { | ||
| FileUtil.writeBitmapToDiskAndCleanupHandles(croppedBitmap, | ||
| ImageType.fromExtension(FileUtil.getExtension(imageFile.getPath())), imageFile); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Failed to save image after cropping", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle bitmap recycling to prevent memory leaks.
The cropped bitmap should be recycled after saving to prevent memory leaks, especially since bitmap operations are memory-intensive.
public static void cropAndSaveImage(Bitmap bitmap, Rect cropArea, File imageFile) {
if (!validateCropArea(bitmap, cropArea)) {
throw new RuntimeException("Cropping failed due to invalid area!");
}
Bitmap croppedBitmap = Bitmap.createBitmap(bitmap, cropArea.left, cropArea.top,
cropArea.right - cropArea.left, cropArea.bottom - cropArea.top);
try {
FileUtil.writeBitmapToDiskAndCleanupHandles(croppedBitmap,
ImageType.fromExtension(FileUtil.getExtension(imageFile.getPath())), imageFile);
+ if (croppedBitmap != bitmap) {
+ croppedBitmap.recycle();
+ }
} catch (IOException e) {
throw new RuntimeException("Failed to save image after cropping", e);
}
}📝 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.
| /** | |
| * Crops an image according to a given area and saves the resulting image | |
| */ | |
| public static void cropAndSaveImage(Bitmap bitmap, Rect cropArea, File imageFile) { | |
| if (!validateCropArea(bitmap, cropArea)) { | |
| throw new RuntimeException("Cropping failed due to invalid area!"); | |
| } | |
| Bitmap croppedBitmap = Bitmap.createBitmap(bitmap, cropArea.left, cropArea.top, | |
| cropArea.right - cropArea.left, cropArea.bottom - cropArea.top); | |
| try { | |
| FileUtil.writeBitmapToDiskAndCleanupHandles(croppedBitmap, | |
| ImageType.fromExtension(FileUtil.getExtension(imageFile.getPath())), imageFile); | |
| } catch (IOException e) { | |
| throw new RuntimeException("Failed to save image after cropping", e); | |
| } | |
| } | |
| /** | |
| * Crops an image according to a given area and saves the resulting image | |
| */ | |
| public static void cropAndSaveImage(Bitmap bitmap, Rect cropArea, File imageFile) { | |
| if (!validateCropArea(bitmap, cropArea)) { | |
| throw new RuntimeException("Cropping failed due to invalid area!"); | |
| } | |
| Bitmap croppedBitmap = Bitmap.createBitmap(bitmap, cropArea.left, cropArea.top, | |
| cropArea.right - cropArea.left, cropArea.bottom - cropArea.top); | |
| try { | |
| FileUtil.writeBitmapToDiskAndCleanupHandles(croppedBitmap, | |
| ImageType.fromExtension(FileUtil.getExtension(imageFile.getPath())), imageFile); | |
| if (croppedBitmap != bitmap) { | |
| croppedBitmap.recycle(); | |
| } | |
| } catch (IOException e) { | |
| throw new RuntimeException("Failed to save image after cropping", e); | |
| } | |
| } |
| public static byte[] compressBitmapToTargetSize(Bitmap bitmap, int targetSize) { | ||
| if (bitmap == null) { | ||
| return null; | ||
| } | ||
|
|
||
| byte[] byteArray = null; | ||
| int quality = 100; | ||
| while (quality != 0) { | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| bitmap.compress(Bitmap.CompressFormat.WEBP, quality, baos); | ||
| byteArray = baos.toByteArray(); | ||
| if (byteArray.length <= targetSize) { | ||
| break; | ||
| } | ||
| quality -= IMAGE_QUALIY_REDUCTION_FACTOR; | ||
| } | ||
| return byteArray; | ||
| } |
There was a problem hiding this comment.
Add null check for ByteArrayOutputStream and handle resource cleanup.
The ByteArrayOutputStream should be properly closed to prevent resource leaks.
public static byte[] compressBitmapToTargetSize(Bitmap bitmap, int targetSize) {
if (bitmap == null) {
return null;
}
byte[] byteArray = null;
int quality = 100;
while (quality != 0) {
- ByteArrayOutputStream baos = new ByteArrayOutputStream();
- bitmap.compress(Bitmap.CompressFormat.WEBP, quality, baos);
- byteArray = baos.toByteArray();
- if (byteArray.length <= targetSize) {
- break;
+ try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
+ bitmap.compress(Bitmap.CompressFormat.WEBP, quality, baos);
+ byteArray = baos.toByteArray();
+ if (byteArray.length <= targetSize) {
+ break;
+ }
+ quality -= IMAGE_QUALIY_REDUCTION_FACTOR;
}
- quality -= IMAGE_QUALIY_REDUCTION_FACTOR;
}
return byteArray;
}📝 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.
| public static byte[] compressBitmapToTargetSize(Bitmap bitmap, int targetSize) { | |
| if (bitmap == null) { | |
| return null; | |
| } | |
| byte[] byteArray = null; | |
| int quality = 100; | |
| while (quality != 0) { | |
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | |
| bitmap.compress(Bitmap.CompressFormat.WEBP, quality, baos); | |
| byteArray = baos.toByteArray(); | |
| if (byteArray.length <= targetSize) { | |
| break; | |
| } | |
| quality -= IMAGE_QUALIY_REDUCTION_FACTOR; | |
| } | |
| return byteArray; | |
| } | |
| public static byte[] compressBitmapToTargetSize(Bitmap bitmap, int targetSize) { | |
| if (bitmap == null) { | |
| return null; | |
| } | |
| byte[] byteArray = null; | |
| int quality = 100; | |
| while (quality != 0) { | |
| try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { | |
| bitmap.compress(Bitmap.CompressFormat.WEBP, quality, baos); | |
| byteArray = baos.toByteArray(); | |
| if (byteArray.length <= targetSize) { | |
| break; | |
| } | |
| quality -= IMAGE_QUALIY_REDUCTION_FACTOR; | |
| } | |
| } | |
| return byteArray; | |
| } |
| public static Bitmap decodeBase64EncodedBitmap(String base64Image){ | ||
| try { | ||
| byte[] decodedString = Base64.decode(base64Image, Base64.DEFAULT); | ||
| return BitmapFactory.decodeByteArray(decodedString, 0, decodedString.length); | ||
| } catch(Exception e){ | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in base64 decoding.
The current implementation silently returns null for any exception. Consider logging the error and handling specific exceptions.
public static Bitmap decodeBase64EncodedBitmap(String base64Image){
+ if (base64Image == null) {
+ return null;
+ }
try {
byte[] decodedString = Base64.decode(base64Image, Base64.DEFAULT);
return BitmapFactory.decodeByteArray(decodedString, 0, decodedString.length);
- } catch(Exception e){
+ } catch(IllegalArgumentException e){
+ Logger.log(LogTypes.TYPE_ERROR_ASSERTION, "Invalid base64 string: " + e.getMessage());
+ return null;
+ } catch(OutOfMemoryError e) {
+ Logger.log(LogTypes.TYPE_ERROR_ASSERTION, "Out of memory decoding base64 bitmap: " + e.getMessage());
return null;
}
}📝 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.
| public static Bitmap decodeBase64EncodedBitmap(String base64Image){ | |
| try { | |
| byte[] decodedString = Base64.decode(base64Image, Base64.DEFAULT); | |
| return BitmapFactory.decodeByteArray(decodedString, 0, decodedString.length); | |
| } catch(Exception e){ | |
| return null; | |
| } | |
| } | |
| public static Bitmap decodeBase64EncodedBitmap(String base64Image){ | |
| if (base64Image == null) { | |
| return null; | |
| } | |
| try { | |
| byte[] decodedString = Base64.decode(base64Image, Base64.DEFAULT); | |
| return BitmapFactory.decodeByteArray(decodedString, 0, decodedString.length); | |
| } catch(IllegalArgumentException e){ | |
| Logger.log(LogTypes.TYPE_ERROR_ASSERTION, "Invalid base64 string: " + e.getMessage()); | |
| return null; | |
| } catch(OutOfMemoryError e) { | |
| Logger.log(LogTypes.TYPE_ERROR_ASSERTION, "Out of memory decoding base64 bitmap: " + e.getMessage()); | |
| return null; | |
| } | |
| } |
| <androidx.camera.view.PreviewView | ||
| android:id="@+id/view_finder" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add content description for accessibility.
The PreviewView should have a content description for screen readers.
<androidx.camera.view.PreviewView
android:id="@+id/view_finder"
android:layout_width="match_parent"
- android:layout_height="match_parent" />
+ android:layout_height="match_parent"
+ android:contentDescription="@string/camera_preview_content_description" />📝 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.
| <androidx.camera.view.PreviewView | |
| android:id="@+id/view_finder" | |
| android:layout_width="match_parent" | |
| android:layout_height="match_parent" /> | |
| <androidx.camera.view.PreviewView | |
| android:id="@+id/view_finder" | |
| android:layout_width="match_parent" | |
| android:layout_height="match_parent" | |
| android:contentDescription="@string/camera_preview_content_description" /> |
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
app/src/org/commcare/utils/MediaUtil.java (1)
571-578: 🛠️ Refactor suggestionImprove error handling in base64 decoding.
The current implementation silently returns null for any exception. Consider:
- Adding null check for input
- Handling specific exceptions separately
- Adding logging for debugging
Apply this diff to improve error handling:
- public static Bitmap decodeBase64EncodedBitmap(String base64Image){ + public static Bitmap decodeBase64EncodedBitmap(String base64Image) { + if (base64Image == null) { + return null; + } try { byte[] decodedString = Base64.decode(base64Image, Base64.DEFAULT); return BitmapFactory.decodeByteArray(decodedString, 0, decodedString.length); - } catch(Exception e){ + } catch(IllegalArgumentException e) { + Logger.log(LogTypes.TYPE_ERROR_ASSERTION, "Invalid base64 string: " + e.getMessage()); + return null; + } catch(OutOfMemoryError e) { + Logger.log(LogTypes.TYPE_ERROR_ASSERTION, "Out of memory decoding base64 bitmap: " + e.getMessage()); return null; } }app/res/layout/micro_image_widget.xml (1)
9-12: 🛠️ Refactor suggestionAdd content description for accessibility.
The PreviewView should have a content description for screen readers.
Apply this diff to improve accessibility:
<androidx.camera.view.PreviewView android:id="@+id/view_finder" android:layout_width="match_parent" - android:layout_height="match_parent" /> + android:layout_height="match_parent" + android:contentDescription="@string/camera_preview_content_description" />
🧹 Nitpick comments (12)
app/src/org/commcare/views/FaceCaptureView.java (3)
27-29: Consider moving interface out of the view class.
Defining theImageStabilizedListenerinterface insideFaceCaptureViewtightly couples the listener definition to this view. Consider extracting this interface to promote reusability across other face-capture-related components.
48-60: Parameterize default image dimensions.
Currently,DEFAULT_IMAGE_WIDTHandDEFAULT_IMAGE_HEIGHTare set to 480x640 or swapped based on orientation. In certain devices or use cases, these hard-coded defaults may be unsuitable. Consider making these values configurable via XML attributes or resource values.
129-138: Provide clearer null handling.
WhenupdateFace(null)is called, the code simply resets the face. Clarify or document the scenario under which face detection yields a null face. This ensures future maintainers understand the intended flow for missed or lost face-tracking events.app/src/org/commcare/views/widgets/MicroImageWidget.java (2)
33-37: Validate correct handling of existing base64 images.
When an existingBase64ImageDatais found, the widget setsmBinarybut does not preview it. If you'd like the user to see a preview in micro form, consider displaying the decoded and scaled image directly in the widget.
94-115: Refactor scaleImage for improved clarity.
The logic is functional, but consider extracting ratio calculations or including safety checks for edge cases (e.g., zero-width or zero-height images). More descriptive naming could enhance maintainability.app/src/org/commcare/fragments/MicroImageActivity.java (2)
46-51: Consider adding state restoration.The activity holds important state in member variables but doesn't implement
onSaveInstanceState(). This could lead to state loss during configuration changes (e.g., rotation).Add state restoration:
+private static final String KEY_INPUT_IMAGE = "input_image"; + +@Override +protected void onSaveInstanceState(@NonNull Bundle outState) { + super.onSaveInstanceState(outState); + if (inputImage != null) { + outState.putParcelable(KEY_INPUT_IMAGE, inputImage); + } +} + +@Override +protected void onRestoreInstanceState(@NonNull Bundle savedInstanceState) { + super.onRestoreInstanceState(savedInstanceState); + if (savedInstanceState.containsKey(KEY_INPUT_IMAGE)) { + inputImage = savedInstanceState.getParcelable(KEY_INPUT_IMAGE); + } +}
110-119: Consider adding configuration options for image analysis.The image analyzer configuration is hardcoded. Consider making it configurable for different use cases.
Extract configuration to builder pattern:
+public static class Config { + private Size targetResolution; + private int targetRotation; + private ImageAnalysis.BackpressureStrategy backpressureStrategy = + ImageAnalysis.STRATEGY_KEEP_ONLY_LATEST; + + public static Config.Builder builder() { + return new Builder(); + } + + public static class Builder { + private final Config config = new Config(); + + public Builder setTargetResolution(Size resolution) { + config.targetResolution = resolution; + return this; + } + + public Config build() { + return config; + } + } +}app/src/org/commcare/views/widgets/MediaWidget.java (1)
359-361: LGTM! Consider adding user feedback.The check for file existence prevents crashes, but users might benefit from feedback when media is missing.
Add user feedback:
if (!mediaFile.exists()) { + Toast.makeText(context, + StringUtils.getStringSpannableRobust(context, R.string.media_not_found), + Toast.LENGTH_SHORT).show(); return; }app/src/org/commcare/views/widgets/ImageWidget.java (2)
67-67: LGTM! Document protected members.The change in access modifiers enables subclassing, but protected members should be documented for subclass implementers.
Add documentation:
+ /** + * Button for choosing images from gallery. Protected to allow customization in subclasses. + */ protected final Button mChooseButton; + /** + * Name of the binary file containing the image. Protected to allow access in subclasses. + */ protected String mBinaryName; + /** + * Interface for handling pending callouts. Protected to allow customization in subclasses. + */ protected final PendingCalloutInterface pendingCalloutInterface;Also applies to: 71-71, 78-78
247-247: LGTM! Consider adding @OverRide annotation.The protected methods are intended for subclassing, but missing @OverRide annotations in subclasses could lead to subtle bugs.
Add documentation encouraging @OverRide:
+ /** + * Takes a picture using the device camera. Subclasses must use @Override when implementing. + */ protected void takePicture() { + /** + * Deletes the current media file. Subclasses must use @Override when implementing. + */ protected void deleteMedia() {Also applies to: 273-273
app/assets/locales/android_translatable_strings.txt (1)
995-1001: Nitpick: Enhance Consistency and Clarity in Micro Image Error Messages
The new error message entries for the micro image widget are clear and follow a similar key format to existing messages. However, for improved clarity and consistency across messages, consider revising the wording and punctuation. For instance, the cropping error currently reads:microimage.cropping.failed=Image cropping failed, make sure the target is inside the capture areaA suggested revision would be:
- microimage.cropping.failed=Image cropping failed, make sure the target is inside the capture area + microimage.cropping.failed=Image cropping failed. Please ensure the target is within the capture area.Additionally, you might want to standardize the punctuation in the other messages so that each sentence ends with a period (if that matches the style used elsewhere in your locales). For example:
Consider changing
microimage.decoding.no.image=Image processing failed. Please try againto
microimage.decoding.no.image=Image processing failed. Please try again.And similarly for
microimage.scalingdown.compression.error=Image compression failed. Please try againto
microimage.scalingdown.compression.error=Image compression failed. Please try again.These refinements are optional but can help maintain a consistent tone and improve user understanding of the error prompts.
app/src/org/commcare/activities/FormEntryActivity.java (1)
361-362: LGTM! Consider adding documentation for the micro image capture case.The combined case statement for handling both regular and micro image captures is clean and follows DRY principles. However, adding a brief comment explaining the micro image capture functionality would improve code maintainability.
Add a comment like:
+// Handle both regular and micro image captures using the same processing logic case FormEntryConstants.IMAGE_CAPTURE, FormEntryConstants.MICRO_IMAGE_CAPTURE: ImageCaptureProcessing.processCaptureResponse(this, FormEntryInstanceState.getInstanceFolder(), true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/AndroidManifest.xml(2 hunks)app/assets/locales/android_translatable_strings.txt(1 hunks)app/build.gradle(1 hunks)app/res/layout/micro_image_widget.xml(1 hunks)app/res/values/attrs.xml(1 hunks)app/res/values/colors.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/FormEntryActivity.java(1 hunks)app/src/org/commcare/activities/components/FormEntryConstants.java(1 hunks)app/src/org/commcare/fragments/MicroImageActivity.java(1 hunks)app/src/org/commcare/utils/MediaUtil.java(4 hunks)app/src/org/commcare/views/FaceCaptureView.java(1 hunks)app/src/org/commcare/views/widgets/ImageWidget.java(4 hunks)app/src/org/commcare/views/widgets/MediaWidget.java(1 hunks)app/src/org/commcare/views/widgets/MicroImageWidget.java(1 hunks)app/src/org/commcare/views/widgets/WidgetFactory.java(1 hunks)build.gradle(1 hunks)
🔇 Additional comments (17)
app/src/org/commcare/views/FaceCaptureView.java (2)
118-127: Evaluate the concurrency approach.
onDraw()usessynchronized(lock)to guardfaceOvalGraphic, but concurrency control for other interactions (e.g., calls toupdateFace()) may be incomplete. Validate whether multiple threads can modifyfaceOvalGraphicor accessfaceCaptureAreasimultaneously, and ensure thread-safety is uniformly applied if so.
234-236: Clarify coordinate space consistency.
You draw the face oval using translated coordinates, but call the listener with the raw bounding box (currFace.getBoundingBox()). If the listener expects view coordinates, provide the translated bounding box. If it expects original image coordinates, confirm that the current usage is correct.app/src/org/commcare/activities/components/FormEntryConstants.java (1)
24-24: Ensure the new request code is uniquely handled.
MICRO_IMAGE_CAPTUREis set to17; verify there are no conflicts with other request codes. Also confirm that higher-level logic properly branches for micro versus standard image capture.app/src/org/commcare/views/widgets/MicroImageWidget.java (2)
46-75: Add explicit OutOfMemoryError handling.
You catch genericException, but notOutOfMemoryError. A low-memory device could trigger OOM during bitmap operations. Handling OOM separately can improve clarity and facilitate targeted recovery or fallbacks.
25-27: Assess 2KB image limit.
MICRO_IMAGE_MAX_SIZE_BYTESis 2KB, which is highly restrictive and may result in extremely low-resolution images. Verify that this meets use-case requirements. If the images are rendered unusable, consider relaxing the limit or introducing separate settings.app/src/org/commcare/views/widgets/WidgetFactory.java (1)
54-56:❓ Verification inconclusive
LGTM! Consider adding validation.
The implementation correctly adds support for the new control type. However, consider validating that the required dependencies for MicroImageWidget are available.
🏁 Script executed:
#!/bin/bash # Check if ML Kit dependencies are available in the project rg -l "implementation.*'com.google.mlkit:face-detection'"Length of output: 65
Action: Verify ML Kit Dependency Inclusion
The new control type (MicroImageWidget) is correctly added in
app/src/org/commcare/views/widgets/WidgetFactory.java(lines 54–56). However, please ensure that any required dependencies—such as the ML Kit face detection library (com.google.mlkit:face-detection)—are present in your build configuration. Manual verification of this dependency is recommended since the automated check did not return any output.app/src/org/commcare/utils/MediaUtil.java (2)
519-539: LGTM! Proper bitmap handling and error handling.The implementation correctly:
- Validates the crop area
- Handles bitmap recycling in a finally block
- Includes proper error handling with descriptive messages
548-569: LGTM! Efficient compression with proper resource management.The implementation correctly:
- Uses try-with-resources for proper resource cleanup
- Implements progressive quality reduction
- Includes logging for monitoring compression cycles
- Handles null input gracefully
app/src/org/commcare/activities/FormEntryActivity.java (2)
361-362: LGTM! Clean integration of micro image capture.The changes efficiently handle both standard and micro image capture using the same processing method.
361-362:✅ Verification successful
Verify micro image capture dependencies.
Please ensure that:
- The
FormEntryConstants.MICRO_IMAGE_CAPTUREconstant is properly defined.- The
ImageCaptureProcessing.processCaptureResponsemethod can handle both regular and micro image captures correctly.
🏁 Script executed:
#!/bin/bash # Check FormEntryConstants for MICRO_IMAGE_CAPTURE definition echo "Checking for MICRO_IMAGE_CAPTURE constant definition..." rg "MICRO_IMAGE_CAPTURE\s*=\s*\d+" -A 2 # Check ImageCaptureProcessing implementation echo "Checking ImageCaptureProcessing implementation..." rg "class ImageCaptureProcessing" -A 10Length of output: 1944
Micro Image Capture Dependencies Verified
- Constant Verification: The
FormEntryConstants.MICRO_IMAGE_CAPTUREis defined correctly (value 17).- Processing Logic: The
ImageCaptureProcessingclass exists and itsprocessCaptureResponsemethod appears to be used identically for both regular and micro image captures without additional branching.No further action is required.
app/res/layout/micro_image_widget.xml (1)
14-29: LGTM! Well-structured layout with proper weights.The LinearLayout is properly configured with:
- Vertical orientation
- Center gravity
- Proper weight distribution for the FaceCaptureView
app/res/values/attrs.xml (1)
56-61: New FaceCaptureView Attributes Added
The new<declare-styleable name="FaceCaptureView">block correctly defines the custom attributes used by the view (i.e.background_color,face_capture_area_delimiter_color,face_marker_color, andcountdown_text_size). Ensure that these attributes are consistently referenced in the layout (e.g. in micro_image_widget.xml) and that their formats (color and dimension) match your design expectations.app/res/values/colors.xml (1)
89-89: New Semi-Transparent Color Added
The new color<color name="cc_neutral_bg_tr">#80D6D6D4</color>introduces a semi-transparent neutral background. Verify that its opacity and hue fit with the overall UI palette and that it is correctly applied in the new face capture widget.app/build.gradle (1)
69-73: Addition of CameraX and ML Kit Face Detection Dependencies
The dependencies added for the CameraX libraries and ML Kit face detection (using$cameraX_versionand the fixed version for ML Kit) are essential for the new micro image widget functionality. Please double-check thatcameraX_versionis defined appropriately elsewhere in the build script and that these versions are compatible with other parts of the application.app/AndroidManifest.xml (2)
142-144: Meta-data for ML Kit Face Detection Integrated
The new<meta-data>element withandroid:name="com.google.mlkit.vision.DEPENDENCIES"andandroid:value="face"is correctly added. This entry informs ML Kit which vision modules to load. Verify that this configuration matches the requirements of the ML Kit version you are using.
324-327: New MicroImageActivity Declaration
The<activity>declaration fororg.commcare.fragments.MicroImageActivitywithandroid:windowSoftInputMode="adjustResize"is added properly. Ensure that the associated activity implementation correctly leverages the new camera preview and FaceCaptureView features defined in other parts of the PR.app/res/values/strings.xml (1)
464-465: New String for Micro Image Activity Title
The string resource<string name="micro_image_activity_title" cc:translatable="true">Capture Face Picture</string>has been added. This will label the new activity appropriately. Confirm that all UI references to this resource correctly display the updated text and that it follows localization conventions.
| @Override | ||
| public void onImageStabilizedListener(Rect faceArea) { | ||
| try { | ||
| MediaUtil.cropAndSaveImage(inputImage, faceArea, ImageWidget.getTempFileForImageCapture()); | ||
| setResult(AppCompatActivity.RESULT_OK); | ||
| finish(); | ||
| } catch (Exception e) { | ||
| logErrorAndExit(e.getMessage(), "microimage.cropping.failed", e.getCause()); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup in onImageStabilizedListener.
The method should clean up resources after saving the image.
Add cleanup:
@Override
public void onImageStabilizedListener(Rect faceArea) {
try {
MediaUtil.cropAndSaveImage(inputImage, faceArea, ImageWidget.getTempFileForImageCapture());
setResult(AppCompatActivity.RESULT_OK);
+ cleanup();
finish();
} catch (Exception e) {
+ cleanup();
logErrorAndExit(e.getMessage(), "microimage.cropping.failed", e.getCause());
}
}
+
+ private void cleanup() {
+ if (inputImage != null) {
+ inputImage.recycle();
+ inputImage = null;
+ }
+ }📝 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.
| @Override | |
| public void onImageStabilizedListener(Rect faceArea) { | |
| try { | |
| MediaUtil.cropAndSaveImage(inputImage, faceArea, ImageWidget.getTempFileForImageCapture()); | |
| setResult(AppCompatActivity.RESULT_OK); | |
| finish(); | |
| } catch (Exception e) { | |
| logErrorAndExit(e.getMessage(), "microimage.cropping.failed", e.getCause()); | |
| } | |
| } | |
| @Override | |
| public void onImageStabilizedListener(Rect faceArea) { | |
| try { | |
| MediaUtil.cropAndSaveImage(inputImage, faceArea, ImageWidget.getTempFileForImageCapture()); | |
| setResult(AppCompatActivity.RESULT_OK); | |
| cleanup(); | |
| finish(); | |
| } catch (Exception e) { | |
| cleanup(); | |
| logErrorAndExit(e.getMessage(), "microimage.cropping.failed", e.getCause()); | |
| } | |
| } | |
| private void cleanup() { | |
| if (inputImage != null) { | |
| inputImage.recycle(); | |
| inputImage = null; | |
| } | |
| } |
There was a problem hiding this comment.
@avazirna is this not necessary ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| private void handleErrorDuringDetection(Exception e) { | ||
| Log.e(TAG, "Error during face detection: " + e); | ||
| Toast.makeText(this, "microimage.face.detection.mode.failed", Toast.LENGTH_LONG).show(); | ||
| // TODO: decide whether to switch to manual mode or close activity | ||
| } |
There was a problem hiding this comment.
Improve error handling in handleErrorDuringDetection.
The TODO comment indicates incomplete error handling. The method should make a clear decision about the failure mode.
Implement proper error handling:
private void handleErrorDuringDetection(Exception e) {
Log.e(TAG, "Error during face detection: " + e);
- Toast.makeText(this, "microimage.face.detection.mode.failed", Toast.LENGTH_LONG).show();
- // TODO: decide whether to switch to manual mode or close activity
+ if (e instanceof MlKitException) {
+ // ML Kit errors can be recovered by switching to manual mode
+ switchToManualMode();
+ } else {
+ // Other errors require activity closure
+ logErrorAndExit("Face detection failed", "microimage.face.detection.mode.failed", e);
+ }
}
+
+ private void switchToManualMode() {
+ // Implement manual mode logic here
+ Toast.makeText(this, "Switching to manual mode", Toast.LENGTH_LONG).show();
+ }Committable suggestion skipped: line range outside the PR's diff.
| @Override | ||
| public void analyze(@NonNull ImageProxy imageProxy) { | ||
| @SuppressLint("UnsafeOptInUsageError") Image mediaImage = imageProxy.getImage(); | ||
| if (mediaImage != null) { | ||
| InputImage image = InputImage.fromMediaImage(mediaImage, imageProxy.getImageInfo().getRotationDegrees()); | ||
|
|
||
| FaceDetectorOptions realTimeOpts = new FaceDetectorOptions.Builder() | ||
| .setContourMode(FaceDetectorOptions.CONTOUR_MODE_NONE) | ||
| .setPerformanceMode(FaceDetectorOptions.PERFORMANCE_MODE_ACCURATE) | ||
| .build(); | ||
| FaceDetector faceDetector = FaceDetection.getClient(realTimeOpts); | ||
| // process image with the face detector | ||
| faceDetector.process(image) | ||
| .addOnSuccessListener(faces -> processFaceDetectionResult(faces, image)) | ||
| .addOnFailureListener(e -> handleErrorDuringDetection(e)) | ||
| .addOnCompleteListener(task -> { | ||
| imageProxy.close(); | ||
| }); | ||
| } else { | ||
| imageProxy.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error recovery mechanism in analyze method.
The image analysis could fail silently if there's an exception during processing. Consider adding retry logic or graceful degradation.
Add error recovery:
@Override
public void analyze(@NonNull ImageProxy imageProxy) {
+ int retryCount = 0;
+ final int MAX_RETRIES = 3;
+
+ while (retryCount < MAX_RETRIES) {
@SuppressLint("UnsafeOptInUsageError") Image mediaImage = imageProxy.getImage();
if (mediaImage != null) {
try {
InputImage image = InputImage.fromMediaImage(mediaImage,
imageProxy.getImageInfo().getRotationDegrees());
processImage(image, imageProxy);
+ return;
+ } catch (Exception e) {
+ retryCount++;
+ if (retryCount == MAX_RETRIES) {
+ handleErrorDuringDetection(e);
+ }
+ }
}
+ }
imageProxy.close();
}Committable suggestion skipped: line range outside the PR's diff.
| private void startCamera() throws ExecutionException, InterruptedException { | ||
| ListenableFuture<ProcessCameraProvider> cameraProviderFuture = ProcessCameraProvider.getInstance(this); | ||
|
|
||
| cameraProviderFuture.addListener(() -> { | ||
| ProcessCameraProvider cameraProvider = null; | ||
| try { | ||
| cameraProvider = cameraProviderFuture.get(); | ||
| } catch (ExecutionException | InterruptedException e) { | ||
| logErrorAndExit("Error acquiring camera provider", "microimage.camera.start.failed", e); | ||
| } | ||
| bindUseCases(cameraProvider); | ||
| }, ContextCompat.getMainExecutor(this)); | ||
| } |
There was a problem hiding this comment.
Potential memory leak in camera listener.
The lambda passed to addListener captures this which could lead to a memory leak if the activity is destroyed before the future completes.
Use a weak reference or move the listener to a separate class:
- cameraProviderFuture.addListener(() -> {
+ cameraProviderFuture.addListener(new Runnable() {
+ @Override
+ public void run() {
+ if (isDestroyed()) {
+ return;
+ }
ProcessCameraProvider cameraProvider = null;
try {
cameraProvider = cameraProviderFuture.get();
} catch (ExecutionException | InterruptedException e) {
logErrorAndExit("Error acquiring camera provider", "microimage.camera.start.failed", e);
}
bindUseCases(cameraProvider);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| @@ -30,4 +30,5 @@ allprojects { | |||
| ext { | |||
| markwon_version = '4.6.2' | |||
| lifecycle_version = '2.5.1' | |||
| cameraX_version = '1.2.3' | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify CameraX version.
Let's check if version 1.2.3 is the latest stable version of CameraX.
🌐 Web query:
What is the latest stable version of CameraX?
💡 Result:
The latest stable version of CameraX is 1.4.0, released on October 30, 2024[1][5]. This version focuses on stability improvements and bug fixes from earlier release candidates (RCs), including critical issues like crashes during lifecycle binding and zoom control enhancements[1]. It also introduces foundational support for upcoming features like Jetpack Compose integration through the camera-compose library (currently in alpha as part of CameraX 1.5.0)[1][3].
Key details about CameraX 1.4.0:
- Stability: Resolved crashes related to lifecycle management (
bindToLifecyclewith destroyed owners)[1] - Feature Support: Added mirror mode for Preview (Android 13+), expanded HDR capabilities (HDR Preview and Ultra HDR), and improved effect framework integration[3]
- Compatibility: Part of AndroidX releases up to at least January 2025[5]
Developers can access this version via Google Maven repositories[1]. The next major version in development is CameraX 1.5.0-alpha05 (as of January 2025), which introduces experimental features like dual-camera support[9].
Citations:
- 1: https://groups.google.com/a/android.com/g/camerax-developers/c/JFZ951-t924
- 2: https://github.com/androidx-releases/CameraX/releases
- 3: https://android-developers.googleblog.com/2024/12/whats-new-in-camerax-140-and-jetpack-compose-support.html
- 4: https://pub.dev/packages/camerax/versions
- 5: https://developer.android.com/jetpack/androidx/versions/all-channel
- 6: https://mvnrepository.com/artifact/androidx.camera
- 7: https://pub.dev/packages/camera_android_camerax/versions/0.6.10+2/changelog
- 8: https://proandroiddev.com/lets-build-an-android-camera-app-camerax-compose-9ea47356aa80
- 9: https://www.developer-tech.com/news/android-camerax-library-dual-camera-functionality-boost/
Action: Update CameraX version in build.gradle
The current version is set to 1.2.3, but the latest stable release is now 1.4.0 (released on October 30, 2024). Please update the variable accordingly to ensure you benefit from the latest stability improvements and bug fixes.
- File: build.gradle
- Location: Line 33
- Suggested Change:
- cameraX_version = '1.2.3'
- cameraX_version = '1.4.0'
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
`````suggestion
cameraX_version = '1.4.0'
There was a problem hiding this comment.
@shubham1g5 Great! Thanks for confirming the CameraX version update to 1.4.0. This will ensure your project benefits from the latest stability improvements and bug fixes.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
avazirna
force-pushed
the
sc2975_create-micro-image-widget
branch
from
554cfcb to
03894d2
Compare
avazirna
force-pushed
the
sc2975_create-micro-image-widget
branch
from
03894d2 to
b74b2b2
Compare
| @@ -317,6 +321,10 @@ | |||
| android:name="org.commcare.activities.FormRecordListActivity" | |||
| android:windowSoftInputMode="adjustResize"> | |||
| </activity> | |||
| <activity | |||
| android:name="org.commcare.fragments.MicroImageActivity" | |||
| android:windowSoftInputMode="adjustResize"> | |||
| implementation "androidx.camera:camera-view:$cameraX_version" | ||
| implementation "androidx.camera:camera-core:$cameraX_version" | ||
| implementation "androidx.camera:camera-camera2:$cameraX_version" | ||
| implementation "androidx.camera:camera-lifecycle:$cameraX_version" | ||
| implementation 'com.google.android.gms:play-services-mlkit-face-detection:17.1.0' |
There was a problem hiding this comment.
what's the impact on apk size due to these additions ?
| @@ -0,0 +1,190 @@ | |||
| package org.commcare.fragments; | |||
There was a problem hiding this comment.
package seems incorrect given it's an activity
| import androidx.camera.view.PreviewView; | ||
| import androidx.core.content.ContextCompat; | ||
|
|
||
| public class MicroImageActivity extends AppCompatActivity implements ImageAnalysis.Analyzer, FaceCaptureView.ImageStabilizedListener { |
There was a problem hiding this comment.
can we add a class level javadoc here.
| } | ||
| } | ||
|
|
||
| private void startCamera() throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
we are both throwing and catching the exceptions in this method which seems un-necessary
| if (mBinaryName != null) { | ||
| deleteMedia(); | ||
| } | ||
|
|
||
| File f = new File(binaryPath.toString()); |
There was a problem hiding this comment.
can just call super.setBinaryData() instead ?
| if (originalImage == null) { | ||
| showToast("microimage.decoding.no.image"); | ||
| Logger.log(LogTypes.TYPE_EXCEPTION,"Error decoding image "); | ||
| return; | ||
| } |
There was a problem hiding this comment.
this almost certainly signifies a code error and should crash the application instead
| mBinary = null; | ||
| } | ||
|
|
||
| // TODO: Refactor |
There was a problem hiding this comment.
pending ?
Also can we explore re-using FileUtil.getBitmapScaledByMaxDimen or MediaUtil.scaleDownToTargetOrContainer instead ?
| Logger.exception("Error while scaling down and compressing image: ", e); | ||
| return; | ||
| } finally { | ||
| originalImage.recycle(); |
There was a problem hiding this comment.
should not we also delete the original media file here ?
| scaledDownBitmap = scaleImage(originalImage, IMAGE_DIMEN_SCALED_MAX_PX, IMAGE_DIMEN_SCALED_MAX_PX); | ||
| compressedBitmapByteArray = MediaUtil.compressBitmapToTargetSize(scaledDownBitmap, MICRO_IMAGE_MAX_SIZE_BYTES); | ||
| mBinary = Base64.encodeToString(compressedBitmapByteArray, Base64.DEFAULT); | ||
| } catch (Exception e) { |
Product Description
This PR is the first of a series of three, each responsible for a specific aspect of the Micro Image feature:
The main purpose of the Micro image widget is to capture a facial image, compress it to up to 2KB and store it as a normal case property in Base64 format. In order to achieve this, the widget relies on the Google ML Kit Face detection module to identify and analyze faces in the image stream displayed in the preview surface, triggering the next steps when the image is considered stable.
The workflow involves:
1.1. Face didn't move - the face is considered stable and the countdown is triggered. Once the countdown gets to zero, the face is considered stabilized and the capture process triggered.
1.2. Image moved - the image is not considered stable and the workflow goes back to the identification step and the counter reset.
The video below depicts the process:
micro_image_widget.mp4
Technical Summary
Tech spec: https://docs.google.com/document/d/14yDNK6RyQGB-8H6DioGgoj0c2mwJuI8nWck4zFMYFAs
Feature Flag
FF: https://www.commcarehq.org/hq/flags/edit/case_micro_image/
Safety Assurance
Safety story
This feature was successfully tested locally and has undergone a couple of rounds of UAT.
Automated test coverage
QA Plan
The QA Test plan will be to be updated to cover this feature.
Labels and Review
cross-request: dimagi/commcare-core#1459