From 836dac49c2fc1739580d3d1dfdf9030a3c304b50 Mon Sep 17 00:00:00 2001 From: Jorim Jaggi Date: Tue, 8 Aug 2017 15:12:20 +0200 Subject: [PATCH] Fix out-of-order transactions (1/2) 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 --- .../com/android/server/wm/WindowAnimator.java | 73 +++++++++++-------- .../server/wm/WindowManagerService.java | 43 +++++++---- 2 files changed, 71 insertions(+), 45 deletions(-) diff --git a/services/core/java/com/android/server/wm/WindowAnimator.java b/services/core/java/com/android/server/wm/WindowAnimator.java index fe5b7f23d5e0f..079ae40bcb483 100644 --- a/services/core/java/com/android/server/wm/WindowAnimator.java +++ b/services/core/java/com/android/server/wm/WindowAnimator.java @@ -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; } diff --git a/services/core/java/com/android/server/wm/WindowManagerService.java b/services/core/java/com/android/server/wm/WindowManagerService.java index b0775dd3a8592..03e1813a79002 100644 --- a/services/core/java/com/android/server/wm/WindowManagerService.java +++ b/services/core/java/com/android/server/wm/WindowManagerService.java @@ -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() {