From 2cf250c0872687cd3443fa1b8f6b7707625d7a05 Mon Sep 17 00:00:00 2001 From: ramindani Date: Wed, 1 Feb 2023 14:54:19 -0800 Subject: [PATCH] Error handling when looper is different for the getChoreographer than originally sourced. getChoreographer without the looper will return the cached Choreographer instance. Test: atest ChoreographerTests BUG: 258235754 Change-Id: I02ca535730627d7ca932ebeedb211f102c6aa0e0 --- core/java/android/view/Choreographer.java | 8 ++++ core/java/android/view/SurfaceControl.java | 42 ++++++++++++------- .../AttachedChoreographerTest.java | 36 +++++++++++++--- 3 files changed, 65 insertions(+), 21 deletions(-) diff --git a/core/java/android/view/Choreographer.java b/core/java/android/view/Choreographer.java index 3dc79cf13422f..d0acedb3437b9 100644 --- a/core/java/android/view/Choreographer.java +++ b/core/java/android/view/Choreographer.java @@ -366,6 +366,14 @@ public final class Choreographer { dispose(); } + /** + * Check if the sourced looper and the current looper are same. + * @hide + */ + boolean isTheLooperSame(Looper looper) { + return mLooper == looper; + } + /** * The amount of time, in milliseconds, between each frame of the animation. *

diff --git a/core/java/android/view/SurfaceControl.java b/core/java/android/view/SurfaceControl.java index 4ff86d33ea491..e2316030b826b 100644 --- a/core/java/android/view/SurfaceControl.java +++ b/core/java/android/view/SurfaceControl.java @@ -1274,27 +1274,36 @@ public final class SurfaceControl implements Parcelable { } /** - * Returns the associated {@link Choreographer} instance with the - * current instance of the SurfaceControl. - * Must be called from a thread that already has a {@link android.os.Looper} - * associated with it. - * If there is no {@link Choreographer} associated with the SurfaceControl then a new instance - * of the {@link Choreographer} is created. + * When called for the first time a new instance of the {@link Choreographer} is created + * with a {@link android.os.Looper} of the current thread. Every subsequent call will return + * the same instance of the Choreographer. + * + * @see #getChoreographer(Looper) to create Choreographer with a different + * looper than current thread looper. * * @hide */ @TestApi public @NonNull Choreographer getChoreographer() { - return getChoreographer(Looper.myLooper()); + checkNotReleased(); + synchronized (mChoreographerLock) { + if (mChoreographer == null) { + return getChoreographer(Looper.myLooper()); + } + return mChoreographer; + } } /** - * Returns the associated {@link Choreographer} instance with the - * current instance of the SurfaceControl. - * If there is no {@link Choreographer} associated with the SurfaceControl then a new instance - * of the {@link Choreographer} is created. + * When called for the first time a new instance of the {@link Choreographer} is created with + * the sourced {@link android.os.Looper}. Every subsequent call will return the same + * instance of the Choreographer. * - * @param looper the choreographer is attached on this looper + * @see #getChoreographer() + * + * @throws IllegalStateException when a {@link Choreographer} instance exists with a different + * looper than sourced. + * @param looper the choreographer is attached on this looper. * * @hide */ @@ -1302,11 +1311,12 @@ public final class SurfaceControl implements Parcelable { public @NonNull Choreographer getChoreographer(@NonNull Looper looper) { checkNotReleased(); synchronized (mChoreographerLock) { - if (mChoreographer != null) { - return mChoreographer; + if (mChoreographer == null) { + mChoreographer = Choreographer.getInstanceForSurfaceControl(mNativeHandle, looper); + } else if (!mChoreographer.isTheLooperSame(looper)) { + throw new IllegalStateException( + "Choreographer already exists with a different looper"); } - - mChoreographer = Choreographer.getInstanceForSurfaceControl(mNativeHandle, looper); return mChoreographer; } } diff --git a/tests/ChoreographerTests/src/main/java/android/view/choreographertests/AttachedChoreographerTest.java b/tests/ChoreographerTests/src/main/java/android/view/choreographertests/AttachedChoreographerTest.java index 3ea9651045fac..48d050ce43914 100644 --- a/tests/ChoreographerTests/src/main/java/android/view/choreographertests/AttachedChoreographerTest.java +++ b/tests/ChoreographerTests/src/main/java/android/view/choreographertests/AttachedChoreographerTest.java @@ -16,7 +16,9 @@ package android.view.choreographertests; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -63,6 +65,7 @@ public class AttachedChoreographerTest { private DisplayManager mDisplayManager; private SurfaceView mSurfaceView; private SurfaceHolder mSurfaceHolder; + private Choreographer mChoreographer; private boolean mIsFirstCallback = true; private int mCallbackMissedCounter = 0; @@ -127,37 +130,60 @@ public class AttachedChoreographerTest { @Test public void testCreateChoreographer() { + Looper testLooper = Looper.myLooper(); mScenario.onActivity(activity -> { if (waitForCountDown(mSurfaceCreationCountDown, /* timeoutInSeconds */ 1L)) { fail("Unable to create surface within 1 Second"); } SurfaceControl sc = mSurfaceView.getSurfaceControl(); + mChoreographer = sc.getChoreographer(); mTestCompleteSignal.countDown(); SurfaceControl sc1 = new SurfaceControl(sc, "AttachedChoreographerTests"); // Create attached choreographer with getChoreographer - sc1.getChoreographer(); + Choreographer choreographer1 = sc1.getChoreographer(); assertTrue(sc1.hasChoreographer()); assertTrue(sc1.isValid()); - sc1.release(); + assertEquals(choreographer1, sc1.getChoreographer()); + assertEquals(choreographer1, sc1.getChoreographer(Looper.myLooper())); + assertEquals(choreographer1, sc1.getChoreographer(Looper.getMainLooper())); + assertThrows(IllegalStateException.class, () -> sc1.getChoreographer(testLooper)); SurfaceControl sc2 = new SurfaceControl(sc, "AttachedChoreographerTests"); // Create attached choreographer with Looper.myLooper - sc2.getChoreographer(Looper.myLooper()); + Choreographer choreographer2 = sc2.getChoreographer(Looper.myLooper()); assertTrue(sc2.hasChoreographer()); assertTrue(sc2.isValid()); - sc2.release(); + assertEquals(choreographer2, sc2.getChoreographer(Looper.myLooper())); + assertEquals(choreographer2, sc2.getChoreographer(Looper.getMainLooper())); + assertEquals(choreographer2, sc2.getChoreographer()); + assertThrows(IllegalStateException.class, () -> sc2.getChoreographer(testLooper)); SurfaceControl sc3 = new SurfaceControl(sc, "AttachedChoreographerTests"); // Create attached choreographer with Looper.myLooper - sc3.getChoreographer(Looper.getMainLooper()); + Choreographer choreographer3 = sc3.getChoreographer(Looper.getMainLooper()); assertTrue(sc3.hasChoreographer()); assertTrue(sc3.isValid()); + assertEquals(choreographer3, sc3.getChoreographer(Looper.getMainLooper())); + assertEquals(choreographer3, sc3.getChoreographer(Looper.myLooper())); + assertEquals(choreographer3, sc3.getChoreographer()); + assertThrows(IllegalStateException.class, () -> sc3.getChoreographer(testLooper)); + + assertNotEquals(choreographer1, choreographer2); + assertNotEquals(choreographer1, choreographer3); + assertNotEquals(choreographer2, choreographer3); + sc1.release(); + sc2.release(); sc3.release(); mTestCompleteSignal.countDown(); }); if (waitForCountDown(mTestCompleteSignal, /* timeoutInSeconds */ 2L)) { fail("Test not finished in 2 Seconds"); } + SurfaceControl surfaceControl = mSurfaceView.getSurfaceControl(); + assertTrue(surfaceControl.hasChoreographer()); + assertEquals(mChoreographer, surfaceControl.getChoreographer()); + assertThrows(IllegalStateException.class, + () -> surfaceControl.getChoreographer(testLooper)); } @Test