From 2ec7b90b516ff741453affc03b78bb78fd28f3d3 Mon Sep 17 00:00:00 2001 From: Matt Casey Date: Wed, 16 Jun 2021 15:13:18 -0400 Subject: [PATCH] Long screenshot fixes for apps that return less than the requested rect - Use pixel height instead of tile count to determine ideal long screenshot height. - After adding each tile, go back and see if there are gaps in the tile set, if so then try to fill in the gap before continuing walking up or down. - Set max tiles to a constant as it's for system safety and not a completion criteria in the common cases. - Remove ImageTileSet's unused OnBoundsChangedListener (definition got more complicated now that we're dealing with gaps). Test: atest ScrollCaptureControllerTest Test: atest FakeSessionTest Bug: 189475409 Change-Id: I0cfc9519e7fa444d662ac41e0bef4fced785a98e --- .../systemui/screenshot/ImageTileSet.java | 66 ++++++------------- .../screenshot/ScrollCaptureClient.java | 23 ++++--- .../screenshot/ScrollCaptureController.java | 34 +++++----- .../systemui/screenshot/FakeSession.java | 12 +++- .../systemui/screenshot/FakeSessionTest.java | 54 ++++++++------- .../ScrollCaptureControllerTest.java | 26 ++++---- 6 files changed, 102 insertions(+), 113 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/screenshot/ImageTileSet.java b/packages/SystemUI/src/com/android/systemui/screenshot/ImageTileSet.java index 730702ec86859..51cc32ad39c1f 100644 --- a/packages/SystemUI/src/com/android/systemui/screenshot/ImageTileSet.java +++ b/packages/SystemUI/src/com/android/systemui/screenshot/ImageTileSet.java @@ -20,6 +20,7 @@ import android.graphics.Bitmap; import android.graphics.HardwareRenderer; import android.graphics.RecordingCanvas; import android.graphics.Rect; +import android.graphics.Region; import android.graphics.RenderNode; import android.graphics.drawable.Drawable; import android.os.Handler; @@ -46,7 +47,6 @@ class ImageTileSet { private static final String TAG = "ImageTileSet"; - private CallbackRegistry mOnBoundsListeners; private CallbackRegistry mContentListeners; @Inject @@ -54,14 +54,6 @@ class ImageTileSet { mHandler = handler; } - interface OnBoundsChangedListener { - /** - * Reports an update to the bounding box that contains all active tiles. These are virtual - * (capture) coordinates which can be either negative or positive. - */ - void onBoundsChanged(int left, int top, int right, int bottom); - } - interface OnContentChangedListener { /** * Mark as dirty and rebuild display list. @@ -70,25 +62,9 @@ class ImageTileSet { } private final List mTiles = new ArrayList<>(); - private final Rect mBounds = new Rect(); + private final Region mRegion = new Region(); private final Handler mHandler; - void addOnBoundsChangedListener(OnBoundsChangedListener listener) { - if (mOnBoundsListeners == null) { - mOnBoundsListeners = new CallbackRegistry<>( - new NotifierCallback() { - @Override - public void onNotifyCallback(OnBoundsChangedListener callback, - ImageTileSet sender, - int arg, Rect newBounds) { - callback.onBoundsChanged(newBounds.left, newBounds.top, newBounds.right, - newBounds.bottom); - } - }); - } - mOnBoundsListeners.add(listener); - } - void addOnContentChangedListener(OnContentChangedListener listener) { if (mContentListeners == null) { mContentListeners = new CallbackRegistry<>( @@ -110,14 +86,8 @@ class ImageTileSet { mHandler.post(() -> addTile(tile)); return; } - final Rect newBounds = new Rect(mBounds); - final Rect newRect = tile.getLocation(); mTiles.add(tile); - newBounds.union(newRect); - if (!newBounds.equals(mBounds)) { - mBounds.set(newBounds); - notifyBoundsChanged(mBounds); - } + mRegion.op(tile.getLocation(), mRegion, Region.Op.UNION); notifyContentChanged(); } @@ -127,12 +97,6 @@ class ImageTileSet { } } - private void notifyBoundsChanged(Rect bounds) { - if (mOnBoundsListeners != null) { - mOnBoundsListeners.notifyCallbacks(this, 0, bounds); - } - } - /** * Returns a drawable to paint the combined contents of the tiles. Drawable dimensions are * zero-based and map directly to {@link #getLeft()}, {@link #getTop()}, {@link #getRight()}, @@ -153,6 +117,15 @@ class ImageTileSet { return mTiles.size(); } + /** + * @return the bounding rect around any gaps in the tiles. + */ + Rect getGaps() { + Region difference = new Region(); + difference.op(mRegion.getBounds(), mRegion, Region.Op.DIFFERENCE); + return difference.getBounds(); + } + ImageTile get(int i) { return mTiles.get(i); } @@ -182,41 +155,40 @@ class ImageTileSet { } int getLeft() { - return mBounds.left; + return mRegion.getBounds().left; } int getTop() { - return mBounds.top; + return mRegion.getBounds().top; } int getRight() { - return mBounds.right; + return mRegion.getBounds().right; } int getBottom() { - return mBounds.bottom; + return mRegion.getBounds().bottom; } int getWidth() { - return mBounds.width(); + return mRegion.getBounds().width(); } int getHeight() { - return mBounds.height(); + return mRegion.getBounds().height(); } void clear() { if (mTiles.isEmpty()) { return; } - mBounds.setEmpty(); + mRegion.setEmpty(); Iterator i = mTiles.iterator(); while (i.hasNext()) { ImageTile next = i.next(); next.close(); i.remove(); } - notifyBoundsChanged(mBounds); notifyContentChanged(); } } diff --git a/packages/SystemUI/src/com/android/systemui/screenshot/ScrollCaptureClient.java b/packages/SystemUI/src/com/android/systemui/screenshot/ScrollCaptureClient.java index 94e3149487797..ce6e46937c253 100644 --- a/packages/SystemUI/src/com/android/systemui/screenshot/ScrollCaptureClient.java +++ b/packages/SystemUI/src/com/android/systemui/screenshot/ScrollCaptureClient.java @@ -56,6 +56,7 @@ import javax.inject.Inject; public class ScrollCaptureClient { private static final int TILE_SIZE_PX_MAX = 4 * (1024 * 1024); private static final int TILES_PER_PAGE = 2; // increase once b/174571735 is addressed + private static final int MAX_TILES = 30; @VisibleForTesting static final int MATCH_ANY_TASK = ActivityTaskManager.INVALID_TASK_ID; @@ -83,11 +84,12 @@ public class ScrollCaptureClient { int getMaxTiles(); /** - * @return the maximum combined capture height for this session, in pixels. + * Target pixel height for acquisition this session. Session may yield more or less data + * than this, but acquiring this height is considered sufficient for completion. + * + * @return target height in pixels. */ - default int getMaxHeight() { - return getMaxTiles() * getTileHeight(); - } + int getTargetHeight(); /** * @return the height of each image tile @@ -234,11 +236,11 @@ public class ScrollCaptureClient { private final int mTileWidth; private Rect mRequestRect; private boolean mStarted; + private final int mTargetHeight; private ICancellationSignal mCancellationSignal; private final Rect mWindowBounds; private final Rect mBoundsInWindow; - private final int mMaxTiles; private Completer mStartCompleter; private Completer mTileRequestCompleter; @@ -256,7 +258,7 @@ public class ScrollCaptureClient { mTileWidth = mBoundsInWindow.width(); mTileHeight = pxPerTile / mBoundsInWindow.width(); - mMaxTiles = (int) Math.ceil(maxPages * TILES_PER_PAGE); + mTargetHeight = (int) (mBoundsInWindow.height() * maxPages); if (DEBUG_SCROLL) { Log.d(TAG, "boundsInWindow: " + mBoundsInWindow); @@ -285,7 +287,7 @@ public class ScrollCaptureClient { private void start(Completer completer) { mReader = ImageReader.newInstance(mTileWidth, mTileHeight, PixelFormat.RGBA_8888, - mMaxTiles, HardwareBuffer.USAGE_GPU_SAMPLED_IMAGE); + MAX_TILES, HardwareBuffer.USAGE_GPU_SAMPLED_IMAGE); mStartCompleter = completer; try { mCancellationSignal = mConnection.startCapture(mReader.getSurface(), this); @@ -409,9 +411,14 @@ public class ScrollCaptureClient { return new Rect(mBoundsInWindow); } + @Override + public int getTargetHeight() { + return mTargetHeight; + } + @Override public int getMaxTiles() { - return mMaxTiles; + return MAX_TILES; } } } diff --git a/packages/SystemUI/src/com/android/systemui/screenshot/ScrollCaptureController.java b/packages/SystemUI/src/com/android/systemui/screenshot/ScrollCaptureController.java index bbcfdbd99bef1..4c1f6a19b96c8 100644 --- a/packages/SystemUI/src/com/android/systemui/screenshot/ScrollCaptureController.java +++ b/packages/SystemUI/src/com/android/systemui/screenshot/ScrollCaptureController.java @@ -199,22 +199,20 @@ public class ScrollCaptureController { Log.d(TAG, "onCaptureResult: " + result + " scrolling " + (mScrollingUp ? "UP" : "DOWN") + " finish on boundary: " + mFinishOnBoundary); boolean emptyResult = result.captured.height() == 0; - boolean partialResult = !emptyResult - && result.captured.height() < result.requested.height(); - boolean finish = false; if (emptyResult) { // Potentially reached a vertical boundary. Extend in the other direction. if (mFinishOnBoundary) { - Log.d(TAG, "Partial/empty: finished!"); - finish = true; + Log.d(TAG, "Empty: finished!"); + finishCapture(); + return; } else { // We hit a boundary, clear the tiles, capture everything in the opposite direction, // then finish. mImageTileSet.clear(); mFinishOnBoundary = true; mScrollingUp = !mScrollingUp; - Log.d(TAG, "Partial/empty: cleared, switch direction to finish"); + Log.d(TAG, "Empty: cleared, switch direction to finish"); } } else { // Got a non-empty result, but may already have enough bitmap data now @@ -223,12 +221,14 @@ public class ScrollCaptureController { Log.d(TAG, "Hit max tiles: finished"); // If we ever hit the max tiles, we've got enough bitmap data to finish // (even if we weren't sure we'd finish on this pass). - finish = true; + finishCapture(); + return; } else { if (mScrollingUp && !mFinishOnBoundary) { // During the initial scroll up, we only want to acquire the portion described // by IDEAL_PORTION_ABOVE. - if (expectedTiles >= mSession.getMaxTiles() * IDEAL_PORTION_ABOVE) { + if (mImageTileSet.getHeight() + result.captured.height() + >= mSession.getTargetHeight() * IDEAL_PORTION_ABOVE) { Log.d(TAG, "Hit ideal portion above: clear and switch direction"); // We got enough above the start point, now see how far down it can go. mImageTileSet.clear(); @@ -246,15 +246,15 @@ public class ScrollCaptureController { + " - " + mImageTileSet.getRight() + "," + mImageTileSet.getBottom() + " (" + mImageTileSet.getWidth() + "x" + mImageTileSet.getHeight() + ")"); - - // Stop when "too tall" - if (mImageTileSet.getHeight() > MAX_HEIGHT) { - Log.d(TAG, "Max height reached."); - finish = true; + Rect gapBounds = mImageTileSet.getGaps(); + if (!gapBounds.isEmpty()) { + Log.d(TAG, "Found gaps in tileset: " + gapBounds + ", requesting " + gapBounds.top); + requestNextTile(gapBounds.top); + return; } - if (finish) { - Log.d(TAG, "Stop."); + if (mImageTileSet.getHeight() >= mSession.getTargetHeight()) { + Log.d(TAG, "Target height reached."); finishCapture(); return; } @@ -268,8 +268,8 @@ public class ScrollCaptureController { : result.requested.bottom; } else { nextTop = (mScrollingUp) - ? result.captured.top - mSession.getTileHeight() - : result.captured.bottom; + ? mImageTileSet.getTop() - mSession.getTileHeight() + : mImageTileSet.getBottom(); } requestNextTile(nextTop); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/screenshot/FakeSession.java b/packages/SystemUI/tests/src/com/android/systemui/screenshot/FakeSession.java index 9c68f0d8a7ffd..478658eb232d5 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/screenshot/FakeSession.java +++ b/packages/SystemUI/tests/src/com/android/systemui/screenshot/FakeSession.java @@ -57,16 +57,19 @@ class FakeSession implements ScrollCaptureClient.Session { private int mScrollDelta; private int mPageHeight; + private int mTargetHeight; FakeSession(int pageHeight, float maxPages, int tileHeight, int visiblePageTop, - int visiblePageBottom, int availableTop, int availableBottom) { + int visiblePageBottom, int availableTop, int availableBottom, + int maxTiles) { mPageHeight = pageHeight; mTileHeight = tileHeight; mAvailable = new Rect(0, availableTop, getPageWidth(), availableBottom); mAvailableTop = new Rect(mAvailable); mAvailableTop.inset(0, 0, 0, pageHeight); mVisiblePage = new Rect(0, visiblePageTop, getPageWidth(), visiblePageBottom); - mMaxTiles = (int) Math.ceil((pageHeight * maxPages) / mTileHeight); + mTargetHeight = (int) (pageHeight * maxPages); + mMaxTiles = maxTiles; } private static Image mockImage() { @@ -157,6 +160,11 @@ class FakeSession implements ScrollCaptureClient.Session { return mMaxTiles; } + @Override + public int getTargetHeight() { + return mTargetHeight; + } + @Override public int getTileHeight() { return mTileHeight; diff --git a/packages/SystemUI/tests/src/com/android/systemui/screenshot/FakeSessionTest.java b/packages/SystemUI/tests/src/com/android/systemui/screenshot/FakeSessionTest.java index 2520af94380c6..4c8a4b0f8f61f 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/screenshot/FakeSessionTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/screenshot/FakeSessionTest.java @@ -41,21 +41,6 @@ import org.junit.runner.RunWith; @SmallTest @RunWith(AndroidTestingRunner.class) public class FakeSessionTest extends SysuiTestCase { - @Test - public void testMaxTiles() { - FakeSession session = new FakeSession( - /* pageHeight */ 100, - /* maxPages */ 2.25f, - /* tileHeight */ 10, - /* visiblePageTop */ 0, - /* visiblePageBottom */ 100, - /* availableTop */ -250, - /* availableBottom */ 250); - - // (pageHeight * maxPages) / tileHeight - assertEquals("getMaxTiles()", 23, session.getMaxTiles()); - } - @Test public void testNonEmptyResult_hasImage() { FakeSession session = new FakeSession( @@ -65,7 +50,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ 0, - /* availableBottom */ 100); + /* availableBottom */ 100, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(0)); assertNotNull("result.image", result.image); assertNotNull("result.image.getHardwareBuffer()", result.image.getHardwareBuffer()); @@ -80,7 +66,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ 0, - /* availableBottom */ 100); + /* availableBottom */ 100, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(-100)); assertNull("result.image", result.image); } @@ -94,7 +81,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ -250, - /* availableBottom */ 250); + /* availableBottom */ 250, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(0)); assertEquals("requested top", 0, result.requested.top); @@ -113,7 +101,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ -250, - /* availableBottom */ 250); + /* availableBottom */ 250, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(90)); assertEquals("requested top", 90, result.requested.top); @@ -132,7 +121,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ -250, - /* availableBottom */ 250); + /* availableBottom */ 250, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(-100)); assertEquals("requested top", -100, result.requested.top); @@ -151,7 +141,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ -250, - /* availableBottom */ 250); + /* availableBottom */ 250, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(150)); assertEquals("requested top", 150, result.requested.top); @@ -170,7 +161,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ -100, - /* availableBottom */ 100); + /* availableBottom */ 100, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(-125)); assertEquals("requested top", -125, result.requested.top); @@ -189,7 +181,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ -100, - /* availableBottom */ 100); + /* availableBottom */ 100, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(75)); assertEquals("requested top", 75, result.requested.top); @@ -211,7 +204,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 25, // <<-- /* visiblePageBottom */ 100, /* availableTop */ -150, - /* availableBottom */ 150); + /* availableBottom */ 150, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(-150)); assertEquals("requested top", -150, result.requested.top); @@ -233,7 +227,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 75, /* availableTop */ -150, - /* availableBottom */ 150); + /* availableBottom */ 150, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(50)); assertEquals("requested top", 50, result.requested.top); @@ -252,7 +247,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ -100, - /* availableBottom */ 200); + /* availableBottom */ 200, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(-150)); assertTrue("captured rect is empty", result.captured.isEmpty()); } @@ -266,7 +262,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 0, /* visiblePageBottom */ 100, /* availableTop */ -100, - /* availableBottom */ 200); + /* availableBottom */ 200, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(200)); assertTrue("captured rect is empty", result.captured.isEmpty()); } @@ -280,7 +277,8 @@ public class FakeSessionTest extends SysuiTestCase { /* visiblePageTop */ 60, // <<--- /* visiblePageBottom */ 0, /* availableTop */ -150, - /* availableBottom */ 150); + /* availableBottom */ 150, + /* max Tiles */ 30); ScrollCaptureClient.CaptureResult result = getUnchecked(session.requestTile(0)); assertEquals("requested top", 0, result.requested.top); diff --git a/packages/SystemUI/tests/src/com/android/systemui/screenshot/ScrollCaptureControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/screenshot/ScrollCaptureControllerTest.java index 5bab1bcddb6a0..10c878a92745b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/screenshot/ScrollCaptureControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/screenshot/ScrollCaptureControllerTest.java @@ -92,14 +92,16 @@ public class ScrollCaptureControllerTest extends SysuiTestCase { // Each tile is cropped to the visible page size, which is inset 5px from the TOP // requested result // 0, 50 5, 50 - // -45, 5 -40, 5 <-- clear previous / top - // 5, 55 5, 55 (not cropped, target is positioned fully within visible range) - // 55, 105 55, 105 - // 105, 155 105, 155 - // 155, 205 155, 205 <-- bottom + // -45, 5 -40, 5 + // -90, -40 -85, -40 <-- clear previous / top + // -40, 10 -40, 10 (not cropped, target is positioned fully within visible range) + // 10, 60 10, 60 + // 60, 110 60, 110 + // 110, 160 110, 160 + // 160, 210 160, 210 <-- bottom - assertEquals("top", -40, screenshot.getTop()); - assertEquals("bottom", 205, screenshot.getBottom()); + assertEquals("top", -85, screenshot.getTop()); + assertEquals("bottom", 210, screenshot.getBottom()); } @Test @@ -119,13 +121,14 @@ public class ScrollCaptureControllerTest extends SysuiTestCase { // requested result // 0, 50 0, 50 // not cropped, positioned within visible range // -50, 0 -50, 0 <-- clear previous/reverse - // 0, 50 - 0, 45 // target now positioned at page bottom, bottom cropped + // 0, 50 0, 45 // target now positioned at page bottom, bottom cropped // 45, 95, 45, 90 // 90, 140, 140, 135 - // 135, 185 185, 180 <-- bottom + // 135, 185 185, 180 + // 180, 230 180, 225 <-- bottom assertEquals("top", -50, screenshot.getTop()); - assertEquals("bottom", 180, screenshot.getBottom()); + assertEquals("bottom", 225, screenshot.getBottom()); } @Test @@ -265,7 +268,8 @@ public class ScrollCaptureControllerTest extends SysuiTestCase { mLocalVisibleBottom = mPageHeight; } Session session = new FakeSession(mPageHeight, mMaxPages, mTileHeight, - mLocalVisibleTop, mLocalVisibleBottom, mAvailableTop, mAvailableBottom); + mLocalVisibleTop, mLocalVisibleBottom, mAvailableTop, mAvailableBottom, + /* maxTiles */ 30); ScrollCaptureClient client = mock(ScrollCaptureClient.class); when(client.start(/* response */ any(), /* maxPages */ anyFloat())) .thenReturn(immediateFuture(session));