From 2bd95bf6ad6cd6e16f4b3b4f8f8ae90285dc322b Mon Sep 17 00:00:00 2001
From: Robert Carr <racarr@google.com>
Date: Tue, 18 Feb 2020 09:00:03 -0800
Subject: ViewRootImpl: Fix overlapping BLAST Sync issues.

Currently we aren't well equipped to handle overlapping synchronization.
That is to say where a second BLAST Sync begins before the first one finishes.
We track enabling this in b/149747443. In the mean-time we take a few steps
to prevent overlap:
   1. Call finishBLASTSync directly from the RT callback rather than posting
      to the UI thread.
   2. Before consuming "mNextDrawUseBLASTSync" fence the threaded renderer to make sure
      any previous draw has finished and emitted its callback.
   3. Use seperate variable for tracking whether the BLAST transaction has been applied
      so that an earlier draw finishing inbetween us calling setNextDrawUseBLASTSync
      and calling performDraw, will not lear the value of mNextDrawUseBlastSync.

Test: Flip the flag, play with youtube.
Bug: 149747443
Bug: 146598493
Bug: 149251083
Bug: 149315421
Change-Id: I6cfd976cd72345acc814fc99d182b50456014a0e
---
 core/java/android/view/ViewRootImpl.java | 36 +++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java
index 857bc5058d60..b32ebe9e4ffd 100644
--- a/core/java/android/view/ViewRootImpl.java
+++ b/core/java/android/view/ViewRootImpl.java
@@ -664,7 +664,22 @@ public final class ViewRootImpl implements ViewParent,
         int localChanges;
     }
 
+    // If set, ViewRootImpl will call BLASTBufferQueue::setNextTransaction with
+    // mRtBLASTSyncTransaction, prior to invoking draw. This provides a way
+    // to redirect the buffers in to transactions.
     private boolean mNextDrawUseBLASTSyncTransaction;
+    // Set when calling setNextTransaction, we can't just reuse mNextDrawUseBLASTSyncTransaction
+    // because, imagine this scenario:
+    //     1. First draw is using BLAST, mNextDrawUseBLAST = true
+    //     2. We call perform draw and are waiting on the callback
+    //     3. After the first perform draw but before the first callback and the
+    //        second perform draw, a second draw sets mNextDrawUseBLAST = true (it already was)
+    //     4. At this point the callback fires and we set mNextDrawUseBLAST = false;
+    //     5. We get to performDraw and fail to sync as we intended because mNextDrawUseBLAST
+    //        is now false.
+    // This is why we use a two-step latch with the two booleans, one consumed from
+    // performDraw and one consumed from finishBLASTSync()
+    private boolean mNextReportConsumeBLAST;
     // Be very careful with the threading here. This is used from the render thread while
     // the UI thread is paused and then applied and cleared from the UI thread right after
     // draw returns.
@@ -3694,9 +3709,9 @@ public final class ViewRootImpl implements ViewParent,
             usingAsyncReport = mReportNextDraw;
             if (needFrameCompleteCallback) {
                 final Handler handler = mAttachInfo.mHandler;
-                mAttachInfo.mThreadedRenderer.setFrameCompleteCallback((long frameNr) ->
+                mAttachInfo.mThreadedRenderer.setFrameCompleteCallback((long frameNr) -> {
+                        finishBLASTSync();
                         handler.postAtFrontOfQueue(() -> {
-                            finishBLASTSync();
                             if (reportNextDraw) {
                                 // TODO: Use the frame number
                                 pendingDrawFinished();
@@ -3706,12 +3721,23 @@ public final class ViewRootImpl implements ViewParent,
                                     commitCallbacks.get(i).run();
                                 }
                             }
-                        }));
+                        });});
             }
         }
 
         try {
             if (mNextDrawUseBLASTSyncTransaction) {
+                // TODO(b/149747443)
+                // We aren't prepared to handle overlapping use of mRtBLASTSyncTransaction
+                // so if we are BLAST syncing we make sure the previous draw has
+                // totally finished.
+                if (mAttachInfo.mThreadedRenderer != null) {
+                    mAttachInfo.mThreadedRenderer.fence();
+                }
+
+                mNextReportConsumeBLAST = true;
+                mNextDrawUseBLASTSyncTransaction = false;
+
                 mBlastBufferQueue.setNextTransaction(mRtBLASTSyncTransaction);
             }
             boolean canUseAsync = draw(fullRedrawNeeded);
@@ -9551,8 +9577,8 @@ public final class ViewRootImpl implements ViewParent,
     }
 
     private void finishBLASTSync() {
-        if (mNextDrawUseBLASTSyncTransaction) {
-            mNextDrawUseBLASTSyncTransaction = false;
+        if (mNextReportConsumeBLAST) {
+            mNextReportConsumeBLAST = false;
             mRtBLASTSyncTransaction.apply();
         }
     }
-- 
cgit v1.2.3-59-g8ed1b