fix: avoid extra padding in screenshots on devices that use row-padde…

…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:`).

AccessibilityInsightsForAndroidService/app/src/main/java/com/microsoft/accessibilityinsightsforandroidservice/ImageFormatException.java

@@ -0,0 +1,10 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+package com.microsoft.accessibilityinsightsforandroidservice;
+
+class ImageFormatException extends Exception {
+  public ImageFormatException(String message) {
+    super(message);
+  }
+}

AccessibilityInsightsForAndroidService/app/src/main/java/com/microsoft/accessibilityinsightsforandroidservice/OnScreenshotAvailable.java

@@ -11,6 +11,9 @@
 import java.util.function.Consumer;
 
 public class OnScreenshotAvailable implements ImageReader.OnImageAvailableListener {
+  private final Bitmap.Config IMAGE_BITMAP_FORMAT = Bitmap.Config.ARGB_8888;
+  private final int IMAGE_PIXEL_STRIDE = 4; // Implied by ARGB_8888 (4 bytes per pixel)
+
   private static final String TAG = "OnScreenshotAvailable";
   private boolean imageAlreadyProcessed = false;
   private Consumer<Bitmap> bitmapConsumer;
@@ -31,26 +34,93 @@ public synchronized void onImageAvailable(ImageReader imageReader) {
     }
 
     Image image = imageReader.acquireLatestImage();
-    Bitmap screenshotBitmap = getBitmapFromImage(image);
-    image.close();
-    imageAlreadyProcessed = true;
-    bitmapConsumer.accept(screenshotBitmap);
+    Bitmap screenshotBitmap = null;
+    try {
+      screenshotBitmap = getBitmapFromImage(image);
+    } catch (ImageFormatException e) {
+      Logger.logError(TAG, "ImageFormatException: " + e.toString());
+    } finally {
+      image.close();
+    }
+
+    // If we failed to convert the image, we just log an error and don't forward anything on to the
+    // consumer that's forming the API response. From the API consumer's perspective, it will
+    // propagate as results with no screenshot data available.
+    if (screenshotBitmap != null) {
+      imageAlreadyProcessed = true;
+      bitmapConsumer.accept(screenshotBitmap);
+    }
   }
 
-  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 Bitmap getBitmapFromImage(Image image) throws ImageFormatException {
+    int width = image.getWidth();
+    int height = image.getHeight();
+    if (width != metrics.widthPixels || height != metrics.heightPixels) {
+      Logger.logError(
+          TAG,
+          "Received image of dimensions "
+              + width
+              + "x"
+              + height
+              + ", mismatches device DisplayMetrics "
+              + metrics.widthPixels
+              + "x"
+              + metrics.heightPixels);
+    }
+
+    Bitmap bitmap = bitmapProvider.createBitmap(width, height, IMAGE_BITMAP_FORMAT);
+    copyPixelsFromImagePlane(bitmap, image.getPlanes()[0], width, height);
+
+    return bitmap;
   }
 
-  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 source Image.Plane and the destination Bitmap use the same byte encoding for image data,
+  // 4 bytes per pixel in normal reading order, *except* that the Image.Plane can optionally contain
+  // padding bytes at the end of each row's worth of pixel data, which the Bitmap doesn't support.
+  //
+  // The "row stride" refers to the number of bytes per row, *including* any optional padding.
+  //
+  // If the source doesn't use any padding, we copy its backing ByteBuffer directly into the
+  // destination. If it *does* use padding, we create an intermediate ByteBuffer of our own and
+  // selectively copy just the real/unpadded pixel data into it first.
+  private void copyPixelsFromImagePlane(
+      Bitmap destination, Image.Plane source, int width, int height) throws ImageFormatException {
+    int sourcePixelStride = source.getPixelStride(); // bytes per pixel
+    int sourceRowStride = source.getRowStride(); // bytes per row, including any source row-padding
+    int unpaddedRowStride = width * sourcePixelStride; // bytes per row in destination
+
+    if (sourcePixelStride != IMAGE_PIXEL_STRIDE) {
+      throw new ImageFormatException(
+          "Invalid source Image: sourcePixelStride="
+              + sourcePixelStride
+              + ", expected "
+              + IMAGE_PIXEL_STRIDE);
+    }
+    if (sourceRowStride < unpaddedRowStride) {
+      throw new ImageFormatException(
+          "Invalid source Image: sourceRowStride "
+              + sourceRowStride
+              + " is too small for width "
+              + width
+              + " at sourcePixelStride "
+              + sourcePixelStride);
+    }
+
+    ByteBuffer sourceBuffer = source.getBuffer();
+    ByteBuffer bitmapPixelDataWithoutRowPadding;
+
+    if (sourceRowStride == unpaddedRowStride) {
+      bitmapPixelDataWithoutRowPadding = sourceBuffer;
+    } else {
+      bitmapPixelDataWithoutRowPadding = ByteBuffer.allocate(unpaddedRowStride * height);
+      for (int row = 0; row < height; ++row) {
+        int sourceOffset = row * sourceRowStride;
+        int destOffset = row * unpaddedRowStride;
+        sourceBuffer.position(sourceOffset);
+        sourceBuffer.get(bitmapPixelDataWithoutRowPadding.array(), destOffset, unpaddedRowStride);
+      }
+    }
+
+    destination.copyPixelsFromBuffer(bitmapPixelDataWithoutRowPadding);
   }
 }

AccessibilityInsightsForAndroidService/app/src/test/java/com/microsoft/accessibilityinsightsforandroidservice/OnScreenshotAvailableTest.java

@@ -4,6 +4,7 @@
 package com.microsoft.accessibilityinsightsforandroidservice;
 
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -20,37 +21,40 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 
-@RunWith(MockitoJUnitRunner.class)
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({Logger.class})
 public class OnScreenshotAvailableTest {
 
   @Mock ImageReader imageReaderMock;
   @Mock Consumer<Bitmap> bitmapConsumerMock;
   @Mock Image imageMock;
   @Mock Image.Plane imagePlaneMock;
-  @Mock ByteBuffer bufferMock;
   @Mock BitmapProvider bitmapProviderMock;
   @Mock Bitmap bitmapMock;
 
   Image.Plane[] imagePlanesStub;
   OnScreenshotAvailable testSubject;
-  int widthStub = 100;
-  int heightStub = 200;
+  int widthStub;
+  int heightStub;
+  ByteBuffer imagePlaneStubBuffer;
   int pixelStrideStub;
   int rowStrideStub;
-  int rowPadding;
-  int expectedBitmapWidth;
 
   @Before
   public void prepare() {
+    PowerMockito.mockStatic(Logger.class);
+
     DisplayMetrics metricsStub = new DisplayMetrics();
     metricsStub.widthPixels = widthStub;
     metricsStub.heightPixels = heightStub;
-    pixelStrideStub = 20;
-    rowStrideStub = 10;
-    rowPadding = rowStrideStub - pixelStrideStub * widthStub;
-    expectedBitmapWidth = widthStub + rowPadding / pixelStrideStub;
+    widthStub = 100;
+    heightStub = 200;
+    pixelStrideStub = 4;
+    rowStrideStub = widthStub * pixelStrideStub;
     imagePlanesStub = new Image.Plane[1];
     imagePlanesStub[0] = imagePlaneMock;
 
@@ -63,12 +67,60 @@ public void onScreenshotAvailableIsNotNull() {
   }
 
   @Test
-  public void onImageAvailableCreatesCorrectScreenshotBitmap() {
+  public void onImageAvailableIgnoresImagesWithInvalidPixelStrides() {
+    // pixelStride should be the number of bytes per pixel; our input should always be in ARGB_8888
+    // format, so it should be fixed at 4. But if it's not, we shouldn't crash.
+    pixelStrideStub = 5;
+
+    setupMocksToCreateBitmap();
+
+    testSubject.onImageAvailable(imageReaderMock);
+
+    verify(bitmapMock, times(0)).copyPixelsFromBuffer(imagePlaneStubBuffer);
+    verify(bitmapConsumerMock, times(0)).accept(bitmapMock);
+  }
+
+  @Test
+  public void onImageAvailableIgnoresImagesWithInvalidRowStrides() {
+    // rowStride should be the number of bytes per row plus optionally some padding; it should
+    // never be lower than widthStub * pixelStrideStub, but if it is, we shouldn't crash.
+    rowStrideStub = widthStub * pixelStrideStub - 1;
+
+    setupMocksToCreateBitmap();
+
+    testSubject.onImageAvailable(imageReaderMock);
+
+    verify(bitmapMock, times(0)).copyPixelsFromBuffer(imagePlaneStubBuffer);
+    verify(bitmapConsumerMock, times(0)).accept(bitmapMock);
+  }
+
+  @Test
+  public void onImageAvailableWithUnpaddedImageBufferCreatesBitmapDirectlyFromSourceBuffer() {
+    setupMocksToCreateBitmap();
+
+    testSubject.onImageAvailable(imageReaderMock);
+
+    verify(bitmapMock, times(1)).copyPixelsFromBuffer(imagePlaneStubBuffer);
+    verify(bitmapConsumerMock, times(1)).accept(bitmapMock);
+  }
+
+  @Test
+  public void onImageAvailableWithPaddedImageBufferStripsPaddingBeforeCopyingPixels() {
+    widthStub = 2;
+    heightStub = 2;
+    int rowPaddingBytes = 1;
+    rowStrideStub = (pixelStrideStub * widthStub + rowPaddingBytes);
+
+    imagePlaneStubBuffer =
+        ByteBuffer.wrap(new byte[] {1, 1, 1, 1, 2, 2, 2, 2, 0, 3, 3, 3, 3, 4, 4, 4, 4, 0});
+    ByteBuffer bufferWithPaddingRemoved =
+        ByteBuffer.wrap(new byte[] {1, 1, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4});
+
     setupMocksToCreateBitmap();
 
     testSubject.onImageAvailable(imageReaderMock);
 
-    verify(bitmapMock, times(1)).copyPixelsFromBuffer(bufferMock);
+    verify(bitmapMock, times(1)).copyPixelsFromBuffer(eq(bufferWithPaddingRemoved));
     verify(bitmapConsumerMock, times(1)).accept(bitmapMock);
   }
 
@@ -81,7 +133,6 @@ public void onImageAvailableProcessesImageOnlyOnce() {
         bitmapConsumerMock,
         imageMock,
         imagePlaneMock,
-        bufferMock,
         bitmapProviderMock,
         bitmapMock);
 
@@ -97,8 +148,9 @@ private void setupMocksToCreateBitmap() {
     when(imagePlaneMock.getPixelStride()).thenReturn(pixelStrideStub);
     when(imagePlaneMock.getRowStride()).thenReturn(rowStrideStub);
     when(imageMock.getWidth()).thenReturn(widthStub);
-    when(imagePlaneMock.getBuffer()).thenReturn(bufferMock);
-    when(bitmapProviderMock.createBitmap(expectedBitmapWidth, heightStub, Bitmap.Config.ARGB_8888))
+    when(imageMock.getHeight()).thenReturn(heightStub);
+    when(imagePlaneMock.getBuffer()).thenReturn(imagePlaneStubBuffer);
+    when(bitmapProviderMock.createBitmap(widthStub, heightStub, Bitmap.Config.ARGB_8888))
         .thenReturn(bitmapMock);
   }
 }