diff options
| -rw-r--r-- | core/tests/coretests/src/android/graphics/PaintNativeInstanceTest.kt | 161 | ||||
| -rw-r--r-- | graphics/java/android/graphics/ColorFilter.java | 4 | ||||
| -rw-r--r-- | graphics/java/android/graphics/ComposeShader.java | 10 | ||||
| -rw-r--r-- | graphics/java/android/graphics/Paint.java | 7 | ||||
| -rw-r--r-- | graphics/java/android/graphics/Shader.java | 21 |
5 files changed, 186 insertions, 17 deletions
diff --git a/core/tests/coretests/src/android/graphics/PaintNativeInstanceTest.kt b/core/tests/coretests/src/android/graphics/PaintNativeInstanceTest.kt new file mode 100644 index 000000000000..ac88601a83f0 --- /dev/null +++ b/core/tests/coretests/src/android/graphics/PaintNativeInstanceTest.kt @@ -0,0 +1,161 @@ +/* + * Copyright 2020 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 android.graphics + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import java.util.concurrent.Callable +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +// Verify that various calls to getNativeInstance do not deadlock or otherwise fail. +@RunWith(AndroidJUnit4::class) +class PaintNativeInstanceTest { + + // Force a GC after each test, so that if there was a double free, it would happen now, rather + // than later during other tests. + @After + fun runGcAndFinalizersSync() { + Runtime.getRuntime().gc() + Runtime.getRuntime().runFinalization() + val fence = CountDownLatch(1) + object : Any() { + @Throws(Throwable::class) + protected fun finalize() = fence.countDown() + } + try { + do { + Runtime.getRuntime().gc() + Runtime.getRuntime().runFinalization() + } while (!fence.await(100, TimeUnit.MILLISECONDS)) + } catch (ex: InterruptedException) { + throw RuntimeException(ex) + } + } + + private fun setupComposeShader(test: (Paint, ComposeShader, Shader, Shader) -> Unit) { + val size = 255f + val blue = LinearGradient(0f, 0f, size, 0f, Color.GREEN, Color.BLUE, + Shader.TileMode.MIRROR) + val red = LinearGradient(0f, 0f, 0f, size, Color.GREEN, Color.RED, + Shader.TileMode.MIRROR) + val compose = ComposeShader(blue, red, BlendMode.SCREEN) + val paint = Paint().apply { + shader = compose + } + test(paint, compose, blue, red) + } + + // Change the matrix arbitrarily to invalidate the shader. + private fun Shader.changeMatrix() { + val matrix = Matrix().apply { + setScale(2f, 2f) + } + setLocalMatrix(matrix) + } + + @Test + fun testUnchangedPaintNativeInstance() = setupComposeShader { + paint, compose, shaderA, shaderB -> + val nativeInstance = paint.nativeInstance + for (shader in listOf(compose, shaderA, shaderB)) { + shader.changeMatrix() + // Although the shader is invalidated, the Paint's nativeInstance remains the same. + assertEquals(nativeInstance, paint.nativeInstance) + } + } + + @Test + fun testInvalidateSubShader() = setupComposeShader { + paint, compose, shaderA, shaderB -> + // Trigger the creation of native objects. + shaderA.nativeInstance + compose.nativeInstance + val instanceB = shaderB.nativeInstance + + // Changing shaderA's matrix invalidates shaderA and compose. A new instance will be lazily + // created for each of them. We cannot assert that the new nativeInstance does not match, + // since it might be allocated at the same location. But we can verify that shaderB did not + // change, and that there was no deadlock. + shaderA.changeMatrix() + assertEquals(instanceB, shaderB.nativeInstance) + paint.nativeInstance + } + + @Test + fun testInvalidateSubShaderDraw() = setupComposeShader { + paint, _, _, shaderB -> + + val original = PaintTask(paint).call() + + // Change one of the subshaders and verify that the paint now draws differently. + shaderB.changeMatrix() + val changed = PaintTask(paint).call() + assertFalse(changed.sameAs(original)) + } + + /* + * This task will trigger the creation of native objects, if they have not already been + * created. + */ + class PaintTask(private val mPaint: Paint) : Callable<Bitmap> { + private val size = 255 // matches size of gradients in setupComposeShader + override fun call(): Bitmap = Bitmap.createBitmap(size, size, + Bitmap.Config.ARGB_8888).apply { + val canvas = Canvas(this) + canvas.drawPaint(mPaint) + } + } + + @Test + fun testMultiThreadShader() = setupComposeShader { + paint, _, _, _ -> + // Create an arbitrary number of tasks and try to start them at approximately the same time. + // They will race to create the native objects, but this should be safe. + val tasks = List(5) { PaintTask(paint) } + val results = Executors.newCachedThreadPool().invokeAll(tasks) + var expectedBitmap: Bitmap? = null + for (result in results) { + if (expectedBitmap == null) { + expectedBitmap = result.get() + } else { + assertTrue(expectedBitmap.sameAs(result.get())) + } + } + } + + @Test + fun testMultiThreadColorFilter() { + val paint = Paint().apply { + color = Color.MAGENTA + colorFilter = LightingColorFilter(Color.BLUE, Color.GREEN) + } + // Create an arbitrary number of tasks and try to start them at approximately the same time. + // They will race to create the native objects, but this should be safe. + val tasks = List(5) { PaintTask(paint) } + val results = Executors.newCachedThreadPool().invokeAll(tasks) + for (result in results) { + assertEquals(Color.CYAN, result.get().getPixel(0, 0)) + } + } +} diff --git a/graphics/java/android/graphics/ColorFilter.java b/graphics/java/android/graphics/ColorFilter.java index 4c2ef84404e2..8fd6f7f609c6 100644 --- a/graphics/java/android/graphics/ColorFilter.java +++ b/graphics/java/android/graphics/ColorFilter.java @@ -48,7 +48,7 @@ public class ColorFilter { return 0; } - void discardNativeInstance() { + synchronized final void discardNativeInstance() { if (mNativeInstance != 0) { mCleaner.run(); mCleaner = null; @@ -57,7 +57,7 @@ public class ColorFilter { } /** @hide */ - public long getNativeInstance() { + public synchronized final long getNativeInstance() { if (mNativeInstance == 0) { mNativeInstance = createNativeInstance(); diff --git a/graphics/java/android/graphics/ComposeShader.java b/graphics/java/android/graphics/ComposeShader.java index 279e2937a80a..b840f3f7654d 100644 --- a/graphics/java/android/graphics/ComposeShader.java +++ b/graphics/java/android/graphics/ComposeShader.java @@ -95,13 +95,9 @@ public class ComposeShader extends Shader { /** @hide */ @Override - protected void verifyNativeInstance() { - if (mShaderA.getNativeInstance() != mNativeInstanceShaderA - || mShaderB.getNativeInstance() != mNativeInstanceShaderB) { - // Child shader native instance has been updated, - // so our cached native instance is no longer valid - discard it - discardNativeInstance(); - } + protected boolean shouldDiscardNativeInstance() { + return mShaderA.getNativeInstance() != mNativeInstanceShaderA + || mShaderB.getNativeInstance() != mNativeInstanceShaderB; } private static native long nativeCreate(long nativeMatrix, diff --git a/graphics/java/android/graphics/Paint.java b/graphics/java/android/graphics/Paint.java index 3b586242e5b1..28d7911c771f 100644 --- a/graphics/java/android/graphics/Paint.java +++ b/graphics/java/android/graphics/Paint.java @@ -674,10 +674,15 @@ public class Paint { * Return the pointer to the native object while ensuring that any * mutable objects that are attached to the paint are also up-to-date. * + * Note: Although this method is |synchronized|, this is simply so it + * is not thread-hostile to multiple threads calling this method. It + * is still unsafe to attempt to change the Shader/ColorFilter while + * another thread attempts to access the native object. + * * @hide */ @UnsupportedAppUsage - public long getNativeInstance() { + public synchronized long getNativeInstance() { long newNativeShader = mShader == null ? 0 : mShader.getNativeInstance(); if (newNativeShader != mNativeShader) { mNativeShader = newNativeShader; diff --git a/graphics/java/android/graphics/Shader.java b/graphics/java/android/graphics/Shader.java index fb15d0794dd7..8154ebf1e508 100644 --- a/graphics/java/android/graphics/Shader.java +++ b/graphics/java/android/graphics/Shader.java @@ -150,7 +150,12 @@ public class Shader { /** * @hide Only to be used by subclasses in the graphics package. */ - protected final void discardNativeInstance() { + protected synchronized final void discardNativeInstance() { + discardNativeInstanceLocked(); + } + + // For calling inside a synchronized method. + private void discardNativeInstanceLocked() { if (mNativeInstance != 0) { mCleaner.run(); mCleaner = null; @@ -159,11 +164,12 @@ public class Shader { } /** - * Callback for subclasses to call {@link #discardNativeInstance()} if the most recently - * constructed native instance is no longer valid. + * Callback for subclasses to specify whether the most recently + * constructed native instance is still valid. * @hide Only to be used by subclasses in the graphics package. */ - protected void verifyNativeInstance() { + protected boolean shouldDiscardNativeInstance() { + return false; } @@ -171,9 +177,10 @@ public class Shader { * @hide so it can be called by android.graphics.drawable but must not be called from outside * the module. */ - public final long getNativeInstance() { - // verify mNativeInstance is valid - verifyNativeInstance(); + public synchronized final long getNativeInstance() { + if (shouldDiscardNativeInstance()) { + discardNativeInstanceLocked(); + } if (mNativeInstance == 0) { mNativeInstance = createNativeInstance(mLocalMatrix == null |