From 38cf61f7046592bb4f28b6ef537aa848ae9e3041 Mon Sep 17 00:00:00 2001 From: Galia Peycheva Date: Thu, 2 Sep 2021 14:25:16 +0200 Subject: [PATCH] Fix missing position updates to blur regions The PositionUpdateListener sometimes gets called after the FrameDrawingCallback, because there is no ordering guarantee between these two callbacks. This causes the BlurRegions sent to SF to not have up-to-date positions. For example, an empty Rect gets sent as the blur region bounds, because the position update hasn't arrived when the surface transaction is sent. When the position update arrives it set the correct bounds, but it requires another draw to happen so that the correct position is picked up. This CL fixes this issue by saving the blur regions that were last sent to SF in FrameDrawingCallback and sending another transaction when the position update arrives. That transaction is merged into the previous one for the same frame, so the final transaction sent to SF has correct values. The CL also moves the FrameDrawingCallback registering logic entirely in the BlurAggregator in the first onPreDraw. This cleans up VRI Bug: 197239228 Test: atest --iterations 100 BlurTest#testBackgroundblurSimple Test: atest BlurAggregatorTest Change-Id: Ia122df40fdf2aa124299461c2e2597b61fa92699 --- core/java/android/view/ViewRootImpl.java | 21 ----- .../drawable/BackgroundBlurDrawable.java | 93 +++++++++++++------ .../src/android/view/BlurAggregatorTest.java | 22 ++--- 3 files changed, 78 insertions(+), 58 deletions(-) diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index 6665ab62703b2..e92e189b745b7 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -4184,26 +4184,6 @@ public final class ViewRootImpl implements ViewParent, }); } - @Nullable - private void registerFrameDrawingCallbackForBlur() { - if (!isHardwareEnabled()) { - return; - } - final boolean hasBlurUpdates = mBlurRegionAggregator.hasUpdates(); - final boolean needsCallbackForBlur = hasBlurUpdates || mBlurRegionAggregator.hasRegions(); - - if (!needsCallbackForBlur) { - return; - } - - final BackgroundBlurDrawable.BlurRegion[] blurRegionsForFrame = - mBlurRegionAggregator.getBlurRegionsCopyForRT(); - - // The callback will run on the render thread. - registerRtFrameCallback((frame) -> mBlurRegionAggregator - .dispatchBlurTransactionIfNeeded(frame, blurRegionsForFrame, hasBlurUpdates)); - } - private void registerCallbackForPendingTransactions() { registerRtFrameCallback(new FrameDrawingCallback() { @Override @@ -4241,7 +4221,6 @@ public final class ViewRootImpl implements ViewParent, mIsDrawing = true; Trace.traceBegin(Trace.TRACE_TAG_VIEW, "draw"); - registerFrameDrawingCallbackForBlur(); addFrameCommitCallbackIfNeeded(); boolean usingAsyncReport = isHardwareEnabled() && mSyncBufferCallback != null; diff --git a/core/java/com/android/internal/graphics/drawable/BackgroundBlurDrawable.java b/core/java/com/android/internal/graphics/drawable/BackgroundBlurDrawable.java index 402d7fed90c5a..4e1ecc281df32 100644 --- a/core/java/com/android/internal/graphics/drawable/BackgroundBlurDrawable.java +++ b/core/java/com/android/internal/graphics/drawable/BackgroundBlurDrawable.java @@ -36,6 +36,7 @@ import android.util.ArraySet; import android.util.Log; import android.util.LongSparseArray; import android.view.ViewRootImpl; +import android.view.ViewTreeObserver; import com.android.internal.R; import com.android.internal.annotations.GuardedBy; @@ -232,9 +233,12 @@ public final class BackgroundBlurDrawable extends Drawable { private final ArraySet mDrawables = new ArraySet(); @GuardedBy("mRtLock") private final LongSparseArray> mFrameRtUpdates = new LongSparseArray(); + private long mLastFrameNumber = 0; + private BlurRegion[] mLastFrameBlurRegions = null; private final ViewRootImpl mViewRoot; private BlurRegion[] mTmpBlurRegionsForFrame = new BlurRegion[0]; private boolean mHasUiUpdates; + private ViewTreeObserver.OnPreDrawListener mOnPreDrawListener; public Aggregator(ViewRootImpl viewRoot) { mViewRoot = viewRoot; @@ -277,6 +281,38 @@ public final class BackgroundBlurDrawable extends Drawable { Log.d(TAG, "Remove " + drawable); } } + + if (mOnPreDrawListener == null && mViewRoot.getView() != null + && hasRegions()) { + registerPreDrawListener(); + } + } + + private void registerPreDrawListener() { + mOnPreDrawListener = () -> { + final boolean hasUiUpdates = hasUpdates(); + + if (hasUiUpdates || hasRegions()) { + final BlurRegion[] blurRegionsForNextFrame = getBlurRegionsCopyForRT(); + + mViewRoot.registerRtFrameCallback(frame -> { + synchronized (mRtLock) { + mLastFrameNumber = frame; + mLastFrameBlurRegions = blurRegionsForNextFrame; + handleDispatchBlurTransactionLocked( + frame, blurRegionsForNextFrame, hasUiUpdates); + } + }); + } + if (!hasRegions() && mViewRoot.getView() != null) { + mViewRoot.getView().getViewTreeObserver() + .removeOnPreDrawListener(mOnPreDrawListener); + mOnPreDrawListener = null; + } + return true; + }; + + mViewRoot.getView().getViewTreeObserver().addOnPreDrawListener(mOnPreDrawListener); } // Called from a thread pool @@ -290,7 +326,14 @@ public final class BackgroundBlurDrawable extends Drawable { mFrameRtUpdates.put(frameNumber, frameRtUpdates); } frameRtUpdates.add(update); + + if (mLastFrameNumber == frameNumber) { + // The transaction for this frame has already been sent, so we have to manually + // trigger sending a transaction here in order to apply this position update + handleDispatchBlurTransactionLocked(frameNumber, mLastFrameBlurRegions, true); + } } + } /** @@ -329,29 +372,27 @@ public final class BackgroundBlurDrawable extends Drawable { /** * Called on RenderThread. * - * @return all blur regions if there are any ui or position updates for this frame, - * null otherwise + * @return true if it is necessary to send an update to Sf this frame */ + @GuardedBy("mRtLock") @VisibleForTesting - public float[][] getBlurRegionsToDispatchToSf(long frameNumber, - BlurRegion[] blurRegionsForFrame, boolean hasUiUpdatesForFrame) { - synchronized (mRtLock) { - if (!hasUiUpdatesForFrame && (mFrameRtUpdates.size() == 0 - || mFrameRtUpdates.keyAt(0) > frameNumber)) { - return null; - } + public float[][] getBlurRegionsForFrameLocked(long frameNumber, + BlurRegion[] blurRegionsForFrame, boolean forceUpdate) { + if (!forceUpdate && (mFrameRtUpdates.size() == 0 + || mFrameRtUpdates.keyAt(0) > frameNumber)) { + return null; + } - // mFrameRtUpdates holds position updates coming from a thread pool span from - // RenderThread. At this point, all position updates for frame frameNumber should - // have been added to mFrameRtUpdates. - // Here, we apply all updates for frames <= frameNumber in case some previous update - // has been missed. This also protects mFrameRtUpdates from memory leaks. - while (mFrameRtUpdates.size() != 0 && mFrameRtUpdates.keyAt(0) <= frameNumber) { - final ArraySet frameUpdates = mFrameRtUpdates.valueAt(0); - mFrameRtUpdates.removeAt(0); - for (int i = 0; i < frameUpdates.size(); i++) { - frameUpdates.valueAt(i).run(); - } + // mFrameRtUpdates holds position updates coming from a thread pool span from + // RenderThread. At this point, all position updates for frame frameNumber should + // have been added to mFrameRtUpdates. + // Here, we apply all updates for frames <= frameNumber in case some previous update + // has been missed. This also protects mFrameRtUpdates from memory leaks. + while (mFrameRtUpdates.size() != 0 && mFrameRtUpdates.keyAt(0) <= frameNumber) { + final ArraySet frameUpdates = mFrameRtUpdates.valueAt(0); + mFrameRtUpdates.removeAt(0); + for (int i = 0; i < frameUpdates.size(); i++) { + frameUpdates.valueAt(i).run(); } } @@ -370,13 +411,13 @@ public final class BackgroundBlurDrawable extends Drawable { } /** - * Called on RenderThread in FrameDrawingCallback. - * Dispatch all blur regions if there are any ui or position updates. + * Dispatch all blur regions if there are any ui or position updates for that frame. */ - public void dispatchBlurTransactionIfNeeded(long frameNumber, - BlurRegion[] blurRegionsForFrame, boolean hasUiUpdatesForFrame) { - final float[][] blurRegionsArray = getBlurRegionsToDispatchToSf(frameNumber, - blurRegionsForFrame, hasUiUpdatesForFrame); + @GuardedBy("mRtLock") + private void handleDispatchBlurTransactionLocked(long frameNumber, BlurRegion[] blurRegions, + boolean forceUpdate) { + float[][] blurRegionsArray = + getBlurRegionsForFrameLocked(frameNumber, blurRegions, forceUpdate); if (blurRegionsArray != null) { mViewRoot.dispatchBlurRegions(blurRegionsArray, frameNumber); } diff --git a/core/tests/coretests/src/android/view/BlurAggregatorTest.java b/core/tests/coretests/src/android/view/BlurAggregatorTest.java index b01f2755efdda..ded925e50a6a8 100644 --- a/core/tests/coretests/src/android/view/BlurAggregatorTest.java +++ b/core/tests/coretests/src/android/view/BlurAggregatorTest.java @@ -65,7 +65,7 @@ public class BlurAggregatorTest { drawable.setBlurRadius(TEST_BLUR_RADIUS); final boolean hasUpdates = mAggregator.hasUpdates(); final BlurRegion[] blurRegions = mAggregator.getBlurRegionsCopyForRT(); - mAggregator.getBlurRegionsToDispatchToSf(TEST_FRAME_NUMBER, blurRegions, hasUpdates); + mAggregator.getBlurRegionsForFrameLocked(TEST_FRAME_NUMBER, blurRegions, hasUpdates); return drawable; } @@ -154,7 +154,7 @@ public class BlurAggregatorTest { assertEquals(1, blurRegions.length); mDrawable.mPositionUpdateListener.positionChanged(TEST_FRAME_NUMBER, 1, 2, 3, 4); - mAggregator.getBlurRegionsToDispatchToSf(TEST_FRAME_NUMBER, blurRegions, + mAggregator.getBlurRegionsForFrameLocked(TEST_FRAME_NUMBER, blurRegions, mAggregator.hasUpdates()); assertEquals(1, blurRegions[0].rect.left); assertEquals(2, blurRegions[0].rect.top); @@ -169,7 +169,7 @@ public class BlurAggregatorTest { final BlurRegion[] blurRegions = mAggregator.getBlurRegionsCopyForRT(); assertEquals(1, blurRegions.length); - float[][] blurRegionsForSf = mAggregator.getBlurRegionsToDispatchToSf( + float[][] blurRegionsForSf = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER, blurRegions, hasUpdates); assertNull(blurRegionsForSf); } @@ -182,7 +182,7 @@ public class BlurAggregatorTest { final BlurRegion[] blurRegions = mAggregator.getBlurRegionsCopyForRT(); assertEquals(1, blurRegions.length); - float[][] blurRegionsForSf = mAggregator.getBlurRegionsToDispatchToSf( + float[][] blurRegionsForSf = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER, blurRegions, hasUpdates); assertNotNull(blurRegionsForSf); assertEquals(1, blurRegionsForSf.length); @@ -197,7 +197,7 @@ public class BlurAggregatorTest { assertEquals(1, blurRegions.length); mDrawable.mPositionUpdateListener.positionChanged(TEST_FRAME_NUMBER, 1, 2, 3, 4); - float[][] blurRegionsForSf = mAggregator.getBlurRegionsToDispatchToSf( + float[][] blurRegionsForSf = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER, blurRegions, hasUpdates); assertNotNull(blurRegionsForSf); assertEquals(1, blurRegionsForSf.length); @@ -216,7 +216,7 @@ public class BlurAggregatorTest { assertEquals(1, blurRegions.length); mDrawable.mPositionUpdateListener.positionChanged(TEST_FRAME_NUMBER, 1, 2, 3, 4); - float[][] blurRegionsForSf = mAggregator.getBlurRegionsToDispatchToSf( + float[][] blurRegionsForSf = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER + 1, blurRegions, hasUpdates); assertNotNull(blurRegionsForSf); assertEquals(1, blurRegionsForSf.length); @@ -237,19 +237,19 @@ public class BlurAggregatorTest { assertEquals(2, blurRegions.length); // Check that an update in one of the drawables triggers a dispatch of all blur regions - float[][] blurRegionsForSf = mAggregator.getBlurRegionsToDispatchToSf( + float[][] blurRegionsForSf = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER, blurRegions, hasUpdates); assertNotNull(blurRegionsForSf); assertEquals(2, blurRegionsForSf.length); // Check that the Aggregator deleted all position updates for frame TEST_FRAME_NUMBER - blurRegionsForSf = mAggregator.getBlurRegionsToDispatchToSf( + blurRegionsForSf = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER, blurRegions, /* hasUiUpdates= */ false); assertNull(blurRegionsForSf); // Check that a position update triggers a dispatch of all blur regions drawable2.mPositionUpdateListener.positionChanged(TEST_FRAME_NUMBER, 1, 2, 3, 4); - blurRegionsForSf = mAggregator.getBlurRegionsToDispatchToSf( + blurRegionsForSf = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER + 1, blurRegions, hasUpdates); assertNotNull(blurRegionsForSf); assertEquals(2, blurRegionsForSf.length); @@ -292,7 +292,7 @@ public class BlurAggregatorTest { mDrawable.mPositionUpdateListener.positionChanged(TEST_FRAME_NUMBER, 1, 2, 3, 4); mDrawable.mPositionUpdateListener.positionChanged(TEST_FRAME_NUMBER + 1, 5, 6, 7, 8); - final float[][] blurRegionsForSf = mAggregator.getBlurRegionsToDispatchToSf( + final float[][] blurRegionsForSf = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER, blurRegions, /* hasUiUpdates= */ false); assertNotNull(blurRegionsForSf); assertEquals(1, blurRegionsForSf.length); @@ -303,7 +303,7 @@ public class BlurAggregatorTest { assertEquals(3f, blurRegionsForSf[0][4]); assertEquals(4f, blurRegionsForSf[0][5]); - final float[][] blurRegionsForSfForNextFrame = mAggregator.getBlurRegionsToDispatchToSf( + final float[][] blurRegionsForSfForNextFrame = mAggregator.getBlurRegionsForFrameLocked( TEST_FRAME_NUMBER + 1, blurRegions, /* hasUiUpdates= */ false); assertNotNull(blurRegionsForSfForNextFrame); assertEquals(1, blurRegionsForSfForNextFrame.length);