[camera]remove "selfRef" for SavePhotoDelegate and ensure thread safety #4780

hellohuanlin · 2022-02-09T20:26:30Z

selfReference is used to keep FLTSavePhotoDelegate around. This is a nice trick, but should only be used as a workaround. In our case this delegate object has a clear owner - the delegating object (FLTCam), which controls the lifecycle of this delegate.

The main idea is to have a map of refs in FLTCam (inProgressSavePhotoDelegates), and delete the ref in completion handler. We need to use the map (instead of a single pointer) because photo capture may overlap (e.g. live photos, though i don't think we support live photos yet)

Also fixes a thread safety issue when updating the delegate ref. Fix it together as it's an easy fix.

Both issues are tracked here: flutter/flutter#96429

Pre-launch Checklist

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

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

packages/camera/camera/ios/Classes/FLTSavePhotoDelegate.h

packages/camera/camera/ios/Classes/FLTSavePhotoDelegate.m

packages/camera/camera/ios/Classes/FLTCam.m

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

jmagman

The mocking looks much better.

jmagman · 2022-02-12T01:46:39Z

packages/camera/camera/ios/Classes/QueueHelper.m

 + (void)ensureToRunOnMainQueue:(void (^)(void))block {
  if (!NSThread.isMainThread) {
    dispatch_async(dispatch_get_main_queue(), block);
  } else {
    block();
  }
 }
+
+ (dispatch_queue_t)createQueueWithLabel:(const char *)label specific:(const char *)specific {


I generally don't like utility helpers that are only a few lines. It essentially makes developers who are probably already familiar with gcd APIs now have to do the mental jump of understanding what the camera createQueueWithLabel method is doing. This wraps a well-known standard library APIs around a bespoke helper method that doesn't do much and is more the letter than the spirit of DRY.

I don't have a strong opinion here, so will update.

On the one hand, I think the benefit of createQueueWithLabel:specific util function is that it combines 2 calls, so that the caller site won't forget to set the queue specific data; And the benefit of isCurrentlyOnQueueWithSpecific: util function is that it makes the assertion readable like english sentence (e.g. NSAssert([QueueHelper isCurrentlyOnQueueWithSpecific:fooSpec]) vs NSAssert(dispatch_get_specific(fooSpec))

But on the other hand you also made a very good point that these are 2 extra functions that developers have to learn.

…e thread safety"

format

jmagman

LGTM

jmagman · 2022-02-16T20:02:38Z

packages/camera/camera/ios/Classes/FLTSavePhotoDelegate.m

+
+@interface FLTSavePhotoDelegate ()
+/// The file path for the captured photo.
+@property(readonly, nonatomic) NSString *path;


Suggested change

@property(readonly, nonatomic) NSString *path;

@property(readonly, nonatomic, copy) NSString *path;

oh this is already readonly so i leave out copy

packages/camera/camera/ios/Classes/FLTCam.m

Co-authored-by: Jenn Magder <magder@google.com>

jmagman · 2022-02-16T21:14:27Z

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

@@ -37,7 +38,10 @@ - (instancetype)initWithRegistry:(NSObject<FlutterTextureRegistry> *)registry
  NSAssert(self, @"super init cannot be nil");
  _registry = [[FLTThreadSafeTextureRegistry alloc] initWithTextureRegistry:registry];
  _messenger = messenger;
-  _captureSessionQueue = dispatch_queue_create("io.flutter.camera.captureSessionQueue", NULL);
+  _captureSessionQueue = dispatch_queue_create(io.flutter.camera.captureSessionQueue, NULL);


error: use of undeclared identifier 'io' _captureSessionQueue = dispatch_queue_create(io.flutter.camera.captureSessionQueue, NULL);

fluttergithubbot · 2022-02-16T21:20:21Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite publishable has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite darwin-lint_podspecs has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite ios-build_all_plugins has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite ios-build_all_plugins has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite ios-platform_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite ios-platform_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

* google/master: (340 commits) [camera]remove "selfRef" for SavePhotoDelegate and ensure thread safety (flutter#4780) Roll Flutter from 919d205 to adafd66 (5 revisions) (flutter#4876) [google_sign_in] Update platform interface analysis options (flutter#4872) Roll Flutter from b623279 to 919d205 (2 revisions) (flutter#4871) Roll Flutter from 14a2b13 to b623279 (5 revisions) (flutter#4870) [image_picker] Update platform interface analysis options (flutter#4837) [ci] Re-enable stable webview Android tests (flutter#4867) Roll Flutter from 286c975 to 14a2b13 (1 revision) (flutter#4865) [local_auth] support localizedFallbackTitle in IOSAuthMessages (flutter#3806) [image_picker] Update app-facing and web analysis options (flutter#4838) Roll Flutter from 8386344 to 286c975 (4 revisions) (flutter#4864) Roll Flutter from e9f83cf to 8386344 (6 revisions) (flutter#4862) [camera] Switch web package to new analysis options (flutter#4834) [flutter_plugin_tool] Fix iOS/macOS naming (flutter#4861) [webview_flutter] Fix debuggingEnabled on Android (flutter#4859) Roll Flutter from ca2a751 to e9f83cf (10 revisions) (flutter#4857) fix license (flutter#4858) [video_player] add allowBackgroundPlayback option platform interface changes (flutter#4807) [video_player] Updated Pigeon version (flutter#4726) Roll Flutter from 381cb28 to ca2a751 (5 revisions) (flutter#4850) ... # Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/android/build.gradle # packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/Camera.java # packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/MethodCallHandlerImpl.java # packages/camera/camera/ios/Classes/CameraPlugin.m # packages/camera/camera/ios/camera.podspec # packages/camera/camera/lib/camera.dart # packages/camera/camera/lib/src/camera_controller.dart # packages/camera/camera/pubspec.yaml

…hread safety (flutter/plugins#4780)

github-actions bot added p: camera platform-ios labels Feb 9, 2022

hellohuanlin force-pushed the camera_delegate_ref_on_session_queue_and_remove_self_ref branch 8 times, most recently from 11f590d to ecffecf Compare February 9, 2022 21:41

hellohuanlin marked this pull request as ready for review February 9, 2022 22:01

hellohuanlin requested review from jmagman, stuartmorgan-g and cyanglaz February 9, 2022 22:02

jmagman reviewed Feb 10, 2022

View reviewed changes

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

stuartmorgan-g reviewed Feb 10, 2022

View reviewed changes

hellohuanlin force-pushed the camera_delegate_ref_on_session_queue_and_remove_self_ref branch 6 times, most recently from 44eb047 to fec77c1 Compare February 11, 2022 20:56

hellohuanlin requested review from stuartmorgan-g and jmagman February 11, 2022 20:57

hellohuanlin force-pushed the camera_delegate_ref_on_session_queue_and_remove_self_ref branch from fec77c1 to d7466e8 Compare February 11, 2022 21:00

jmagman reviewed Feb 12, 2022

View reviewed changes

hellohuanlin added 4 commits February 15, 2022 21:00

[camera]remove self ref pointer for save photo delegate, and make sur…

21a3eb6

…e thread safety"

remove queue test

8cc0bce

swap path error order

384b4cb

add asserts capture queue specific

3e9a5bb

hellohuanlin added 3 commits February 15, 2022 21:00

format

3c6dc57

format

create queue helper

04de994

remove queue helper functions that sets and getes queue specific data

e561222

hellohuanlin force-pushed the camera_delegate_ref_on_session_queue_and_remove_self_ref branch from d7466e8 to e561222 Compare February 16, 2022 05:06

hellohuanlin requested a review from jmagman February 16, 2022 05:08

jmagman approved these changes Feb 16, 2022

View reviewed changes

Update packages/camera/camera/ios/Classes/FLTCam.m

9ca4084

Co-authored-by: Jenn Magder <magder@google.com>

hellohuanlin 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 Feb 16, 2022

jmagman reviewed Feb 16, 2022

View reviewed changes

fluttergithubbot removed 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 Feb 16, 2022

fix compiler error

99a0b87

jmagman mentioned this pull request Feb 16, 2022

improve the iOS camera plugin thread model flutter/flutter#96429

Closed

14 tasks

bump version

2d4a4ab

hellohuanlin 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 Feb 17, 2022

fluttergithubbot merged commit 0b969a4 into flutter:main Feb 17, 2022

engine-flutter-autoroll mentioned this pull request Feb 17, 2022

Roll Plugins from 136f49b3e8ec to 0b969a4282dd (10 revisions) flutter/flutter#98659

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2022

0b969a428 [camera]remove "selfRef" for SavePhotoDelegate and ensure t…

6f6fb9f

…hread safety (flutter/plugins#4780)

engine-flutter-autoroll mentioned this pull request Feb 17, 2022

Roll Plugins from 136f49b3e8ec to ed29396208cd (12 revisions) flutter/flutter#98666

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2022

0b969a428 [camera]remove "selfRef" for SavePhotoDelegate and ensure t…

3f921e5

…hread safety (flutter/plugins#4780)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[camera]remove "selfRef" for SavePhotoDelegate and ensure thread safety #4780

[camera]remove "selfRef" for SavePhotoDelegate and ensure thread safety #4780

Uh oh!

hellohuanlin commented Feb 9, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmagman left a comment

Uh oh!

jmagman Feb 12, 2022

Uh oh!

hellohuanlin Feb 15, 2022

Uh oh!

jmagman left a comment

Uh oh!

jmagman Feb 16, 2022

Uh oh!

hellohuanlin Feb 16, 2022

Uh oh!

Uh oh!

jmagman Feb 16, 2022

Uh oh!

hellohuanlin Feb 16, 2022

Uh oh!

fluttergithubbot commented Feb 16, 2022

Uh oh!

Uh oh!

	@property(readonly, nonatomic) NSString *path;
	@property(readonly, nonatomic, copy) NSString *path;

[camera]remove "selfRef" for SavePhotoDelegate and ensure thread safety #4780

[camera]remove "selfRef" for SavePhotoDelegate and ensure thread safety #4780

Uh oh!

Conversation

hellohuanlin commented Feb 9, 2022 • 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!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Feb 12, 2022

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmagman Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Feb 16, 2022

Uh oh!

Uh oh!

hellohuanlin commented Feb 9, 2022 •

edited

Loading