Respect image orientation while previewing the image in mobile app #3022
Conversation
|
Can one of the admins verify this patch? |
📝 WalkthroughWalkthroughThe changes update the image scaling functionality in Sequence Diagram(s)sequenceDiagram
participant Caller
participant MediaUtil
participant ExifInterface
participant Bitmap
Caller->>MediaUtil: scaleDownImage(imagePath)
MediaUtil->>Bitmap: loadBitmap(imagePath)
alt Bitmap is null
MediaUtil->>Caller: return null
else
MediaUtil->>ExifInterface: read EXIF orientation(imagePath)
ExifInterface-->>MediaUtil: return orientation
MediaUtil->>MediaUtil: rotateBitmap(bitmap, orientation)
alt Bitmap is rotated
MediaUtil->>Bitmap: recycle original bitmap
end
MediaUtil->>Caller: return rotated bitmap
end
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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 (2)
app/src/org/commcare/utils/MediaUtil.java (2)
379-387: Appropriate EXIF metadata handling with error logging.The code correctly retrieves the orientation information with a proper default value and includes error handling for cases where EXIF data cannot be read.
Consider using the class constant
TAGinstead of hardcoded "ImageScaling" for consistent logging:- Log.e("ImageScaling", "Unable to read EXIF data: " + e.getMessage()); + Log.e(TAG, "Unable to read EXIF data: " + e.getMessage());
410-446: Well-implemented bitmap rotation helper method.The
rotateBitmapmethod handles all standard EXIF orientation values correctly with appropriate transformations for each case. The implementation is efficient with early returns for null bitmaps and normal orientation.Consider adding OutOfMemoryError handling similar to other bitmap operations in this class:
private static Bitmap rotateBitmap(Bitmap bitmap, int orientation) { if (bitmap == null || orientation == ExifInterface.ORIENTATION_NORMAL) { return bitmap; } Matrix matrix = new Matrix(); switch (orientation) { case ExifInterface.ORIENTATION_ROTATE_90: matrix.postRotate(90); break; // cases omitted for brevity default: return bitmap; } + try { return Bitmap.createBitmap(bitmap, 0, 0, bitmap.getWidth(), bitmap.getHeight(), matrix, true); + } catch (OutOfMemoryError e) { + Log.d(TAG, "Ran out of memory attempting to rotate bitmap"); + return bitmap; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/utils/MediaUtil.java(2 hunks)
🔇 Additional comments (4)
app/src/org/commcare/utils/MediaUtil.java (4)
7-9: Good addition of necessary imports.The addition of Matrix and ExifInterface are required for the new image rotation functionality being introduced.
375-377: Good defensive programming with null check.This null check prevents potential NullPointerExceptions when bitmap loading fails, which improves error handling.
388-392: Good memory management when rotating bitmaps.The code properly recycles the original bitmap if a new bitmap was created during rotation, which prevents memory leaks.
394-407: Correctly returning rotated bitmap.The return statements have been appropriately updated to use the rotated bitmap instead of the original bitmap.
|
@damagatchi ok to test |
|
@hashamyounis9 Would you be able to complete our CLA agreement if you have not already. |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Looks great to me, just made one minor request reg. exception handling.
| ExifInterface exif = new ExifInterface(imageFilepath); | ||
| orientation = exif.getAttributeInt(ExifInterface.TAG_ORIENTATION, ExifInterface.ORIENTATION_NORMAL); | ||
| } catch (IOException e) { | ||
| Log.e(TAG, "Unable to read EXIF data: " + e.getMessage()); |
There was a problem hiding this comment.
can we log this using Logger.exception instead.
| try { | ||
| return Bitmap.createBitmap(bitmap, 0, 0, bitmap.getWidth(), bitmap.getHeight(), matrix, true); | ||
| } catch (OutOfMemoryError e) { | ||
| Log.d(TAG, "Ran out of memory attempting to rotate bitmap"); |
There was a problem hiding this comment.
would be great if we can use Logger.exception here and in the other OutOfMemoryError catch abvove in the calling method.
|
@hashamyounis9 Seems like we have failing unit tests - |
do I have to sign the CLA separately for each contribution or just once? coz I did sign the CLA during my last |
|
@hashamyounis9 Just once is fine, so if you have done it already, no need to do it again. |
|
Is the build fail caused by my commit? can you provide me with the error we are getting? |
|
are we still having the same unit tests to fail? |
|
@hashamyounis9 yes, are you not able to replicate failure locally ? |
nope, always works for me |
|
I am not sure why they are passing for you and failing on Jenkins, but here is a more detailed error from Jenkins if it helps - |
so I guess it does help as I was able to find out that image that we use for testing which lives inside directory |
|
@hashamyounis9 Why would not the test fail for you if that was the error, also even after the change, the error in test failure remains to be present on Jenkins. |
|
So as we discussed here that PNGs do not contain Let me know if any other changes required... |
| Bitmap b = inflateImageSafe(imageFilepath, approximateScaleDownFactor).first; | ||
|
|
||
| int orientation = ExifInterface.ORIENTATION_NORMAL; | ||
| if (!imageFilepath.endsWith("png")) { |
There was a problem hiding this comment.
I don't think we can practically maintain a list of what image formats have Exif data vs not and think our strategy here should be to try and get exif data safely for all file formats, but if that's not available simply don't do any exif manipulations and have the core functionality to display the image work without any crashes.
|
5562446 tries to read Let me know if any changes required |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
some minor comments, but looks good as a whole.
| } catch (IOException e) { | ||
| Logger.exception("Unable to read image file from disk: ", e); | ||
| } catch (Exception e) { | ||
| Logger.exception("Unable to read exif data from image file: ", e); | ||
| } |
There was a problem hiding this comment.
You can remove the catch (IOException e) { from here and just have a generic catch as we are not handling those 2 very differently.
| return Bitmap.createScaledBitmap(rotatedBitmap, newWidth, newHeight, false); | ||
| } catch (OutOfMemoryError e) { | ||
| Log.d(TAG, "Ran out of memory attempting to scale image at: " + imageFilepath); | ||
| Logger.exception("Ran out of memory attempting to scale image at: ", e); |
There was a problem hiding this comment.
missing imageFilepath from message.
There was a problem hiding this comment.
looks like we don't need it anymore.
|
made the changes as asked! |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Thanks. looks good, waiting for build to complete to merge.
3 participants
/claim #2743
Product Description
In commcare-android, the image displaying mechanism for captured images was not respecting the orientation of the image.
Technical Summary
MediaUtil.scaleDownToTargetOrContainerwas just displaying theBitmapof the image while not taking care of itsorientationthat we preserved previously in the the PR Passthrough EXIF properties while scaling image #3014Introduced the method
rotateBitmapwhich rotates theBitmapof the image according to theExifproperty of image regardingorientationbefore the Bitmap gets displayed in theImageWidget.This ensures that resized images display with the correct orientation, consistent with how they were captured by the device.
Note
I checked that all the
Exifproperties we retained in #3014 are getting retained correctly and the underlyingencryptionalso gets applied correctly, I also tested the images uploaded to server for further confirmation.Feature Flag
Not behind any feature flag.
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review