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

Commit 046e68e8 authored by Chavi Weingarten's avatar Chavi Weingarten
Browse files

Don't post finishDrawing call on client UI thread. 

If the finishDrawing call is posted on the client UI thread, there's a
chance that the client is already on the UI thread and starting to draw
again. This could lead to a deadlock because the UI thread is waiting
for a free buffer to unblock RT, but the buffer is stuck waiting to be
sent to WMS to get applied.

There's really no reason the call into WMS#finishDrawing needs to be
done on the UI thread and is actually more efficient if it's NOT done on
the UI thread because there's nothing we need to wait on. This means WMS
can apply the transactions even earlier, which may improve performance.

Test: 3-button on device with TaskBar
Fixes: 263340863
Change-Id: Icc7b54405496ed54ebe4f4a82946c9da21848002
parent 7e23f541

core/java/android/view/ViewRootImpl.java

+31 −26
Original line number Diff line number Diff line
@@ -674,12 +674,6 @@ public final class ViewRootImpl implements ViewParent, 


    private BLASTBufferQueue mBlastBufferQueue;



    /**

     * Transaction object that can be used to synchronize child SurfaceControl changes with

     * ViewRootImpl SurfaceControl changes by the server. The object is passed along with

     * the SurfaceChangedCallback.

     */

    private final Transaction mSurfaceChangedTransaction = new Transaction();

    /**

     * Child container layer of {@code mSurface} with the same bounds as its parent, and cropped to

     * the surface insets. This surface is created only if a client requests it via {@link
@@ -2139,9 +2133,9 @@ public final class ViewRootImpl implements ViewParent, 
        mSurfaceChangedCallbacks.remove(c);

    }



    private void notifySurfaceCreated() {

    private void notifySurfaceCreated(Transaction t) {

        for (int i = 0; i < mSurfaceChangedCallbacks.size(); i++) {

            mSurfaceChangedCallbacks.get(i).surfaceCreated(mSurfaceChangedTransaction);

            mSurfaceChangedCallbacks.get(i).surfaceCreated(t);

        }

    }
@@ -2150,9 +2144,9 @@ public final class ViewRootImpl implements ViewParent, 
     * called if a new surface is created, only if the valid surface has been replaced with another

     * valid surface.

     */

    private void notifySurfaceReplaced() {

    private void notifySurfaceReplaced(Transaction t) {

        for (int i = 0; i < mSurfaceChangedCallbacks.size(); i++) {

            mSurfaceChangedCallbacks.get(i).surfaceReplaced(mSurfaceChangedTransaction);

            mSurfaceChangedCallbacks.get(i).surfaceReplaced(t);

        }

    }
@@ -3505,17 +3499,24 @@ public final class ViewRootImpl implements ViewParent, 
            }

        }



        boolean didUseTransaction = false;

        // These callbacks will trigger SurfaceView SurfaceHolder.Callbacks and must be invoked

        // after the measure pass. If its invoked before the measure pass and the app modifies

        // the view hierarchy in the callbacks, we could leave the views in a broken state.

        if (surfaceCreated) {

            notifySurfaceCreated();

            notifySurfaceCreated(mTransaction);

            didUseTransaction = true;

        } else if (surfaceReplaced) {

            notifySurfaceReplaced();

            notifySurfaceReplaced(mTransaction);

            didUseTransaction = true;

        } else if (surfaceDestroyed)  {

            notifySurfaceDestroyed();

        }



        if (didUseTransaction) {

            applyTransactionOnDraw(mTransaction);

        }



        if (triggerGlobalLayoutListener) {

            mAttachInfo.mRecomputeGlobalAttributes = false;

            mAttachInfo.mTreeObserver.dispatchOnGlobalLayout();
@@ -3720,15 +3721,8 @@ public final class ViewRootImpl implements ViewParent, 
        final int seqId = mSyncSeqId;

        mWmsRequestSyncGroupState = WMS_SYNC_PENDING;

        mWmsRequestSyncGroup = new SurfaceSyncGroup(t -> {

            mWmsRequestSyncGroupState = WMS_SYNC_RETURNED;

            // Callback will be invoked on executor thread so post to main thread.

            mHandler.postAtFrontOfQueue(() -> {

                if (t != null) {

                    mSurfaceChangedTransaction.merge(t);

                }

            mWmsRequestSyncGroupState = WMS_SYNC_MERGED;

                reportDrawFinished(seqId);

            });

            reportDrawFinished(t, seqId);

        });

        if (DEBUG_BLAST) {

            Log.d(mTag, "Setup new sync id=" + mWmsRequestSyncGroup);
@@ -4363,18 +4357,22 @@ public final class ViewRootImpl implements ViewParent, 
        }

    }



    private void reportDrawFinished(int seqId) {

    private void reportDrawFinished(@Nullable Transaction t, int seqId) {

        if (DEBUG_BLAST) {

            Log.d(mTag, "reportDrawFinished " + Debug.getCallers(5));

            Log.d(mTag, "reportDrawFinished");

        }



        try {

            mWindowSession.finishDrawing(mWindow, mSurfaceChangedTransaction, seqId);

            mWindowSession.finishDrawing(mWindow, t, seqId);

        } catch (RemoteException e) {

            Log.e(mTag, "Unable to report draw finished", e);

            mSurfaceChangedTransaction.apply();

            if (t != null) {

                t.apply();

            }

        } finally {

            mSurfaceChangedTransaction.clear();

            if (t != null) {

                t.clear();

            }

        }

    }
@@ -11302,6 +11300,13 @@ public final class ViewRootImpl implements ViewParent, 
            if (!mIsInTraversal && !mTraversalScheduled) {

                scheduleTraversals();

            }

            if (DEBUG_BLAST) {

                Log.d(mTag, "Creating new active sync group " + mActiveSurfaceSyncGroup);

            }

        } else {

            if (DEBUG_BLAST) {

                Log.d(mTag, "Return already created active sync group " + mActiveSurfaceSyncGroup);

            }

        }

        return mActiveSurfaceSyncGroup;

    };

core/java/android/window/SurfaceSyncGroup.java

+19 −1
Original line number Diff line number Diff line
@@ -115,6 +115,9 @@ public class SurfaceSyncGroup { 
    public SurfaceSyncGroup() {

        this(transaction -> {

            if (transaction != null) {

                if (DEBUG) {

                    Log.d(TAG, "Applying transaction " + transaction);

                }

                transaction.apply();

            }

        });
@@ -133,6 +136,10 @@ public class SurfaceSyncGroup { 
     */

    public SurfaceSyncGroup(Consumer<Transaction> transactionReadyCallback) {

        mTransactionReadyCallback = transaction -> {

            if (DEBUG && transaction != null) {

                Log.d(TAG,

                        "Sending non null transaction " + transaction + " to callback for " + this);

            }

            transactionReadyCallback.accept(transaction);

            synchronized (mLock) {

                for (Pair<Executor, Runnable> callback : mSyncCompleteCallbacks) {
@@ -238,6 +245,10 @@ public class SurfaceSyncGroup { 
        TransactionReadyCallback transactionReadyCallback = new TransactionReadyCallback() {

            @Override

            public void onTransactionReady(Transaction t) {

                if (DEBUG) {

                    Log.d(TAG, "onTransactionReady called for" + surfaceSyncGroup + " and sent to "

                            + this);

                }

                synchronized (mLock) {

                    if (t != null) {

                        // When an older parent sync group is added due to a child syncGroup getting
@@ -262,7 +273,12 @@ public class SurfaceSyncGroup { 
                return false;

            }

            mPendingSyncs.add(transactionReadyCallback);

            if (DEBUG) {

                Log.d(TAG, "addToSync " + surfaceSyncGroup + " to " + this + " mSyncReady="

                        + mSyncReady + " mPendingSyncs=" + mPendingSyncs.size());

            }

        }



        surfaceSyncGroup.onAddedToSyncGroup(this, transactionReadyCallback);

        return true;

    }
@@ -316,7 +332,9 @@ public class SurfaceSyncGroup { 
                // from the original parent are also combined with the new parent SurfaceSyncGroup.

                if (mParentSyncGroup != null && mParentSyncGroup != parentSyncGroup) {

                    if (DEBUG) {

                        Log.d(TAG, "Already part of sync group " + mParentSyncGroup + " " + this);

                        Log.d(TAG, "Trying to add to " + parentSyncGroup

                                + " but already part of sync group " + mParentSyncGroup + " "

                                + this);

                    }

                    parentSyncGroup.addToSync(mParentSyncGroup, true /* parentSyncGroupMerge */);

                }