[camera] Run iOS methods on UI thread by default #4140

renefloor · 2021-07-07T11:47:56Z

In flutter/flutter#25959 we found that not everything in the iOS camera implementation should be run on the UI thread, as that blocks the app UI. The solution was to run everything on a different thread.

In flutter/flutter#52578 we found that we should not run everything on a different thread, because for example sending the result should be done on the UI thread.

In PR #4007 it is decided that things should run on UI thread by default and exceptions are made for heavy operations.

In flutter/flutter#25959 the author stated that "it's recommended that AVCaptureSession operations be called on a dedicated serial DispatchQueue". That means that initWithCameraName, start and stop need to be run on a different thread, but I'm not sure if these are the only ones blocking the UI.

Pre-launch Checklist

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

…ading

packages/camera/camera/ios/Classes/CameraPlugin.m

# Conflicts: # packages/camera/camera/CHANGELOG.md

packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m

Co-authored-by: Maurits van Beusekom <[email protected]>

packages/camera/camera/ios/Classes/CameraPlugin.m

packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h

packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m

stuartmorgan-g · 2021-07-21T18:52:16Z

packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m

+/**
+ Called when result is successful. Sends "successWithData" to the notification center.
+ */
+- (void)successWithData:(id)data {


Shouldn't there be a thread assertion in this code?

What do you mean with the thread assertion? That we verify that it is not the main thread?

I don't remember what I meant by having that comment in this location unfortunately; maybe I was forgetting the flow and thinking this was after the bounce back to the main thread.

However, the more general questions is still valid: the goal of this PR is to ensure that methods are getting callbacks on the main thread, so why isn't there any code in the test that calls into the camera handler with a result object controlled by the test, and actually asserts that the callback was on the main thread? Like this: https://github.com/flutter/plugins/blob/master/packages/local_auth/example/ios/RunnerTests/FLTLocalAuthPluginTests.m#L62-L64

It is not clear to me what in this test would deterministically fail without the fix from this PR.

@stuartmorgan I have now added ThreadSafeFlutterResultTests that test that the result is always called on the main thread. These tests in CameraMethodChannelTests just verify that ThreadSafeFlutterResult is used, not how that is working.

packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h

packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m

…ading # Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/ios/Classes/CameraPlugin.m # packages/camera/camera/pubspec.yaml

…eading

stuartmorgan-g

Looks good overall, mostly just nits around test fixtures.

stuartmorgan-g · 2021-10-15T02:57:18Z

packages/camera/camera/example/ios/RunnerTests/CameraOrientationTests.m


+- (void)tearDown {
+  self.mockMessenger = nil;
+  self.cameraPlugin = nil;


I would actually much rather the ivars, the setup, and the teardown all be removed, and those two lines move into the test. Per the article I linked in a recent PR comment, stateless fixtures >> stateful fixtures, and in this case we're only saving two lines of boilerplate (assuming we eventually have more tests in this file; right now this is actually more code in order to theoretically reduce future duplication.)

stuartmorgan-g · 2021-10-15T02:59:02Z

packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m

+
+@interface CameraMethodChannelTests : XCTestCase
+@property(readonly, nonatomic) CameraPlugin *camera;
+@property(readonly, nonatomic) MockFLTThreadSafeFlutterResult *resultObject;


Per the comment in the other file: please remove these and make them local to the test. (Having the object under test be part of the fixture state is always a major red flag to me unless there's a very compelling reason.)

packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m

stuartmorgan-g · 2021-10-15T03:00:59Z

packages/camera/camera/example/ios/RunnerTests/CameraPreviewPauseTests.m

 - (void)setUp {
  _camera = [[FLTCam alloc] init];
+
+  _resultObject = [[MockFLTThreadSafeFlutterResult alloc] init];


This should be local (and while you're here, you could move _camera into the tests. It's one line, and locality of setup of the object under test is absolutely worth one extra line of boilerplate.)

stuartmorgan-g · 2021-10-15T03:02:31Z

packages/camera/camera/example/ios/RunnerTests/MockFLTThreadSafeFlutterResult.h

+ * read the received result.
+ */
+@interface MockFLTThreadSafeFlutterResult : FLTThreadSafeFlutterResult
+@property(readonly, nonatomic) XCTestExpectation *_Nonnull expectation;


Can't this use the nonnull @property decoration?

Yes this is possible, I used the Xcode "Fix" feature when the analyser warned me to specify a nonnull attribute and it added the _Nonnull declaration behind the pointer. It can be replaced with the nonnull attribute in the @property aswell (see update).

stuartmorgan-g · 2021-10-15T03:08:17Z

packages/camera/camera/ios/Classes/CameraPlugin_Test.h

    NS_DESIGNATED_INITIALIZER;
 - (instancetype)init NS_UNAVAILABLE;

+- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FLTThreadSafeFlutterResult *)result;


Needs a declaration comment.

stuartmorgan-g · 2021-10-15T03:08:35Z

packages/camera/camera/ios/Classes/CameraPlugin_Test.h

 - (instancetype)init NS_UNAVAILABLE;

+- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FLTThreadSafeFlutterResult *)result;
 - (void)orientationChanged:(NSNotification *)notification;


Blank line between methods please.

stuartmorgan-g · 2021-10-15T03:08:48Z

packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h

+/**
+ * Gets the original FlutterResult object wrapped by this FLTThreadSafeFlutterResult instance.
+ */
+@property(readonly, nonatomic) FlutterResult _Nonnull flutterResult;


stuartmorgan-g · 2021-10-15T03:09:11Z

packages/camera/camera/example/ios/RunnerTests/MockFLTThreadSafeFlutterResult.h

+@property(readonly, nonatomic) XCTestExpectation *_Nonnull expectation;
+@property(nonatomic, nullable) id receivedResult;
+
+- (instancetype _Nonnull)initWithExpectation:(XCTestExpectation *_Nonnull)expectation;


These should be nonnull as well.

stuartmorgan-g · 2021-10-15T03:09:40Z

packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h

+ * Initializes with a FlutterResult object.
+ * @param result The FlutterResult object that the result will be given to.
+ */
+- (nonnull id)initWithResult:(nonnull FlutterResult)result;


instancetype

mvanbeusekom

@stuartmorgan I resolved all nits, would appreciate it if you could have another look.

mvanbeusekom · 2021-10-18T07:33:08Z

packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m

+
+@interface CameraMethodChannelTests : XCTestCase
+@property(readonly, nonatomic) CameraPlugin *camera;
+@property(readonly, nonatomic) MockFLTThreadSafeFlutterResult *resultObject;


packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m

mvanbeusekom · 2021-10-18T07:34:03Z

packages/camera/camera/example/ios/RunnerTests/CameraOrientationTests.m


+- (void)tearDown {
+  self.mockMessenger = nil;
+  self.cameraPlugin = nil;


mvanbeusekom · 2021-10-18T07:34:27Z

packages/camera/camera/example/ios/RunnerTests/CameraPreviewPauseTests.m

 - (void)setUp {
  _camera = [[FLTCam alloc] init];
+
+  _resultObject = [[MockFLTThreadSafeFlutterResult alloc] init];


mvanbeusekom · 2021-10-18T07:36:50Z

packages/camera/camera/example/ios/RunnerTests/MockFLTThreadSafeFlutterResult.h

+ * read the received result.
+ */
+@interface MockFLTThreadSafeFlutterResult : FLTThreadSafeFlutterResult
+@property(readonly, nonatomic) XCTestExpectation *_Nonnull expectation;


Yes this is possible, I used the Xcode "Fix" feature when the analyser warned me to specify a nonnull attribute and it added the _Nonnull declaration behind the pointer. It can be replaced with the nonnull attribute in the @property aswell (see update).

mvanbeusekom · 2021-10-18T07:39:13Z

packages/camera/camera/example/ios/RunnerTests/MockFLTThreadSafeFlutterResult.m

+ * Called when result is successful.
+ *
+ * Fulfills the expectation.
+ */


mvanbeusekom · 2021-10-18T07:42:05Z

packages/camera/camera/ios/Classes/CameraPlugin_Test.h

    NS_DESIGNATED_INITIALIZER;
 - (instancetype)init NS_UNAVAILABLE;

+- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FLTThreadSafeFlutterResult *)result;


mvanbeusekom · 2021-10-18T07:42:07Z

packages/camera/camera/ios/Classes/CameraPlugin_Test.h

 - (instancetype)init NS_UNAVAILABLE;

+- (void)handleMethodCallAsync:(FlutterMethodCall *)call result:(FLTThreadSafeFlutterResult *)result;
 - (void)orientationChanged:(NSNotification *)notification;


mvanbeusekom · 2021-10-18T07:42:15Z

packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h

+/**
+ * Gets the original FlutterResult object wrapped by this FLTThreadSafeFlutterResult instance.
+ */
+@property(readonly, nonatomic) FlutterResult _Nonnull flutterResult;


mvanbeusekom · 2021-10-18T07:42:40Z

packages/camera/camera/ios/Classes/FLTThreadSafeFlutterResult.h

+ * Initializes with a FlutterResult object.
+ * @param result The FlutterResult object that the result will be given to.
+ */
+- (nonnull id)initWithResult:(nonnull FlutterResult)result;


…eading

stuartmorgan-g

LGTM with some last nits. Thanks for getting this over the line!

stuartmorgan-g · 2021-10-18T17:59:54Z

packages/camera/camera/example/ios/RunnerTests/CameraPreviewPauseTests.m

-    XCTAssertNil(result);
-    [resultExpectation fulfill];
-  }];
-  [self waitForExpectationsWithTimeout:2.0 handler:nil];


Don't you still need this (and an expectation passed to the MockFLTThreadSafeFlutterResult initialization) so that the assertions are guaranteed to run after the result callback?

I don't think so, the [CameraPlugin pausePreviewWithResult:] method that is tested has a very simple (synchronised) implementation and will not run its logic on a different queue (dispatching on different queue is done by the [CameraPlugin handleMethodCall] method).

Also the result object that is passed in is a simple mock implementation of the FLTThreadSafeFlutterResult class which simply echo's the value that is received by calling the [MockFLTThreadSafeFlutterResult sendSuccessWithData:] method through the MockFLTThreadSafeFlutterResult.receivedResult property.

So as far as I understand everything will run synchronously after each other.

stuartmorgan-g · 2021-10-18T18:00:02Z

packages/camera/camera/example/ios/RunnerTests/CameraPreviewPauseTests.m

-    XCTAssertNil(result);
-    [resultExpectation fulfill];
-  }];
-  [self waitForExpectationsWithTimeout:2.0 handler:nil];


See explanation above.

stuartmorgan-g · 2021-10-18T18:01:00Z

packages/camera/camera/example/ios/RunnerTests/MockFLTThreadSafeFlutterResult.h

+ * The expectation is fullfilled when a result is called allowing tests to await the result in an
+ * asynchronous manner.
+ */
+- (instancetype _Nonnull)initWithExpectation:(nonnull XCTestExpectation *)expectation;


nonnull instancetype

stuartmorgan-g · 2021-10-18T18:01:58Z

packages/camera/camera/ios/Classes/CameraPlugin_Test.h

+/// Exposes the [CameraPlugin handleMethodCallAsync:result:] method for unit testing.
+///
+/// This method should always be dispatched on a background queue to prevent deadlocks.
+


Remove the blank line here please.

stuartmorgan-g · 2021-10-18T18:03:45Z

packages/camera/camera/ios/Classes/CameraPlugin_Test.h

+/// Hide the default public constructor.
 - (instancetype)init NS_UNAVAILABLE;

+/// Exposes the [CameraPlugin handleMethodCallAsync:result:] method for unit testing.


The comments on these methods should be normal declaration comments explaining what the method actually does. The "exposed for unit testing" part belongs as a comment on the category itself (where it already is, correctly).

Improved the documentation as suggested.

stuartmorgan-g

Changes LGTM (this didn't need another review round)

…eading

Only run initWithCameraName on background thread.

dea50fc

renefloor requested a review from BeMacized July 7, 2021 11:47

google-cla bot added the cla: yes label Jul 7, 2021

github-actions bot added p: camera platform-ios labels Jul 7, 2021

renefloor added 12 commits July 7, 2021 14:02

run camera start async

da83b51

Start with migrating to ThreadSafeFlutterResult

277d923

Migrated all results to thread safe class

9bd75bd

make data nonNullable

a079af9

copyright

bd7d8eb

Merge remote-tracking branch 'flutter/master' into bugfix/camera-thre…

f0a2bb0

…ading

Added method channel tests

4125244

Extend unit test

71e321e

replace dispatch loop with notifications

bdfde1a

Made test much cleaner

e6b848c

add return for error

d59a80d

changelog and pubspec

0f35095

renefloor commented Jul 13, 2021

View reviewed changes

packages/camera/camera/ios/Classes/CameraPlugin.m Show resolved Hide resolved

renefloor added 2 commits July 13, 2021 15:12

Add documentation

0b63dea

Merge branch 'master' into bugfix/camera-threading

8bc557f

# Conflicts: # packages/camera/camera/CHANGELOG.md

renefloor force-pushed the bugfix/camera-threading branch from 266f96e to 8bc557f Compare July 13, 2021 14:18

mvanbeusekom reviewed Jul 14, 2021

View reviewed changes

packages/camera/camera/example/ios/RunnerTests/CameraMethodChannelTests.m Outdated Show resolved Hide resolved

renefloor and others added 2 commits July 14, 2021 09:17

Make interface an extension

b53ab3e

Co-authored-by: Maurits van Beusekom <[email protected]>

Increase timeout

b6acf82

renefloor marked this pull request as ready for review July 14, 2021 07:58

renefloor requested a review from bparrishMines as a code owner July 14, 2021 07:58

renefloor requested a review from stuartmorgan-g July 19, 2021 07:37

stuartmorgan-g suggested changes Jul 21, 2021

View reviewed changes

renefloor added 2 commits August 16, 2021 14:55

Merge remote-tracking branch 'flutter/master' into bugfix/camera-thre…

9f31156

…ading # Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/ios/Classes/CameraPlugin.m # packages/camera/camera/pubspec.yaml

update broken test

a4cf79e

Merge remote-tracking branch 'upstream/master' into bugfix/camera-thr…

98fe08a

…eading

mvanbeusekom force-pushed the bugfix/camera-threading branch from 9e3fb80 to 98fe08a Compare October 14, 2021 11:20

mvanbeusekom added 4 commits October 14, 2021 13:22

Remove development team from project file

a4e5262

Remove custom setting from XCScheme

39d1438

Clean up mock

3a12cbf

Clean reference to CameraPlugin

8e8bdfc

stuartmorgan-g suggested changes Oct 15, 2021

View reviewed changes

mvanbeusekom reviewed Oct 18, 2021

View reviewed changes

mvanbeusekom added 2 commits October 18, 2021 09:43

Apply feedback from PR.

3e86910

Merge remote-tracking branch 'upstream/master' into bugfix/camera-thr…

9093887

…eading

mvanbeusekom force-pushed the bugfix/camera-threading branch from 7362ca7 to 9093887 Compare October 18, 2021 07:48

stuartmorgan-g approved these changes Oct 18, 2021

View reviewed changes

mvanbeusekom requested a review from stuartmorgan-g October 18, 2021 19:58

mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Oct 18, 2021

Apply review feedback.

1d51203

mvanbeusekom force-pushed the bugfix/camera-threading branch from 19ca7bd to 1d51203 Compare October 18, 2021 19:59

stuartmorgan-g approved these changes Oct 19, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into bugfix/camera-thr…

f0b9f53

…eading

fluttergithubbot merged commit b8aea6d into flutter:master Oct 20, 2021

engine-flutter-autoroll mentioned this pull request Oct 20, 2021

Roll Plugins from fecd22e1de55 to b8aea6d0e73d (3 revisions) flutter/flutter#92192

Merged

AlexV525 mentioned this pull request Jan 14, 2022

🐛 Drop initialize when the controller has been already initialized fluttercandies/flutter_wechat_camera_picker#70

Merged

AlexV525 mentioned this pull request Jan 25, 2022

[camera] Permissions request are not well-awaiting on iOS flutter/flutter#97199

Closed

[camera] Run iOS methods on UI thread by default #4140

[camera] Run iOS methods on UI thread by default #4140

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g 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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

mvanbeusekom left a comment

Choose a reason for hiding this comment

Uh oh!

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

renefloor commented Jul 7, 2021 •

edited

Loading