RUM-2708: Add tests for ResourceResolver

features/dd-sdk-android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ImageWireframeHelper.kt

@@ -22,9 +22,9 @@ import com.datadog.android.sessionreplay.model.MobileSegment
 import com.datadog.android.sessionreplay.utils.UniqueIdentifierGenerator
 import java.util.Locale
 
-// This should not have a callback but it should just create a placeholder for resourcesSerializer
-// The resourcesSerializer dependency should be removed from here
-// TODO: RUM-0000 Remove the resourcesSerializer dependency from here
+// This should not have a callback but it should just create a placeholder for resourceResolver
+// The resourceResolver dependency should be removed from here
+// TODO: RUM-0000 Remove the resourceResolver dependency from here
 internal class ImageWireframeHelper(
     private val logger: InternalLogger,
     private val resourceResolver: ResourceResolver,
@@ -115,7 +115,7 @@ internal class ImageWireframeHelper(
             drawable = drawableProperties.drawable,
             drawableWidth = width,
             drawableHeight = height,
-            resourcesSerializerCallback = object : ResourcesSerializerCallback {
+            resourceResolverCallback = object : ResourceResolverCallback {
                 override fun onSuccess(resourceId: String) {
                     populateResourceIdInWireframe(resourceId, imageWireframe)
                     imageWireframeHelperCallback.onFinished()

features/dd-sdk-android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ResolveResourceCallback.kt

@@ -7,6 +7,6 @@
 package com.datadog.android.sessionreplay.internal.recorder.resources
 
 internal interface ResolveResourceCallback {
-    fun onResolved(resourceId: String, byteArray: ByteArray)
+    fun onResolved(resourceId: String, resourceData: ByteArray)
     fun onFailed()
 }

features/dd-sdk-android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ResourceItemCreationHandler.kt

@@ -20,13 +20,13 @@ internal class ResourceItemCreationHandler(
     @VisibleForTesting internal val resourceIdsSeen: MutableSet<String> =
         Collections.synchronizedSet(HashSet<String>())
 
-    internal fun queueItem(resourceId: String, byteArray: ByteArray) {
+    internal fun queueItem(resourceId: String, resourceData: ByteArray) {
         if (!resourceIdsSeen.contains(resourceId)) {
             resourceIdsSeen.add(resourceId)
 
             recordedDataQueueHandler.addResourceItem(
                 identifier = resourceId,
-                resourceData = byteArray,
+                resourceData = resourceData,
                 applicationId = applicationId
             )
         }

features/dd-sdk-android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ResourceResolver.kt

@@ -48,7 +48,7 @@ internal class ResourceResolver(
         drawable: Drawable,
         drawableWidth: Int,
         drawableHeight: Int,
-        resourcesSerializerCallback: ResourcesSerializerCallback
+        resourceResolverCallback: ResourceResolverCallback
     ) {
         bitmapCachesManager.registerCallbacks(applicationContext)
 
@@ -57,7 +57,7 @@ internal class ResourceResolver(
         if (resourceId != null) {
             // if we got here it means we saw the bitmap before,
             // so we don't need to send the resource again
-            resourcesSerializerCallback.onSuccess(resourceId)
+            resourceResolverCallback.onSuccess(resourceId)
             return
         }
 
@@ -70,21 +70,21 @@ internal class ResourceResolver(
         // do in the background
         threadPoolExecutor.executeSafe("resolveResourceId", logger) {
             @Suppress("ThreadSafety") // this runs inside an executor
-            createBitmapAsync(
+            createBitmap(
                 resources = resources,
                 drawable = drawable,
                 drawableWidth = drawableWidth,
                 drawableHeight = drawableHeight,
                 displayMetrics = displayMetrics,
                 bitmapFromDrawable = bitmapFromDrawable,
                 resolveResourceCallback = object : ResolveResourceCallback {
-                    override fun onResolved(resourceId: String, byteArray: ByteArray) {
-                        resourceItemCreationHandler.queueItem(resourceId, byteArray)
-                        resourcesSerializerCallback.onSuccess(resourceId)
+                    override fun onResolved(resourceId: String, resourceData: ByteArray) {
+                        resourceItemCreationHandler.queueItem(resourceId, resourceData)
+                        resourceResolverCallback.onSuccess(resourceId)
                     }
 
                     override fun onFailed() {
-                        resourcesSerializerCallback.onFailure()
+                        resourceResolverCallback.onFailure()
                     }
                 }
             )
@@ -96,7 +96,7 @@ internal class ResourceResolver(
     // region private
 
     @WorkerThread
-    private fun createBitmapAsync(
+    private fun createBitmap(
         resources: Resources,
         drawable: Drawable,
         drawableWidth: Int,
@@ -105,13 +105,14 @@ internal class ResourceResolver(
         bitmapFromDrawable: Bitmap?,
         resolveResourceCallback: ResolveResourceCallback
     ) {
-        var handledBitmap: Bitmap? = null
-        if (bitmapFromDrawable != null) {
-            handledBitmap = tryToGetBitmapFromBitmapDrawable(
+        val handledBitmap = if (bitmapFromDrawable != null) {
+            tryToGetBitmapFromBitmapDrawable(
                 drawable = drawable as BitmapDrawable,
                 bitmapFromDrawable = bitmapFromDrawable,
                 resolveResourceCallback = resolveResourceCallback
             )
+        } else {
+        null
         }
 
         if (handledBitmap == null) {
@@ -131,18 +132,18 @@ internal class ResourceResolver(
     private fun resolveResourceHash(
         drawable: Drawable,
         bitmap: Bitmap,
-        byteArray: ByteArray,
+        compressedBitmapBytes: ByteArray,
         shouldCacheBitmap: Boolean,
         resolveResourceCallback: ResolveResourceCallback
     ) {
         // failed to get image data
-        if (byteArray.isEmpty()) {
+        if (compressedBitmapBytes.isEmpty()) {
             // we are already logging the failure in webpImageCompression
             resolveResourceCallback.onFailed()
             return
         }
 
-        val resourceId = md5HashGenerator.generate(byteArray)
+        val resourceId = md5HashGenerator.generate(compressedBitmapBytes)
 
         // failed to resolve bitmap identifier
         if (resourceId == null) {
@@ -158,7 +159,7 @@ internal class ResourceResolver(
             drawable = drawable
         )
 
-        resolveResourceCallback.onResolved(resourceId, byteArray)
+        resolveResourceCallback.onResolved(resourceId, compressedBitmapBytes)
     }
 
     private fun cacheIfNecessary(
@@ -191,13 +192,19 @@ internal class ResourceResolver(
             displayMetrics = displayMetrics,
             bitmapCreationCallback = object : BitmapCreationCallback {
                 override fun onReady(bitmap: Bitmap) {
-                    val byteArray = webPImageCompression.compressBitmap(bitmap)
+                    val compressedBitmapBytes = webPImageCompression.compressBitmap(bitmap)
+
+                    // failed to compress bitmap
+                    if (compressedBitmapBytes.isEmpty()) {
+                        resolveResourceCallback.onFailed()
+                        return
+                    }
 
                     @Suppress("ThreadSafety") // this runs inside an executor
                     resolveResourceHash(
                         drawable = drawable,
                         bitmap = bitmap,
-                        byteArray = byteArray,
+                        compressedBitmapBytes = compressedBitmapBytes,
                         shouldCacheBitmap = true,
                         resolveResourceCallback = resolveResourceCallback
                     )
@@ -217,34 +224,37 @@ internal class ResourceResolver(
         bitmapFromDrawable: Bitmap,
         resolveResourceCallback: ResolveResourceCallback
     ): Bitmap? {
-        drawableUtils.createScaledBitmap(bitmapFromDrawable)?.let { scaledBitmap ->
-            val byteArray = webPImageCompression.compressBitmap(scaledBitmap)
+        val scaledBitmap = drawableUtils.createScaledBitmap(bitmapFromDrawable)
+            ?: return null
 
-            // failed to get byteArray potentially because the bitmap was recycled before imageCompression
-            // we'll now failover to attempting to create a new bitmap from the drawable
-            if (byteArray.isEmpty() && scaledBitmap.isRecycled) {
-                return null
-            }
+        val compressedBitmapBytes = webPImageCompression.compressBitmap(scaledBitmap)
 
-            /**
-             * Check whether the scaled bitmap is the same as the original.
-             * Since Bitmap.createScaledBitmap will return the original bitmap if the
-             * requested dimensions match the dimensions of the original
-             */
-            val shouldCacheBitmap = scaledBitmap != drawable.bitmap
+        // failed to get byteArray potentially because the bitmap was recycled before imageCompression
+        if (compressedBitmapBytes.isEmpty()) {
+            return null
+        }
 
-            resolveResourceHash(
-                drawable = drawable,
-                bitmap = scaledBitmap,
-                byteArray = byteArray,
-                shouldCacheBitmap = shouldCacheBitmap,
-                resolveResourceCallback = resolveResourceCallback
-            )
+        /**
+         * Check whether the scaled bitmap is the same as the original.
+         * Since Bitmap.createScaledBitmap will return the original bitmap if the
+         * requested dimensions match the dimensions of the original
+         * Add a specific check for isRecycled, because getting width/height from a recycled bitmap
+         * is undefined behavior
+         */
+        val shouldCacheBitmap = !bitmapFromDrawable.isRecycled && (
+        scaledBitmap.width < bitmapFromDrawable.width ||
+                scaledBitmap.height < bitmapFromDrawable.height
+        )
 
-            return scaledBitmap
-        }
+        resolveResourceHash(
+            drawable = drawable,
+            bitmap = scaledBitmap,
+            compressedBitmapBytes = compressedBitmapBytes,
+            shouldCacheBitmap = shouldCacheBitmap,
+            resolveResourceCallback = resolveResourceCallback
+        )
 
-        return null
+        return scaledBitmap
     }
 
     private fun tryToGetResourceFromCache(

features/dd-sdk-android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ResourcesSerializerCallback.kt → features/dd-sdk-android-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ResourceResolverCallback.kt

@@ -6,7 +6,7 @@
 
 package com.datadog.android.sessionreplay.internal.recorder.resources
 
-internal interface ResourcesSerializerCallback {
+internal interface ResourceResolverCallback {
     fun onSuccess(resourceId: String)
 
     fun onFailure()

features/dd-sdk-android-session-replay/src/test/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ImageWireframeHelperTest.kt

@@ -373,7 +373,7 @@ internal class ImageWireframeHelperTest {
                 any()
             )
         ).thenAnswer {
-            val callback = it.arguments[6] as ResourcesSerializerCallback
+            val callback = it.arguments[6] as ResourceResolverCallback
             callback.onSuccess(fakeResourceId)
         }
 
@@ -461,7 +461,7 @@ internal class ImageWireframeHelperTest {
         wireframes[0] as MobileSegment.Wireframe.ImageWireframe
 
         // Then
-        val argumentCaptor = argumentCaptor<ResourcesSerializerCallback>()
+        val argumentCaptor = argumentCaptor<ResourceResolverCallback>()
 
         verify(mockResourceResolver).resolveResourceId(
             any(),
@@ -506,7 +506,7 @@ internal class ImageWireframeHelperTest {
         wireframes[0] as MobileSegment.Wireframe.ImageWireframe
 
         // Then
-        val argumentCaptor = argumentCaptor<ResourcesSerializerCallback>()
+        val argumentCaptor = argumentCaptor<ResourceResolverCallback>()
         verify(mockResourceResolver, times(2)).resolveResourceId(
             any(),
             any(),
@@ -582,7 +582,7 @@ internal class ImageWireframeHelperTest {
             drawable = any(),
             drawableWidth = captor.capture(),
             drawableHeight = captor.capture(),
-            resourcesSerializerCallback = any()
+            resourceResolverCallback = any()
         )
         assertThat(captor.allValues).containsExactly(fakeViewWidth, fakeViewHeight)
     }
@@ -613,7 +613,7 @@ internal class ImageWireframeHelperTest {
             drawable = any(),
             drawableWidth = captor.capture(),
             drawableHeight = captor.capture(),
-            resourcesSerializerCallback = any()
+            resourceResolverCallback = any()
 
         )
         assertThat(captor.allValues).containsExactly(fakeDrawableWidth, fakeDrawableHeight)
@@ -695,7 +695,7 @@ internal class ImageWireframeHelperTest {
             drawable = any(),
             drawableWidth = any(),
             drawableHeight = any(),
-            resourcesSerializerCallback = any()
+            resourceResolverCallback = any()
         )
     }
 

features/dd-sdk-android-session-replay/src/test/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ResourceItemCreationHandlerTest.kt

@@ -11,6 +11,7 @@ import com.datadog.android.sessionreplay.internal.async.DataQueueHandler
 import fr.xgouchet.elmyr.annotation.StringForgery
 import fr.xgouchet.elmyr.junit5.ForgeConfiguration
 import fr.xgouchet.elmyr.junit5.ForgeExtension
+import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.extension.ExtendWith
@@ -48,7 +49,7 @@ internal class ResourceItemCreationHandlerTest {
     }
 
     @Test
-    fun `M queue item W queueItemIfNotPreviouslySeen() { not previously seen }`() {
+    fun `M queue item W queueItem() { not previously seen }`() {
         // Given
         val fakeByteArray = fakeResourceId.toByteArray()
 
@@ -64,7 +65,7 @@ internal class ResourceItemCreationHandlerTest {
     }
 
     @Test
-    fun `M not queue item W queueItemIfNotPreviouslySeen() { previously seen }`() {
+    fun `M not queue item W queueItem() { previously seen }`() {
         // Given
         val fakeByteArray = fakeResourceId.toByteArray()
 
@@ -81,7 +82,7 @@ internal class ResourceItemCreationHandlerTest {
     }
 
     @Test
-    fun `M add unique resourceId only once W queueItemIfNotPreviouslySeen()`() {
+    fun `M add unique resourceId only once W queueItem()`() {
         // Given
         val fakeByteArray = fakeResourceId.toByteArray()
 
@@ -90,6 +91,6 @@ internal class ResourceItemCreationHandlerTest {
         testedHandler.queueItem(fakeResourceId, fakeByteArray)
 
         // Then
-        assert(testedHandler.resourceIdsSeen.size == 1)
+        assertThat(testedHandler.resourceIdsSeen).hasSize(1)
     }
 }