diff options
| -rw-r--r-- | services/core/java/com/android/server/vibrator/VibrationThread.java | 161 | ||||
| -rw-r--r-- | services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java | 41 |
2 files changed, 146 insertions, 56 deletions
diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java index dd4e260c6d91..e85fa234a4ce 100644 --- a/services/core/java/com/android/server/vibrator/VibrationThread.java +++ b/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,53 +402,52 @@ 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(); - if (nextStep != null) { - // This might turn on the vibrator and have a HAL latency. Execute this outside 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; + // Vibrator callbacks should wait until the polled step is played and the next steps are + // added back to the queue, so they can handle the callback. + markWaitToProcessVibratorCallbacks(); + try { + Step nextStep = pollNext(); + if (nextStep != null) { + // This might turn on the vibrator and have a HAL latency. Execute this outside + // 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); } - mNextSteps.addAll(nextSteps); + } + } finally { + synchronized (mLock) { + processVibratorCallbacks(); } } } /** - * Notify the step in this queue that should be anticipated by the vibrator completion - * callback and keep it separate to be consumed by {@link #consumeNext()}. + * 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. - * - * <p>This assumes only one of the next steps is waiting on this given vibrator, so the - * first step found will be anticipated by this method, in no particular order. */ @GuardedBy("mLock") public void notifyVibratorComplete(int vibratorId) { - Iterator<Step> it = mNextSteps.iterator(); - while (it.hasNext()) { - Step step = it.next(); - if (step.shouldPlayWhenVibratorComplete(vibratorId)) { - it.remove(); - mPendingOnVibratorCompleteSteps.offer(step); - break; - } + mNotifiedVibrators.offer(vibratorId); + if (!mWaitToProcessVibratorCallbacks) { + // No step is being played or cancelled now, process the callback right away. + processVibratorCallbacks(); } } @@ -455,15 +458,24 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { * {@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); + // 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(); + } } } @@ -473,14 +485,22 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { * <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; + // 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(); + } } } @@ -494,6 +514,39 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { return mNextSteps.poll(); } } + + private void markWaitToProcessVibratorCallbacks() { + synchronized (mLock) { + mWaitToProcessVibratorCallbacks = true; + } + } + + /** + * Notify the step in this queue that should be anticipated by the vibrator completion + * callback and keep it separate to be consumed by {@link #consumeNext()}. + * + * <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. + * + * <p>This assumes only one of the next steps is waiting on this given vibrator, so the + * first step found will be anticipated by this method, in no particular order. + */ + @GuardedBy("mLock") + private void processVibratorCallbacks() { + mWaitToProcessVibratorCallbacks = false; + while (!mNotifiedVibrators.isEmpty()) { + int vibratorId = mNotifiedVibrators.poll(); + Iterator<Step> it = mNextSteps.iterator(); + while (it.hasNext()) { + Step step = it.next(); + if (step.shouldPlayWhenVibratorComplete(vibratorId)) { + it.remove(); + mPendingOnVibratorCompleteSteps.offer(step); + break; + } + } + } + } } /** @@ -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 = - SystemClock.uptimeMillis() + mVibratorOnResult + CALLBACKS_EXTRA_TIMEOUT; - return nextSteps(nextVibratorOffTimeout, nextVibratorOffTimeout, segmentsPlayed); + long nextStartTime = SystemClock.uptimeMillis() + mVibratorOnResult; + long nextVibratorOffTimeout = nextStartTime + CALLBACKS_EXTRA_TIMEOUT; + return nextSteps(nextStartTime, nextVibratorOffTimeout, segmentsPlayed); } /** diff --git a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java b/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java index 2e5c24c1a95c..61b5c2b09bb2 100644 --- a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java +++ b/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; + } + } } |