Add native Android image decoders supported by API 28+ #26746

bdero · 2021-06-14T21:00:13Z

Relevant docs: flutter.dev/go/image-decoding-registry and flutter.dev/go/fallback-image-decoding

Continuation of flutter/flutter#17356 and flutter/flutter#82603.
Resolves flutter/flutter#20522 for API 28+.

The NDK decoder (API 30+) is provided by Skia.
The SDK decoder (API 28+) is adapted from this branch by @dnfield.
- The primary difference is that it now performs the image decode on the IO thread and doesn't block the UI thread during the full decode.
- However, the UI thread still needs to block until the decoding task has finished parsing the header. This is because the Android SDK ImageDecoder does not split the header parsing and image decoding stages into separate operations such that they can be executed by different threads -- instead, when image decoding starts, the SDK's ImageDecoder provides the header data through a callback on the same thread before continuing on with decoding immediately.
- I also attempted to avoid the memcpy in GetPixels by retrieving an AHardwareBuffer for the decoded SDK Bitmap in order to produce an SkImage from it. But unfortunately, AndroidBitmap_getHardwareBuffer (NDK) and Bitmap.getHardwareBuffer (SDK) are only available in API 30+ and 31 respectively, and I didn't manage to work out any other API-permitting ways to do this.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

dnfield

Needs tests.

/cc @blasten FYI, @jason-simmons

shell/common/shell.cc

dnfield · 2021-06-15T18:26:48Z

shell/platform/android/android_image_generator.cc

+  // Since this decoder is only installed for API 28 and 29, there is no
+  // supported way to work around this unfortunate copy. It's possible to
+  // produce an `SkImage` direcly from an `AHardwareBuffer`, but
+  // `AndroidBitmap_getHardwareBuffer` and `Bitmap.getHardwareBuffer` are only
+  // available in API 30+ and 31+ respectively.


There is! But we'd have to do some dynamic lookups to see whether symbols are available.

I'm ok having this in place without doing that yet, but we should file an issue if one doesn't exist and add a TODO here for it.

I got most of the way through resolving/calling AndroidBitmap_getHardwareBuffer at runtime, but even SkImage::MakeFromAHardwareBuffer and the underlying GrAHardwareBufferImageGenerator are preprocessed out with with __ANDROID_API__ >= 26 due to reliance on e.g. AHardwareBuffer_acquire... :(

I could probably propose a reasonable patch to cheaply resolve the symbols in Skia instead of gating like this, but we gotta make some behavior decisions/maybe provide a way to query support, and so I made a note to discuss with Skia friends and follow up.

@LeonScroggins

I think it would make sense to update Skia to check for the symbols rather than using the prepocessor to remove them entirely.

I think for now we should just add a TODO to fix this when Skia API is available.

shell/platform/android/android_image_generator.cc

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

shell/platform/android/android_shell_holder.cc

shell/platform/android/android_image_generator.cc

bdero · 2021-06-15T18:58:29Z

@dnfield Ah sorry, this is still quite broken/untested, but thanks for the helpful comments. :)

dnfield · 2021-06-15T19:22:55Z

Ahh ok, I saw the review request and started looking. Feel free to let me know when it's ready for another pass.

bdero · 2021-07-12T17:05:36Z

@dnfield I finally got around to fixing this up and it's good to go for another pass.
I flipped on the grilled chicken heic test for Android and added coverage for the shell method. Let me know if this seems sufficient or if you think there are other kinds of tests/scenarios that should be tossed in.

bdero · 2021-07-13T21:41:09Z

Hmm, there's definitely a crash bug when running on arm64 caused by the AndroidImageGenerator address not making it back to NativeImageHeaderCallback correctly. Investigating it:

[ +396 ms] E/flutter ( 9134): [ERROR:flutter/shell/platform/android/android_image_generator.cc(95)] Old address: 526384806704
[  +11 ms] E/flutter ( 9134): [ERROR:flutter/shell/platform/android/android_image_generator.cc(177)] New address: 526653397424

bdero · 2021-07-15T05:18:18Z

All fixed!

shell/common/fixtures/shell_test.dart

shell/platform/android/android_image_generator.cc

dnfield · 2021-07-15T20:07:40Z

shell/platform/android/android_image_generator.cc

+  // The generator kicks off an IO task to decode everything, and calls to
+  // "GetInfo()" block until the header has been decoded. The decoder explicitly
+  // marks the height as -1 if the image is invalid or if the SDK image decoder
+  // is unavailable.


Is this true? I'm not seeing any IO tasks getting kicked off, decoding is all happening on the platform thread right?

AndroidImageGenerator::MakeFromData kicks off the task and is the only reasonable way to produce this ImageGenerator.

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

dnfield

Looking closer

dnfield · 2021-07-19T21:22:58Z

shell/platform/android/android_image_generator.cc

+std::unique_ptr<ImageGenerator> AndroidImageGenerator::MakeFromData(
+    sk_sp<SkData> data,
+    fml::RefPtr<fml::TaskRunner> task_runner) {
+  auto* generator = new AndroidImageGenerator(std::move(data));


Please use std::make_unique instead of the manual memory management here.

Ownership semantics needed to be deferred here due to the task capturing the generator, but I just realized there's a possible use-after-free that could happen if the framework were to discard/collect an ImageDescriptor without attempting to decode first (doesn't currently happen AFAICT).

I changed the API to serve shared_ptrs because of this.

dnfield

LGTM with nit about unique_ptr usage.

zanderso · 2021-07-21T15:21:31Z

shell/platform/android/android_image_generator.cc

+    return;
+  }
+
+  SkData::ReleaseProc on_release = [](const void* ptr, void* context) -> void {


Do we need to do anything to make sure that this runs on the correct thread?

Good question -- I think this is safe as-is. In practice, this currently always happens on a different thread. No explicit mentioning of thread safety in the docs for AndroidBitmap_unlockPixels, though.

SkData is thread safe.

And if we end up on a different thread, AttachCurrentThread will make sure we're allowed to make JNI calls on it. Or reuse the JNIEnv for that thread if it already attached.

zanderso · 2021-07-21T15:23:10Z

A rebase should fix the Linux Web Engine check.

…r/engine#26746)

This reverts commit 94beed9.

flutter-dashboard bot added the platform-android label Jun 14, 2021

google-cla bot added the cla: yes label Jun 14, 2021

bdero force-pushed the bdero/embedder-image-decoders branch 3 times, most recently from 726902a to 29d5d5c Compare June 14, 2021 22:06

dnfield suggested changes Jun 15, 2021

View reviewed changes

bdero force-pushed the bdero/embedder-image-decoders branch 3 times, most recently from b27c8d7 to 96dc5b5 Compare June 21, 2021 20:18

bdero force-pushed the bdero/embedder-image-decoders branch from 7b196ce to 36358d3 Compare June 29, 2021 07:35

bdero force-pushed the bdero/embedder-image-decoders branch 5 times, most recently from aebdac6 to 4d859e6 Compare July 9, 2021 23:31

bdero changed the title ~~WIP: Add native Android image decoders supported by API 28+~~ Add native Android image decoders supported by API 28+ Jul 9, 2021

bdero force-pushed the bdero/embedder-image-decoders branch from 93cdc4e to 846f27c Compare July 12, 2021 16:47

bdero requested a review from dnfield July 13, 2021 18:56

bdero force-pushed the bdero/embedder-image-decoders branch from 846f27c to f8d3df8 Compare July 15, 2021 05:04

dnfield reviewed Jul 15, 2021

View reviewed changes

shell/common/fixtures/shell_test.dart Outdated Show resolved Hide resolved

dnfield reviewed Jul 15, 2021

View reviewed changes

shell/platform/android/android_image_generator.cc Show resolved Hide resolved

dnfield reviewed Jul 15, 2021

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java Show resolved Hide resolved

dnfield reviewed Jul 15, 2021

View reviewed changes

bdero force-pushed the bdero/embedder-image-decoders branch from f8d3df8 to 54d2a59 Compare July 15, 2021 20:24

dnfield reviewed Jul 19, 2021

View reviewed changes

dnfield approved these changes Jul 19, 2021

View reviewed changes

bdero force-pushed the bdero/embedder-image-decoders branch 3 times, most recently from a5c427b to 954170f Compare July 20, 2021 20:51

zanderso reviewed Jul 21, 2021

View reviewed changes

bdero added 14 commits July 21, 2021 11:31

Add method to shell for installing image decoders

64fd3d9

Add Android NDK+SDK image decoders

ac48422

Fix SDK decoder build failures

58399e3

Pull GetImage into ImageGenerator to make it overridable per platform

7316599

Remove NDK decoder for now

6653854

Missing import

b4b86bb

Formatting

d445f4a

Address comments and include android in heic test

4185187

Add test for shell image decoder registration

fa71265

Run RegisterImageDecoder in platform task

8f17f3a

Fix native callback

3d90225

Address comments

291984f

Ref count ImageGenerators

ada9c3b

Fix deadlock with incompatible images

5962097

bdero force-pushed the bdero/embedder-image-decoders branch from d237dad to 5962097 Compare July 21, 2021 18:31

Add TODO for GetImage to use and avoid a copy in non-resizing cases

efbfdca

bdero merged commit 94beed9 into flutter:master Jul 21, 2021

engine-flutter-autoroll mentioned this pull request Jul 21, 2021

Roll Engine from 0c943571acc5 to 9970b947935d (4 revisions) flutter/flutter#86825

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 21, 2021

94beed9 Add native Android image decoder supported by API 28+ (flutte…

2972680

…r/engine#26746)

bdero mentioned this pull request Jul 22, 2021

Image widget does not support HEIC images flutter/flutter#20522

Closed

bdero added a commit that referenced this pull request Jul 23, 2021

Revert "Add native Android image decoder supported by API 28+ (#26746)"

bafeccf

This reverts commit 94beed9.

bdero mentioned this pull request Jul 23, 2021

Revert "Add native Android image decoders supported by API 28+" #27672

Closed

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Add native Android image decoder supported by API 28+ (flutter#26746)

8eadb26

Add native Android image decoders supported by API 28+ #26746

Add native Android image decoders supported by API 28+ #26746

Uh oh!

Conversation

bdero commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bdero commented Jun 15, 2021

Uh oh!

dnfield commented Jun 15, 2021

Uh oh!

bdero commented Jul 12, 2021

Uh oh!

bdero commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdero commented Jul 15, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso commented Jul 21, 2021

Uh oh!

Uh oh!

bdero commented Jun 14, 2021 •

edited

Loading

bdero commented Jul 13, 2021 •

edited

Loading

bdero Jul 21, 2021 •

edited

Loading