[expo-media-library] [Android] Flip dimensions based on media rotation data #7980
Conversation
| @@ -56,8 +56,8 @@ | |||
| put(SORT_BY_CREATION_TIME, MediaStore.Images.Media.DATE_TAKEN); | |||
| put(SORT_BY_MODIFICATION_TIME, MediaStore.Images.Media.DATE_MODIFIED); | |||
| put(SORT_BY_MEDIA_TYPE, MediaStore.Files.FileColumns.MEDIA_TYPE); | |||
| put(SORT_BY_WIDTH, MediaStore.Images.Media.WIDTH); | |||
| put(SORT_BY_HEIGHT, MediaStore.Images.Media.HEIGHT); | |||
| put(SORT_BY_WIDTH, MediaStore.MediaColumns.WIDTH); | |||
There was a problem hiding this comment.
I don't exactly recall if this change was necessary. I think my intention was to use a media-type-agnostic identifier here, with the hope that it would help for videos
There was a problem hiding this comment.
I looked into this a bit more. These two constants are exactly the same.
I think I prefer MediaStore.MediaColumns.WIDTH since the name doesn't appear to be image-specific, but I can revert these changes in the interest of preserving git blame. Let me know!
There was a problem hiding this comment.
So, if there are the same and we also use them when we deal with videos, MediaColumns is better ;)
Thank for investigating this 🥇
...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java
Outdated
Show resolved
Hide resolved
...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java
Outdated
Show resolved
Hide resolved
...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java
Outdated
Show resolved
Hide resolved
...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java
Outdated
Show resolved
Hide resolved
...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java
Outdated
Show resolved
Hide resolved
...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java
Outdated
Show resolved
Hide resolved
| if ((cursor.getType(widthIndex) == Cursor.FIELD_TYPE_NULL || | ||
| cursor.getType(heightIndex) == Cursor.FIELD_TYPE_NULL) && | ||
| mediaType == Files.FileColumns.MEDIA_TYPE_IMAGE) { |
There was a problem hiding this comment.
Is it safe to omit these conditions?
There was a problem hiding this comment.
Oops, good catch. The getType checks shouldn't be necessary, but we should keep the mediaType check, if only to avoid the perf hit of a useless check
| @@ -56,8 +56,8 @@ | |||
| put(SORT_BY_CREATION_TIME, MediaStore.Images.Media.DATE_TAKEN); | |||
| put(SORT_BY_MODIFICATION_TIME, MediaStore.Images.Media.DATE_MODIFIED); | |||
| put(SORT_BY_MEDIA_TYPE, MediaStore.Files.FileColumns.MEDIA_TYPE); | |||
| put(SORT_BY_WIDTH, MediaStore.Images.Media.WIDTH); | |||
| put(SORT_BY_HEIGHT, MediaStore.Images.Media.HEIGHT); | |||
| put(SORT_BY_WIDTH, MediaStore.MediaColumns.WIDTH); | |||
There was a problem hiding this comment.
I looked into this a bit more. These two constants are exactly the same.
I think I prefer MediaStore.MediaColumns.WIDTH since the name doesn't appear to be image-specific, but I can revert these changes in the interest of preserving git blame. Let me know!
...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java
Outdated
Show resolved
Hide resolved
| if ((cursor.getType(widthIndex) == Cursor.FIELD_TYPE_NULL || | ||
| cursor.getType(heightIndex) == Cursor.FIELD_TYPE_NULL) && | ||
| mediaType == Files.FileColumns.MEDIA_TYPE_IMAGE) { |
There was a problem hiding this comment.
Oops, good catch. The getType checks shouldn't be necessary, but we should keep the mediaType check, if only to avoid the perf hit of a useless check
|
Ping! :) |
|
@Ashoat, sorry for the late response 🙇♂️Please, add a changelog entry. To do it, edit |
|
Rebased and added the |
|
Thanks 🥇I'll wait for the CI. If everything will be green, I will merge it. Great job 🎉 |
Why
Right now, the dimensions returned by
MediaLibrary.getAssetsAsyncon Android do not consider rotation data. In contrast, the iOS version of the module flips the dimensions. In my case, I rely on these dimension values to render appropriately sized containers for media while it is loading.<Image>and<Video>both consider rotation data when rendering media.How
It basically fetches EXIF data for photos and uses
MediaMetadataRetrieverfor videos.Test Plan
I am using this patch on my app in production. I've tried it on an old Android 4.4 device as well as newer emulators. I've tried it with photos and video.
I hesitated a bit in putting up this PR because I haven't been able to do it full diligence. I haven't analyzed its performance impacts across the spectrum of older devices and I'm missing tests. I don't think I'll be able to get those things done soon, but I wanted to share this patch to at least get a conversation started.