From eb67e23b0143e92f9208b65f07c8d24a718b106a Mon Sep 17 00:00:00 2001 From: Lais Andrade Date: Wed, 14 Jul 2021 09:57:40 +0100 Subject: [PATCH] Increase timeout on VibrationThread to wait for vibrator callbacks Some IVibrator#compose calls are taking longer than the estimated duration to finish and are being cut short by the VibrationThread with the existing timeout. Increating the timeout to 1s. Bug: 186441334 Test: manual Change-Id: Idc0b58f0d068a62ad0df70ab8b2bb3bbe5355cf1 --- .../server/vibrator/VibrationThread.java | 211 +++++++++++------- .../server/vibrator/VibrationThreadTest.java | 41 +++- 2 files changed, 171 insertions(+), 81 deletions(-) diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java index dd4e260c6d91e..e85fa234a4ce9 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 mNextSteps = new PriorityQueue<>(); @GuardedBy("mLock") private final Queue mPendingOnVibratorCompleteSteps = new LinkedList<>(); + @GuardedBy("mLock") + private final Queue 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(); - 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 nextSteps = nextStep.play(); - synchronized (mLock) { - if (nextStep.getVibratorOnDuration() > 0) { - mSuccessfulVibratorOnSteps++; + // 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 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. + * + *

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. + * + *

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 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. + * + *

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) { - Iterator it = mNextSteps.iterator(); - while (it.hasNext()) { - Step step = it.next(); - if (step.shouldPlayWhenVibratorComplete(vibratorId)) { - it.remove(); - mPendingOnVibratorCompleteSteps.offer(step); - break; + private void processVibratorCallbacks() { + mWaitToProcessVibratorCallbacks = false; + while (!mNotifiedVibrators.isEmpty()) { + int vibratorId = mNotifiedVibrators.poll(); + Iterator it = mNextSteps.iterator(); + while (it.hasNext()) { + Step step = it.next(); + if (step.shouldPlayWhenVibratorComplete(vibratorId)) { + it.remove(); + mPendingOnVibratorCompleteSteps.offer(step); + break; + } } } } - - /** - * Cancel the current queue, replacing all remaining steps with respective clean-up steps. - * - *

This will remove all steps and replace them with respective - * {@link Step#cancel()}. - */ - public void cancel() { - List 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. - * - *

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 = - 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 2e5c24c1a95c4..61b5c2b09bb2d 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; + } + } }