REPLAY-1891: Optimize bitmap processing by jonathanmos · Pull Request #1576 · DataDog/dd-sdk-android

jonathanmos · 2023-08-16T10:01:45Z

What does this PR do?

• Gets the bitmap from BitmapDrawable if available (use new EnrichedBitmap class to know if we should later cache it)
• Extraction of the base64 logic from the ImageButtonMapper to ImageWireframeHelper
• Gets background images if they exist (added an example to the sample app)
• Support scaletypes (atm only fitXY and center-crop)
• Move registration of bitmapPool for callbacks from DrawableUtils to Base64Serializer

Motivation

Additional improvements necessary to rollout base64 imagebutton support

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

codecov-commenter · 2023-08-20T13:09:05Z

Codecov Report

Merging #1576 (e814de7) into feature/base64-jmoskovich (69f88d0) will decrease coverage by 0.09%.
The diff coverage is 93.83%.

@@                      Coverage Diff                      @@
##           feature/base64-jmoskovich    #1576      +/-   ##
=============================================================
- Coverage                      83.38%   83.29%   -0.09%     
=============================================================
  Files                            444      446       +2     
  Lines                          15095    15181      +86     
  Branches                        2279     2288       +9     
=============================================================
+ Hits                           12586    12644      +58     
- Misses                          1910     1932      +22     
- Partials                         599      605       +6

Files Changed	Coverage Δ
...sionreplay/material/MaskInputTabWireframeMapper.kt	`0.00% <ø> (ø)`
...d/sessionreplay/material/MaskTabWireframeMapper.kt	`100.00% <ø> (ø)`
...y/internal/recorder/base64/ImageWireframeHelper.kt	`90.91% <90.91%> (ø)`
...play/internal/recorder/mapper/ImageButtonMapper.kt	`93.33% <92.31%> (-0.28%)`	⬇️
...ay/internal/recorder/mapper/BaseWireframeMapper.kt	`86.76% <93.88%> (+16.18%)`	⬆️
...roid/sessionreplay/internal/utils/DrawableUtils.kt	`95.74% <94.44%> (-1.23%)`	⬇️
...eplay/internal/recorder/base64/Base64Serializer.kt	`84.52% <95.24%> (+0.02%)`	⬆️
...droid/sessionreplay/material/TabWireframeMapper.kt	`97.83% <100.00%> (+2.27%)`	⬆️
...adog/android/sessionreplay/SessionReplayPrivacy.kt	`100.00% <100.00%> (ø)`
...y/internal/recorder/base64/WebPImageCompression.kt	`90.00% <100.00%> (ø)`
... and 2 more

... and 17 files with indirect coverage changes

0xnm

nice! I left some comments, let me know wdyt.

0xnm · 2023-08-23T13:47:55Z

+
+import android.graphics.Bitmap
+
+internal data class EnrichedBitmap(


I find this class name a bit generic. There is no enrichment here, it is just an additional flag attached to the bitmap. Maybe we can have some better name?

You're right - I've eliminated the class and moved it to being just an additional boolean flag

0xnm · 2023-08-23T14:11:43Z

+        view: ImageView,
+        drawable: Drawable,
+        density: Float
+    ): Pair<Long, Long> {


we probably need to document that first is width, last is height.

Changing it to DrawableDimensions

0xnm · 2023-08-23T14:17:36Z

+        )
+
+        // Then
+        verify(mockDrawableUtils, times(1)).createBitmapOfApproxSizeFromDrawable(


it is times(1) by default, so I guess we can omit this argument here and in similar calls

This is to make it explicit that the call should happen only once. If I omit times(1) then the test will pass regardless of how many calls were made to the method (>0)

0xnm · 2023-08-23T14:34:11Z

+        )
+
+        // Then
+        verify(mockBitmapPool, times(0)).put(any())


it is better to use never instead of times(0) or maybe something like verifyNoInteractions even.

0xnm · 2023-08-23T14:36:02Z

+    @LongForgery(min = 1, max = 1000)
+    var fakeDrawableHeight: Long = 0L
+
+    val mockDrawableXY: Pair<Long, Long> = Pair(30, 30)


strictly speaking it is not a mock. maybe we can also randomize the point? knowing dimensions of the drawable.

mariusc83

Apart from @0xnm comments LGTM, very nice job there.

The base branch was changed.

xgouchet · 2023-08-28T07:44:39Z

+            drawable.bitmap != null &&
+            !drawable.bitmap.isRecycled
+        ) {
+            shouldCacheBitmap = false


It seems this is not required as shouldCacheBitmap was initialised as falsed a few lines above.
Another option would be to do something like:

val (bitmap, shouldCache) = if (…) { drawable.bitmap to false } else { drawableUtils.createBitmapOfApproxSizeFromDrawable(…) to true }

xgouchet · 2023-08-28T07:55:16Z

        }

-        bitmapPool?.put(bitmap)
+        if (shouldCacheBitmap) {


Should we add some size limit to the cachable bitmaps? E.g.: only cache bitmaps smaller than x Mb?

All the cached bitmaps will be after resizing, so they would at most be around 15kbs each. The original bitmaps that aren't resized won't be cached

mariusc83

I am letting this go but I strongly recommend that you refactor the way you resolve your View backgrounds in the BaseWireframeMapper

mariusc83 · 2023-08-30T07:17:30Z

        base64Serializer?.registerAsyncLoadingCallback(asyncImageProcessingCallback)
    }

+    private fun resolveViewBackground(


looks way better now, tks for this change.

Get Bitmap from BitmapDrawable if available Extract base64 logic from mapper to ImageWireframeHelper Get background images if they exist Support scaletypes

jonathanmos changed the base branch from feature/base64-jmoskovich to jmoskovich/replay-1943/refactor-cache-singletons August 16, 2023 10:02

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch 3 times, most recently from 4b3f6bc to bc46d0d Compare August 20, 2023 12:53

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch 2 times, most recently from ae02b0d to 48d83f7 Compare August 21, 2023 12:25

jonathanmos force-pushed the jmoskovich/replay-1943/refactor-cache-singletons branch from c7b5c2f to 0f66105 Compare August 23, 2023 09:41

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch from 48d83f7 to 8e2d12a Compare August 23, 2023 11:25

jonathanmos marked this pull request as ready for review August 23, 2023 11:27

jonathanmos requested a review from a team as a code owner August 23, 2023 11:27

0xnm reviewed Aug 23, 2023

View reviewed changes

mariusc83 previously approved these changes Aug 24, 2023

View reviewed changes

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch from 8e2d12a to c8741a0 Compare August 24, 2023 10:30

jonathanmos force-pushed the jmoskovich/replay-1943/refactor-cache-singletons branch from 0f66105 to be9a3a8 Compare August 24, 2023 11:23

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch from c8741a0 to 2929fee Compare August 24, 2023 11:28

jonathanmos force-pushed the jmoskovich/replay-1943/refactor-cache-singletons branch from be9a3a8 to 5be1cc0 Compare August 24, 2023 12:03

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch from 2929fee to 923b9e0 Compare August 24, 2023 12:16

Base automatically changed from jmoskovich/replay-1943/refactor-cache-singletons to feature/base64-jmoskovich August 24, 2023 13:08

jonathanmos commented Aug 27, 2023

View reviewed changes

Comment thread ...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/mapper/BaseWireframeMapper.kt Outdated

xgouchet reviewed Aug 28, 2023

View reviewed changes

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch 3 times, most recently from a6c445b to 556a0ca Compare August 29, 2023 07:18

jonathanmos requested review from 0xnm, mariusc83 and xgouchet August 29, 2023 07:50

mariusc83 reviewed Aug 29, 2023

View reviewed changes

Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/recorder/mapper/ImageButtonMapper.kt

mariusc83 previously approved these changes Aug 29, 2023

View reviewed changes

Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/recorder/mapper/ImageButtonMapper.kt Outdated

jonathanmos dismissed mariusc83’s stale review via 793bc4f August 29, 2023 12:25

jonathanmos requested a review from mariusc83 August 29, 2023 14:36

mariusc83 previously approved these changes Aug 30, 2023

View reviewed changes

xgouchet previously approved these changes Aug 30, 2023

View reviewed changes

0xnm previously approved these changes Aug 30, 2023

View reviewed changes

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch 2 times, most recently from fb59d82 to 7753112 Compare August 30, 2023 09:46

Optimize Bitmap Processing

e814de7

Get Bitmap from BitmapDrawable if available Extract base64 logic from mapper to ImageWireframeHelper Get background images if they exist Support scaletypes

jonathanmos dismissed stale reviews from 0xnm, mariusc83, and xgouchet via e814de7 August 30, 2023 10:11

jonathanmos force-pushed the jmoskovich/replay-1891/optimize-bitmapdrawable branch from 7753112 to e814de7 Compare August 30, 2023 10:11

xgouchet approved these changes Aug 30, 2023

View reviewed changes

jonathanmos merged commit d4c8e5b into feature/base64-jmoskovich Aug 30, 2023

jonathanmos deleted the jmoskovich/replay-1891/optimize-bitmapdrawable branch August 30, 2023 11:08

xgouchet added this to the 2.2.0 milestone Dec 13, 2023


		import android.graphics.Bitmap

		internal data class EnrichedBitmap(

Uh oh!

Conversation

jonathanmos commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanmos Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mariusc83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanmos Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mariusc83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jonathanmos commented Aug 16, 2023 •

edited

Loading

codecov-commenter commented Aug 20, 2023 •

edited

Loading

jonathanmos Aug 24, 2023 •

edited

Loading

jonathanmos Aug 28, 2023 •

edited

Loading