-
Notifications
You must be signed in to change notification settings - Fork 782
Reducing bitmap memory footprint #5276
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
Conversation
- this effectively halves the amount of image memory used by notifications
…e is relatively small and there's no cap on the image size
- avoid creating new bitmaps each time the room list changes
@@ -83,11 +87,7 @@ class ShortcutCreator @Inject constructor( | |||
|
|||
private fun Bitmap.toProfileImageIcon(): IconCompat { | |||
return if (useAdaptiveIcon) { | |||
val insetBmp = Bitmap.createBitmap(adaptiveIconSize, adaptiveIconSize, Bitmap.Config.ARGB_8888) |
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.
this was the cause of our extra allocations, the fix was to transform the bitmap transformation into a cachable glide transformation https://github.com./vector-im/element-android/pull/5276/files#diff-1f9afdb46350b185b8e9cfc6698b2885bb19fb15d98c1c65dfedfe5371615d81R173
Matrix SDKIntegration Tests Results:
|
…e stored with unique keys
.asBitmap() | ||
.avatarOrText(matrixItem, iconSize) | ||
.transform(CenterCrop(), AdaptiveIconTransformation(adaptiveIconSize, adaptiveIconOuterSides)) | ||
.signature(ObjectKey("adaptive-icon")) |
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.
caches the transformed icon under with an extra unique key, to avoid overwriting other cached images on the same room url
import javax.inject.Singleton | ||
|
||
@Singleton | ||
class NotificationBitmapLoader @Inject constructor(private val context: Context) { |
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 notification helpers have been flattened into a single Notification Loader
to help give more context to the usage
|
||
@WorkerThread | ||
private fun loadRoomBitmap(path: String): Bitmap? { | ||
return try { |
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 previous in memory caches have been removed, they were repeating the glide in memory cache and potentially causing leaks (as we never cleaned up the references)
avatarRenderer.shortcutDrawable(GlideApp.with(context), roomSummary.toMatrixItem(), iconSize) | ||
val glideRequests = GlideApp.with(context) | ||
val matrixItem = roomSummary.toMatrixItem() | ||
when (useAdaptiveIcon) { |
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.
haha +1 for the boolean when
Nice work Adam 🦾 ! Do you think we should do more tests in different devices & android versions or the fix do not depend that much on that? |
@ariskotsomitopoulos for this case the extra bitmaps were being created as part of the adaptive icon logic which is only applied to android 8+ it's possible that different android versions handle garbage collecting the orphaned bitmap native memory differently but I'm not sure if it's worth testing now that we avoid creating all the extra bitmaps 🤔 |
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.
Nice work, thanks!
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.
LGTM, thanks!
} | ||
} | ||
|
||
override fun hashCode() = Util.hashCode(ADAPTIVE_TRANSFORMATION_ID.hashCode(), Util.hashCode(adaptiveIconSize, Util.hashCode(adaptiveIconOuterSides))) |
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.
Not sure if it is clearer, but I could not resist to try to rewrite it like:
override fun hashCode() = listOf(ADAPTIVE_TRANSFORMATION_ID.hashCode(), adaptiveIconSize, floatToIntBits(adaptiveIconOuterSides))
.fold(0) { acc, i -> Util.hashCode(acc, i) }
Please just ignore me :)
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 glide transformation api is a bit clunky, I didn't want to stray tooo far from the examples 😅
|
||
override fun equals(other: Any?): Boolean { | ||
return if (other is AdaptiveIconTransformation) { | ||
other.adaptiveIconSize == adaptiveIconSize && other.adaptiveIconOuterSides == adaptiveIconOuterSides |
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.
Why not just compare hashCodes here?
Type of change
Content
Motivation and context
To reduce our memory footprint, currently the app is constantly creating resized bitmaps for the shortcuts each time the room list changes.
Screenshots / GIFs
Tests
Tested devices