Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Micro Image widget - 1/3 #2954

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@
android:name="com.google.firebase.messaging.default_notification_channel_id"
android:value="@string/fcm_default_notification_channel" />

<meta-data
android:name="com.google.mlkit.vision.DEPENDENCIES"
android:value="face" />

<activity
android:label="@string/application_name"
android:exported="true"
Expand Down Expand Up @@ -317,6 +321,10 @@
android:name="org.commcare.activities.FormRecordListActivity"
android:windowSoftInputMode="adjustResize">
</activity>
<activity
android:name="org.commcare.fragments.MicroImageActivity"
android:windowSoftInputMode="adjustResize">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed ?

</activity>
<service
android:enabled="true"
android:foregroundServiceType="specialUse"
Expand Down
6 changes: 6 additions & 0 deletions app/assets/locales/android_translatable_strings.txt
Original file line number Diff line number Diff line change
Expand Up @@ -992,3 +992,9 @@ background.sync.fail=Background sync failed. Please try to trigger a normal Sync
android.package.name.org.commcare.dalvik.reminders=CommCare Reminders
android.package.name.callout.commcare.org.sendussd=Commcare USSD
android.package.name.org.commcare.dalvik.abha=CommCare ABHA

microimage.camera.start.failed=Camera failed to start
microimage.face.detection.mode.failed=Face detection mode failed
microimage.cropping.failed=Image cropping failed, make sure the target is inside the capture area
microimage.decoding.no.image=Image processing failed. Please try again
microimage.scalingdown.compression.error=Image compression failed. Please try again
5 changes: 5 additions & 0 deletions app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ dependencies {

// this syntax doesn't work for compiling .aar files, so those have to be loaded manually
implementation fileTree(include: '*.jar', exclude: 'regexp-me.jar', dir: 'libs')
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'
Comment on lines +69 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the impact on apk size due to these additions ?

implementation(name: 'htmlspanner-custom', ext: 'aar')
implementation 'com.github.dimagi:zebra-print-android:v1.3'
implementation (name: 'LibSimprints-1.0.12', ext: 'aar')
Expand Down
30 changes: 30 additions & 0 deletions app/res/layout/micro_image_widget.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?xml version="1.0" encoding="utf-8"?>
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent"
tools:viewBindingIgnore="true">

<androidx.camera.view.PreviewView
android:id="@+id/view_finder"
android:layout_width="match_parent"
android:layout_height="match_parent" />
Comment on lines +9 to +12
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
<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" />


<LinearLayout
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical"
android:layout_gravity="center">

<org.commcare.views.FaceCaptureView
android:id="@+id/face_overlay"
android:layout_width="match_parent"
android:layout_height="0dp"
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"/>
</LinearLayout>
</FrameLayout>
7 changes: 6 additions & 1 deletion app/res/values/attrs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,10 @@
<declare-styleable name="FilePreference">
<attr name="file_type" format="string"/>
</declare-styleable>

<declare-styleable name="FaceCaptureView">
<attr name="background_color" format="color" />
<attr name="face_capture_area_delimiter_color" format="color" />
<attr name="face_marker_color" format="color" />
<attr name="countdown_text_size" format="dimension" />
</declare-styleable>
</resources>
1 change: 1 addition & 0 deletions app/res/values/colors.xml
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
<!-- Neutral -->
<color name="cc_neutral_color">#685C53</color>
<color name="cc_neutral_bg">#D6D6D4</color>
<color name="cc_neutral_bg_tr">#80D6D6D4</color>
<color name="cc_neutral_text">#373534</color>

<!-- Attention Positive -->
Expand Down
1 change: 1 addition & 0 deletions app/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -461,4 +461,5 @@
<string name="fcm_default_notification_channel">notification-channel-push-notifications</string>
<string name="app_with_id_not_found">Required CommCare App is not installed on device</string>
<string name="audio_recording_notification">Audio Recording Notification</string>
<string name="micro_image_activity_title" cc:translatable="true">Capture Face Picture</string>
</resources>
5 changes: 2 additions & 3 deletions app/src/org/commcare/activities/FormEntryActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,8 @@ public void onActivityResultSessionSafe(int requestCode, int resultCode, Intent
Localization.get("intent.callout.unable.to.process"), Toast.LENGTH_SHORT).show();
}
break;
case FormEntryConstants.IMAGE_CAPTURE:
ImageCaptureProcessing.processCaptureResponse(this,
FormEntryInstanceState.getInstanceFolder(), true);
case FormEntryConstants.IMAGE_CAPTURE, FormEntryConstants.MICRO_IMAGE_CAPTURE:
ImageCaptureProcessing.processCaptureResponse(this, FormEntryInstanceState.getInstanceFolder(), true);
break;
case FormEntryConstants.SIGNATURE_CAPTURE:
Logger.log(LogTypes.SOFT_ASSERT, "Signature captured successfully");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public class FormEntryConstants {
public static final int INTENT_LOCATION_PERMISSION = 14;
public static final int INTENT_LOCATION_EXCEPTION = 15;
public static final int VIEW_VIDEO_FULLSCREEN = 16;

public static final int MICRO_IMAGE_CAPTURE = 17;
public static final String NAV_STATE_NEXT = "next";
public static final String NAV_STATE_DONE = "done";
public static final String NAV_STATE_QUIT = "quit";
Expand Down
190 changes: 190 additions & 0 deletions app/src/org/commcare/fragments/MicroImageActivity.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
package org.commcare.fragments;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package seems incorrect given it's an activity


import android.annotation.SuppressLint;
import android.graphics.Bitmap;
import android.graphics.Rect;
import android.media.Image;
import android.os.Bundle;
import android.util.Size;
import android.widget.Toast;

import com.google.common.util.concurrent.ListenableFuture;
import com.google.mlkit.common.MlKitException;
import com.google.mlkit.vision.common.InputImage;
import com.google.mlkit.vision.common.internal.ImageConvertUtils;
import com.google.mlkit.vision.face.Face;
import com.google.mlkit.vision.face.FaceDetection;
import com.google.mlkit.vision.face.FaceDetector;
import com.google.mlkit.vision.face.FaceDetectorOptions;

import org.commcare.dalvik.R;
import org.commcare.util.LogTypes;
import org.commcare.utils.MediaUtil;
import org.commcare.views.FaceCaptureView;
import org.commcare.views.widgets.ImageWidget;
import org.javarosa.core.services.Logger;
import org.javarosa.core.services.locale.Localization;

import java.util.List;
import java.util.concurrent.ExecutionException;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.appcompat.app.ActionBar;
import androidx.appcompat.app.AppCompatActivity;
import androidx.camera.core.CameraSelector;
import androidx.camera.core.ImageAnalysis;
import androidx.camera.core.ImageProxy;
import androidx.camera.core.Preview;
import androidx.camera.core.UseCase;
import androidx.camera.lifecycle.ProcessCameraProvider;
import androidx.camera.view.PreviewView;
import androidx.core.content.ContextCompat;

public class MicroImageActivity extends AppCompatActivity implements ImageAnalysis.Analyzer, FaceCaptureView.ImageStabilizedListener {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a class level javadoc here.

private static final String TAG = MicroImageActivity.class.toString();
private PreviewView cameraView;
private FaceCaptureView faceCaptureView;
private Bitmap inputImage;

@Override
protected void onCreate(@Nullable Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.micro_image_widget);

faceCaptureView = findViewById(R.id.face_overlay);
cameraView = findViewById(R.id.view_finder);

ActionBar actionBar = getSupportActionBar();
if (actionBar != null) {
actionBar.setTitle(R.string.micro_image_activity_title);
}

faceCaptureView.setImageStabilizedListener(this);

try {
startCamera();
} catch (ExecutionException | InterruptedException e) {
logErrorAndExit("Error starting camera", "microimage.camera.start.failed", e);
}
}

private void startCamera() throws ExecutionException, InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are both throwing and catching the exceptions in this method which seems un-necessary

ListenableFuture<ProcessCameraProvider> cameraProviderFuture = ProcessCameraProvider.getInstance(this);

cameraProviderFuture.addListener(() -> {
ProcessCameraProvider cameraProvider;
try {
cameraProvider = cameraProviderFuture.get();
} catch (ExecutionException | InterruptedException e) {
logErrorAndExit("Error acquiring camera provider", "microimage.camera.start.failed", e);
return;
Comment on lines +79 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a practical scenario where we anticipate these exceptions to happen ? If not, we should just let the application hard crash.

}
bindUseCases(cameraProvider);
}, ContextCompat.getMainExecutor(this));
}

private void bindUseCases(ProcessCameraProvider cameraProvider) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you mean buildUseCases ?

int targetRotation = getWindowManager().getDefaultDisplay().getRotation();
Size targetResolution = new Size(faceCaptureView.getImageWidth(), faceCaptureView.getImageHeight());

// Preview use case
Preview preview = new Preview.Builder()
.setTargetResolution(targetResolution)
.setTargetRotation(targetRotation)
.build();
preview.setSurfaceProvider(cameraView.getSurfaceProvider());

UseCase imageAnalyzer = buildImageAnalysisUseCase(targetResolution, targetRotation);

CameraSelector cameraSelector = CameraSelector.DEFAULT_BACK_CAMERA;

// Unbind any previous use cases before binding new ones
cameraProvider.unbindAll();

// Bind the use cases to the camera
cameraProvider.bindToLifecycle(this, cameraSelector, preview, imageAnalyzer);
}

private UseCase buildImageAnalysisUseCase(Size targetResolution, int targetRotation) {
ImageAnalysis imageAnalyzer = new ImageAnalysis.Builder()
.setBackpressureStrategy(ImageAnalysis.STRATEGY_KEEP_ONLY_LATEST)
.setTargetResolution(targetResolution)
.setTargetRotation(targetRotation)
.build();

imageAnalyzer.setAnalyzer(ContextCompat.getMainExecutor(getApplicationContext()), this);
return imageAnalyzer;
}

private void logErrorAndExit(String logMessage, String userMessageKey, Throwable e) {
if (e == null) {
Logger.log(LogTypes.TYPE_EXCEPTION, logMessage);
} else {
Logger.exception(logMessage, e);
}
Toast.makeText(this, Localization.get(userMessageKey), Toast.LENGTH_LONG).show();
setResult(AppCompatActivity.RESULT_CANCELED);
finish();
}

@Override
public void analyze(@NonNull ImageProxy imageProxy) {
@SuppressLint("UnsafeOptInUsageError") Image mediaImage = imageProxy.getImage();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not suppress lint here but add @OptIn(markerClass = ExperimentalGetImage.class) to the method to denote we are using an experimental API here.

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();
}
}
Comment on lines +131 to +152
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 handleErrorDuringDetection(Exception e) {
Logger.exception("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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending todo ?

}

private void processFaceDetectionResult(List<Face> faces, InputImage image) {
if (faces.size() > 0) {
// Only one face is processed, this can be increased if needed
Face newFace = faces.get(0);

// this will draw a bounding circle around the first detected face
faceCaptureView.updateFace(newFace);
try {
inputImage = ImageConvertUtils.getInstance().convertToUpRightBitmap(image);
} catch (MlKitException e) {
Logger.exception("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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending ?

}
} else {
faceCaptureView.updateFace(null);
}
}

@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());
}
Comment on lines +185 to +187
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we must not catch generic exceptions as it will potentially hide coding bugs under the rug.


}
Comment on lines +179 to +189
Copy link

@coderabbitai coderabbitai bot Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
@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;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avazirna is this not necessary ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}
Loading
Loading