-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
WPMediaUtils - Set normal orientation for the exif data #21024
Conversation
Quality Gate passedIssues Measures |
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #21024 +/- ##
==========================================
- Coverage 40.98% 40.98% -0.01%
==========================================
Files 1522 1522
Lines 69625 69626 +1
Branches 11490 11490
==========================================
Hits 28539 28539
- Misses 38497 38498 +1
Partials 2589 2589 ☔ View full report in Codecov by Sentry. |
@@ -84,6 +85,9 @@ public static Uri getOptimizedMedia(Context context, String path, boolean isVide | |||
AppLog.e(AppLog.T.EDITOR, "Optimized picture was null!"); | |||
AnalyticsTracker.track(AnalyticsTracker.Stat.MEDIA_PHOTO_OPTIMIZE_ERROR); | |||
} else { | |||
// Set the default orientation tag for the EXIF data | |||
exifData.put("Orientation", String.valueOf(ExifInterface.ORIENTATION_NORMAL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image I was testing with didn’t have an orientation value set in its EXIF data, but it was being assigned the Orientation=6
value, which corresponds to ORIENTATION_ROTATE_90.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any documentation about what ORIENTATION_NORMAL
does - but I assume it says to keep the original orientation 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested both portrait and landscape orientations, both from Choose from device
& Insert from URL
options and they all worked as expected.
I didn't confirm the issue on trunk
since there is a GIF showcasing it in the PR description - @geriux thanks for that btw!
@@ -84,6 +85,9 @@ public static Uri getOptimizedMedia(Context context, String path, boolean isVide | |||
AppLog.e(AppLog.T.EDITOR, "Optimized picture was null!"); | |||
AnalyticsTracker.track(AnalyticsTracker.Stat.MEDIA_PHOTO_OPTIMIZE_ERROR); | |||
} else { | |||
// Set the default orientation tag for the EXIF data | |||
exifData.put("Orientation", String.valueOf(ExifInterface.ORIENTATION_NORMAL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any documentation about what ORIENTATION_NORMAL
does - but I assume it says to keep the original orientation 🤷♂️
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Fixes a regression introduced after #20955
There is an issue when uploading portrait images where they get rotated 90 degrees, this does not affect landscape orientations.
I've tested this PR with different images:
landscape
,portrait
, and an edited portrait photo into landscape.To Test:
Note: It'd be good to test landscape orientations as well.
20240701_145941.mp4
20240701_173021.mp4
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):