Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Screenshots are output in wrong width on Moto G7 Power #19

Closed
dbjorge opened this issue Jun 2, 2020 · 1 comment
Closed

Screenshots are output in wrong width on Moto G7 Power #19

dbjorge opened this issue Jun 2, 2020 · 1 comment
Labels
bug Something isn't working status: resolved This issue has been merged into main and deployed to canary.

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Jun 2, 2020

Describe the bug

On some devices (I saw this on a Moto G7 Power), the screenshot image data returned from the service includes some padding on the right side that shouldn't be there:

image

The root cause of this is our logic for copying data from the Image that we get back from the screenshot-grabbing routines into a Bitmap suitable for passing as output from the service. Specifically, our logic is a little off for cases where the Image we are given has a getRowStride() different from its getWidth().

Here's the code in question:

private Bitmap getBitmapFromImage(Image image) {
Image.Plane[] imagePlanes = image.getPlanes();
int bitmapWidth = getBitmapWidth(image, imagePlanes);
Bitmap screenshotBitmap =
bitmapProvider.createBitmap(bitmapWidth, metrics.heightPixels, Bitmap.Config.ARGB_8888);
ByteBuffer buffer = imagePlanes[0].getBuffer();
screenshotBitmap.copyPixelsFromBuffer(buffer);
return screenshotBitmap;
}
private int getBitmapWidth(Image image, Image.Plane[] imagePlanes) {
int pixelStride = imagePlanes[0].getPixelStride();
int rowStride = imagePlanes[0].getRowStride();
int rowPadding = rowStride - pixelStride * metrics.widthPixels;
return image.getWidth() + rowPadding / pixelStride;
}

The issue occurs when rowStride is larger than the image width (ie, there is some empty padding in the raw pixel data at the end of each row). Our code is correctly accounting for the extra padding when it initially creates the the Bitmap, such that the Bitmap's backing data buffer will be sized appropriately to receive the original data including the extra padding. However, the resulting Bitmap doesn't understand that width != stride, so it treats the extra padding as part of the image; that empty padding is what you're seeing in the top screenshot (the padding is actually transparent pixels).

Unfortunately, Bitmap::copyPixelsFromBuffer doesn't have a variant that accepts a stride parameter. Bitmap::createBitmap and Bitmap::setPixels have variants that accept strides, but only in combination with other bitmaps or int[] data, and the buffer we get back from the original Image is a "direct" buffer that doesn't allow array-like to its underlying buffer array. We may need to either do an intermediate copy to our own array to use with createBitmap, or write our own stride-aware version copyPixelsFromBuffer.

To Reproduce
Steps to reproduce the behavior:

  1. Install the service on a Moto G7 Power
    note: we aren't sure whether it's the specific device or the specific display metrics that trigger Android to output the Image properties required for the repro; if you don't have access to this specific device, you might try creating an emulator with the same OS level (9 / Pie) and same DisplayMetrics (720x1520 320dpi)
  2. Use Accessibility Insights for Android (prod or dev are both fine) to run a scan
  3. Observe the extra padding and wrongly-offset failure highlighting

Expected behavior

No extra padding, failure highlights line up with elements

Context (please complete the following information)

  • Android Version: 9 (Pie)
  • Service Version & Environment: Repros in 1.1.0 and the 1.2.0 release candidate
  • Target Application: Any
@dbjorge dbjorge added the bug Something isn't working label Jun 2, 2020
@ghost ghost added the status: new This issue is new and requires triage by DRI. label Jun 2, 2020
@ghost ghost assigned pownkel Jun 2, 2020
@pownkel pownkel assigned ferBonnin and unassigned pownkel Jun 2, 2020
@pownkel pownkel added status: ready for triage This issue is ready to be triaged by the Accessibility Insights team. and removed status: new This issue is new and requires triage by DRI. labels Jun 2, 2020
@ghost
Copy link

ghost commented Jun 2, 2020

This issue has been marked as ready for team triage; we will triage it in our weekly review and update the issue. Thank you for contributing to Accessibility Insights!

dbjorge added a commit that referenced this issue Jun 4, 2020
…d images (#20)

#### Description of changes

This fixes the issue described by #19 by detecting the case where the system has provided an image data with row padding and creating an intermediate, unpadded image buffer before invoking `Bitmap::copyPixelsFromBuffer`.

I explored a few other implementation options in the hopes of avoiding having to allocate the intermediate buffer, but they all came up short because `copyPixelsFromBuffer` is the only method `Bitmap` exposes that copies the backing data *exactly as-is* from the original source data, without applying any color transformations on the data. In particular, the variants of `Bitmap::createBitmap` and `Bitmap::setPixels` that accept `int[] colors` parameters look promising because some of them have built-in support for explicit `stride` parameters, but all of them perform color transformations (with or without `Bitmap::setPremultiplied`) that result in screenshots with incorrect colors.

Besides unit tests, will verify before merging against:
* [x] A device that exhibited the bad padding behavior before the fix (my Moto G7 Power)
* [x] An emulator that did not previously exhibit the bad padding behavior

**Before (note the extra padding at the right side of the screenshot image and the misaligned failure highlight):**

![image](https://user-images.githubusercontent.com/376284/83585855-c1027180-a4ff-11ea-8c01-375903509453.png)

**After (note padding is gone and highlight lines up):**

![image](https://user-images.githubusercontent.com/376284/83585805-a6c89380-a4ff-11ea-9de9-d96a44aff33c.png)


#### Pull request checklist

<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->

- [x] Addresses an existing issue: #19 
- [x] Added/updated relevant unit test(s)
- [x] Ran `./gradlew fastpass` from `AccessibilityInsightsForAndroidService`
- [x] PR title _AND_ final merge commit title both start with a semantic tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`).
@dbjorge dbjorge added the status: resolved This issue has been merged into main and deployed to canary. label Jun 4, 2020
@ghost ghost removed the status: ready for triage This issue is ready to be triaged by the Accessibility Insights team. label Jun 4, 2020
@dbjorge dbjorge closed this as completed Jun 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working status: resolved This issue has been merged into main and deployed to canary.
Projects
None yet
Development

No branches or pull requests

3 participants