Fix vibration race condition in Ringer.
Currently, there is a "stopped" check at the start of the play
action but this may become stale while loading the ringtone,
and specifically before starting the vibration. This can result
in starting the vibration after its stop signal, and it never
ending.
This is fixed by recording the vibration as pending ("reserving")
when it is being setup. This way, a stop and start are both covered
with checks under the same lock, and the start won't happen if its
reservation was cleared.
MUST_SLEEP to imitate slow ringtone load.
Bug: 278073224
Test: extended RingerTest
Change-Id: Ia0103ea990fe86dc4481b4b2105bf6377ff164ba
diff --git a/src/com/android/server/telecom/AsyncRingtonePlayer.java b/src/com/android/server/telecom/AsyncRingtonePlayer.java
index 2712b23..3fbac1f 100644
--- a/src/com/android/server/telecom/AsyncRingtonePlayer.java
+++ b/src/com/android/server/telecom/AsyncRingtonePlayer.java
@@ -140,9 +140,15 @@
return;
}
Ringtone ringtone = null;
+ boolean hasStopped = false;
try {
ringtone = ringtoneSupplier.get();
-
+ // Ringtone supply can be slow. Re-check for stop event.
+ if (mHandler.hasMessages(EVENT_STOP)) {
+ hasStopped = true;
+ ringtone.stop(); // proactively release the ringtone.
+ return;
+ }
// setRingtone even if null - it also stops any current ringtone to be consistent
// with the overall state.
setRingtone(ringtone);
@@ -162,7 +168,7 @@
mRingtone.play();
Log.i(this, "Play ringtone, looping.");
} finally {
- ringtoneConsumer.accept(ringtone, /* stopped= */ false);
+ ringtoneConsumer.accept(ringtone, hasStopped);
}
} finally {
Log.cancelSubsession(session);
diff --git a/src/com/android/server/telecom/Ringer.java b/src/com/android/server/telecom/Ringer.java
index b6aa4cc..cdacab0 100644
--- a/src/com/android/server/telecom/Ringer.java
+++ b/src/com/android/server/telecom/Ringer.java
@@ -176,7 +176,7 @@
/**
* Call objects that are ringing, vibrating or call-waiting. These are used only for logging
- * purposes.
+ * purposes (except mVibratingCall is also used to ensure consistency).
*/
private Call mRingingCall;
private Call mVibratingCall;
@@ -406,17 +406,32 @@
foregroundCall, mVolumeShaperConfig, finalHapticChannelsMuted);
}
+ // If vibration will be done, reserve the vibrator.
+ boolean vibratorReserved = isVibratorEnabled && attributes.shouldRingForContact()
+ && tryReserveVibration(foregroundCall);
+ if (!vibratorReserved) {
+ foregroundCall.setUserMissed(USER_MISSED_NO_VIBRATE);
+ Log.addEvent(foregroundCall, LogUtils.Events.SKIP_VIBRATION,
+ "hasVibrator=%b, userRequestsVibrate=%b, ringerMode=%d, "
+ + "isVibratorEnabled=%b",
+ mVibrator.hasVibrator(),
+ mSystemSettingsUtil.isRingVibrationEnabled(mContext),
+ mAudioManager.getRingerMode(), isVibratorEnabled);
+ }
+
// The vibration logic depends on the loaded ringtone, but we need to defer the ringtone
// load to the async ringtone thread. Hence, we bundle up the final part of this method
// for that thread to run after loading the ringtone. This logic is intended to run even
// if the loaded ringtone is null. However if a stop event arrives before the ringtone
// creation finishes, then this consumer can be skipped.
final boolean finalUseCustomVibrationEffect = useCustomVibrationEffect;
- final RingerAttributes finalAttributes = attributes;
- BiConsumer<Ringtone, Boolean> vibrationLogic = (Ringtone ringtone, Boolean stopped) -> {
+ BiConsumer<Ringtone, Boolean> afterRingtoneLogic =
+ (Ringtone ringtone, Boolean stopped) -> {
try {
- if (stopped.booleanValue()) {
- return; // don't start vibration if the ringing is already abandoned.
+ if (stopped.booleanValue() || !vibratorReserved) {
+ // don't start vibration if the ringing is already abandoned, or the
+ // vibrator wasn't reserved. This still triggers the mBlockOnRingingFuture.
+ return;
}
final VibrationEffect vibrationEffect;
if (ringtone != null && finalUseCustomVibrationEffect) {
@@ -431,8 +446,7 @@
boolean isUsingAudioCoupledHaptics =
!finalHapticChannelsMuted && ringtone != null
&& ringtone.hasHapticChannels();
- vibrateIfNeeded(isUsingAudioCoupledHaptics, finalAttributes, foregroundCall,
- vibrationEffect, isVibratorEnabled);
+ vibrateIfNeeded(isUsingAudioCoupledHaptics, foregroundCall, vibrationEffect);
} finally {
// This is used to signal to tests that the async play() call has completed.
if (mBlockOnRingingFuture != null) {
@@ -442,9 +456,9 @@
};
deferBlockOnRingingFuture = true; // Run in vibrationLogic.
if (ringtoneSupplier != null) {
- mRingtonePlayer.play(ringtoneSupplier, vibrationLogic);
+ mRingtonePlayer.play(ringtoneSupplier, afterRingtoneLogic);
} else {
- vibrationLogic.accept(/* ringtone= */ null, /* stopped= */ false);
+ afterRingtoneLogic.accept(/* ringtone= */ null, /* stopped= */ false);
}
// shouldAcquireAudioFocus is meant to be true, but that check is deferred to here
@@ -460,10 +474,31 @@
}
}
- private void vibrateIfNeeded(boolean isUsingAudioCoupledHaptics, RingerAttributes attributes,
- Call foregroundCall, VibrationEffect effect, boolean isVibratorEnabled) {
- final boolean shouldRingForContact = attributes.shouldRingForContact();
+ /**
+ * Try to reserve the vibrator for this call, returning false if it's already committed.
+ * The vibration will be started by AsyncRingtonePlayer to ensure timing is aligned with the
+ * audio. The logic uses mVibratingCall to say which call is currently getting ready to vibrate,
+ * or actually vibrating (indicated by mIsVibrating).
+ *
+ * Once reserved, the vibrateIfNeeded method is expected to be called. Note that if
+ * audio-coupled haptics were used instead of vibrator, the reservation still stays until
+ * ringing is stopped, because the vibrator is exclusive to a single vibration source.
+ *
+ * Note that this "reservation" is only local to the Ringer - it's not locking the vibrator, so
+ * if it's busy with some other important vibration, this ringer's one may not displace it.
+ */
+ private boolean tryReserveVibration(Call foregroundCall) {
+ synchronized (mLock) {
+ if (mVibratingCall != null || mIsVibrating) {
+ return false;
+ }
+ mVibratingCall = foregroundCall;
+ return true;
+ }
+ }
+ private void vibrateIfNeeded(boolean isUsingAudioCoupledHaptics, Call foregroundCall,
+ VibrationEffect effect) {
if (isUsingAudioCoupledHaptics) {
Log.addEvent(
foregroundCall, LogUtils.Events.SKIP_VIBRATION, "using audio-coupled haptics");
@@ -471,22 +506,17 @@
}
synchronized (mLock) {
- if (isVibratorEnabled && !mIsVibrating && shouldRingForContact) {
+ // Ensure the reservation is live. The mIsVibrating check should be redundant.
+ if (foregroundCall == mVibratingCall && !mIsVibrating) {
Log.addEvent(foregroundCall, LogUtils.Events.START_VIBRATOR,
"hasVibrator=%b, userRequestsVibrate=%b, ringerMode=%d, isVibrating=%b",
mVibrator.hasVibrator(), mSystemSettingsUtil.isRingVibrationEnabled(mContext),
mAudioManager.getRingerMode(), mIsVibrating);
- mVibratingCall = foregroundCall;
mIsVibrating = true;
mVibrator.vibrate(effect, VIBRATION_ATTRIBUTES);
Log.i(this, "start vibration.");
- } else {
- foregroundCall.setUserMissed(USER_MISSED_NO_VIBRATE);
- Log.addEvent(foregroundCall, LogUtils.Events.SKIP_VIBRATION,
- "hasVibrator=%b, userRequestsVibrate=%b, ringerMode=%d, isVibrating=%b",
- mVibrator.hasVibrator(), mSystemSettingsUtil.isRingVibrationEnabled(mContext),
- mAudioManager.getRingerMode(), mIsVibrating);
}
+ // else stopped already: this isn't started unless a reservation was made.
}
}
@@ -565,8 +595,8 @@
Log.addEvent(mVibratingCall, LogUtils.Events.STOP_VIBRATOR);
mVibrator.cancel();
mIsVibrating = false;
- mVibratingCall = null;
}
+ mVibratingCall = null; // Prevents vibrations from starting via AsyncRingtonePlayer.
}
}
diff --git a/tests/src/com/android/server/telecom/tests/RingerTest.java b/tests/src/com/android/server/telecom/tests/RingerTest.java
index cf5b791..abbfe34 100644
--- a/tests/src/com/android/server/telecom/tests/RingerTest.java
+++ b/tests/src/com/android/server/telecom/tests/RingerTest.java
@@ -505,8 +505,20 @@
@Test
public void testStopFlashNotificationWhenRingStops() throws Exception {
- ensureRingtoneMocked();
+ Ringtone mockRingtone = mock(Ringtone.class);
+ when(mockRingtoneFactory.getRingtone(
+ any(Call.class), nullable(VolumeShaper.Configuration.class), anyBoolean()))
+ .thenAnswer(x -> {
+ // Be slow to create ringtone.
+ try {
+ Thread.sleep(300);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ return mockRingtone;
+ });
// Start call waiting to make sure that it doesn't stop when we start ringing
+ enableVibrationWhenRinging();
mRingerUnderTest.startCallWaiting(mockCall1);
when(mockCall2.wasDndCheckComputedForCall()).thenReturn(false);
when(mockCall2.getHandle()).thenReturn(Uri.parse(""));
@@ -518,6 +530,8 @@
verify(mockAccessibilityManagerAdapter, atLeastOnce())
.stopFlashNotificationSequence(any(Context.class));
mRingCompletionFuture.get(); // Don't leak async work.
+ verify(mockVibrator, never()) // cancelled before it started.
+ .vibrate(any(VibrationEffect.class), any(VibrationAttributes.class));
}
@SmallTest