From 5452169c16889d6ab11aa1b02d26df1c49350458 Mon Sep 17 00:00:00 2001 From: chaviw Date: Fri, 12 Jun 2020 14:34:30 -0700 Subject: [PATCH] Don't clear mUsingBLASTSyncTransaction until sending callback If a window has drawn, it doesn't mean the client will immediately get the callback. The applySync transaction may be waiting on other windows to draw. In that case, we want to continue adding any transaction to the blast sync transaction to ensure we don't submit changes until the client gets a chance to handle the transaction. This is done by delaying when mUsingBLASTSyncTransaction is set to false so all requests for the WindowContainers are still added to the syncTransaction Additionally, don't report back that the window completed the draw until we allow for one more performSurfacePlacmement. It's possible that the window has other changes that need to be added to the syncTransaction so only notify onTransactionReady when we allow one more pass to gather the transaction information. Test: Split screen Test: WindowOrganizerTests Fixes: 158793236 Change-Id: If0a4071869fa1ba70f59aad25606a6e3cb7dafee --- .../android/server/wm/BLASTSyncEngine.java | 33 +++++++------ .../android/server/wm/WindowContainer.java | 16 +++++-- .../server/wm/WindowOrganizerController.java | 20 ++++++-- .../com/android/server/wm/WindowState.java | 30 ++++++++++-- .../server/wm/WindowOrganizerTests.java | 47 +++++++++++++++++-- 5 files changed, 116 insertions(+), 30 deletions(-) diff --git a/services/core/java/com/android/server/wm/BLASTSyncEngine.java b/services/core/java/com/android/server/wm/BLASTSyncEngine.java index efcb558fef66f..6305fda4924c2 100644 --- a/services/core/java/com/android/server/wm/BLASTSyncEngine.java +++ b/services/core/java/com/android/server/wm/BLASTSyncEngine.java @@ -16,17 +16,19 @@ package com.android.server.wm; -import android.view.SurfaceControl; +import android.util.ArrayMap; +import android.util.ArraySet; -import java.util.HashMap; +import java.util.Set; /** - * Utility class for collecting and merging transactions from various sources asynchronously. + * Utility class for collecting WindowContainers that will merge transactions. * For example to use to synchronously resize all the children of a window container * 1. Open a new sync set, and pass the listener that will be invoked * int id startSyncSet(TransactionReadyListener) * the returned ID will be eventually passed to the TransactionReadyListener in combination - * with the prepared transaction. You also use it to refer to the operation in future steps. + * with a set of WindowContainers that are ready, meaning onTransactionReady was called for + * those WindowContainers. You also use it to refer to the operation in future steps. * 2. Ask each child to participate: * addToSyncSet(int id, WindowContainer wc) * if the child thinks it will be affected by a configuration change (a.k.a. has a visible @@ -38,35 +40,37 @@ import java.util.HashMap; * setReady(int id) * 5. If there were no sub windows anywhere in the hierarchy to wait on, then * transactionReady is immediately invoked, otherwise all the windows are poked - * to redraw and to deliver a buffer to WMS#finishDrawing. - * Once all this drawing is complete the combined transaction of all the buffers - * and pending transaction hierarchy changes will be delivered to the TransactionReadyListener + * to redraw and to deliver a buffer to {@link WindowState#finishDrawing}. + * Once all this drawing is complete the WindowContainer that's ready will be added to the + * set of ready WindowContainers. When the final onTransactionReady is called, it will merge + * the transactions of the all the WindowContainers and will be delivered to the + * TransactionReadyListener */ class BLASTSyncEngine { private static final String TAG = "BLASTSyncEngine"; interface TransactionReadyListener { - void onTransactionReady(int mSyncId, SurfaceControl.Transaction mergedTransaction); + void onTransactionReady(int mSyncId, Set windowContainersReady); }; // Holds state associated with a single synchronous set of operations. class SyncState implements TransactionReadyListener { int mSyncId; - SurfaceControl.Transaction mMergedTransaction; int mRemainingTransactions; TransactionReadyListener mListener; boolean mReady = false; + Set mWindowContainersReady = new ArraySet<>(); private void tryFinish() { if (mRemainingTransactions == 0 && mReady) { - mListener.onTransactionReady(mSyncId, mMergedTransaction); + mListener.onTransactionReady(mSyncId, mWindowContainersReady); mPendingSyncs.remove(mSyncId); } } - public void onTransactionReady(int mSyncId, SurfaceControl.Transaction mergedTransaction) { + public void onTransactionReady(int mSyncId, Set windowContainersReady) { mRemainingTransactions--; - mMergedTransaction.merge(mergedTransaction); + mWindowContainersReady.addAll(windowContainersReady); tryFinish(); } @@ -86,14 +90,13 @@ class BLASTSyncEngine { SyncState(TransactionReadyListener l, int id) { mListener = l; mSyncId = id; - mMergedTransaction = new SurfaceControl.Transaction(); mRemainingTransactions = 0; } }; - int mNextSyncId = 0; + private int mNextSyncId = 0; - final HashMap mPendingSyncs = new HashMap(); + private final ArrayMap mPendingSyncs = new ArrayMap<>(); BLASTSyncEngine() { } diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java index dd08f4208eca3..845b9deda16dd 100644 --- a/services/core/java/com/android/server/wm/WindowContainer.java +++ b/services/core/java/com/android/server/wm/WindowContainer.java @@ -93,6 +93,7 @@ import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Comparator; import java.util.LinkedList; +import java.util.Set; import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; @@ -2673,11 +2674,13 @@ class WindowContainer extends ConfigurationContainer< } @Override - public void onTransactionReady(int mSyncId, SurfaceControl.Transaction mergedTransaction) { - mergedTransaction.merge(mBLASTSyncTransaction); - mUsingBLASTSyncTransaction = false; + public void onTransactionReady(int mSyncId, Set windowContainersReady) { + if (mWaitingListener == null) { + return; + } - mWaitingListener.onTransactionReady(mWaitingSyncId, mergedTransaction); + windowContainersReady.add(this); + mWaitingListener.onTransactionReady(mWaitingSyncId, windowContainersReady); mWaitingListener = null; mWaitingSyncId = -1; @@ -2728,4 +2731,9 @@ class WindowContainer extends ConfigurationContainer< boolean useBLASTSync() { return mUsingBLASTSyncTransaction; } + + void mergeBlastSyncTransaction(Transaction t) { + t.merge(mBLASTSyncTransaction); + mUsingBLASTSyncTransaction = false; + } } diff --git a/services/core/java/com/android/server/wm/WindowOrganizerController.java b/services/core/java/com/android/server/wm/WindowOrganizerController.java index a3faa86758cdd..fbc5afadac6ba 100644 --- a/services/core/java/com/android/server/wm/WindowOrganizerController.java +++ b/services/core/java/com/android/server/wm/WindowOrganizerController.java @@ -44,6 +44,7 @@ import android.window.IWindowOrganizerController; import android.window.WindowContainerToken; import android.window.WindowContainerTransaction; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.function.pooled.PooledConsumer; import com.android.internal.util.function.pooled.PooledLambda; @@ -51,6 +52,7 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; /** * Server side implementation for the interface for organizing windows @@ -142,7 +144,7 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub // operations so we don't end up splitting effects between the WM // pending transaction and the BLASTSync transaction. if (syncId >= 0) { - mBLASTSyncEngine.addToSyncSet(syncId, wc); + addToSyncSet(syncId, wc); } int containerEffect = applyWindowContainerChange(wc, entry.getValue()); @@ -164,7 +166,7 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub continue; } if (syncId >= 0) { - mBLASTSyncEngine.addToSyncSet(syncId, wc); + addToSyncSet(syncId, wc); } effects |= sanitizeAndApplyHierarchyOp(wc, hop); } @@ -396,21 +398,33 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub return mDisplayAreaOrganizerController; } + @VisibleForTesting int startSyncWithOrganizer(IWindowContainerTransactionCallback callback) { int id = mBLASTSyncEngine.startSyncSet(this); mTransactionCallbacksByPendingSyncId.put(id, callback); return id; } + @VisibleForTesting void setSyncReady(int id) { mBLASTSyncEngine.setReady(id); } + @VisibleForTesting + void addToSyncSet(int syncId, WindowContainer wc) { + mBLASTSyncEngine.addToSyncSet(syncId, wc); + } + @Override - public void onTransactionReady(int mSyncId, SurfaceControl.Transaction mergedTransaction) { + public void onTransactionReady(int mSyncId, Set windowContainersReady) { final IWindowContainerTransactionCallback callback = mTransactionCallbacksByPendingSyncId.get(mSyncId); + SurfaceControl.Transaction mergedTransaction = new SurfaceControl.Transaction(); + for (WindowContainer container : windowContainersReady) { + container.mergeBlastSyncTransaction(mergedTransaction); + } + try { callback.onTransactionReady(mSyncId, mergedTransaction); } catch (RemoteException e) { diff --git a/services/core/java/com/android/server/wm/WindowState.java b/services/core/java/com/android/server/wm/WindowState.java index 5fc519c86f111..8329dff606491 100644 --- a/services/core/java/com/android/server/wm/WindowState.java +++ b/services/core/java/com/android/server/wm/WindowState.java @@ -241,8 +241,10 @@ import com.android.server.wm.utils.WmDisplayCutout; import java.io.PrintWriter; import java.lang.ref.WeakReference; import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.Set; import java.util.function.Predicate; /** A window in the window manager. */ @@ -654,6 +656,15 @@ class WindowState extends WindowContainer implements WindowManagerP private static final StringBuilder sTmpSB = new StringBuilder(); + /** + * Whether the next surfacePlacement call should notify that the blast sync is ready. + * This is set to true when {@link #finishDrawing(Transaction)} is called so + * {@link #onTransactionReady(int, Set)} is called after the next surfacePlacement. This allows + * Transactions to get flushed into the syncTransaction before notifying {@link BLASTSyncEngine} + * that this WindowState is ready. + */ + private boolean mNotifyBlastOnSurfacePlacement; + /** * Compares two window sub-layers and returns -1 if the first is lesser than the second in terms * of z-order and 1 otherwise. @@ -5335,6 +5346,7 @@ class WindowState extends WindowContainer implements WindowManagerP updateFrameRateSelectionPriorityIfNeeded(); mWinAnimator.prepareSurfaceLocked(true); + notifyBlastSyncTransaction(); super.prepareSurfaces(); } @@ -5826,6 +5838,17 @@ class WindowState extends WindowContainer implements WindowManagerP mBLASTSyncTransaction.merge(postDrawTransaction); } + mNotifyBlastOnSurfacePlacement = true; + return mWinAnimator.finishDrawingLocked(null); + } + + @VisibleForTesting + void notifyBlastSyncTransaction() { + if (!mNotifyBlastOnSurfacePlacement || mWaitingListener == null) { + mNotifyBlastOnSurfacePlacement = false; + return; + } + // If localSyncId is >0 then we are syncing with children and will // invoke transaction ready from our own #transactionReady callback // we just need to signal our side of the sync (setReady). But if we @@ -5833,15 +5856,14 @@ class WindowState extends WindowContainer implements WindowManagerP // be invoked and we need to invoke it ourself. if (mLocalSyncId >= 0) { mBLASTSyncEngine.setReady(mLocalSyncId); - return mWinAnimator.finishDrawingLocked(null); + return; } - mWaitingListener.onTransactionReady(mWaitingSyncId, mBLASTSyncTransaction); - mUsingBLASTSyncTransaction = false; + mWaitingListener.onTransactionReady(mWaitingSyncId, Collections.singleton(this)); mWaitingSyncId = 0; mWaitingListener = null; - return mWinAnimator.finishDrawingLocked(null); + mNotifyBlastOnSurfacePlacement = false; } private boolean requestResizeForBlastSync() { diff --git a/services/tests/wmtests/src/com/android/server/wm/WindowOrganizerTests.java b/services/tests/wmtests/src/com/android/server/wm/WindowOrganizerTests.java index 71dabc56719bc..7ce0c1edfb4c4 100644 --- a/services/tests/wmtests/src/com/android/server/wm/WindowOrganizerTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/WindowOrganizerTests.java @@ -67,6 +67,7 @@ import android.util.Rational; import android.view.Display; import android.view.SurfaceControl; import android.window.ITaskOrganizer; +import android.window.IWindowContainerTransactionCallback; import android.window.WindowContainerTransaction; import androidx.test.filters.SmallTest; @@ -728,7 +729,7 @@ public class WindowOrganizerTests extends WindowTestsBase { // We should be rejected from the second sync since we are already // in one. assertEquals(false, bse.addToSyncSet(id2, task)); - w.finishDrawing(null); + finishAndNotifyDrawing(w); assertEquals(true, bse.addToSyncSet(id2, task)); bse.setReady(id2); } @@ -752,7 +753,7 @@ public class WindowOrganizerTests extends WindowTestsBase { // Since we have a window we have to wait for it to draw to finish sync. verify(transactionListener, never()) .onTransactionReady(anyInt(), any()); - w.finishDrawing(null); + finishAndNotifyDrawing(w); verify(transactionListener) .onTransactionReady(anyInt(), any()); } @@ -820,14 +821,14 @@ public class WindowOrganizerTests extends WindowTestsBase { int id = bse.startSyncSet(transactionListener); assertEquals(true, bse.addToSyncSet(id, task)); bse.setReady(id); - w.finishDrawing(null); + finishAndNotifyDrawing(w); // Since we have a child window we still shouldn't be done. verify(transactionListener, never()) .onTransactionReady(anyInt(), any()); reset(transactionListener); - child.finishDrawing(null); + finishAndNotifyDrawing(child); // Ah finally! Done verify(transactionListener) .onTransactionReady(anyInt(), any()); @@ -979,4 +980,42 @@ public class WindowOrganizerTests extends WindowTestsBase { new IRequestFinishCallback.Default()); verify(organizer, times(1)).onBackPressedOnTaskRoot(any()); } + + @Test + public void testBLASTCallbackWithMultipleWindows() throws Exception { + final ActivityStack stackController = createStack(); + final Task task = createTask(stackController); + final ITaskOrganizer organizer = registerMockOrganizer(); + final WindowState w1 = createAppWindow(task, TYPE_APPLICATION, "Enlightened Window 1"); + final WindowState w2 = createAppWindow(task, TYPE_APPLICATION, "Enlightened Window 2"); + makeWindowVisible(w1); + makeWindowVisible(w2); + + IWindowContainerTransactionCallback mockCallback = + mock(IWindowContainerTransactionCallback.class); + int id = mWm.mAtmService.mWindowOrganizerController.startSyncWithOrganizer(mockCallback); + + mWm.mAtmService.mWindowOrganizerController.addToSyncSet(id, task); + mWm.mAtmService.mWindowOrganizerController.setSyncReady(id); + + // Since we have a window we have to wait for it to draw to finish sync. + verify(mockCallback, never()).onTransactionReady(anyInt(), any()); + assertTrue(w1.useBLASTSync()); + assertTrue(w2.useBLASTSync()); + finishAndNotifyDrawing(w1); + + // Even though one Window finished drawing, both windows should still be using blast sync + assertTrue(w1.useBLASTSync()); + assertTrue(w2.useBLASTSync()); + + finishAndNotifyDrawing(w2); + verify(mockCallback).onTransactionReady(anyInt(), any()); + assertFalse(w1.useBLASTSync()); + assertFalse(w2.useBLASTSync()); + } + + private void finishAndNotifyDrawing(WindowState ws) { + ws.finishDrawing(null); + ws.notifyBlastSyncTransaction(); + } }