Pr fixes

Add concurrent tests and do not clear caches when going to background
DataDog · Jul 31, 2023 · ba3545f · ba3545f
1 parent f0c7a76
commit ba3545f
Show file tree

Hide file tree

Showing 15 changed files with 191 additions and 201 deletions.
diff --git a/features/dd-sdk-android-session-replay/api/apiSurface b/features/dd-sdk-android-session-replay/api/apiSurface
@@ -25,10 +25,10 @@ data class com.datadog.android.sessionreplay.internal.recorder.SystemInformation
 class com.datadog.android.sessionreplay.internal.recorder.base64.Base64Serializer
 interface com.datadog.android.sessionreplay.internal.recorder.base64.ImageCompression
   fun getMimeType(): String?
-  fun compressBitmapToStream(android.graphics.Bitmap): java.io.ByteArrayOutputStream
+  fun compressBitmap(android.graphics.Bitmap): ByteArray
 class com.datadog.android.sessionreplay.internal.recorder.base64.WebPImageCompression : ImageCompression
   override fun getMimeType(): String?
-  override fun compressBitmapToStream(android.graphics.Bitmap): java.io.ByteArrayOutputStream
+  override fun compressBitmap(android.graphics.Bitmap): ByteArray
   companion object 
 abstract class com.datadog.android.sessionreplay.internal.recorder.mapper.BaseWireframeMapper<T: android.view.View, S: com.datadog.android.sessionreplay.model.MobileSegment.Wireframe> : WireframeMapper<T, S>, com.datadog.android.sessionreplay.internal.AsyncImageProcessingCallback
   constructor(com.datadog.android.sessionreplay.utils.StringUtils = StringUtils, com.datadog.android.sessionreplay.utils.ViewUtils = ViewUtils, com.datadog.android.sessionreplay.internal.recorder.base64.ImageCompression = WebPImageCompression(), com.datadog.android.sessionreplay.utils.UniqueIdentifierGenerator = UniqueIdentifierGenerator, com.datadog.android.sessionreplay.internal.recorder.base64.Base64Serializer = Base64Serializer.Builder().build())

diff --git a/features/dd-sdk-android-session-replay/api/dd-sdk-android-session-replay.api b/features/dd-sdk-android-session-replay/api/dd-sdk-android-session-replay.api
@@ -92,13 +92,13 @@ public final class com/datadog/android/sessionreplay/internal/recorder/base64/Ba
 }
 
 public abstract interface class com/datadog/android/sessionreplay/internal/recorder/base64/ImageCompression {
-	public abstract fun compressBitmapToStream (Landroid/graphics/Bitmap;)Ljava/io/ByteArrayOutputStream;
+	public abstract fun compressBitmap (Landroid/graphics/Bitmap;)[B
 	public abstract fun getMimeType ()Ljava/lang/String;
 }
 
 public final class com/datadog/android/sessionreplay/internal/recorder/base64/WebPImageCompression : com/datadog/android/sessionreplay/internal/recorder/base64/ImageCompression {
 	public static final field Companion Lcom/datadog/android/sessionreplay/internal/recorder/base64/WebPImageCompression$Companion;
-	public fun compressBitmapToStream (Landroid/graphics/Bitmap;)Ljava/io/ByteArrayOutputStream;
+	public fun compressBitmap (Landroid/graphics/Bitmap;)[B
 	public fun getMimeType ()Ljava/lang/String;
 }
 

diff --git a/features/dd-sdk-android-session-replay/build.gradle.kts b/features/dd-sdk-android-session-replay/build.gradle.kts
@@ -27,6 +27,7 @@ plugins {
     id("com.github.ben-manes.versions")
 
     // Tests
+    id("de.mobilej.unmock")
     id("org.jetbrains.kotlinx.kover")
 
     // Internal Generation
@@ -103,6 +104,7 @@ dependencies {
     }
     testImplementation(libs.bundles.jUnit5)
     testImplementation(libs.bundles.testTools)
+    unmock(libs.robolectric)
 
     // TODO MTG-12 detekt(project(":tools:detekt"))
     // TODO MTG-12 detekt(libs.detektCli)
@@ -111,6 +113,10 @@ dependencies {
 apply(from = "clone_session_replay_schema.gradle.kts")
 apply(from = "generate_session_replay_models.gradle.kts")
 
+unMock {
+    keep("android.util.LruCache")
+}
+
 kotlinConfig(jvmBytecodeTarget = JvmTarget.JVM_11)
 junitConfig()
 javadocConfig()

diff --git a/.../main/kotlin/com/datadog/android/sessionreplay/internal/async/RecordedDataQueueHandler.kt b/.../main/kotlin/com/datadog/android/sessionreplay/internal/async/RecordedDataQueueHandler.kt
@@ -6,15 +6,12 @@
 
 package com.datadog.android.sessionreplay.internal.async
 
-import android.graphics.drawable.Drawable
 import androidx.annotation.MainThread
 import androidx.annotation.VisibleForTesting
 import androidx.annotation.WorkerThread
 import com.datadog.android.sessionreplay.internal.processor.RecordedDataProcessor
 import com.datadog.android.sessionreplay.internal.processor.RumContextDataHandler
 import com.datadog.android.sessionreplay.internal.recorder.SystemInformation
-import com.datadog.android.sessionreplay.internal.recorder.base64.Base64LRUCache
-import com.datadog.android.sessionreplay.internal.recorder.base64.Cache
 import com.datadog.android.sessionreplay.internal.utils.TimeProvider
 import com.datadog.android.sessionreplay.model.MobileSegment
 import java.lang.ClassCastException
@@ -37,18 +34,15 @@ internal class RecordedDataQueueHandler {
     private var processor: RecordedDataProcessor
     private var rumContextDataHandler: RumContextDataHandler
     private var timeProvider: TimeProvider
-    private var cache: Cache<Drawable, String>
 
     internal constructor(
         processor: RecordedDataProcessor,
         rumContextDataHandler: RumContextDataHandler,
-        timeProvider: TimeProvider,
-        cache: Cache<Drawable, String> = Base64LRUCache
+        timeProvider: TimeProvider
     ) : this(
         processor = processor,
         rumContextDataHandler = rumContextDataHandler,
         timeProvider = timeProvider,
-        cache = cache,
 
         /**
          * TODO: RUMM-0000 consider change to LoggingThreadPoolExecutor once V2 is merged.
@@ -70,14 +64,12 @@ internal class RecordedDataQueueHandler {
         processor: RecordedDataProcessor,
         rumContextDataHandler: RumContextDataHandler,
         timeProvider: TimeProvider,
-        executorService: ExecutorService,
-        cache: Cache<Drawable, String>
+        executorService: ExecutorService
     ) {
         this.processor = processor
         this.rumContextDataHandler = rumContextDataHandler
         this.executorService = executorService
         this.timeProvider = timeProvider
-        this.cache = cache
     }
 
     // region internal

diff --git a/...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Base64Serializer.kt b/...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Base64Serializer.kt
@@ -124,9 +124,9 @@ class Base64Serializer private constructor(
     private fun convertBmpToBase64(drawable: Drawable, bitmap: Bitmap): String {
         val base64Result: String
 
-        val byteArrayOutputStream = webPImageCompression.compressBitmapToStream(bitmap)
+        val byteArray = webPImageCompression.compressBitmap(bitmap)
 
-        base64Result = base64Utils.serializeToBase64String(byteArrayOutputStream)
+        base64Result = base64Utils.serializeToBase64String(byteArray)
 
         if (base64Result.isNotEmpty()) {
             // if we got a base64 string then cache it

diff --git a/.../src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/BitmapPool.kt b/.../src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/BitmapPool.kt
@@ -10,22 +10,25 @@ import android.content.ComponentCallbacks2
 import android.content.res.Configuration
 import android.graphics.Bitmap
 import android.graphics.Bitmap.Config
+import android.os.Build
 import android.util.LruCache
 import androidx.annotation.VisibleForTesting
 import com.datadog.android.api.InternalLogger
 import com.datadog.android.sessionreplay.internal.utils.CacheUtils
 import java.util.concurrent.atomic.AtomicInteger
 
+@Suppress("TooManyFunctions")
 internal object BitmapPool : Cache<String, Bitmap>, ComponentCallbacks2 {
+    private const val BITMAP_OPERATION_FAILED = "operation failed for bitmap pool"
+
+    @VisibleForTesting
     @Suppress("MagicNumber")
-    private val MAX_CACHE_MEMORY_SIZE_BYTES = 4 * 1024 * 1024 // 4MB
+    internal val MAX_CACHE_MEMORY_SIZE_BYTES = 4 * 1024 * 1024 // 4MB
 
     private var bitmapsBySize = HashMap<String, HashSet<Bitmap>>()
     private var usedBitmaps = HashSet<Bitmap>()
     private val logger = InternalLogger.UNBOUND
 
-    private const val BITMAP_OPERATION_FAILED = "operation failed for bitmap pool"
-
     @VisibleForTesting
     internal var bitmapIndex = AtomicInteger(0)
 
@@ -48,6 +51,7 @@ internal object BitmapPool : Cache<String, Bitmap>, ComponentCallbacks2 {
                 val bitmapGroup = bitmapsBySize[dimensionsKey] ?: HashSet()
 
                 safeCall {
+                    @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block
                     bitmapGroup.remove(oldValue)
                 }
                 markBitmapAsFree(oldValue)
@@ -80,45 +84,33 @@ internal object BitmapPool : Cache<String, Bitmap>, ComponentCallbacks2 {
 
         val key = generateKey(value)
 
-        val bitmapExistsInPool = safeCallWithBoolean { bitmapsBySize[key]?.contains(value) ?: false }
+        val bitmapExistsInPool = safeCallWithBoolean {
+            @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block
+            bitmapsBySize[key]?.contains(value) ?: false
+        }
 
-        if (bitmapExistsInPool) {
-            markBitmapAsFree(value)
-        } else {
-            val cacheIndex = bitmapIndex.incrementAndGet()
-            val cacheKey = "$key-$cacheIndex"
-            cache.put(cacheKey, value)
-            if (bitmapsBySize[key] == null) bitmapsBySize[key] = HashSet()
-            bitmapsBySize[key]?.add(value)
+        if (!bitmapExistsInPool) {
+            addBitmapToPool(key, value)
         }
+
+        markBitmapAsFree(value)
     }
 
-    @Synchronized
     override fun size(): Int = cache.size()
 
     @Synchronized
     override fun clear() = cache.evictAll()
 
     @Synchronized
     override fun get(element: String): Bitmap? {
-        var bitmap: Bitmap? = null
-        val bitmapsOfRightSize = bitmapsBySize.get(element)
-        if (!bitmapsOfRightSize.isNullOrEmpty()) {
-            // filter out all the used bitmaps
-            val freeBitmaps = bitmapsOfRightSize.filterNot {
-                safeCallWithBoolean { usedBitmaps.contains(it) }
-            }
+        val bitmapsWithReqDimensions = bitmapsBySize[element] ?: return null
 
-            if (freeBitmaps.isNotEmpty()) {
-                bitmap = freeBitmaps.first()
-                markBitmapAsUsed(bitmap)
-            }
-        }
-
-        return bitmap
+        // find the first unused bitmap, mark it as used and return it
+        return bitmapsWithReqDimensions.find {
+            safeCallWithBoolean { !usedBitmaps.contains(it) }
+        }?.apply { markBitmapAsUsed(this) }
     }
 
-    @Synchronized
     internal fun getBitmapByProperties(width: Int, height: Int, config: Config): Bitmap? {
         val key = generateKey(width, height, config)
         return get(key)
@@ -132,10 +124,26 @@ internal object BitmapPool : Cache<String, Bitmap>, ComponentCallbacks2 {
 
     private fun markBitmapAsUsed(bitmap: Bitmap) {
         safeCall {
+            @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block
             usedBitmaps.add(bitmap)
         }
     }
 
+    private fun addBitmapToPool(key: String, bitmap: Bitmap) {
+        val cacheIndex = bitmapIndex.incrementAndGet()
+        val cacheKey = "$key-$cacheIndex"
+        cache.put(cacheKey, bitmap)
+
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
+            @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block
+            safeCall { bitmapsBySize.putIfAbsent(key, HashSet()) }
+        } else {
+            if (bitmapsBySize[key] == null) bitmapsBySize[key] = HashSet()
+        }
+        @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block
+        safeCall { bitmapsBySize[key]?.add(bitmap) }
+    }
+
     private fun generateKey(bitmap: Bitmap) =
         generateKey(bitmap.width, bitmap.height, bitmap.config)
 

diff --git a/...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/ImageCompression.kt b/...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/ImageCompression.kt
@@ -22,5 +22,5 @@ interface ImageCompression {
     /**
      * Compress the bitmap to a [ByteArrayOutputStream].
      */
-    fun compressBitmapToStream(bitmap: Bitmap): ByteArrayOutputStream
+    fun compressBitmap(bitmap: Bitmap): ByteArray
 }
diff --git a/...kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/WebPImageCompression.kt b/...kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/WebPImageCompression.kt
@@ -21,13 +21,13 @@ class WebPImageCompression internal constructor() : ImageCompression {
         MimeTypeMap.getSingleton().getMimeTypeFromExtension(WEBP_EXTENSION)
 
     @WorkerThread
-    override fun compressBitmapToStream(bitmap: Bitmap): ByteArrayOutputStream {
-        val byteArrayOutputStream = ByteArrayOutputStream()
+    override fun compressBitmap(bitmap: Bitmap): ByteArray {
+        // preallocate stream size
+        val byteArrayOutputStream = ByteArrayOutputStream(bitmap.allocationByteCount)
         val imageFormat = getImageCompressionFormat()
-        // stream is not null and image quality is between 0 and 100
-        @Suppress("UnsafeThirdPartyFunctionCall")
+        @Suppress("UnsafeThirdPartyFunctionCall") // stream is not null and image quality is between 0 and 100
         bitmap.compress(imageFormat, IMAGE_QUALITY, byteArrayOutputStream)
-        return byteArrayOutputStream
+        return byteArrayOutputStream.toByteArray()
     }
 
     private fun getImageCompressionFormat(): Bitmap.CompressFormat =

diff --git a/...on-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/Base64Utils.kt b/...on-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/Base64Utils.kt
@@ -9,12 +9,11 @@ package com.datadog.android.sessionreplay.internal.utils
 import android.util.Base64
 import androidx.annotation.WorkerThread
 import com.datadog.android.sessionreplay.internal.recorder.wrappers.Base64Wrapper
-import java.io.ByteArrayOutputStream
 
 internal class Base64Utils(
     private val base64Wrapper: Base64Wrapper = Base64Wrapper()
 ) {
     @WorkerThread
-    internal fun serializeToBase64String(byteArrayOutputStream: ByteArrayOutputStream): String =
-        base64Wrapper.encodeToString(byteArrayOutputStream.toByteArray(), Base64.DEFAULT)
+    internal fun serializeToBase64String(byteArray: ByteArray): String =
+        base64Wrapper.encodeToString(byteArray, Base64.DEFAULT)
 }
diff --git a/...ion-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/CacheUtils.kt b/...ion-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/CacheUtils.kt
@@ -42,9 +42,7 @@ internal class CacheUtils<K : Any, V : Any> {
                 cache.trimToSize(onModerateMemorySizeBytes)
             }
 
-            ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN -> {
-                cache.evictAll()
-            }
+            ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN -> {}
 
             else -> {
                 cache.evictAll()

diff --git a/...-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/DrawableUtils.kt b/...-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/DrawableUtils.kt
@@ -87,26 +87,9 @@ internal class DrawableUtils(
         width: Int,
         height: Int,
         config: Config
-    ): Bitmap? {
-        var bitmap = getReusableBitmapFromPool(width, height, config)
-        if (bitmap == null) {
-            bitmap = createNewBitmap(displayMetrics, width, height, config)
-        }
-        return bitmap
-    }
-
-    private fun getReusableBitmapFromPool(
-        width: Int,
-        height: Int,
-        config: Config
-    ) = bitmapPool.getBitmapByProperties(width, height, config)
-
-    private fun createNewBitmap(
-        displayMetrics: DisplayMetrics,
-        width: Int,
-        height: Int,
-        config: Config
-    ): Bitmap? = bitmapWrapper.createBitmap(displayMetrics, width, height, config)
+    ): Bitmap? =
+        bitmapPool.getBitmapByProperties(width, height, config)
+            ?: bitmapWrapper.createBitmap(displayMetrics, width, height, config)
 
     @MainThread
     private fun registerBitmapPoolForCallbacks(applicationContext: Context) {

diff --git a/...t/kotlin/com/datadog/android/sessionreplay/internal/async/RecordedDataQueueHandlerTest.kt b/...t/kotlin/com/datadog/android/sessionreplay/internal/async/RecordedDataQueueHandlerTest.kt
@@ -6,15 +6,13 @@
 
 package com.datadog.android.sessionreplay.internal.async
 
-import android.graphics.drawable.Drawable
 import com.datadog.android.sessionreplay.forge.ForgeConfigurator
 import com.datadog.android.sessionreplay.internal.async.RecordedDataQueueHandler.Companion.MAX_DELAY_MS
 import com.datadog.android.sessionreplay.internal.processor.RecordedDataProcessor
 import com.datadog.android.sessionreplay.internal.processor.RumContextData
 import com.datadog.android.sessionreplay.internal.processor.RumContextDataHandler
 import com.datadog.android.sessionreplay.internal.recorder.Node
 import com.datadog.android.sessionreplay.internal.recorder.SystemInformation
-import com.datadog.android.sessionreplay.internal.recorder.base64.Cache
 import com.datadog.android.sessionreplay.internal.time.SessionReplayTimeProvider
 import com.datadog.android.sessionreplay.model.MobileSegment
 import fr.xgouchet.elmyr.Forge
@@ -53,25 +51,22 @@ import java.util.concurrent.TimeUnit
 @MockitoSettings(strictness = Strictness.LENIENT)
 @ForgeConfiguration(ForgeConfigurator::class)
 internal class RecordedDataQueueHandlerTest {
-    lateinit var testedHandler: RecordedDataQueueHandler
+    private lateinit var testedHandler: RecordedDataQueueHandler
 
     @Mock
     lateinit var mockProcessor: RecordedDataProcessor
 
     @Mock
     lateinit var mockRumContextDataHandler: RumContextDataHandler
 
-    lateinit var mockExecutorService: ExecutorService
+    private lateinit var mockExecutorService: ExecutorService
 
     @Mock
     lateinit var mockSystemInformation: SystemInformation
 
     @Mock
     lateinit var mockTimeProvider: SessionReplayTimeProvider
 
-    @Mock
-    lateinit var mockBase64LruCache: Cache<Drawable, String>
-
     @Forgery
     lateinit var fakeRumContextData: RumContextData
 
@@ -111,7 +106,6 @@ internal class RecordedDataQueueHandlerTest {
             processor = mockProcessor,
             rumContextDataHandler = mockRumContextDataHandler,
             timeProvider = mockTimeProvider,
-            cache = mockBase64LruCache,
             executorService = mockExecutorService
         )
     }
@@ -240,7 +234,7 @@ internal class RecordedDataQueueHandlerTest {
         mockExecutorService.awaitTermination(1, TimeUnit.SECONDS)
 
         // Then
-        assertThat(testedHandler.recordedDataQueue.isEmpty()).isTrue()
+        assertThat(testedHandler.recordedDataQueue.isEmpty()).isTrue
         verifyNoMoreInteractions(mockProcessor)
     }