Merge "Increase timeout on VibrationThread to wait for vibrator callbacks" into sc-dev

services/core/java/com/android/server/vibrator/VibrationThread.java

@@ -59,7 +59,7 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
      * Extra timeout added to the end of each vibration step to ensure it finishes even when
      * vibrator callbacks are lost.
      */
-    private static final long CALLBACKS_EXTRA_TIMEOUT = 100;
+    private static final long CALLBACKS_EXTRA_TIMEOUT = 1_000;
 
     /** Threshold to prevent the ramp off steps from trying to set extremely low amplitudes. */
     private static final float RAMP_OFF_AMPLITUDE_MIN = 1e-3f;
@@ -341,6 +341,8 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
         private final PriorityQueue<Step> mNextSteps = new PriorityQueue<>();
         @GuardedBy("mLock")
         private final Queue<Step> mPendingOnVibratorCompleteSteps = new LinkedList<>();
+        @GuardedBy("mLock")
+        private final Queue<Integer> mNotifiedVibrators = new LinkedList<>();
 
         @GuardedBy("mLock")
         private int mPendingVibrateSteps;
@@ -348,6 +350,8 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
         private int mConsumedStartVibrateSteps;
         @GuardedBy("mLock")
         private int mSuccessfulVibratorOnSteps;
+        @GuardedBy("mLock")
+        private boolean mWaitToProcessVibratorCallbacks;
 
         public void offer(@NonNull Step step) {
             synchronized (mLock) {
@@ -398,30 +402,122 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
         /**
          * Play and remove the step at the top of this queue, and also adds the next steps generated
          * to be played next.
-         *
-         * @return the number of steps played
          */
         public void consumeNext() {
-            Step nextStep = pollNext();
+            // Vibrator callbacks should wait until the polled step is played and the next steps are
-            if (nextStep != null) {
+            // added back to the queue, so they can handle the callback.
-                // This might turn on the vibrator and have a HAL latency. Execute this outside any
+            markWaitToProcessVibratorCallbacks();
-                // lock to avoid blocking other interactions with the thread.
+            try {
-                List<Step> nextSteps = nextStep.play();
+                Step nextStep = pollNext();
-                synchronized (mLock) {
+                if (nextStep != null) {
-                    if (nextStep.getVibratorOnDuration() > 0) {
+                    // This might turn on the vibrator and have a HAL latency. Execute this outside
-                        mSuccessfulVibratorOnSteps++;
+                    // any lock to avoid blocking other interactions with the thread.
+                    List<Step> nextSteps = nextStep.play();
+                    synchronized (mLock) {
+                        if (nextStep.getVibratorOnDuration() > 0) {
+                            mSuccessfulVibratorOnSteps++;
+                        }
+                        if (nextStep instanceof StartVibrateStep) {
+                            mConsumedStartVibrateSteps++;
+                        }
+                        if (!nextStep.isCleanUp()) {
+                            mPendingVibrateSteps--;
+                        }
+                        for (int i = 0; i < nextSteps.size(); i++) {
+                            mPendingVibrateSteps += nextSteps.get(i).isCleanUp() ? 0 : 1;
+                        }
+                        mNextSteps.addAll(nextSteps);
                     }
-                    if (nextStep instanceof StartVibrateStep) {
-                        mConsumedStartVibrateSteps++;
-                    }
-                    if (!nextStep.isCleanUp()) {
-                        mPendingVibrateSteps--;
-                    }
-                    for (int i = 0; i < nextSteps.size(); i++) {
-                        mPendingVibrateSteps += nextSteps.get(i).isCleanUp() ? 0 : 1;
-                    }
-                    mNextSteps.addAll(nextSteps);
                 }
+            } finally {
+                synchronized (mLock) {
+                    processVibratorCallbacks();
+                }
+            }
+        }
+
+        /**
+         * Notify the vibrator completion.
+         *
+         * <p>This is a lightweight method that do not trigger any operation from {@link
+         * VibratorController}, so it can be called directly from a native callback.
+         */
+        @GuardedBy("mLock")
+        public void notifyVibratorComplete(int vibratorId) {
+            mNotifiedVibrators.offer(vibratorId);
+            if (!mWaitToProcessVibratorCallbacks) {
+                // No step is being played or cancelled now, process the callback right away.
+                processVibratorCallbacks();
+            }
+        }
+
+        /**
+         * Cancel the current queue, replacing all remaining steps with respective clean-up steps.
+         *
+         * <p>This will remove all steps and replace them with respective
+         * {@link Step#cancel()}.
+         */
+        public void cancel() {
+            // Vibrator callbacks should wait until all steps from the queue are properly cancelled
+            // and clean up steps are added back to the queue, so they can handle the callback.
+            markWaitToProcessVibratorCallbacks();
+            try {
+                List<Step> cleanUpSteps = new ArrayList<>();
+                Step step;
+                while ((step = pollNext()) != null) {
+                    cleanUpSteps.addAll(step.cancel());
+                }
+                synchronized (mLock) {
+                    // All steps generated by Step.cancel() should be clean-up steps.
+                    mPendingVibrateSteps = 0;
+                    mNextSteps.addAll(cleanUpSteps);
+                }
+            } finally {
+                synchronized (mLock) {
+                    processVibratorCallbacks();
+                }
+            }
+        }
+
+        /**
+         * Cancel the current queue immediately, clearing all remaining steps and skipping clean-up.
+         *
+         * <p>This will remove and trigger {@link Step#cancelImmediately()} in all steps, in order.
+         */
+        public void cancelImmediately() {
+            // Vibrator callbacks should wait until all steps from the queue are properly cancelled.
+            markWaitToProcessVibratorCallbacks();
+            try {
+                Step step;
+                while ((step = pollNext()) != null) {
+                    // This might turn off the vibrator and have a HAL latency. Execute this outside
+                    // any lock to avoid blocking other interactions with the thread.
+                    step.cancelImmediately();
+                }
+                synchronized (mLock) {
+                    mPendingVibrateSteps = 0;
+                }
+            } finally {
+                synchronized (mLock) {
+                    processVibratorCallbacks();
+                }
+            }
+        }
+
+        @Nullable
+        private Step pollNext() {
+            synchronized (mLock) {
+                // Prioritize the steps anticipated by a vibrator complete callback.
+                if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
+                    return mPendingOnVibratorCompleteSteps.poll();
+                }
+                return mNextSteps.poll();
+            }
+        }
+
+        private void markWaitToProcessVibratorCallbacks() {
+            synchronized (mLock) {
+                mWaitToProcessVibratorCallbacks = true;
             }
         }
 
@@ -436,64 +532,21 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
          * first step found will be anticipated by this method, in no particular order.
          */
         @GuardedBy("mLock")
-        public void notifyVibratorComplete(int vibratorId) {
+        private void processVibratorCallbacks() {
-            Iterator<Step> it = mNextSteps.iterator();
+            mWaitToProcessVibratorCallbacks = false;
-            while (it.hasNext()) {
+            while (!mNotifiedVibrators.isEmpty()) {
-                Step step = it.next();
+                int vibratorId = mNotifiedVibrators.poll();
-                if (step.shouldPlayWhenVibratorComplete(vibratorId)) {
+                Iterator<Step> it = mNextSteps.iterator();
-                    it.remove();
+                while (it.hasNext()) {
-                    mPendingOnVibratorCompleteSteps.offer(step);
+                    Step step = it.next();
-                    break;
+                    if (step.shouldPlayWhenVibratorComplete(vibratorId)) {
+                        it.remove();
+                        mPendingOnVibratorCompleteSteps.offer(step);
+                        break;
+                    }
                 }
             }
         }
-
-        /**
-         * Cancel the current queue, replacing all remaining steps with respective clean-up steps.
-         *
-         * <p>This will remove all steps and replace them with respective
-         * {@link Step#cancel()}.
-         */
-        public void cancel() {
-            List<Step> cleanUpSteps = new ArrayList<>();
-            Step step;
-            while ((step = pollNext()) != null) {
-                cleanUpSteps.addAll(step.cancel());
-            }
-            synchronized (mLock) {
-                // All steps generated by Step.cancel() should be clean-up steps.
-                mPendingVibrateSteps = 0;
-                mNextSteps.addAll(cleanUpSteps);
-            }
-        }
-
-        /**
-         * Cancel the current queue immediately, clearing all remaining steps and skipping clean-up.
-         *
-         * <p>This will remove and trigger {@link Step#cancelImmediately()} in all steps, in order.
-         */
-        public void cancelImmediately() {
-            Step step;
-            while ((step = pollNext()) != null) {
-                // This might turn off the vibrator and have a HAL latency. Execute this outside
-                // any lock to avoid blocking other interactions with the thread.
-                step.cancelImmediately();
-            }
-            synchronized (mLock) {
-                mPendingVibrateSteps = 0;
-            }
-        }
-
-        @Nullable
-        private Step pollNext() {
-            synchronized (mLock) {
-                // Prioritize the steps anticipated by a vibrator complete callback.
-                if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
-                    return mPendingOnVibratorCompleteSteps.poll();
-                }
-                return mNextSteps.poll();
-            }
-        }
     }
 
     /**
@@ -894,9 +947,9 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
                 // Vibration was not started, so just skip the played segments and keep timings.
                 return skipToNextSteps(segmentsPlayed);
             }
-            long nextVibratorOffTimeout =
+            long nextStartTime = SystemClock.uptimeMillis() + mVibratorOnResult;
-                    SystemClock.uptimeMillis() + mVibratorOnResult + CALLBACKS_EXTRA_TIMEOUT;
+            long nextVibratorOffTimeout = nextStartTime + CALLBACKS_EXTRA_TIMEOUT;
-            return nextSteps(nextVibratorOffTimeout, nextVibratorOffTimeout, segmentsPlayed);
+            return nextSteps(nextStartTime, nextVibratorOffTimeout, segmentsPlayed);
         }
 
         /**

services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java

@@ -58,6 +58,7 @@ import androidx.test.InstrumentationRegistry;
 
 import com.android.internal.app.IBatteryStats;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -106,6 +107,7 @@ public class VibrationThreadTest {
     private DeviceVibrationEffectAdapter mEffectAdapter;
     private PowerManager.WakeLock mWakeLock;
     private TestLooper mTestLooper;
+    private TestLooperAutoDispatcher mCustomTestLooperDispatcher;
 
     @Before
     public void setUp() throws Exception {
@@ -121,6 +123,13 @@ public class VibrationThreadTest {
         mockVibrators(VIBRATOR_ID);
     }
 
+    @After
+    public void tearDown() {
+        if (mCustomTestLooperDispatcher != null) {
+            mCustomTestLooperDispatcher.cancel();
+        }
+    }
+
     @Test
     public void vibrate_noVibrator_ignoresVibration() {
         mVibratorProviders.clear();
@@ -508,7 +517,7 @@ public class VibrationThreadTest {
                 .addPrimitive(VibrationEffect.Composition.PRIMITIVE_CLICK, 1f)
                 .addPrimitive(VibrationEffect.Composition.PRIMITIVE_TICK, 0.5f)
                 .addEffect(VibrationEffect.get(VibrationEffect.EFFECT_CLICK))
-                .addEffect(VibrationEffect.get(VibrationEffect.EFFECT_CLICK), 10)
+                .addEffect(VibrationEffect.get(VibrationEffect.EFFECT_CLICK), /* delay= */ 100)
                 .compose();
         VibrationThread thread = startThreadAndDispatcher(vibrationId, effect);
         waitForCompletion(thread);
@@ -1290,7 +1299,9 @@ public class VibrationThreadTest {
             thread.vibratorComplete(answer.getArgument(0));
             return null;
         }).when(mControllerCallbacks).onComplete(anyInt(), eq(vib.id));
-        mTestLooper.startAutoDispatch();
+        // TestLooper.AutoDispatchThread has a fixed 1s duration. Use a custom auto-dispatcher.
+        mCustomTestLooperDispatcher = new TestLooperAutoDispatcher(mTestLooper);
+        mCustomTestLooperDispatcher.start();
         thread.start();
         return thread;
     }
@@ -1316,6 +1327,7 @@ public class VibrationThreadTest {
         } catch (InterruptedException e) {
         }
         assertFalse(thread.isAlive());
+        mCustomTestLooperDispatcher.cancel();
         mTestLooper.dispatchAll();
     }
 
@@ -1364,4 +1376,29 @@ public class VibrationThreadTest {
         verify(mThreadCallbacks).onVibrationCompleted(eq(vibrationId), eq(expectedStatus));
         verify(mThreadCallbacks).onVibratorsReleased();
     }
+
+    private final class TestLooperAutoDispatcher extends Thread {
+        private final TestLooper mTestLooper;
+        private boolean mCancelled;
+
+        TestLooperAutoDispatcher(TestLooper testLooper) {
+            mTestLooper = testLooper;
+        }
+
+        @Override
+        public void run() {
+            while (!mCancelled) {
+                mTestLooper.dispatchAll();
+                try {
+                    Thread.sleep(10);
+                } catch (InterruptedException e) {
+                    return;
+                }
+            }
+        }
+
+        public void cancel() {
+            mCancelled = true;
+        }
+    }
 }