Passthrough EXIF properties while scaling image #3014
Conversation
|
Can one of the admins verify this patch? |
📝 WalkthroughWalkthroughThis change introduces two new private methods within the Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant FileUtil as FileUtil
participant SrcExif as Source ExifInterface
participant DestExif as Dest ExifInterface
Caller->>FileUtil: call scaleAndSaveImage(imageFile, Bitmap)
FileUtil->>SrcExif: read original EXIF data from image file
FileUtil->>FileUtil: scale image and save scaled file
FileUtil->>DestExif: create ExifInterface for scaled file
FileUtil->>FileUtil: call copyExifData(SrcExif, DestExif, Bitmap)
FileUtil->>DestExif: update with selected EXIF attributes
FileUtil->>FileUtil: call logExifAttributesToCheckRetention(SrcExif, DestExif)
FileUtil->>Caller: return success or failure status
Possibly related issues
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
🧹 Nitpick comments (3)
app/src/org/commcare/utils/FileUtil.java (3)
625-642: Consider refactoring duplicated EXIF tag list.The EXIF tags list is duplicated in both methods. Consider defining this list once as a private static final field to improve maintainability.
+ private static final String[] EXIF_TAGS = { + ExifInterface.TAG_DATETIME, + ExifInterface.TAG_MAKE, + ExifInterface.TAG_MODEL, + ExifInterface.TAG_ORIENTATION, + ExifInterface.TAG_GPS_LATITUDE, + ExifInterface.TAG_GPS_LONGITUDE, + ExifInterface.TAG_GPS_LATITUDE_REF, + ExifInterface.TAG_GPS_LONGITUDE_REF, + ExifInterface.TAG_FLASH, + ExifInterface.TAG_EXPOSURE_TIME, + }; private static void copyExifData(ExifInterface sourceExif, ExifInterface destExif, Bitmap scaledBitmap) { - String[] exifTags = { - ExifInterface.TAG_DATETIME, - ExifInterface.TAG_MAKE, - ExifInterface.TAG_MODEL, - ExifInterface.TAG_ORIENTATION, - ExifInterface.TAG_GPS_LATITUDE, - ExifInterface.TAG_GPS_LONGITUDE, - ExifInterface.TAG_GPS_LATITUDE_REF, - ExifInterface.TAG_GPS_LONGITUDE_REF, - ExifInterface.TAG_FLASH, - ExifInterface.TAG_EXPOSURE_TIME, - }; - - for (String tag : exifTags) { + for (String tag : EXIF_TAGS) {And similar change in the logExifAttributesToCheckRetention method.
640-640: Consider using class-specific log tag.The generic "ImageTest" log tag could make it difficult to filter logs. Consider using LOG_TOKEN which is already defined for this class.
- Log.i("ImageTest", tag + " : " + src.getAttribute(tag) + " -> " + dest.getAttribute(tag)); + Log.i(LOG_TOKEN, tag + " : " + src.getAttribute(tag) + " -> " + dest.getAttribute(tag));
673-682: Consider conditional logging for production code.Line 681 has a comment suggesting this is logging that might need to be removed. Instead of requiring code modification, consider using conditional logging based on a debug flag.
- logExifAttributesToCheckRetention(originalExif, newExif); // comment this line if logging is not required + if (BuildConfig.DEBUG) { + logExifAttributesToCheckRetention(originalExif, newExif); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/utils/FileUtil.java(3 hunks)
🔇 Additional comments (4)
app/src/org/commcare/utils/FileUtil.java (4)
10-10: Import addition looks good.Adding the androidx ExifInterface import is appropriate for working with EXIF metadata in images.
599-623: Well-implemented EXIF data copying.The method effectively copies important EXIF metadata from source to destination while also updating image dimensions appropriately. The implementation handles a comprehensive set of EXIF tags including datetime, location, and camera metadata.
658-664: Good error handling for EXIF reading.The code correctly handles potential exceptions when reading EXIF data, ensuring the method gracefully returns false if there are issues accessing the metadata.
673-682: Functionality looks good.The implementation successfully preserves EXIF metadata during image scaling by reading the original data, saving the scaled image, and then applying the metadata to the new file.
…e appropriate log token
|
@damagatchi ok to test |
|
seeing an "Access Denied" error while trying to open the jenkins job, already logged in but don't have permission to view it. Could you please help or grant access if needed? |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Hello, thanks for submitting a PR.
Unfortunately our Jenkins Jobs are restricted, but this is the error we are getting for you reference -
FileUtil.java:10: error: package androidx.exifinterface.media does not exist
Also in terms of testing this, can you make sure all these following cases are working correctly -
- Basic EXIF Check
Snap a pic with GPS turned on.
Save the form.
Check that all EXIF data is intact using your favorite EXIF viewer.
- Scaled Image Test
Take a high-res image.
Verify the EXIF data after it gets scaled down.
- Encrypted Image Test
Capture a GPS-tagged image.
Confirm EXIF data survived the encryption.
- Edge Cases
Use images with no EXIF data.
Test images from various sources (camera, gallery, etc.).
Try different formats (JPG, PNG).
| try { | ||
| originalExif = new ExifInterface(originalImage.getAbsolutePath()); | ||
| } catch (IOException e) { | ||
| return false; |
There was a problem hiding this comment.
Think we don't want to retreat our normal functionality due to errors with exif manipulation. As such we should not return from here but just log the exceptions using Logger.exception
|
2c17817 resolves the package not found issue, updates the EXIF_TAGS list as asked, also retains the default functionality of keep going if the exif properties were not read. Here is what we are capturing now with GPS on: GPS off: |
|
thanks @hashamyounis9 , can you confirm the test cases I outlined above as well ? |
|
also can you please update me with error in jenkins job? |
Sure, I'll do in a bit... |
|
confirming the mentioned cases, it works well in the situations mentioned above, here are some catches
|
|
Got it, that makes sense to me. However we must make sure that the exif data survives the image encryption. Have you been able to check on that ? |
|
can you please help me understanding when does the encryption trigger, here is what I did: I started filling the form and captured the image using camera and completed the form and form got sent to the server. During all this encryption didn't occur as I did not see the debug logs that I put in the encryption method. |
|
For the sake of testing if the EXIF metadata was preserved or not in the images that are sent to the server, I exported the form media from the CommCare HQ for my app sent by mobile worker and viewed the EXIF metadata using some online tool making sure that all the EXIF metadata was present in the images that were sent to the server! |
|
Ahh Sorry, that might have been incorrect path, Curious what does this check returns for you ? And if |
So I've checked it and the check you mentioned returns true for me and the underlying encryption method gets called, which essentially means that encryption is getting applied and the EXIF metadata survives the encryption, as the images exported from the CommCare HQ contains the retained EXIF metadata! |
|
@hashamyounis9 that's great, Is it possible for you to provide a commcare hq link to one such form with image here. Thanks! @damagatchi retest this please |
@shubham1g5 you mean qr code for the app? I'll need to share the qr code and create a user(mobile worker) for you and then share the credentials for that user, the form will be available in the app |
no I meant the link to the form itself where you uploaded the image. |
oh, can you guide me how to share this? |
|
@damagatchi retest this please |
|
@hashamyounis9 Can you provide the link to the final image you are seeing on HQ. You can just take the link from the browser address bar where you see the image and paste it here or upload the image here itself or on Google drive, somewhere I can take the image from and verify the exif attributes on my end. |
Here is the zip exported from CommCare HQ, with multimedia only I used this tool to view exif metadata |
|
@hashamyounis9 thanks! let me verify it. Also we are facing an unrealted error with our CI job and I am looking into it. |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
one minor omition request, but all things look good otherwise.
|
b9dc391 removes the logs |
|
@hashamyounis9 we just merged an unrelated fix for the test failure that's causing the job to fail here, would you be able to rebase your branch from our |
|
rebase done with master, please let me know if it resolves the issue. |
|
Thanks for your guidance and support along the way! ❤️ |
3 participants
/claim #2689
Product Description
This PR ensures that EXIF metadata (like orientation, timestamp, GPS, etc.) is preserved when an image is scaled down in the CommCare Android app. This improves user experience in workflows that rely on image metadata.
Technical Summary
Passthrough EXIF metadata while scaling images
Modified the image scaling logic to preserve EXIF metadata by copying it from the original image to the scaled-down version as can be seen in the following screenshot
Values on the left side of
->are coming from original image and those on the right side are coming from the scaled image.Feature Flag
Not behind a feature flag.
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review