From 7b5c08e5e69484534ed20a8236da3642e8f14d8c Mon Sep 17 00:00:00 2001 From: chaviw Date: Thu, 1 Oct 2020 11:55:47 -0700 Subject: [PATCH] Ensure thread safety with render thread and UI Thread. There were a few places that were not thread safe. 1. finishBLASTSync is called from the Render Thread. It was updating mSurfaceChangedTransaction, which send to WMS on the UI Thread. Instead, create a new local Transaction object to allow the Render Thread to merge the mRtBLASTSyncTransaction into it. Then on the UI thread, merge the temporary transaction into mSurfaceChangedTransaction 2. finishBLASTSync was getting called if the draw was unable to run asynchronously. This would mean it would get executed on the UI Thread, possibly causing a race. Instead, remove since there should be nothing on the blast sync transaction, mRtNextFrameReportedConsumeWithBlast would never have been set, and mSendNextFrameToWm is set to false beforehand. Test: YT with and without Blast Change-Id: I72e70fea258a933f51aaaf78c7056a0d3fbac8b3 --- core/java/android/view/ViewRootImpl.java | 134 +++++++++++++++-------- 1 file changed, 86 insertions(+), 48 deletions(-) diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index 7e02549a0aaad..35c21c55121f3 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -105,6 +105,7 @@ import android.graphics.BLASTBufferQueue; import android.graphics.Canvas; import android.graphics.Color; import android.graphics.FrameInfo; +import android.graphics.HardwareRenderer; import android.graphics.HardwareRenderer.FrameDrawingCallback; import android.graphics.Insets; import android.graphics.Matrix; @@ -3810,6 +3811,82 @@ public final class ViewRootImpl implements ViewParent, } } + /** + * The callback will run on the render thread. + */ + private HardwareRenderer.FrameCompleteCallback createFrameCompleteCallback(Handler handler, + boolean reportNextDraw, ArrayList commitCallbacks) { + return frameNr -> { + // Use a new transaction here since mRtBLASTSyncTransaction can only be accessed by + // the render thread and mSurfaceChangedTransaction can only be accessed by the UI + // thread. The temporary transaction is used so mRtBLASTSyncTransaction can be merged + // with mSurfaceChangedTransaction without synchronization issues. + final Transaction t = new Transaction(); + finishBLASTSyncOnRT(!mSendNextFrameToWm, t); + handler.postAtFrontOfQueue(() -> { + mSurfaceChangedTransaction.merge(t); + if (reportNextDraw) { + // TODO: Use the frame number + pendingDrawFinished(); + } + if (commitCallbacks != null) { + for (int i = 0; i < commitCallbacks.size(); i++) { + commitCallbacks.get(i).run(); + } + } + }); + }; + } + + private boolean addFrameCompleteCallbackIfNeeded() { + if (mAttachInfo.mThreadedRenderer == null || !mAttachInfo.mThreadedRenderer.isEnabled()) { + return false; + } + + ArrayList commitCallbacks = mAttachInfo.mTreeObserver + .captureFrameCommitCallbacks(); + final boolean needFrameCompleteCallback = + mNextDrawUseBLASTSyncTransaction || mReportNextDraw + || (commitCallbacks != null && commitCallbacks.size() > 0); + if (needFrameCompleteCallback) { + mAttachInfo.mThreadedRenderer.setFrameCompleteCallback( + createFrameCompleteCallback(mAttachInfo.mHandler, mReportNextDraw, + commitCallbacks)); + return true; + } + return false; + } + + /** + * The callback will run on a worker thread pool from the render thread. + */ + private HardwareRenderer.FrameDrawingCallback createFrameDrawingCallback() { + return frame -> { + mRtNextFrameReportedConsumeWithBlast = true; + if (mBlastBufferQueue != null) { + // We don't need to synchronize mRtBLASTSyncTransaction here since it's not + // being modified and only sent to BlastBufferQueue. + mBlastBufferQueue.setNextTransaction(mRtBLASTSyncTransaction); + } + }; + } + + private void addFrameCallbackIfNeeded() { + if (!mNextDrawUseBLASTSyncTransaction) { + return; + } + + // Frame callbacks will always occur after submitting draw requests and before + // the draw actually occurs. This will ensure that we set the next transaction + // for the frame that's about to get drawn and not on a previous frame that. + // + // This is thread safe since mRtNextFrameReportConsumeWithBlast will only be + // modified in onFrameDraw and then again in onFrameComplete. This is to ensure the + // next frame completed should be reported with the blast sync transaction. + registerRtFrameCallback(createFrameDrawingCallback()); + mNextDrawUseBLASTSyncTransaction = false; + } + private void performDraw() { if (mAttachInfo.mDisplayState == Display.STATE_OFF && !mReportNextDraw) { return; @@ -3823,58 +3900,14 @@ public final class ViewRootImpl implements ViewParent, mIsDrawing = true; Trace.traceBegin(Trace.TRACE_TAG_VIEW, "draw"); - boolean usingAsyncReport = false; - boolean reportNextDraw = mReportNextDraw; // Capture the original value - if (mAttachInfo.mThreadedRenderer != null && mAttachInfo.mThreadedRenderer.isEnabled()) { - ArrayList commitCallbacks = mAttachInfo.mTreeObserver - .captureFrameCommitCallbacks(); - final boolean needFrameCompleteCallback = mNextDrawUseBLASTSyncTransaction || - (commitCallbacks != null && commitCallbacks.size() > 0) || - mReportNextDraw; - usingAsyncReport = mReportNextDraw; - if (needFrameCompleteCallback) { - final Handler handler = mAttachInfo.mHandler; - mAttachInfo.mThreadedRenderer.setFrameCompleteCallback((long frameNr) -> { - finishBLASTSync(!mSendNextFrameToWm); - handler.postAtFrontOfQueue(() -> { - if (reportNextDraw) { - // TODO: Use the frame number - pendingDrawFinished(); - } - if (commitCallbacks != null) { - for (int i = 0; i < commitCallbacks.size(); i++) { - commitCallbacks.get(i).run(); - } - } - }); - }); - } - } + boolean usingAsyncReport = addFrameCompleteCallbackIfNeeded(); + addFrameCallbackIfNeeded(); try { - if (mNextDrawUseBLASTSyncTransaction) { - // Frame callbacks will always occur after submitting draw requests and before - // the draw actually occurs. This will ensure that we set the next transaction - // for the frame that's about to get drawn and not on a previous frame that. - // - // This is thread safe since mRtNextFrameReportConsumeWithBlast will only be - // modified in onFrameDraw and then again in onFrameComplete. This is to ensure the - // next frame completed should be reported with the blast sync transaction. - registerRtFrameCallback(frame -> { - mRtNextFrameReportedConsumeWithBlast = true; - if (mBlastBufferQueue != null) { - // We don't need to synchronize mRtBLASTSyncTransaction here since it's not - // being modified and only sent to BlastBufferQueue. - mBlastBufferQueue.setNextTransaction(mRtBLASTSyncTransaction); - } - }); - mNextDrawUseBLASTSyncTransaction = false; - } boolean canUseAsync = draw(fullRedrawNeeded); if (usingAsyncReport && !canUseAsync) { mAttachInfo.mThreadedRenderer.setFrameCompleteCallback(null); usingAsyncReport = false; - finishBLASTSync(true /* apply */); } } finally { mIsDrawing = false; @@ -9839,7 +9872,12 @@ public final class ViewRootImpl implements ViewParent, mNextDrawUseBLASTSyncTransaction = true; } - private void finishBLASTSync(boolean apply) { + /** + * This should only be called from the render thread. + */ + private void finishBLASTSyncOnRT(boolean apply, Transaction t) { + // This is safe to modify on the render thread since the only other place it's modified + // is on the UI thread when the render thread is paused. mSendNextFrameToWm = false; if (mRtNextFrameReportedConsumeWithBlast) { mRtNextFrameReportedConsumeWithBlast = false; @@ -9850,7 +9888,7 @@ public final class ViewRootImpl implements ViewParent, if (apply) { mRtBLASTSyncTransaction.apply(); } else { - mSurfaceChangedTransaction.merge(mRtBLASTSyncTransaction); + t.merge(mRtBLASTSyncTransaction); } } }