[image_picker] Move I/O operations to a separate thread #3506

JeroenWeener · 2023-03-21T15:27:21Z

Many I/O operations in the image_picker are currently carried out on the main thread (at least on Android). This blocks the UI (main) thread, freezing the UI and sometimes even causing an ANR dialog to pop up.

With these PR, many of said I/O operations now run on a separate thread. Specifically, it executes all code that is run in response to a picking result on a separate thread. So when an image is picked, for example, the callback will be ran outside of the UI thread. This change was rather easy, as said code was already making use of async paradigms.

The I/O operations carried out for caching logic (ImagePickerDelegate.retrieveLostImage()) are not changed by this PR, as my guess is that this would call for breaking changes to the API, as the result is currently returned synchronously instead.

When running the example app in StrictMode and picking an image, the problem becomes apparent, as it reports disk read/write violations. This was described in #100966. I have verified that StrictMode no longer reports disk read/write violations after the changes. To check the output of StrictMode, I ran the example app with the changes listed here.

This PR closes several issues related to ANRs.

Fixes #94120
Fixes #114080

Also, it partly implements what is requested in #91393.

Pre-launch Checklist

stuartmorgan · 2023-03-31T02:27:50Z

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java


-    return true;
+    return requestCode == REQUEST_CODE_CHOOSE_IMAGE_FROM_GALLERY


The duplication here is error-prone; couldn't we instead have a local runnable and in each case assign the runnable to a lamba that calls the handle* method, then after the switch execute it if it's non-null and then return true? That would avoid too much duplication within the switch, while not duplicating the handled list.

You are right. I have updated the code with your suggestion.

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

stuartmorgan · 2023-04-11T17:51:07Z

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

+              ? ImagePickerCache.CacheType.IMAGE
+              : ImagePickerCache.CacheType.VIDEO);
+      if (pendingCallState.imageOptions != null) {
+        cache.saveDimensionWithOutputOptions(pendingCallState.imageOptions);


Generally it's better not to make calls inside of locks; the implementation won't have any indication that it's running in a lock, and the code with the lock doesn't control the implementation, so it's very easy to end up in a situation where due to later changes, very expensive code is running in a lock.

Since all this call needs is the image options, you can make a local variable that's set to pendingCallState.imageOptions inside the lock, and then make the call outside.

stuartmorgan · 2023-04-11T17:58:34Z

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

-    if (pendingCameraMediaUri != null) {
-      cache.savePendingCameraMediaUriPath(pendingCameraMediaUri);
+
+    synchronized (pendingCameraMediaUriLock) {


Same here.

In fact, you could probably avoid the pendingCameraMediaUriLock entirely by using locals anywhere you need to use it more than once:

final Uri mediaUrl = pendingMediaUrl; if (mediaUrl != null) { cache.save... }

If everything is just a single read or write of the variable, then it should be safe (based on what I can find about Java member variable atomicity).

stuartmorgan · 2023-04-11T18:00:05Z

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

-            }
-          });
+      synchronized (pendingCameraMediaUriLock) {
+        fileUriResolver.getFullImagePath(


This should definitely not be done inside the lock. Here too you can use a local, and then there's only a single access.

stuartmorgan · 2023-04-11T18:00:11Z

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

-              handleVideoResult(path);
-            }
-          });
+      synchronized (pendingCameraMediaUriLock) {


stuartmorgan · 2023-04-11T18:01:08Z

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

+      if (pendingCallState != null && pendingCallState.imageOptions != null) {
+        ArrayList<String> finalPath = new ArrayList<>();
+        for (int i = 0; i < paths.size(); i++) {
+          String finalImagePath = getResizedImagePath(paths.get(i), pendingCallState.imageOptions);


This is extremely expensive, and should not be done inside the lock. All you need locked is a one-item setting of a local to pendingCallState.imageOptions.

stuartmorgan · 2023-04-11T18:01:15Z

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

-        new File(path).delete();
+    synchronized (pendingCallStateLock) {
+      if (pendingCallState != null && pendingCallState.imageOptions != null) {
+        String finalImagePath = getResizedImagePath(path, pendingCallState.imageOptions);


reidbaker

+1 to Stuarts comments.

Thanks for the contribution!

JeroenWeener · 2023-04-12T10:23:14Z

Thanks for the feedback @stuartmorgan, it was really helpful. I refactored the code according to your comments so that the synchronized blocks contain as little code as they need and made sure that no functions are called inside the blocks.

stuartmorgan

LGTM, thanks!

@reidbaker Can you verify that my understanding of atomicity in Java is correct above, since I have a limited Java background?

tarrinneal · 2023-04-14T21:52:00Z

...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java

@@ -538,27 +557,31 @@ public boolean onRequestPermissionsResult(
  }

  @Override
-  public boolean onActivityResult(int requestCode, int resultCode, Intent data) {
+  public boolean onActivityResult(final int requestCode, final int resultCode, final Intent data) {
+    Runnable handlerRunnable;


is this meant to be handleRunnable? or is this a runnable handler?

It is a Runnable running a handler function (handleChooseImageResult etc.). Would runnableHandler make more sense in your opinion?

tarrinneal · 2023-04-26T18:25:21Z

Quick tag on @reidbaker to verify @stuartmorgan's question from earlier.

reidbaker

FWIW Multi threaded java is not my strong suit but this looks good to me.

* main: (46 commits) [go_router] Cleans up route match API and introduces dart fix (flutter#3819) [camerax] Add `LifecycleOwner` Proxy (flutter#3837) [file_selector] Add getDirectoryPaths implementation for Windows (flutter#3704) [various] Update Android example min SDKs (flutter#3847) Bump github/codeql-action from 2.2.12 to 2.3.2 (flutter#3838) [camerax] Implement Image Streaming (flutter#3454) [various] update agp and gradle for all examples in packages (flutter#3822) Update xcode to 14c18 (flutter#3774) [camera_android] Add NV21 as an image stream format flutter#3277 (flutter#3639) [go_router] Remove unused navigator keys (flutter#3708) [image_picker] Move I/O operations to a separate thread (flutter#3506) [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 1.8.20 to 1.8.21 in /packages/pigeon/platform_tests/test_plugin/android (flutter#3824) [pigeon] Reland: Add an initial example app (flutter#3832) Roll Flutter from c9004ff to 66fa4c5 (68 revisions) (flutter#3830) [various] Conditionalize the namespace in all Android plugins (flutter#3836) [in_app_pur]: Bump com.android.billingclient:billing from 5.1.0 to 5.2.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter#3672) [auick_action_ios] Retries multiple times to not fail ci when there is a flake (flutter#3823) Swap some iOS package CODEOWNERS (flutter#3793) [various] Add `targetCompatibility` to build.gradle (flutter#3825) [various] Set cmake_policy versions (flutter#3828) ...

…ter/packages#3506)

Roll Packages from 7e3f5da to de6131d (41 revisions) flutter/packages@7e3f5da...de6131d 2023-05-01 [email protected] I122213 update non examples (flutter/packages#3846) 2023-04-29 [email protected] [go_router] Cleans up route match API and introduces dart fix (flutter/packages#3819) 2023-04-29 [email protected] [camerax] Add `LifecycleOwner` Proxy (flutter/packages#3837) 2023-04-29 [email protected] [file_selector] Add getDirectoryPaths implementation for Windows (flutter/packages#3704) 2023-04-29 [email protected] [various] Update Android example min SDKs (flutter/packages#3847) 2023-04-28 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.2.12 to 2.3.2 (flutter/packages#3838) 2023-04-28 [email protected] [camerax] Implement Image Streaming (flutter/packages#3454) 2023-04-28 [email protected] [various] update agp and gradle for all examples in packages (flutter/packages#3822) 2023-04-28 [email protected] Update xcode to 14c18 (flutter/packages#3774) 2023-04-28 [email protected] [camera_android] Add NV21 as an image stream format #3277 (flutter/packages#3639) 2023-04-28 [email protected] [go_router] Remove unused navigator keys (flutter/packages#3708) 2023-04-28 [email protected] [image_picker] Move I/O operations to a separate thread (flutter/packages#3506) 2023-04-28 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 1.8.20 to 1.8.21 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#3824) 2023-04-28 [email protected] [pigeon] Reland: Add an initial example app (flutter/packages#3832) 2023-04-28 [email protected] Roll Flutter from c9004ff to 66fa4c5 (68 revisions) (flutter/packages#3830) 2023-04-28 [email protected] [various] Conditionalize the namespace in all Android plugins (flutter/packages#3836) 2023-04-27 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump com.android.billingclient:billing from 5.1.0 to 5.2.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#3672) 2023-04-27 [email protected] [auick_action_ios] Retries multiple times to not fail ci when there is a flake (flutter/packages#3823) 2023-04-26 [email protected] Swap some iOS package CODEOWNERS (flutter/packages#3793) 2023-04-26 [email protected] [various] Add `targetCompatibility` to build.gradle (flutter/packages#3825) 2023-04-26 [email protected] [various] Set cmake_policy versions (flutter/packages#3828) 2023-04-26 [email protected] [path_provider] Allow `win32` up to version 4.x (flutter/packages#3820) 2023-04-26 [email protected] [tool] Move Android lint checks (flutter/packages#3816) 2023-04-25 [email protected] [google_maps_flutter_android] Fix Android lint warnings (flutter/packages#3751) 2023-04-25 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.annotation:annotation from 1.5.0 to 1.6.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#3381) 2023-04-25 [email protected] Update test golden images for the latest Skia roll (flutter/packages#3787) 2023-04-25 [email protected] [various] Adds Android namespace (flutter/packages#3791) 2023-04-25 [email protected] [shared_preferences] Update gradle/agp in example apps (flutter/packages#3809) 2023-04-24 [email protected] [go_router] Adds name to TypedGoRoute (flutter/packages#3702) 2023-04-22 [email protected] [webview_flutter] Adds support to receive permission requests (flutter/packages#3543) 2023-04-21 [email protected] [google_sign_in] Fix Android Java warnings (flutter/packages#3762) 2023-04-21 [email protected] [local_auth] Fix enum return on Android (flutter/packages#3780) 2023-04-21 [email protected] [pigeon] Warn when trying to use enums in collections (flutter/packages#3782) 2023-04-21 [email protected] [webview_flutter_android] [webview_flutter_wkwebview] Platform implementations for supporting permission requests (flutter/packages#3792) 2023-04-21 [email protected] [pigeon] Update for compatibility with a future change to the analyzer. (flutter/packages#3789) 2023-04-21 [email protected] [camera_android] Fix Android lint warnings (flutter/packages#3716) 2023-04-21 [email protected] [webview_flutter_platform_interface] Adds method to receive permission requests (flutter/packages#3767) 2023-04-21 [email protected] [image_picker_ios] In unit test write and read kCGImagePropertyExifUserComment property (flutter/packages#3783) 2023-04-21 [email protected] [go_router_builder] Fixed the return value of the generated push method (flutter/packages#3650) 2023-04-21 [email protected] [image_picker] Mention `launchMode: singleInstance` in README (flutter/packages#3759) 2023-04-21 [email protected] Revert "[pigeon] Add an initial example app" (flutter/packages#3785) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human ...

The plugin accesses the disk on the UI thread at several occasions as reported by flutter/flutter#91393. These instances can easily be found by running the plugin with [StrictMode](https://developer.android.com/reference/android/os/StrictMode) enabled. The occasions that are highlighted are when determining the application's cache directory and when fetching `SharedPreferences`. By running method channel invocations on a background channel using Pidgeon's `@TaskQueue()` annotation and deferring said IO operations to the moment where they are actually needed, this PR makes sure the plugin no longer accesses the disk from the UI thread. This PR is a follow-up to #3506 - Fixes flutter/flutter#91393.

Many I/O operations in the `image_picker` are currently carried out on the main thread (at least on Android). This blocks the UI (main) thread, freezing the UI and sometimes even causing an ANR dialog to pop up. With these PR, many of said I/O operations now run on a separate thread. Specifically, it executes all code that is run in response to a picking result on a separate thread. So when an image is picked, for example, the callback will be ran outside of the UI thread. This change was rather easy, as said code was already making use of async paradigms. The I/O operations carried out for caching logic (`ImagePickerDelegate.retrieveLostImage()`) are **not** changed by this PR, as my guess is that this would call for breaking changes to the API, as the result is currently returned synchronously instead. When running the example app in [StrictMode](https://developer.android.com/reference/android/os/StrictMode) and picking an image, the problem becomes apparent, as it reports disk read/write violations. This was described in [#100966](flutter/flutter#100966). I have verified that StrictMode no longer reports disk read/write violations after the changes. To check the output of StrictMode, I ran the example app with the changes listed [here](math1man/plugins@11f5f8a). This PR closes several issues related to ANRs. * Fixes [#94120](flutter/flutter#94210) * Fixes [#114080](flutter/flutter#114080) Also, it partly implements what is requested in [#91393](flutter/flutter#91393).

github-actions bot added p: image_picker platform-android labels Mar 21, 2023

JeroenWeener marked this pull request as ready for review March 22, 2023 08:53

JeroenWeener requested a review from gmackall as a code owner March 22, 2023 08:53

JeroenWeener force-pushed the enhancement/offload-io-to-separate-thread branch 3 times, most recently from 0322ab2 to b402905 Compare March 24, 2023 09:20

This comment was marked as resolved.

Sign in to view

JeroenWeener force-pushed the enhancement/offload-io-to-separate-thread branch from b402905 to 39d2ef6 Compare March 28, 2023 10:49

stuartmorgan self-requested a review March 30, 2023 15:13

tarrinneal mentioned this pull request Mar 31, 2023

[image_picker] Resizing + compressing multiple images makes app freeze flutter/flutter#123531

Closed

stuartmorgan reviewed Mar 31, 2023

View reviewed changes

reidbaker self-requested a review March 31, 2023 17:31

JeroenWeener force-pushed the enhancement/offload-io-to-separate-thread branch 2 times, most recently from daf6211 to f1ae1f0 Compare April 5, 2023 09:54

JeroenWeener requested a review from stuartmorgan April 5, 2023 10:49

JeroenWeener force-pushed the enhancement/offload-io-to-separate-thread branch from 7ffbad4 to a1676ab Compare April 7, 2023 10:17

stuartmorgan requested changes Apr 11, 2023

View reviewed changes

reidbaker reviewed Apr 11, 2023

View reviewed changes

JeroenWeener force-pushed the enhancement/offload-io-to-separate-thread branch from 60e5c63 to b0113ba Compare April 12, 2023 10:13

JeroenWeener requested review from reidbaker and stuartmorgan April 12, 2023 10:21

stuartmorgan approved these changes Apr 13, 2023

View reviewed changes

tarrinneal reviewed Apr 14, 2023

View reviewed changes

JeroenWeener force-pushed the enhancement/offload-io-to-separate-thread branch from a3dd0c5 to f573ca2 Compare April 19, 2023 07:34

Zazo032 mentioned this pull request Apr 26, 2023

flutter image picker is slow to pick larger files flutter/flutter#124595

Open

reidbaker approved these changes Apr 26, 2023

View reviewed changes

Offloads result handling to separate thread

f1892ea

JeroenWeener added 8 commits April 28, 2023 10:05

Add tests

aeff575

Update version and changelog

7cfc713

Format files

06983b4

Restructure onActivityResult logic

2cba265

Synchronize setPendingOptionsAndResult

7fa1101

Remove unused imports

b3e32b1

Synchronize thread-unsafe code

38e67c1

Format ImagePickerDelegate.java

f42d307

JeroenWeener force-pushed the enhancement/offload-io-to-separate-thread branch from f573ca2 to f42d307 Compare April 28, 2023 08:06

Re-bump version

557bf02

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2023

auto-submit bot merged commit d155228 into flutter:main Apr 28, 2023

engine-flutter-autoroll mentioned this pull request May 1, 2023

Roll Packages from 7e3f5da42e81 to b01736711cc7 (40 revisions) flutter/flutter#125799

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2023

d155228 [image_picker] Move I/O operations to a separate thread (flut…

bb869e4

…ter/packages#3506)

engine-flutter-autoroll mentioned this pull request May 1, 2023

Roll Packages from 7e3f5da42e81 to de6131dbe4f7 (41 revisions) flutter/flutter#125811

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2023

d155228 [image_picker] Move I/O operations to a separate thread (flut…

99b3905

…ter/packages#3506)

JeroenWeener mentioned this pull request May 22, 2023

[image_picker] Move disk accesses to background thread #4058

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[image_picker] Move I/O operations to a separate thread #3506

[image_picker] Move I/O operations to a separate thread #3506

JeroenWeener commented Mar 21, 2023 •

edited by stuartmorgan

Loading

This comment was marked as resolved.

stuartmorgan Mar 31, 2023

JeroenWeener Apr 5, 2023

stuartmorgan Apr 11, 2023

stuartmorgan Apr 11, 2023

stuartmorgan Apr 11, 2023

stuartmorgan Apr 11, 2023

stuartmorgan Apr 11, 2023

stuartmorgan Apr 11, 2023

reidbaker left a comment

JeroenWeener commented Apr 12, 2023

stuartmorgan left a comment

tarrinneal Apr 14, 2023

JeroenWeener Apr 18, 2023

tarrinneal commented Apr 26, 2023

reidbaker left a comment


		return true;
		return requestCode == REQUEST_CODE_CHOOSE_IMAGE_FROM_GALLERY

[image_picker] Move I/O operations to a separate thread #3506

[image_picker] Move I/O operations to a separate thread #3506

Conversation

JeroenWeener commented Mar 21, 2023 • edited by stuartmorgan Loading

Pre-launch Checklist

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidbaker left a comment

Choose a reason for hiding this comment

JeroenWeener commented Apr 12, 2023

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal commented Apr 26, 2023

reidbaker left a comment

Choose a reason for hiding this comment

JeroenWeener commented Mar 21, 2023 •

edited by stuartmorgan

Loading