Cancel current vibration asynchronously in VibratorService

The vibration is now played asynchronously by the VibrationThread, but
cancelling the current vibration before playing a new one (and turning
off the vibrator HAL) is still being done synchronously in the vibrate
method.

This change makes that step also async, handled only by the
VibrationThread.cancel method. The new vibration is only started once
the previous one has completely finished.

Other fixes:

- Cancelling a vibration and playing another one in sequence was causing
the second one to be interrupted by the VibrationThread asynchronous
cancellation (cts for multi-thread was flaky). Now the service is only
starting the second vibration after the first one was completely cancelled.

- VibrationStep was waiting for the entire callback timeout to finish
instead of finishing early after native callback (see vibration async
trace with duration >100ms in b/178179597 perfetto traces). It is now
waiting on a boolean condition instead, to follow the Object.wait
documentation.

- Using a mLock object explicitly to synchronize, wait and notify in the
VibrationThread, instead of using the thread outer instance.

Bug: 178179597
Bug: 178029288
Test: VibratorServiceTest
Change-Id: Ied88e6790019a8c174a25e75f29870793e993a78

2 files changed, 142 insertions, 86 deletions

diff --git a/services/core/java/com/android/server/VibratorService.java b/services/core/java/com/android/server/VibratorService.java
index 3e80709d03d6..97e313e13f6d 100644
--- a/services/core/java/com/android/server/VibratorService.java
+++ b/services/core/java/com/android/server/VibratorService.java
@@ -102,7 +102,10 @@ public class VibratorService extends IVibratorService.Stub {
     private VibrationScaler mVibrationScaler;
     private InputDeviceDelegate mInputDeviceDelegate;
 
-    private volatile VibrationThread mThread;
+    @GuardedBy("mLock")
+    private VibrationThread mThread;
+    @GuardedBy("mLock")
+    private VibrationThread mNextVibrationThread;
 
     @GuardedBy("mLock")
     private Vibration mCurrentVibration;
@@ -132,6 +135,10 @@ public class VibratorService extends IVibratorService.Stub {
                 if (mCurrentVibration != null && mCurrentVibration.id == vibrationId) {
                     mThread = null;
                     reportFinishVibrationLocked(status);
+                    if (mNextVibrationThread != null) {
+                        startVibrationThreadLocked(mNextVibrationThread);
+                        mNextVibrationThread = null;
+                    }
                 }
             }
         }
@@ -258,18 +265,14 @@ public class VibratorService extends IVibratorService.Stub {
     @VisibleForTesting
     public void onVibrationComplete(int vibratorId, long vibrationId) {
         synchronized (mLock) {
-            if (mCurrentVibration != null && mCurrentVibration.id == vibrationId) {
+            if (mCurrentVibration != null && mCurrentVibration.id == vibrationId
+                    && mThread != null) {
                 if (DEBUG) {
                     Slog.d(TAG, "Vibration onComplete callback, notifying VibrationThread");
                 }
-                if (mThread != null) {
-                    // Let the thread playing the vibration handle the callback, since it might be
-                    // expecting the vibrator to turn off multiple times during a single vibration.
-                    mThread.vibratorComplete(vibratorId);
-                } else {
-                    // No vibration is playing in the thread, but clean up service just in case.
-                    doCancelVibrateLocked(Vibration.Status.FINISHED);
-                }
+                // Let the thread playing the vibration handle the callback, since it might be
+                // expecting the vibrator to turn off multiple times during a single vibration.
+                mThread.vibratorComplete(vibratorId);
             }
         }
     }
@@ -462,8 +465,10 @@ public class VibratorService extends IVibratorService.Stub {
                 try {
                     doCancelVibrateLocked(Vibration.Status.CANCELLED);
                     startVibrationLocked(vib);
+                    boolean isNextVibration = mNextVibrationThread != null
+                            && vib.equals(mNextVibrationThread.getVibration());
 
-                    if (!vib.hasEnded() && mCurrentVibration.id != vib.id) {
+                    if (!vib.hasEnded() && !vib.equals(mCurrentVibration) && !isNextVibration) {
                         // Vibration was unexpectedly ignored: add to list for debugging
                         endVibrationLocked(vib, Vibration.Status.IGNORED);
                     }
@@ -532,6 +537,7 @@ public class VibratorService extends IVibratorService.Stub {
                 }
                 final long ident = Binder.clearCallingIdentity();
                 try {
+                    mNextVibrationThread = null;
                     doCancelVibrateLocked(Vibration.Status.CANCELLED);
                 } finally {
                     Binder.restoreCallingIdentity(ident);
@@ -546,16 +552,14 @@ public class VibratorService extends IVibratorService.Stub {
         try {
             if (mThread != null) {
                 mThread.cancel();
-                mThread = null;
             }
+            mInputDeviceDelegate.cancelVibrateIfAvailable();
             if (mCurrentExternalVibration != null) {
                 endVibrationLocked(mCurrentExternalVibration, status);
                 mCurrentExternalVibration.externalVibration.mute();
                 mCurrentExternalVibration = null;
                 mVibratorController.setExternalControl(false);
             }
-            doVibratorOff();
-            reportFinishVibrationLocked(status);
         } finally {
             Trace.traceEnd(Trace.TRACE_TAG_VIBRATOR);
         }
@@ -579,28 +583,30 @@ public class VibratorService extends IVibratorService.Stub {
     private void startVibrationInnerLocked(Vibration vib) {
         Trace.traceBegin(Trace.TRACE_TAG_VIBRATOR, "startVibrationInnerLocked");
         try {
-            // Set current vibration before starting it, so callback will work.
-            mCurrentVibration = vib;
-            VibrationEffect effect = getEffect(vib);
-            Trace.asyncTraceBegin(Trace.TRACE_TAG_VIBRATOR, "vibration", 0);
             boolean inputDevicesAvailable = mInputDeviceDelegate.vibrateIfAvailable(
                     vib.uid, vib.opPkg, vib.getEffect(), vib.reason, vib.attrs);
             if (inputDevicesAvailable) {
-                // The set current vibration is no longer being played by this service, so drop it.
-                mCurrentVibration = null;
                 endVibrationLocked(vib, Vibration.Status.FORWARDED_TO_INPUT_DEVICES);
+            } else if (mThread == null) {
+                startVibrationThreadLocked(new VibrationThread(vib, mVibratorController, mWakeLock,
+                        mBatteryStatsService, mVibrationCallbacks));
             } else {
-                // mThread better be null here. doCancelVibrate should always be
-                // called before startVibrationInnerLocked
-                mThread = new VibrationThread(vib, mVibratorController, mWakeLock,
+                mNextVibrationThread = new VibrationThread(vib, mVibratorController, mWakeLock,
                         mBatteryStatsService, mVibrationCallbacks);
-                mThread.start();
             }
         } finally {
             Trace.traceEnd(Trace.TRACE_TAG_VIBRATOR);
         }
     }
 
+    @GuardedBy("mLock")
+    private void startVibrationThreadLocked(VibrationThread thread) {
+        Trace.asyncTraceBegin(Trace.TRACE_TAG_VIBRATOR, "vibration", 0);
+        mCurrentVibration = thread.getVibration();
+        mThread = thread;
+        mThread.start();
+    }
+
     /** Scale the vibration effect by the intensity as appropriate based its intent. */
     private void applyVibrationIntensityScalingLocked(Vibration vib) {
         vib.updateEffect(mVibrationScaler.scale(vib.getEffect(), vib.attrs.getUsage()));
@@ -665,13 +671,14 @@ public class VibratorService extends IVibratorService.Stub {
 
     @GuardedBy("mLock")
     private void reportFinishVibrationLocked(Vibration.Status status) {
-        Trace.asyncTraceEnd(Trace.TRACE_TAG_VIBRATOR, "vibration", 0);
         Trace.traceBegin(Trace.TRACE_TAG_VIBRATOR, "reportFinishVibrationLocked");
         try {
             if (mCurrentVibration != null) {
                 endVibrationLocked(mCurrentVibration, status);
                 mAppOps.finishOp(AppOpsManager.OP_VIBRATE, mCurrentVibration.uid,
                         mCurrentVibration.opPkg);
+
+                Trace.asyncTraceEnd(Trace.TRACE_TAG_VIBRATOR, "vibration", 0);
                 mCurrentVibration = null;
             }
         } finally {
@@ -697,21 +704,6 @@ public class VibratorService extends IVibratorService.Stub {
         }
     }
 
-    private void doVibratorOff() {
-        Trace.traceBegin(Trace.TRACE_TAG_VIBRATOR, "doVibratorOff");
-        try {
-            if (DEBUG) {
-                Slog.d(TAG, "Turning vibrator off.");
-            }
-            boolean inputDevicesAvailable = mInputDeviceDelegate.cancelVibrateIfAvailable();
-            if (!inputDevicesAvailable) {
-                mVibratorController.off();
-            }
-        } finally {
-            Trace.traceEnd(Trace.TRACE_TAG_VIBRATOR);
-        }
-    }
-
     private boolean isSystemHapticFeedback(Vibration vib) {
         if (vib.attrs.getUsage() != VibrationAttributes.USAGE_TOUCH) {
             return false;
@@ -844,6 +836,7 @@ public class VibratorService extends IVibratorService.Stub {
                     // haptic feedback as part of the transition.  So we don't cancel
                     // system vibrations.
                     if (mCurrentVibration != null && !isSystemHapticFeedback(mCurrentVibration)) {
+                        mNextVibrationThread = null;
                         doCancelVibrateLocked(Vibration.Status.CANCELLED);
                     }
                 }
@@ -910,6 +903,8 @@ public class VibratorService extends IVibratorService.Stub {
                 return IExternalVibratorService.SCALE_MUTE;
             }
 
+            VibrationThread cancelingVibration = null;
+            int scale;
             synchronized (mLock) {
                 if (mCurrentExternalVibration != null
                         && mCurrentExternalVibration.externalVibration.equals(vib)) {
@@ -920,11 +915,9 @@ public class VibratorService extends IVibratorService.Stub {
                 if (mCurrentExternalVibration == null) {
                     // If we're not under external control right now, then cancel any normal
                     // vibration that may be playing and ready the vibrator for external control.
-                    if (DEBUG) {
-                        Slog.d(TAG, "Vibrator going under external control.");
-                    }
+                    mNextVibrationThread = null;
                     doCancelVibrateLocked(Vibration.Status.CANCELLED);
-                    mVibratorController.setExternalControl(true);
+                    cancelingVibration = mThread;
                 } else {
                     endVibrationLocked(mCurrentExternalVibration, Vibration.Status.CANCELLED);
                 }
@@ -941,11 +934,24 @@ public class VibratorService extends IVibratorService.Stub {
                 vib.linkToDeath(mCurrentExternalDeathRecipient);
                 mCurrentExternalVibration.scale = mVibrationScaler.getExternalVibrationScale(
                         vib.getVibrationAttributes().getUsage());
-                if (DEBUG) {
-                    Slog.e(TAG, "Playing external vibration: " + vib);
+                scale = mCurrentExternalVibration.scale;
+            }
+            if (cancelingVibration != null) {
+                try {
+                    cancelingVibration.join();
+                } catch (InterruptedException e) {
+                    Slog.w("Interrupted while waiting current vibration to be cancelled before "
+                            + "starting external vibration", e);
                 }
-                return mCurrentExternalVibration.scale;
             }
+            if (DEBUG) {
+                Slog.d(TAG, "Vibrator going under external control.");
+            }
+            mVibratorController.setExternalControl(true);
+            if (DEBUG) {
+                Slog.e(TAG, "Playing external vibration: " + vib);
+            }
+            return scale;
         }
 
         @Override
diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java
index c36375ef0af5..53552526c936 100644
--- a/services/core/java/com/android/server/vibrator/VibrationThread.java
+++ b/services/core/java/com/android/server/vibrator/VibrationThread.java
@@ -74,6 +74,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
         void onVibrationEnded(long vibrationId, Vibration.Status status);
     }
 
+    private final Object mLock = new Object();
     private final WorkSource mWorkSource = new WorkSource();
     private final PowerManager.WakeLock mWakeLock;
     private final IBatteryStats mBatteryStatsService;
@@ -81,10 +82,10 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
     private final VibrationCallbacks mCallbacks;
     private final SparseArray<VibratorController> mVibrators;
 
-    @GuardedBy("this")
+    @GuardedBy("mLock")
     @Nullable
     private VibrateStep mCurrentVibrateStep;
-    @GuardedBy("this")
+    @GuardedBy("mLock")
     private boolean mForceStop;
 
     // TODO(b/159207608): Remove this constructor once VibratorService is removed
@@ -113,6 +114,10 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
         }
     }
 
+    public Vibration getVibration() {
+        return mVibration;
+    }
+
     @Override
     public void binderDied() {
         cancel();
@@ -136,15 +141,15 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
 
     /** Cancel current vibration and shuts down the thread gracefully. */
     public void cancel() {
-        synchronized (this) {
+        synchronized (mLock) {
             mForceStop = true;
-            notify();
+            mLock.notify();
         }
     }
 
     /** Notify current vibration that a step has completed on given vibrator. */
     public void vibratorComplete(int vibratorId) {
-        synchronized (this) {
+        synchronized (mLock) {
             if (mCurrentVibrateStep != null) {
                 mCurrentVibrateStep.vibratorComplete(vibratorId);
             }
@@ -168,7 +173,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
             final int stepCount = steps.size();
             for (int i = 0; i < stepCount; i++) {
                 Step step = steps.get(i);
-                synchronized (this) {
+                synchronized (mLock) {
                     if (step instanceof VibrateStep) {
                         mCurrentVibrateStep = (VibrateStep) step;
                     } else {
@@ -295,21 +300,48 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
      * Sleeps until given {@code wakeUpTime}.
      *
      * <p>This stops immediately when {@link #cancel()} is called.
+     *
+     * @return true if waited until wake-up time, false if it was cancelled.
      */
-    private void waitUntil(long wakeUpTime) {
-        synchronized (this) {
+    private boolean waitUntil(long wakeUpTime) {
+        synchronized (mLock) {
             long durationRemaining = wakeUpTime - SystemClock.uptimeMillis();
             while (durationRemaining > 0) {
                 try {
-                    VibrationThread.this.wait(durationRemaining);
+                    mLock.wait(durationRemaining);
                 } catch (InterruptedException e) {
                 }
                 if (mForceStop) {
-                    break;
+                    return false;
+                }
+                durationRemaining = wakeUpTime - SystemClock.uptimeMillis();
+            }
+        }
+        return true;
+    }
+
+    /**
+     * Sleeps until given {@link VibrateStep#isVibrationComplete()}, or until {@code wakeUpTime}.
+     *
+     * <p>This stops immediately when {@link #cancel()} is called.
+     *
+     * @return true if finished on vibration complete, false if it was cancelled or timed out.
+     */
+    private boolean waitForVibrationComplete(VibrateStep step, long wakeUpTime) {
+        synchronized (mLock) {
+            long durationRemaining = wakeUpTime - SystemClock.uptimeMillis();
+            while (!step.isVibrationComplete() && durationRemaining > 0) {
+                try {
+                    mLock.wait(durationRemaining);
+                } catch (InterruptedException e) {
+                }
+                if (mForceStop) {
+                    return false;
                 }
                 durationRemaining = wakeUpTime - SystemClock.uptimeMillis();
             }
         }
+        return step.isVibrationComplete();
     }
 
     private void noteVibratorOn(long duration) {
@@ -341,6 +373,9 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
     private interface VibrateStep extends Step {
         /** Callback to notify a vibrator has finished playing a effect. */
         void vibratorComplete(int vibratorId);
+
+        /** Returns true if the vibration played by this step is complete. */
+        boolean isVibrationComplete();
     }
 
     /** Represent a vibration on a single vibrator. */
@@ -348,11 +383,20 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
         private final VibratorController mVibrator;
         private final VibrationEffect mEffect;
 
+        @GuardedBy("mLock")
+        private boolean mVibrationComplete;
+
         SingleVibrateStep(VibratorController vibrator, VibrationEffect effect) {
             mVibrator = vibrator;
             mEffect = effect;
         }
 
+        @GuardedBy("mLock")
+        @Override
+        public boolean isVibrationComplete() {
+            return mVibrationComplete;
+        }
+
         @Override
         public void vibratorComplete(int vibratorId) {
             if (mVibrator.getVibratorInfo().getId() != vibratorId) {
@@ -364,8 +408,9 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
                 return;
             }
             mVibrator.off();
-            synchronized (VibrationThread.this) {
-                VibrationThread.this.notify();
+            synchronized (mLock) {
+                mVibrationComplete = true;
+                mLock.notify();
             }
         }
 
@@ -384,12 +429,13 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
                     noteVibratorOn(duration);
                     // Vibration is playing with no need to control amplitudes, just wait for native
                     // callback or timeout.
-                    waitUntil(startTime + duration + CALLBACKS_EXTRA_TIMEOUT);
-                    if (mForceStop) {
-                        mVibrator.off();
-                        return Vibration.Status.CANCELLED;
+                    if (waitForVibrationComplete(this,
+                            startTime + duration + CALLBACKS_EXTRA_TIMEOUT)) {
+                        return Vibration.Status.FINISHED;
                     }
-                    return Vibration.Status.FINISHED;
+                    // Timed out or vibration cancelled. Stop vibrator anyway.
+                    mVibrator.off();
+                    return mForceStop ? Vibration.Status.CANCELLED : Vibration.Status.FINISHED;
                 }
 
                 startTime = SystemClock.uptimeMillis();
@@ -407,8 +453,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
                     noteVibratorOn(duration);
                 }
                 while (amplitudeStep != null) {
-                    waitUntil(amplitudeStep.startTime);
-                    if (mForceStop) {
+                    if (!waitUntil(amplitudeStep.startTime)) {
                         mVibrator.off();
                         return Vibration.Status.CANCELLED;
                     }
@@ -482,7 +527,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
         private final int mRequiredCapabilities;
         private final int[] mVibratorIds;
 
-        @GuardedBy("VibrationThread.this")
+        @GuardedBy("mLock")
         private int mActiveVibratorCounter;
 
         SyncedVibrateStep(SparseArray<VibrationEffect> effects) {
@@ -496,6 +541,12 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
             }
         }
 
+        @GuardedBy("mLock")
+        @Override
+        public boolean isVibrationComplete() {
+            return mActiveVibratorCounter <= 0;
+        }
+
         @Override
         public void vibratorComplete(int vibratorId) {
             VibrationEffect effect = mEffects.get(vibratorId);
@@ -508,10 +559,9 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
                 return;
             }
             mVibrators.get(vibratorId).off();
-            synchronized (VibrationThread.this) {
-                if (--mActiveVibratorCounter <= 0) {
-                    VibrationThread.this.notify();
-                }
+            synchronized (mLock) {
+                --mActiveVibratorCounter;
+                mLock.notify();
             }
         }
 
@@ -532,8 +582,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
 
                 while (!nextSteps.isEmpty()) {
                     AmplitudeStep step = nextSteps.poll();
-                    waitUntil(step.startTime);
-                    if (mForceStop) {
+                    if (!waitUntil(step.startTime)) {
                         stopAllVibrators();
                         return Vibration.Status.CANCELLED;
                     }
@@ -541,7 +590,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
                     AmplitudeStep nextStep = step.nextStep();
                     if (nextStep == null) {
                         // This vibrator has finished playing the effect for this step.
-                        synchronized (VibrationThread.this) {
+                        synchronized (mLock) {
                             mActiveVibratorCounter--;
                         }
                     } else {
@@ -549,19 +598,18 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
                     }
                 }
 
-                // All OneShot and Waveform effects have finished. Just wait for the other effects
-                // to end via native callbacks before finishing this synced step.
-                synchronized (VibrationThread.this) {
-                    if (mActiveVibratorCounter > 0) {
-                        waitUntil(startTime + timeout + CALLBACKS_EXTRA_TIMEOUT);
+                synchronized (mLock) {
+                    // All OneShot and Waveform effects have finished. Just wait for the other
+                    // effects to end via native callbacks before finishing this synced step.
+                    final long wakeUpTime = startTime + timeout + CALLBACKS_EXTRA_TIMEOUT;
+                    if (mActiveVibratorCounter <= 0 || waitForVibrationComplete(this, wakeUpTime)) {
+                        return Vibration.Status.FINISHED;
                     }
-                }
-                if (mForceStop) {
+
+                    // Timed out or vibration cancelled. Stop all vibrators anyway.
                     stopAllVibrators();
-                    return Vibration.Status.CANCELLED;
+                    return mForceStop ? Vibration.Status.CANCELLED : Vibration.Status.FINISHED;
                 }
-
-                return Vibration.Status.FINISHED;
             } finally {
                 if (timeout > 0) {
                     noteVibratorOff();
@@ -774,8 +822,10 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi
                 if (DEBUG) {
                     Slog.d(TAG, "DelayStep of " + mDelay + "ms starting...");
                 }
-                waitUntil(SystemClock.uptimeMillis() + mDelay);
-                return mForceStop ? Vibration.Status.CANCELLED : Vibration.Status.FINISHED;
+                if (waitUntil(SystemClock.uptimeMillis() + mDelay)) {
+                    return Vibration.Status.FINISHED;
+                }
+                return Vibration.Status.CANCELLED;
             } finally {
                 if (DEBUG) {
                     Slog.d(TAG, "DelayStep done.");