Init color array when decode for bitmap

[utzcoz]

It will try to initialize ShadowBitmap.color when decode those source to try to fix #3743.

It supports decodeByteArray, decodeFile, decodeResource, decodeStream, and decodeResourceStream.

[utzcoz]

@hoisie , could you help to review this PR? Thanks.

[hoisie]

Awesome, thanks for doing this. I'm running it against some tests internally to see if any issues come up.

[hoisie]

There were a few failures that got caught, here is one reduced example of a failure. It's a bitmap that gets serialized to a file as PNG, re-read, and then compared with the original. It passes without these changes:

  @Test
  public void testWriteToFile() throws IOException {
    byte[] bitmapData = new byte[] {23, 100, 23, 52, 23, 18, 76, 43};
    Bitmap bitmap1 = BitmapFactory.decodeByteArray(bitmapData, 0, bitmapData.length);
    Path tempFile = Files.createTempFile("bitmap", null);
    FileOutputStream fileOutputStream = new FileOutputStream(tempFile.toFile());
    bitmap1.compress(Bitmap.CompressFormat.PNG, 100, fileOutputStream); // lossless compression
    fileOutputStream.close();
    Bitmap bitmap2 = BitmapFactory.decodeFile(tempFile.toAbsolutePath().toString());

    ByteBuffer buffer1 = ByteBuffer.allocate(bitmap1.getHeight() * bitmap1.getRowBytes());
    bitmap1.copyPixelsToBuffer(buffer1);

    ByteBuffer buffer2 = ByteBuffer.allocate(bitmap2.getHeight() * bitmap2.getRowBytes());
    bitmap2.copyPixelsToBuffer(buffer2);
    assertThat(buffer1.array()).isEqualTo(buffer2.array());
  }

[utzcoz]

There were a few failures that got caught, here is one reduced example of a failure. It's a bitmap that gets serialized to a file as PNG, re-read, and then compared with the original. It passes without these changes:

  @Test
  public void testWriteToFile() throws IOException {
    byte[] bitmapData = new byte[] {23, 100, 23, 52, 23, 18, 76, 43};
    Bitmap bitmap1 = BitmapFactory.decodeByteArray(bitmapData, 0, bitmapData.length);
    Path tempFile = Files.createTempFile("bitmap", null);
    FileOutputStream fileOutputStream = new FileOutputStream(tempFile.toFile());
    bitmap1.compress(Bitmap.CompressFormat.PNG, 100, fileOutputStream); // lossless compression
    fileOutputStream.close();
    Bitmap bitmap2 = BitmapFactory.decodeFile(tempFile.toAbsolutePath().toString());

    ByteBuffer buffer1 = ByteBuffer.allocate(bitmap1.getHeight() * bitmap1.getRowBytes());
    bitmap1.copyPixelsToBuffer(buffer1);

    ByteBuffer buffer2 = ByteBuffer.allocate(bitmap2.getHeight() * bitmap2.getRowBytes());
    bitmap2.copyPixelsToBuffer(buffer2);
    assertThat(buffer1.array()).isEqualTo(buffer2.array());
  }

Thanks. Maybe the compress method doesn't write color correctly to file. I will add this test method local to retest it.

[utzcoz]

@hoisie , I rebase the pr to the current master branch, and add commits to write color array to stream when compressing. The old logic will use png image writer for png format, and jpeg image writer for other formats(jpeg, webp*), but the jpeg image writer of openjdk can't process alpha channel correctly, so I switch to use png image writer for all formats when compressing. And I don't remove unused format parameter of ImageUtil.writeToStream, because it is a public API, can maybe can already be used by user.

[utzcoz]

Rebase to master again after many merges.

[utzcoz]

Rebase after 4.5 release, @hoisie could you help to review it? Thanks.

[hoisie]

Thanks for the update, I am taking a closer look at this to try to understand it more deeply. As I am sure you have discovered by now, ShadowBitmap and ShadowBitmapFactory suffer from a lot of legacy APIs and buggy implementations, so it is difficult to navigate through that code.

[hoisie]

I just realized the bitmap data I pasted in #6147 (comment) wasn't even a valid bitmap. Perhaps like other APIs in ShadowBitmap or ShadowBitmapFactory, getPixel and compress should only do real actions if the underlying data is valid. Otherwise, they should be no-ops or return 0.

I am still looking into why #6147 (comment) is failing.

Long-term I would really like to enforce that the bitmap data passed to BitmapFactory is real bitmap data.

[hoisie]

I definitely want to make forward progress on this issue, so I'll do some more investigation tomorrow. I'm trying to gauge how many tests depend on the broken behavior of the existing ShadowBitmapFactory. One thing I'd like to do is to have warnings if users supply invalid image data in supplied BitmapFactory.decode*. Real Android will return null if invalid image data is supplied.

[hoisie]

FYI another test that passes before color array init and fails after:

  @Test
  public void testSameAs() {
    Bitmap bitmap = Bitmap.createBitmap(/* width= */ 10, /* height= */ 10, Config.ARGB_8888);
    ByteArrayOutputStream outStream = new ByteArrayOutputStream();
    bitmap.compress(CompressFormat.PNG, /* quality= */ 100, outStream);
    byte[] outBytes = outStream.toByteArray();
    ByteArrayInputStream inStream = new ByteArrayInputStream(outBytes);
    Bitmap bitmap2 = BitmapFactory.decodeStream(inStream);
    assertThat(bitmap.sameAs(bitmap2)).isTrue();
  }

I want to try this on real Android to see what happens.

[hoisie]

A couple other thoughts:

1. It's possible that one of the problems is that the color model (RGB?) of the colors array is not consistent with the color model of Java's BufferedImage (RGBA?). I need to do some more research about this. But it could be why the example above (Init color array when decode for bitmap #6147 (comment)) fails.

2. Longer term I could see us Robolectric towards using Skia to back bitmaps, which is what happens in real Android. If we can get that working, maybe it makes sense to have a separate set of Bitmap shadows that are higher fidelity (e.g. ShadowSkiaBitmap), and rename the old Bitmap shadows to something like ShadowLegacyBitmap, and then over the course of one or two versions deprecate and remove the legacy ones.

[utzcoz]

A couple other thoughts:

1. It's possible that one of the problems is that the color model (RGB?) of the colors array is not consistent with the color model of Java's BufferedImage (RGBA?). I need to do some more research about this. But it could be why the example above (Init color array when decode for bitmap #6147 (comment)) fails.

The ImageWriter supports different color space, and it supports ARGB. I can try it with new commit.

2. Longer term I could see us Robolectric towards using Skia to back bitmaps, which is what happens in real Android. If we can get that working, maybe it makes sense to have a separate set of Bitmap shadows that are higher fidelity (e.g. ShadowSkiaBitmap), and rename the old Bitmap shadows to something like ShadowLegacyBitmap, and then over the course of one or two versions deprecate and remove the legacy ones.

Awesome, it may solve this problem completely.

https://github.com/JetBrains/skija can be a good choice for integrate skia to Java.

[utzcoz]

FYI another test that passes before color array init and fails after:

  @Test
  public void testSameAs() {
    Bitmap bitmap = Bitmap.createBitmap(/* width= */ 10, /* height= */ 10, Config.ARGB_8888);
    ByteArrayOutputStream outStream = new ByteArrayOutputStream();
    bitmap.compress(CompressFormat.PNG, /* quality= */ 100, outStream);
    byte[] outBytes = outStream.toByteArray();
    ByteArrayInputStream inStream = new ByteArrayInputStream(outBytes);
    Bitmap bitmap2 = BitmapFactory.decodeStream(inStream);
    assertThat(bitmap.sameAs(bitmap2)).isTrue();
  }

I want to try this on real Android to see what happens.

@hoisie I add a new commit to add this test, and it passed without error.

[utzcoz]

I will bring back JPEG support, and try to use correct data for test.

[utzcoz]

The new commit bring back the support for JPEGImageWriter. But the downloaded webp test files from webp official website has the alpha value, so the new commit keep to use PngImageWriter for webp format. And If the compressed format is JPEG, the new commit will remove alpha channel from current pixel values.

Another thing should be noted is the JPEG is lossy compression, and the writer will change the pixel value although we use compress quality 100. So we can't get the same color array from compressed JPEG file. The Bitmap.compress for JPEG has
the same behavior on the official Android Emulator, and tested with the following instrumentation test code:

@test
fun testBitmapJpegCompress() {
    val appContext = InstrumentationRegistry.getInstrumentation().targetContext
    val oldBitmap = BitmapFactory.decodeResource(appContext.resources, R.drawable.test_jpeg)
    val baos = ByteArrayOutputStream()
    oldBitmap.compress(Bitmap.CompressFormat.JPEG, 100, baos)
    val bytes = baos.toByteArray()
    val newBitmap = BitmapFactory.decodeByteArray(bytes, 0, bytes.size)
    val oldBuffer = ByteBuffer.allocate(oldBitmap.height * oldBitmap.rowBytes)
    oldBitmap.copyPixelsToBuffer(oldBuffer)
    val newBuffer = ByteBuffer.allocate(newBitmap.height * newBitmap.rowBytes)
    newBitmap.copyPixelsToBuffer(newBuffer)
    assertEquals(oldBitmap.width, newBitmap.width)
    assertEquals(newBitmap.height, newBitmap.height)
    for (i in 0 until oldBitmap.width) {
        for (j in 0 until oldBitmap.height) {
            assertEquals(oldBitmap.getPixel(i, j), newBitmap.getPixel(i, j))
        }
    }
}

So the test method to read color array from compressed JPEG file is removed, because it can't pass on the real device.

@hoisie could you help to review to see whether there are other tests should be added? Thanks.

[utzcoz]

@hoisie rebased on recent master.

[hoisie]

Just one minor mimeType change and I can merge after that.

[utzcoz]

Just one minor mimeType change and I can merge after that.

Thanks. The change has be made by the new commit.

[hoisie]

Looking into one last weird ColorDrawable related issue. Probably minor, and likely has a simple fix.

[hoisie]

Excellent! Thanks for all your hard work and patience!

[utzcoz]

Excellent! Thanks for all your hard work and patience!

Thanks your patience to review it, and your awesome suggestions.