aosp/3137316 and aosp/3137053 together on top of compose-beta branch Test: run AndroidGraphicsLayerTest (cherry picked from https://android-review.googlesource.com/q/commit:f3257d91ab411a0d5c9e13244205073eaa13cefe) Merged-In: I7540268f241ff9929b6d7345af65e3654fe396b5 Change-Id: I7540268f241ff9929b6d7345af65e3654fe396b5
diff --git a/compose/ui/ui-graphics/src/androidInstrumentedTest/kotlin/androidx/compose/ui/graphics/layer/AndroidGraphicsLayerTest.kt b/compose/ui/ui-graphics/src/androidInstrumentedTest/kotlin/androidx/compose/ui/graphics/layer/AndroidGraphicsLayerTest.kt index 051250b..483c5d9 100644 --- a/compose/ui/ui-graphics/src/androidInstrumentedTest/kotlin/androidx/compose/ui/graphics/layer/AndroidGraphicsLayerTest.kt +++ b/compose/ui/ui-graphics/src/androidInstrumentedTest/kotlin/androidx/compose/ui/graphics/layer/AndroidGraphicsLayerTest.kt
@@ -191,7 +191,7 @@ record { drawRect(Color.Red) } - discardDisplayList() + emulateTrimMemory() } drawLayer(layer!!) }, @@ -1443,6 +1443,31 @@ ) } + @Test + fun testReleasingLayerDuringPersistenceLogicIsNotCrashing() { + lateinit var layer1: GraphicsLayer + lateinit var layer2: GraphicsLayer + graphicsLayerTest( + block = { context -> + // creating new layers will also schedule a persistence pass in Handler + layer1 = context.createGraphicsLayer() + layer2 = context.createGraphicsLayer() + layer2.record(Density(1f), Ltr, IntSize(10, 10)) { drawRect(Color.Red) } + layer1.record(Density(1f), Ltr, IntSize(10, 10)) { drawLayer(layer2) } + // we release layer2, but as it is drawn into layer1 its content is not discarded. + context.releaseGraphicsLayer(layer2) + // layer1 loses its content without us updating the dependency tracking + layer1.emulateTrimMemory() + }, + verify = { + // just verifying there is no crash in layer persistence logic + // there was an issue where the next persistence logic will re-draw layer1 content + // and during this draw we fully release layer2. this was removing an item from + // a set which is currently being iterated on. + } + ) + } + private fun PixelMap.verifyQuadrants( topLeft: Color, topRight: Color,
diff --git a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt index e1765683..e6e61da 100644 --- a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt +++ b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt
@@ -124,30 +124,29 @@ override fun createGraphicsLayer(): GraphicsLayer { synchronized(lock) { val ownerId = getUniqueDrawingId(ownerView) - val reusedLayer = layerManager.takeFromCache(ownerId) - val layer = if (reusedLayer != null) { - reusedLayer - } else { - val layerImpl = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { - GraphicsLayerV29() - } else if (isRenderNodeCompatible && - Build.VERSION.SDK_INT >= Build.VERSION_CODES.M + val layerImpl = + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + GraphicsLayerV29(ownerId) + } else if ( + isRenderNodeCompatible && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M ) { try { - GraphicsLayerV23(ownerView) + GraphicsLayerV23(ownerView, ownerId) } catch (_: Throwable) { - // If we ever failed to create an instance of the RenderNode stub based - // GraphicsLayer, always fallback to creation of View based layers as it is - // unlikely that subsequent attempts to create a GraphicsLayer with RenderNode + // If we ever failed to create an instance of the RenderNode stub + // based + // GraphicsLayer, always fallback to creation of View based layers + // as it is + // unlikely that subsequent attempts to create a GraphicsLayer with + // RenderNode // stubs would be successful. isRenderNodeCompatible = false - GraphicsViewLayer(obtainViewLayerContainer(ownerView)) + GraphicsViewLayer(obtainViewLayerContainer(ownerView), ownerId) } } else { - GraphicsViewLayer(obtainViewLayerContainer(ownerView)) + GraphicsViewLayer(obtainViewLayerContainer(ownerView), ownerId) } - GraphicsLayer(layerImpl, layerManager, ownerId) - } + val layer = GraphicsLayer(layerImpl, layerManager) layerManager.persist(layer) return layer }
diff --git a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/AndroidGraphicsLayer.android.kt b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/AndroidGraphicsLayer.android.kt index 5a05a5d..e5529f0 100644 --- a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/AndroidGraphicsLayer.android.kt +++ b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/AndroidGraphicsLayer.android.kt
@@ -45,12 +45,12 @@ import androidx.compose.ui.unit.LayoutDirection import androidx.compose.ui.unit.toSize import androidx.compose.ui.util.fastRoundToInt +import org.jetbrains.annotations.TestOnly @Suppress("NotCloseable") actual class GraphicsLayer internal constructor( internal val impl: GraphicsLayerImpl, - private val layerManager: LayerManager, - ownerViewId: Long + private val layerManager: LayerManager ) { private var density = DefaultDensity private var layoutDirection = LayoutDirection.Ltr @@ -565,10 +565,8 @@ discardContentIfReleasedAndHaveNoParentLayerUsages() } - private var skipOutlineConfiguration = false - private fun configureOutline() { - if (outlineDirty && !skipOutlineConfiguration) { + if (outlineDirty) { val outlineIsNeeded = clip || shadowElevation > 0f if (!outlineIsNeeded) { impl.setOutline(null) @@ -596,8 +594,8 @@ impl.setOutline(roundRectOutline) } } - outlineDirty = false } + outlineDirty = false } private inline fun <T> resolveOutlinePosition(block: (Offset, Size) -> T): T { @@ -671,6 +669,16 @@ } /** + * When the system is sending trim memory request all the render nodes will discard their + * display list. in this case we are not being notified about that and don't update + * [childDependenciesTracker], as it is done when we call [discardDisplayList] manually + */ + @TestOnly + internal fun emulateTrimMemory() { + impl.discardDisplayList() + } + + /** * The ID of the layer. This is used by tooling to match a layer to the associated * LayoutNode. */ @@ -681,8 +689,8 @@ * The uniqueDrawingId of the owner view of this graphics layer. This is used by * tooling to match a layer to the associated owner View. */ - var ownerViewId: Long = ownerViewId - private set + val ownerViewId: Long + get() = impl.ownerId actual val outline: Outline get() { @@ -830,51 +838,6 @@ actual suspend fun toImageBitmap(): ImageBitmap = SnapshotImpl.toBitmap(this).asImageBitmap() - internal fun reuse(ownerViewId: Long) { - // apply new owner id - this.ownerViewId = ownerViewId - - // mark the layer as not released - isReleased = false - - // prepare the implementation to be reused - impl.onReused() - - // forget the previous draw lambda - drawBlock = {} - - // multiple of the setters can cause configureOutline() calls, however we don't want - // to execute it multiple times, so we set this flag to true - skipOutlineConfiguration = true - - // reset properties to the default values - alpha = 1f - blendMode = BlendMode.SrcOver - colorFilter = null - pivotOffset = Offset.Unspecified - scaleX = 1f - scaleY = 1f - translationX = 0f - translationY = 0f - shadowElevation = 0f - rotationX = 0f - rotationY = 0f - rotationZ = 0f - ambientShadowColor = Color.Black - spotShadowColor = Color.Black - cameraDistance = DefaultCameraDistance - renderEffect = null - compositingStrategy = CompositingStrategy.Auto - clip = false - size = IntSize.Zero - topLeft = IntOffset.Zero - setRectOutline() - - // unset this flag. if outlineDirty is true we will call configureOutline() again when - // the layer will be drawn for the first time. - skipOutlineConfiguration = false - } - companion object { private val SnapshotImpl = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) { @@ -897,6 +860,12 @@ val layerId: Long /** + * The uniqueDrawingId of the owner view of this graphics layer. This is used by tooling to + * match a layer to the associated owner AndroidComposeView. + */ + val ownerId: Long + + /** * @see GraphicsLayer.compositingStrategy */ var compositingStrategy: CompositingStrategy @@ -1030,8 +999,6 @@ */ fun calculateMatrix(): android.graphics.Matrix - fun onReused() {} - companion object { val DefaultDrawBlock: DrawScope.() -> Unit = { drawRect(Color.Transparent) } }
diff --git a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsLayerV23.android.kt b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsLayerV23.android.kt index 327d8fe..cd0e712 100644 --- a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsLayerV23.android.kt +++ b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsLayerV23.android.kt
@@ -47,6 +47,7 @@ @RequiresApi(Build.VERSION_CODES.M) internal class GraphicsLayerV23( ownerView: View, + override val ownerId: Long, private val canvasHolder: CanvasHolder = CanvasHolder(), private val canvasDrawScope: CanvasDrawScope = CanvasDrawScope() ) : GraphicsLayerImpl {
diff --git a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt index 0e2ff9b..4251675 100644 --- a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt +++ b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt
@@ -46,6 +46,7 @@ */ @RequiresApi(Build.VERSION_CODES.Q) internal class GraphicsLayerV29( + override val ownerId: Long, private val canvasHolder: CanvasHolder = CanvasHolder(), private val canvasDrawScope: CanvasDrawScope = CanvasDrawScope() ) : GraphicsLayerImpl {
diff --git a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsViewLayer.android.kt b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsViewLayer.android.kt index 68f87f0..458cd87 100644 --- a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsViewLayer.android.kt +++ b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/GraphicsViewLayer.android.kt
@@ -102,10 +102,6 @@ this.parentLayer = parentLayer } - fun resetDrawBlock() { - drawBlock = DefaultDrawBlock - } - init { setWillNotDraw(false) // we WILL draw this.clipBounds = null @@ -159,6 +155,7 @@ internal class GraphicsViewLayer( private val layerContainer: DrawChildContainer, + override val ownerId: Long, val canvasHolder: CanvasHolder = CanvasHolder(), canvasDrawScope: CanvasDrawScope = CanvasDrawScope() ) : GraphicsLayerImpl { @@ -475,12 +472,6 @@ layerContainer.removeViewInLayout(viewLayer) } - override fun onReused() { - viewLayer.resetDrawBlock() - // it was removed in discardDisplayList() - layerContainer.addView(viewLayer) - } - companion object { val mayRenderInSoftware = !isLockHardwareCanvasAvailable()
diff --git a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/LayerManager.android.kt b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/LayerManager.android.kt index 3587227..21e4389 100644 --- a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/LayerManager.android.kt +++ b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/LayerManager.android.kt
@@ -19,13 +19,13 @@ import android.graphics.PixelFormat import android.media.ImageReader import android.os.Build -import android.os.Build.VERSION.SDK_INT -import android.os.Build.VERSION_CODES.M import android.os.Looper import android.os.Message import android.view.Surface import androidx.annotation.RequiresApi +import androidx.collection.MutableObjectList import androidx.collection.ScatterSet +import androidx.collection.mutableObjectListOf import androidx.collection.mutableScatterSetOf import androidx.compose.ui.graphics.CanvasHolder import androidx.core.os.HandlerCompat @@ -38,8 +38,7 @@ */ internal class LayerManager(val canvasHolder: CanvasHolder) { - private val activeLayerSet = mutableScatterSetOf<GraphicsLayer>() - private val nonActiveLayerCache = WeakCache<GraphicsLayer>() + private val layerSet = mutableScatterSetOf<GraphicsLayer>() /** * Create a placeholder ImageReader instance that we will use to issue a single draw call @@ -51,16 +50,15 @@ private var imageReader: ImageReader? = null private val handler = HandlerCompat.createAsync(Looper.getMainLooper()) { - persistLayers(activeLayerSet) + persistLayers(layerSet) true } - fun takeFromCache(ownerId: Long): GraphicsLayer? = nonActiveLayerCache.pop()?.also { - it.reuse(ownerId) - } + private var postponedReleaseRequests: MutableObjectList<GraphicsLayer>? = null + private var persistenceIterationInProgress = false fun persist(layer: GraphicsLayer) { - activeLayerSet.add(layer) + layerSet.add(layer) if (!handler.hasMessages(0)) { // we don't run persistLayers() synchronously in order to do less work as there // might be a lot of new layers created during one frame. however we also want @@ -73,11 +71,17 @@ } fun release(layer: GraphicsLayer) { - if (activeLayerSet.remove(layer)) { - layer.discardDisplayList() - if (SDK_INT >= M) { // L throws during RenderThread when reusing the Views. - nonActiveLayerCache.push(layer) + if (!persistenceIterationInProgress) { + if (layerSet.remove(layer)) { + layer.discardDisplayList() } + } else { + // we can't remove an item from a list, which is currently being iterated. + // so we use a second list to remember such requests + val requests = + postponedReleaseRequests + ?: mutableObjectListOf<GraphicsLayer>().also { postponedReleaseRequests = it } + requests.add(layer) } } @@ -110,12 +114,19 @@ val surface = reader.surface val canvas = LockHardwareCanvasHelper.lockHardwareCanvas(surface) + persistenceIterationInProgress = true canvasHolder.drawInto(canvas) { canvas.save() canvas.clipRect(0, 0, 1, 1) layers.forEach { layer -> layer.drawForPersistence(this) } canvas.restore() } + persistenceIterationInProgress = false + val requests = postponedReleaseRequests + if (requests != null && requests.isNotEmpty()) { + requests.forEach { release(it) } + requests.clear() + } surface.unlockCanvasAndPost(canvas) } } @@ -133,7 +144,7 @@ */ fun updateLayerPersistence() { destroy() - persistLayers(activeLayerSet) + persistLayers(layerSet) } companion object {
diff --git a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/WeakCache.android.kt b/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/WeakCache.android.kt deleted file mode 100644 index e6d27c8..0000000 --- a/compose/ui/ui-graphics/src/androidMain/kotlin/androidx/compose/ui/graphics/layer/WeakCache.android.kt +++ /dev/null
@@ -1,77 +0,0 @@ -/* - * Copyright 2021 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package androidx.compose.ui.graphics.layer - -import androidx.compose.runtime.collection.mutableVectorOf -import java.lang.ref.Reference -import java.lang.ref.ReferenceQueue -import java.lang.ref.WeakReference - -// It is a copy from androidx.compose.ui.platform -/** - * A simple collection that keeps values as [WeakReference]s. - * Elements are added with [push] and removed with [pop]. - */ -internal class WeakCache<T> { - private val values = mutableVectorOf<Reference<T>>() - private val referenceQueue = ReferenceQueue<T>() - - /** - * Add [element] to the collection as a [WeakReference]. It will be removed when - * garbage collected or from [pop]. - */ - fun push(element: T) { - clearWeakReferences() - values += WeakReference(element, referenceQueue) - } - - /** - * Remove an element from the collection and return it. If no element is - * available, `null` is returned. - */ - fun pop(): T? { - clearWeakReferences() - - while (values.isNotEmpty()) { - val item = values.removeAt(values.lastIndex).get() - if (item != null) { - return item - } - } - return null - } - - /** - * The number of elements currently in the collection. This may change between - * calls if the references have been garbage collected. - */ - val size: Int - get() { - clearWeakReferences() - return values.size - } - - private fun clearWeakReferences() { - do { - val item: Reference<out T>? = referenceQueue.poll() - if (item != null) { - @Suppress("UNCHECKED_CAST") - values.remove(item as Reference<T>) - } - } while (item != null) - } -}
diff --git a/compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/GraphicsLayerOwnerLayer.android.kt b/compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/GraphicsLayerOwnerLayer.android.kt index f57eb5f..9fdf988 100644 --- a/compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/GraphicsLayerOwnerLayer.android.kt +++ b/compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/GraphicsLayerOwnerLayer.android.kt
@@ -338,6 +338,7 @@ val context = requireNotNull(context) { "currently reuse is only supported when we manage the layer lifecycle" } + require(graphicsLayer.isReleased) { "layer should have been released before reuse" } // recreate a layer graphicsLayer = context.createGraphicsLayer()