[camera] Convert iOS Obj-C->Dart calls to Pigeon #6568

stuartmorgan · 2024-04-18T19:34:43Z

Converts all of the Obj-C -> Dart calls to Pigeon, using the new suffix-based Pigeon API instantiation feature.

This required decentralizing some threading code slightly: since method channel calls only involve one method (due to the lack of strong types), a wrapper that automatically did thread bouncing was feasible, but with Pigeon it's not since a wrapper would have to duplicate the entire API surface, and that's more work than just doing the dispatches at the call site.

Part of flutter/flutter#117905

Pre-launch Checklist

stuartmorgan · 2024-04-18T19:35:57Z

packages/camera/camera_avfoundation/ios/Classes/CameraProperties.h

@@ -30,49 +32,24 @@ extern AVCaptureFlashMode FLTGetAVCaptureFlashModeForFLTFlashMode(FLTFlashMode m

 #pragma mark - exposure mode

-/// Represents camera's exposure mode. Mirrors ExposureMode in camera.dart.
-typedef NS_ENUM(NSInteger, FLTExposureMode) {


These enums are no longer needed because we can just use the Pigeon-generated enums instead.

stuartmorgan · 2024-04-18T19:37:08Z

packages/camera/camera_avfoundation/ios/Classes/CameraProperties.h

-extern FLTExposureMode FLTGetFLTExposureModeForString(NSString *mode);
+/// Gets FCPPlatformExposureMode from its string representation.
+/// @param mode a string representation of the exposure mode.
+extern FCPPlatformExposureMode FCPGetExposureModeForString(NSString *mode);


These are temporary methods; there are Dart->host calls that send exposure and focus modes, so until I convert those in a follow-up we need to be able to convert method channel strings to the Pigeon enum that replaced the ones removed here.

stuartmorgan · 2024-04-18T19:38:07Z

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

+      _camera.dartAPI = [[FCPCameraEventApi alloc]
+          initWithBinaryMessenger:_messenger
+             messageChannelSuffix:[NSString stringWithFormat:@"%ld", cameraId]];
+      [_camera reportInitializationState];


I moved the actual channel invocation into FLTCam because it seemed strange to have it here when a) the camera instance already has the channel, and b) all of the values here come from the camera instance.

stuartmorgan · 2024-04-18T19:39:17Z

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

-  [UIDevice.currentDevice endGeneratingDeviceOrientationNotifications];
-}
-
- (void)startOrientationListener {


I inlined this as a drive-by cleanup because it was only being called from one place, and that was init, which shouldn't be calling methods.

stuartmorgan · 2024-04-18T19:40:52Z

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

-                     [NSString stringWithFormat:@"Unknown exposure mode %@", modeStr]
-               }]));
-    return;
-  }


I'm now preventing this error from being possible on the Dart side (same below).

stuartmorgan · 2024-04-18T19:44:01Z

packages/camera/camera_avfoundation/lib/src/avfoundation_camera.dart

@@ -552,6 +536,38 @@ class AVFoundationCamera extends CameraPlatform {
    return Texture(textureId: cameraId);
  }

+  String _serializeFocusMode(FocusMode mode) {


I added these instead of continuing to use the ones from the platform interface package (the versions without the _) because having serialization in the platform interface package and deserialization in the implementation package is an anti-pattern since it means changes in one package can break internal communication in another. We created a lot of it by accident during the initial process of federating plugins; Pigeon migration has been eliminating it, but doing it now meant I could remove all the native code that required an "invalid" state for these enums, since I can guarantee within this package that we won't get a state at the native layer that we don't handle.

(These methods will become enum->enum conversion methods instead of enum->string conversion when I finish the Pigeon migration for this package.)

stuartmorgan · 2024-04-18T19:48:27Z

packages/camera/camera_avfoundation/test/avfoundation_camera_test.dart

-      await streamQueue.cancel();
-    });
-
-    test('Should receive resolution changes', () async {


This and the next test are testing functionality that was dead code. The Dart method call handler handled several messages that are never sent by the native code. It doesn't look like Android uses them either, so I'm guessing it was code mistakenly left in during refactoring in code review of a feature.

hellohuanlin

Went through a thorough review. Looks pretty clean.

hellohuanlin · 2024-04-18T21:05:29Z

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

-         arguments:@{@"orientation" : FLTGetStringForUIDeviceOrientation(orientation)}];
+  __weak typeof(self) weakSelf = self;
+  dispatch_async(dispatch_get_main_queue(), ^{
+    [weakSelf.globalEventAPI


I vaguely remember there was an effort to make method channel thread safe. what's the progress on that and do we still need to manually dispatch to main in plugin code?

The current state is that replies are thread-safe, but calls that start on the host side still have to be done from the main thread. I don't think there's active work on addressing that part (although I believe it's still something we do eventually want to do).

hellohuanlin · 2024-04-18T21:08:04Z

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

 @interface CameraOrientationTests : XCTestCase
 @end

 @implementation CameraOrientationTests

+// Ensure that the given queue and then the main queue have both cycled, to wait for any pending
+// async events that may have been bounced between them.
+- (void)waitForRoundTripWithQueue:(dispatch_queue_t)queue {


hellohuanlin · 2024-04-18T21:16:09Z

packages/camera/camera_avfoundation/ios/Classes/CameraProperties.m

-}
-
-FLTFocusMode FLTGetFLTFocusModeForString(NSString *mode) {
+FCPPlatformFocusMode FCPGetFocusModeForString(NSString *mode) {
  if ([mode isEqualToString:@"auto"]) {


will these hard coded strings be gone eventually after pigeon work is done?

Yes, this is just for the remaining non-Pigeon Dart->host calls.

…ackages#6568)

flutter/packages@0e3809d...88a3a56 2024-04-19 stuartmorgan@google.com [camera] Convert iOS Obj-C->Dart calls to Pigeon (flutter/packages#6568) 2024-04-19 highdro0479@gmail.com [flutter_markdown] Ensure customize nested bullet list style. (flutter/packages#6384) 2024-04-18 stuartmorgan@google.com [ci] Add more dev dependency checks, and fix errors (flutter/packages#6563) 2024-04-18 engine-flutter-autoroll@skia.org Roll Flutter from 3882afb to fb110b9 (56 revisions) (flutter/packages#6565) 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 flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…r#147059) flutter/packages@0e3809d...88a3a56 2024-04-19 stuartmorgan@google.com [camera] Convert iOS Obj-C->Dart calls to Pigeon (flutter/packages#6568) 2024-04-19 highdro0479@gmail.com [flutter_markdown] Ensure customize nested bullet list style. (flutter/packages#6384) 2024-04-18 stuartmorgan@google.com [ci] Add more dev dependency checks, and fix errors (flutter/packages#6563) 2024-04-18 engine-flutter-autoroll@skia.org Roll Flutter from 3882afb to fb110b9 (56 revisions) (flutter/packages#6565) 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 flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Converts all of the Obj-C -> Dart calls to Pigeon, using the new suffix-based Pigeon API instantiation feature. This required decentralizing some threading code slightly: since method channel calls only involve one method (due to the lack of strong types), a wrapper that automatically did thread bouncing was feasible, but with Pigeon it's not since a wrapper would have to duplicate the entire API surface, and that's more work than just doing the dispatches at the call site. Part of flutter/flutter#117905

stuartmorgan added 6 commits April 18, 2024 15:33

Convert device rotation to Pigeon

4819ed7

Move the initialization message for consistency

c666b6a

Consolidate error method calls

7cce994

Remove ThreadSafeMethodChannel, as it can't be adapted

ab1f8da

Convert the camera-level flutter API calls

d7b3367

Version bump

80c9df5

stuartmorgan requested a review from hellohuanlin as a code owner April 18, 2024 19:34

github-actions bot added p: camera platform-ios platform-macos labels Apr 18, 2024

stuartmorgan commented Apr 18, 2024

View reviewed changes

hellohuanlin approved these changes Apr 18, 2024

View reviewed changes

stuartmorgan added 2 commits April 18, 2024 22:10

Merge branch 'main' into camera-ios-pigeon-flutter-api

9684752

Re-bump version

f2a1e4c

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

auto-submit bot merged commit 88a3a56 into flutter:main Apr 19, 2024
78 checks passed

engine-flutter-autoroll mentioned this pull request Apr 19, 2024

Roll Packages from 0e3809d995b6 to 88a3a561f16c (4 revisions) flutter/flutter#147059

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 19, 2024

88a3a561f [camera] Convert iOS Obj-C->Dart calls to Pigeon (flutter/p…

391cc27

…ackages#6568)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camera] Convert iOS Obj-C->Dart calls to Pigeon #6568

[camera] Convert iOS Obj-C->Dart calls to Pigeon #6568

stuartmorgan commented Apr 18, 2024

stuartmorgan Apr 18, 2024

stuartmorgan Apr 18, 2024

stuartmorgan Apr 18, 2024

stuartmorgan Apr 18, 2024

stuartmorgan Apr 18, 2024

stuartmorgan Apr 18, 2024

stuartmorgan Apr 18, 2024

hellohuanlin left a comment

hellohuanlin Apr 18, 2024

stuartmorgan Apr 19, 2024

hellohuanlin Apr 18, 2024

hellohuanlin Apr 18, 2024

stuartmorgan Apr 19, 2024

[camera] Convert iOS Obj-C->Dart calls to Pigeon #6568

[camera] Convert iOS Obj-C->Dart calls to Pigeon #6568

Conversation

stuartmorgan commented Apr 18, 2024

Pre-launch Checklist

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

hellohuanlin left a 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