Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix out-of-order transactions (1/2)

Browse Source 
The following sequence of order may happen which cause wrong
surface positions:
- WA.animate updates surfaces properties to S
- WA.animate closes the surface transaction
- Since the previous animation transaction wasn't commited yet,
closeSurfaceTransaction blocks and updating the surface properties
on SF side is deferred.
- In the meantime, since we are not holding WM lock, we have
another thread updating surfaces properties to S'
- Closing the transaction in this thread completes immediately
because it's not a synchronous transaction or animation
transaction.
- After a frame has been processed S gets applied on SF side as
the other transaction is done waiting for the frame to complete.

The issue here is that properties are now set to S instead of S'.
Sad!

We originally started calling closeTransaction without the WM
lock being held because it lead to thread starvation (b/38192114).
However, that fix has this big flaw as described above.

To fix this, we create an empty animation transaction before
updating the animation properties to simulate the back-pressuring
behavior of animation transactions without the WM lock being held.
If that transaction arrives out of order, it doesn't matter at all
because it is empty.

After that, we perform the animation udpate in a transaction that
is not marked as an animation transaction, and thus will not
block, which avoids the starvation issue.

Part of this change is also a change in SF to allow executing
empty animation transactions.

Test: go/wm-smoke
Test: Open VideoPlayer from VRCore, close it, observe no wrong
positiioning of surfaces.
Test: Inspect traces while animating. Ensure back pressuring still
works.

Change-Id: Ie545463e71e0d1bc73439d14381077a290d2f959
Fixes: 63905190
Bug: 38192114
This commit is contained in:
Jorim Jaggi
2017-08-08 15:12:20 +02:00
parent 4499576db0
commit 836dac49c2
2 changed files with 71 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

73
services/core/java/com/android/server/wm/WindowAnimator.java
View File

@@ -33,7 +33,6 @@ import android.view.Choreographer;
import android.view.SurfaceControl;
import android.view.WindowManagerPolicy;
import com.android.internal.view.SurfaceFlingerVsyncChoreographer;
import com.android.server.AnimationThread;
import java.io.PrintWriter;
@@ -134,26 +133,38 @@ public class WindowAnimator {
* sure other threads can make progress if this happens.
*/
private void animate(long frameTimeNs) {
boolean transactionOpen = false;
try {
synchronized (mService.mWindowMap) {
if (!mInitialized) {
return;
}
mCurrentTime = frameTimeNs / TimeUtils.NANOS_PER_MS;
mBulkUpdateParams = SET_ORIENTATION_CHANGE_COMPLETE;
mAnimating = false;
mAppWindowAnimating = false;
if (DEBUG_WINDOW_TRACE) {
Slog.i(TAG, "!!! animate: entry time=" + mCurrentTime);
}
synchronized (mService.mWindowMap) {
if (!mInitialized) {
return;
}
if (SHOW_TRANSACTIONS) Slog.i(TAG, ">>> OPEN TRANSACTION animate");
mService.openSurfaceTransaction();
transactionOpen = true;
SurfaceControl.setAnimationTransaction();
// Schedule next frame already such that back-pressure happens continuously
scheduleAnimation();
}
// Simulate back-pressure by opening and closing an empty animation transaction. This makes
// sure that an animation frame is at least presented once on the screen. We do this outside
// of the regular transaction such that we can avoid holding the window manager lock in case
// we receive back-pressure from SurfaceFlinger. Since closing an animation transaction
// without the window manager locks leads to ordering issues (as the transaction will be
// processed only at the beginning of the next frame which may result in another transaction
// that was executed later in WM side gets executed first on SF side), we don't update any
// Surface properties here such that reordering doesn't cause issues.
mService.executeEmptyAnimationTransaction();
synchronized (mService.mWindowMap) {
mCurrentTime = frameTimeNs / TimeUtils.NANOS_PER_MS;
mBulkUpdateParams = SET_ORIENTATION_CHANGE_COMPLETE;
mAnimating = false;
mAppWindowAnimating = false;
if (DEBUG_WINDOW_TRACE) {
Slog.i(TAG, "!!! animate: entry time=" + mCurrentTime);
}
if (SHOW_TRANSACTIONS) Slog.i(TAG, ">>> OPEN TRANSACTION animate");
mService.openSurfaceTransaction();
try {
final AccessibilityController accessibilityController =
mService.mAccessibilityController;
final int numDisplays = mDisplayContentsAnimators.size();
@@ -216,27 +227,20 @@ public class WindowAnimator {
mAnimating |= mService.mDragState.stepAnimationLocked(mCurrentTime);
}
if (mAnimating) {
mService.scheduleAnimationLocked();
if (!mAnimating) {
cancelAnimation();
}
if (mService.mWatermark != null) {
mService.mWatermark.drawIfNeeded();
}
}
} catch (RuntimeException e) {
Slog.wtf(TAG, "Unhandled exception in Window Manager", e);
} finally {
if (transactionOpen) {
// Do not hold window manager lock while closing the transaction, as this might be
// blocking until the next frame, which can lead to total lock starvation.
mService.closeSurfaceTransaction(false /* withLockHeld */);
} catch (RuntimeException e) {
Slog.wtf(TAG, "Unhandled exception in Window Manager", e);
} finally {
mService.closeSurfaceTransaction();
if (SHOW_TRANSACTIONS) Slog.i(TAG, "<<< CLOSE TRANSACTION animate");
}
}
synchronized (mService.mWindowMap) {
boolean hasPendingLayoutChanges = mService.mRoot.hasPendingLayoutChanges(this);
boolean doRequest = false;
if (mBulkUpdateParams != 0) {
@@ -404,6 +408,13 @@ public class WindowAnimator {
}
}
private void cancelAnimation() {
if (mAnimationFrameCallbackScheduled) {
mAnimationFrameCallbackScheduled = false;
mChoreographer.removeFrameCallback(mAnimationFrameCallback);
}
}
private class DisplayContentsAnimator {
ScreenRotationAnimation mScreenRotationAnimation = null;
}

43
services/core/java/com/android/server/wm/WindowManagerService.java
View File

@@ -908,29 +908,16 @@ public class WindowManagerService extends IWindowManager.Stub
}
}
void closeSurfaceTransaction() {
closeSurfaceTransaction(true /* withLockHeld */);
}
/**
* Closes a surface transaction.
*
* @param withLockHeld Whether to acquire the window manager while doing so. In some cases
* holding the lock my lead to starvation in WM in case closeTransaction
* blocks and we call it repeatedly, like we do for animations.
*/
void closeSurfaceTransaction(boolean withLockHeld) {
void closeSurfaceTransaction() {
try {
Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "closeSurfaceTransaction");
synchronized (mWindowMap) {
if (mRoot.mSurfaceTraceEnabled) {
mRoot.mRemoteEventTrace.closeSurfaceTransaction();
}
if (withLockHeld) {
SurfaceControl.closeTransaction();
}
}
if (!withLockHeld) {
SurfaceControl.closeTransaction();
}
} finally {
@@ -938,6 +925,34 @@ public class WindowManagerService extends IWindowManager.Stub
}
}
/**
* Executes an empty animation transaction without holding the WM lock to simulate
* back-pressure. See {@link WindowAnimator#animate} why this is needed.
*/
void executeEmptyAnimationTransaction() {
try {
Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "openSurfaceTransaction");
synchronized (mWindowMap) {
if (mRoot.mSurfaceTraceEnabled) {
mRoot.mRemoteEventTrace.openSurfaceTransaction();
}
SurfaceControl.openTransaction();
SurfaceControl.setAnimationTransaction();
if (mRoot.mSurfaceTraceEnabled) {
mRoot.mRemoteEventTrace.closeSurfaceTransaction();
}
}
} finally {
Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER);
}
try {
Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "closeSurfaceTransaction");
SurfaceControl.closeTransaction();
} finally {
Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER);
}
}
/** Listener to notify activity manager about app transitions. */
final WindowManagerInternal.AppTransitionListener mActivityManagerAppTransitionNotifier
= new WindowManagerInternal.AppTransitionListener() {