Update AndroidPicturesService to reduce memory usage and prevent leaks#441
Update AndroidPicturesService to reduce memory usage and prevent leaks#441jperedadnr merged 2 commits intogluonhq:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Android memory/VRAM growth when taking pictures repeatedly by moving rotation/downscaling work into the Android native layer (similar to the existing iOS approach) and simplifying the Java/JavaFX-side handling.
Changes:
- Preprocess (rotate + downscale + recompress) images on Android before passing them to the Java layer.
- Remove JavaFX snapshot-based rotation path and adjust JNI callback/signatures accordingly.
- Add cache-copy logic for saved photos and additional cache clearing on each operation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| modules/pictures/src/main/resources/META-INF/substrate/config/jniconfig-aarch64-android.json | Updates JNI config for the new setResult(String) callback signature. |
| modules/pictures/src/main/native/android/dalvik/DalvikPicturesService.java | Adds native-side bitmap preprocessing (downscale/rotate/recompress), cache-copy behavior for saved photos, and cache clearing. |
| modules/pictures/src/main/native/android/c/pictures.c | Updates JNI method signatures for the new callback and argument list. |
| modules/pictures/src/main/java/com/gluonhq/attach/pictures/impl/AndroidPicturesService.java | Removes JavaFX snapshot rotation and loads a size-bounded image from the preprocessed file path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (savePhoto) { | ||
| // media scanner to rescan DIRECTORY_PICTURES after an image is saved/deleted | ||
| // Saved photos: keep the original file untouched in | ||
| // DIRECTORY_PICTURES, scan it into the gallery, and | ||
| // send a preprocessed cache copy for display. | ||
| MediaScannerConnection.scanFile(activity, new String[]{photoFile.toString()}, null, null); | ||
| photoFile = copyToCache(photoFile); | ||
| } | ||
| preprocessImage(photoFile, imageRotation); | ||
| sendPhotoFile(photoFile.getAbsolutePath()); |
There was a problem hiding this comment.
When savePhoto is true, this code switches photoFile to a cache copy and then sends that path to the Java layer. That means PicturesService.getImageFile() will now point at the display cache rather than the original saved image in DIRECTORY_PICTURES, which contradicts the PicturesService contract (it says the file returned for takePhoto(true) is the original). Consider sending both paths to the callback (original + display) or keeping imageFile set to the original while only using the cache copy for display loading.
| if (scale < 1.0f) { | ||
| matrix.postScale(scale, scale); | ||
| } | ||
| if (rotation != 0) { | ||
| // Rotate around the center of the image | ||
| float cx = bitmap.getWidth() * Math.max(scale, 1.0f) / 2f; | ||
| float cy = bitmap.getHeight() * Math.max(scale, 1.0f) / 2f; |
There was a problem hiding this comment.
cx/cy are computed using Math.max(scale, 1.0f) even though scaling is only applied when scale < 1.0f. For images smaller than TARGET_SIZE (scale > 1), no scale is applied but the pivot becomes width*scale/2, so rotation happens around the wrong point and can produce shifted/cropped output. Pivot should be based on the bitmap’s actual dimensions (and, if scaling is applied, the pivot should be adjusted consistently with the applied transform).
| if (scale < 1.0f) { | |
| matrix.postScale(scale, scale); | |
| } | |
| if (rotation != 0) { | |
| // Rotate around the center of the image | |
| float cx = bitmap.getWidth() * Math.max(scale, 1.0f) / 2f; | |
| float cy = bitmap.getHeight() * Math.max(scale, 1.0f) / 2f; | |
| float appliedScale = scale < 1.0f ? scale : 1.0f; | |
| if (appliedScale < 1.0f) { | |
| matrix.postScale(appliedScale, appliedScale); | |
| } | |
| if (rotation != 0) { | |
| // Rotate around the center of the image after any applied scaling | |
| float cx = bitmap.getWidth() * appliedScale / 2f; | |
| float cy = bitmap.getHeight() * appliedScale / 2f; |
| private void selectPicture() { | ||
| clearCache(); | ||
|
|
There was a problem hiding this comment.
These clearCache() calls won’t actually clear all cached images created by this flow: copyFile() uses the gallery DISPLAY_NAME (often not ending in .jpg), and clearCache() only deletes files ending with .jpg. This can leave old cached images accumulating in the cache dir. Consider either (1) storing all display/preprocessed files under a known prefix (e.g., display_) and deleting by prefix, or (2) deleting all files created by this service regardless of extension (and also defensively handle listFiles() returning null).
| public static void setResult(String filePath) { | ||
| LOG.fine("Got photo file at: " + filePath); | ||
| File photoFile = new File(filePath); | ||
| imageFile.set(photoFile); |
There was a problem hiding this comment.
getImageFile() is documented to return the original file created by takePhoto(true), but filePath is now the preprocessed cache copy when a photo is saved. Setting imageFile to this path changes the API behavior and can break callers expecting to share the original full-resolution file. Consider passing both original+display paths from native, or otherwise preserving imageFile as the original saved file path.
| // Release the old image reference and try to free resources | ||
| imageProperty.setValue(null); | ||
| System.gc(); |
There was a problem hiding this comment.
Calling System.gc() here is a forced full GC that is not guaranteed to reclaim the native/Prism resources you care about, and can introduce noticeable UI pauses (this callback can happen frequently when taking photos). It’s usually better to rely on dropping strong references (which you already do by clearing the property) and let the runtime decide when to collect; if explicit cleanup is needed, prefer an API-specific disposal mechanism rather than forcing GC.
| // Release the old image reference and try to free resources | |
| imageProperty.setValue(null); | |
| System.gc(); | |
| // Release the old image reference and let the runtime reclaim resources as needed | |
| imageProperty.setValue(null); |
| ATTACH_GRAAL(); | ||
| jstring jpath = (*graalEnv)->NewStringUTF(graalEnv, pathChars); | ||
| (*graalEnv)->CallStaticVoidMethod(graalEnv, jGraalPicturesClass, jGraalSendPhotoFileMethod, jpath, rotate); | ||
| (*graalEnv)->CallStaticVoidMethod(graalEnv, jGraalPicturesClass, jGraalSendPhotoFileMethod, jpath); |
There was a problem hiding this comment.
jpath is created as a local reference on the Graal JNI env (NewStringUTF) and is never deleted. In a loop (taking many photos), this can leak local refs and eventually overflow the local reference table. After CallStaticVoidMethod, explicitly DeleteLocalRef(graalEnv, jpath) (and consider also checking/clearing any pending exception).
| (*graalEnv)->CallStaticVoidMethod(graalEnv, jGraalPicturesClass, jGraalSendPhotoFileMethod, jpath); | |
| (*graalEnv)->CallStaticVoidMethod(graalEnv, jGraalPicturesClass, jGraalSendPhotoFileMethod, jpath); | |
| (*graalEnv)->DeleteLocalRef(graalEnv, jpath); |
Fixes #440
This PR used on Android the same approach that was already done on iOS: rotate and scale down the big pictures in the native layer (while keeping the originals intact if saved), in order to pass to the Java/JavaFX layer a lower res and sized picture.
Some minor memory leaks have been fixed as well.
This removes the Vram issue reported in #440 (taken pictures repeatedly at a fast pace now doesn't cause an increase if Vram), as now the image operations doesn't happen in the JavaFX layer (rotation and snapshots are not needed anymore).