Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6f4f2c72 authored by Robert Carr's avatar Robert Carr Committed by Rob Carr
Browse files

WindowState: applyWithNextDraw fixes

In this CL we make a few fixes to harden applyWithNextDraw.

1. Currently there is a bug where we apply the transaction directly
   in executeDrawHandlers, this breaks BLAST sync as we need to pass
   the post draw transaction on, not apply it.
2. When we pass it on, we expose a bug in WindowStateAnimator#finishDrawing
   if the state is DRAW_PENDING we merge to mPostDrawTransaction instead
   of applying, but mPostDrawTransaction is only applied if mLastHidden
   is true. Rotation resets mDrawState=DRAW_PENDING but doesnt reset
   mLastHidden so this means transactions that end up in
   mPostDrawTransaction as a result of rotation would be lost. We modify
   finishDrawing to check mLastHidden and see if we will apply later.
3. The timing of prepareDrawHandlers should be moved to the end of
   relayout, so that calling it works while in relayout.
4. We should clear mRedrawForSyncReported when preparingDrawHandlers, if
   the client got the changes in relayout we dont need the client to
   call relayout again.
5. Add a big comment explaining how all this works.

Bug: 168505645
Test: Existing tests pass
Change-Id: Icd79cc98abb01c73cecff053ef9ca990034d0891
parent fbb1317a
Loading
Loading
Loading
Loading
+6 −5
Original line number Diff line number Diff line
@@ -2243,11 +2243,6 @@ public class WindowManagerService extends IWindowManager.Stub
                win.mPendingPositionChanged = null;
            }

            if (mUseBLASTSync && win.useBLASTSync() && viewVisibility != View.GONE) {
                win.prepareDrawHandlers();
                result |= RELAYOUT_RES_BLAST_SYNC;
            }

            int attrChanges = 0;
            int flagChanges = 0;
            int privateFlagChanges = 0;
@@ -2520,6 +2515,12 @@ public class WindowManagerService extends IWindowManager.Stub
            }
            win.mInRelayout = false;

            if (mUseBLASTSync && win.useBLASTSync() && viewVisibility != View.GONE) {
                win.prepareDrawHandlers();
                win.markRedrawForSyncReported();
                result |= RELAYOUT_RES_BLAST_SYNC;
            }

            if (configChanged) {
                Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER,
                        "relayoutWindow: postNewConfigurationToHandler");
+70 −6
Original line number Diff line number Diff line
@@ -763,6 +763,56 @@ class WindowState extends WindowContainer<WindowState> implements WindowManagerP
     * into mReadyDrawHandlers. Finally once we get to finishDrawing we know everything in
     * mReadyDrawHandlers corresponds to state which was observed by the client and we can
     * invoke the consumers.
     *
     * To see in more detail that this works, we can look at it like this:
     *
     * The client is in one of these states:
     *
     * 1. Asleep
     * 2. Traversal scheduled
     * 3. Starting traversal
     * 4. In relayout
     * 5. Already drawing
     *
     * The property we want to implement with the draw handlers is:
     *   If WM code makes a change to client observable state (e.g. configuration),
     *   and registers a draw handler (without releasing the WM lock in between),
     *   the FIRST frame reflecting that change, will be in the Transaction passed
     *   to the draw handler.
     *
     * We describe the expected sequencing in each of the possible client states.
     * We aim to "prove" that the WM can call applyWithNextDraw() with the client
     * starting in any state, and achieve the desired result.
     *
     * 1. Asleep: The client will wake up in response to MSG_RESIZED, call relayout,
     * observe the changes. Relayout will return BLAST_SYNC, and the client will
     * send the transaction to finishDrawing. Since the client was asleep. This
     * will be the first finishDrawing reflecting the change.
     * 2, 3: traversal scheduled/starting traversal: These two states can be considered
     *    together. Each has two sub-states:
     *       a) Traversal will call relayout. In this case we proceed like the starting
     *          from asleep case.
     *       b) Traversal will not call relayout. In this case, the client produced
     *       frame will not include the change. Because there is no call to relayout
     *       there is no call to prepareDrawHandlers() and even if the client calls
     *       finish drawing the draw handler will not be invoked. We have to wait
     *       on the client to receive MSG_RESIZED, and will sync on the next frame
     * 4. In relayout. In this case we are careful to prepare draw handlers and check
     *    whether to return the BLAST flag at the end of relayoutWindow. This means if you
     *    add a draw handler while the client is in relayout, BLAST_SYNC will be
     *    immediately returned, and the client will submit the frame corresponding
     *    to what returns from layout. When we prepare the draw handlers we clear the
     *    flag which would later cause us to report draw for sync. Since we reported
     *    sync through relayout (by luck the client was calling relayout perhaps)
     *    there is no need for a MSG_RESIZED.
     * 5. Already drawing. This works much like cases 2 and 3. If there is no call to
     *    finishDrawing then of course the draw handlers will not be invoked and we just
     *    wait on the next frame for sync. If there is a call to finishDrawing,
     *    the draw handler will not have been prepared (since we did not call relayout)
     *    and we will have to wait on the next frame.
     *
     * By this logic we can see no matter which of the client states we are in when the
     * draw handler is added, it will always execute on the expected frame.
     */
    private final List<Consumer<SurfaceControl.Transaction>> mPendingDrawHandlers
        = new ArrayList<>();
@@ -3697,7 +3747,7 @@ class WindowState extends WindowContainer<WindowState> implements WindowManagerP
        final int displayId = getDisplayId();
        fillClientWindowFrames(mClientWindowFrames);

        mRedrawForSyncReported = true;
        markRedrawForSyncReported();

        try {
            mClient.resized(mClientWindowFrames, reportDraw, mergedConfiguration, forceRelayout,
@@ -5841,7 +5891,7 @@ class WindowState extends WindowContainer<WindowState> implements WindowManagerP
     * "in relayout", the results may be undefined but at all other times the function
     * should sort of transparently work like this:
     *    1. Make changes to WM hierarchy (say change app configuration)
     *    2. Call apply with next draw.
     *    2. Call applyWithNextDraw
     *    3. After finishDrawing, our consumer will be passed the Transaction
     *    containing the buffer, and we can merge in additional operations.
     * See {@link WindowState#mPendingDrawHandlers}
@@ -5870,16 +5920,26 @@ class WindowState extends WindowContainer<WindowState> implements WindowManagerP
     * See {@link WindowState#mPendingDrawHandlers}
     */
    boolean executeDrawHandlers(SurfaceControl.Transaction t) {
        if (t == null) t = mTmpTransaction;
        boolean hadHandlers = false;
        boolean applyHere = false;
        if (t == null) {
            t = mTmpTransaction;
            applyHere = true;
        }

        for (int i = 0; i < mReadyDrawHandlers.size(); i++) {
            mReadyDrawHandlers.get(i).accept(t);
            hadHandlers = true;
        }

        if (hadHandlers) {
            mReadyDrawHandlers.clear();
            mWmService.mH.removeMessages(WINDOW_STATE_BLAST_SYNC_TIMEOUT, this);
        }

        if (applyHere) {
            t.apply();
        }

        return hadHandlers;
    }
@@ -5897,4 +5957,8 @@ class WindowState extends WindowContainer<WindowState> implements WindowManagerP
    @WindowManager.LayoutParams.WindowType int getWindowType() {
        return mAttrs.type;
    }

    void markRedrawForSyncReported() {
       mRedrawForSyncReported = true;
    }
}
+6 −5
Original line number Diff line number Diff line
@@ -272,15 +272,16 @@ class WindowStateAnimator {
            }
            mDrawState = COMMIT_DRAW_PENDING;
            layoutNeeded = true;
        }

        if (postDrawTransaction != null) {
            if (mLastHidden) {
                mPostDrawTransaction.merge(postDrawTransaction);
            }
        } else if (postDrawTransaction != null) {
            // If draw state is not pending we may delay applying this transaction from the client,
            // so apply it now.
                layoutNeeded = true;
            } else {
                postDrawTransaction.apply();
            }
        }

        return layoutNeeded;
    }