[camera] Initial iOS Pigeon conversion #6553

stuartmorgan · 2024-04-17T14:48:45Z

Converts one platform channel method to Pigeon, setting up all the Pigeon plumbing and test scaffolding.

The Camera API surface is relatively large, so this lays a foundation for incremental conversion, minimizing the mixing of Pigeon setup with the individual method conversions.

Part of flutter/flutter#117905

Pre-launch Checklist

Converts one platform channel method to Pigeon, setting up all the Pigeon plumbing and test scaffolding. The Camera API surface is relatively large, so this lays a foundation for incremental conversion, minimizing the mixing of Pigeon setup with the individual method conversions. Part of flutter/flutter#117905

stuartmorgan · 2024-04-17T14:49:28Z

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

@@ -15,8 +15,7 @@ @implementation AvailableCamerasTest

 - (void)testAvailableCamerasShouldReturnAllCamerasOnMultiCameraIPhone {
  CameraPlugin *camera = [[CameraPlugin alloc] initWithRegistry:nil messenger:nil];
-  XCTestExpectation *expectation =
-      [[XCTestExpectation alloc] initWithDescription:@"Result finished"];
+  XCTestExpectation *expectation = [self expectationWithDescription:@"Result finished"];


The change to the utility method allows using waitForExpectationsWithTimeout: instead of having to list every expectation.

stuartmorgan · 2024-04-17T14:50:35Z

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

+        resultValue = result;
+        [expectation fulfill];
+      }];
+  [self waitForExpectationsWithTimeout:30 handler:nil];


I'm not sure why we weren't actually using the expectation before, but now it's necessary because the queue dispatch isn't being bypassed in the test like it was before, so the block above is no longer run synchronously in the test.

stuartmorgan · 2024-04-17T14:51:28Z

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

  if (@available(iOS 13.0, *)) {
-    XCTAssertTrue([dictionaryResult count] == 4);
+    XCTAssertEqual(resultValue.count, 4);


This is just a quality of life improvement for when the test fails. (I had a failure initially because I didn't add the wait, so resultValue was nil, and I got a failure with no actual info.)

stuartmorgan · 2024-04-17T14:55:38Z

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

+                      FlutterError *_Nullable))completion {
+  // This doesn't interact with FLTCam, so can use an arbitrary thread rather than
+  // captureSessionQueue. It should still not be done on the main thread, however.
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{


We will unfortunately need to have the thread dispatch in every individual method in the Pigeon version (instead of handleMethodCall doing it for everything at once). Almost everything needs to use captureSessionQueue, since FLTCam sometimes needs to redispatch to that queue in internal callbacks to interact with instance state, which means we can't just use task queues.

(Also IIRC task queues aren't implemented for macOS, and we want to enable this plugin for macOS at some point.)

It'll just be a couple of lines in each method though, so it shouldn't be too bad. I think we can simplify when we migrate to Swift.

I think it's still better to use captureSessionQueue for all session configurations, in case Apple made any assumptions in its internal implementation (e.g. I wouldn't be surprised if multiple capture session related API access some common internal states).

(AVCaptureDeviceDiscoverySession's API doc doesn't say anything, but Apple's sample project uses this convention)

Done. It hadn't occurred to me that in theory the unique device IDs could be thread-specific; that would be weird, but not implausible.

Ya I bet we should be fine either way, but just wanna be extra safe here.

stuartmorgan · 2024-04-17T14:56:44Z

packages/camera/camera_avfoundation/test/utils_test.dart

        CameraLensDirection.external,
      );
    });

-    test(
-        'Should throw ArgumentException when invalid value is supplied when parsing camera lens direction',


This is no longer possible, so we don't need a test for it.

flutter/packages@d39830e...0e3809d 2024-04-18 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.0 to 3.25.1 (flutter/packages#6562) 2024-04-18 [email protected] [ci] Add Linux desktop support to Linux custom_package_tests (flutter/packages#6551) 2024-04-17 [email protected] [two_dimensional_scrollables] Refactor Spans for common use (flutter/packages#6550) 2024-04-17 [email protected] [in_app_purchase] Add api to expose country code (flutter/packages#6540) 2024-04-17 [email protected] [camera] Initial iOS Pigeon conversion (flutter/packages#6553) 2024-04-17 [email protected] [in_app_purchase] Add countryCode implementation to android and storekit (flutter/packages#6556) 2024-04-17 [email protected] [google_sign_in_ios] Upgrade GoogleSignIn iOS SDK to 7.1 (flutter/packages#6404) 2024-04-17 [email protected] [in_app_purchase_platform_interface] Adds countryCode API (flutter/packages#6548) 2024-04-17 [email protected] [google_maps_flutter] Update app-facing package iOS requirements (flutter/packages#6552) 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 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

emakar · 2024-04-18T18:26:11Z

packages/camera/camera_avfoundation/pubspec.yaml

@@ -20,13 +20,15 @@ dependencies:
  camera_platform_interface: ^2.7.0
  flutter:
    sdk: flutter
+  pigeon: ^18.0.0


shouldn't it come as dev dependency?

Yes, see #6563

flutter/packages@d39830e...0e3809d 2024-04-18 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.0 to 3.25.1 (flutter/packages#6562) 2024-04-18 [email protected] [ci] Add Linux desktop support to Linux custom_package_tests (flutter/packages#6551) 2024-04-17 [email protected] [two_dimensional_scrollables] Refactor Spans for common use (flutter/packages#6550) 2024-04-17 [email protected] [in_app_purchase] Add api to expose country code (flutter/packages#6540) 2024-04-17 [email protected] [camera] Initial iOS Pigeon conversion (flutter/packages#6553) 2024-04-17 [email protected] [in_app_purchase] Add countryCode implementation to android and storekit (flutter/packages#6556) 2024-04-17 [email protected] [google_sign_in_ios] Upgrade GoogleSignIn iOS SDK to 7.1 (flutter/packages#6404) 2024-04-17 [email protected] [in_app_purchase_platform_interface] Adds countryCode API (flutter/packages#6548) 2024-04-17 [email protected] [google_maps_flutter] Update app-facing package iOS requirements (flutter/packages#6552) 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 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 one platform channel method to Pigeon, setting up all the Pigeon plumbing and test scaffolding. The Camera API surface is relatively large, so this lays a foundation for incremental conversion, minimizing the mixing of Pigeon setup with the individual method conversions. Part of flutter/flutter#117905

Convert one method to use Pigeon to setup the backbone of future conversions. Android analog of #6553 Part of flutter/flutter#117905 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [ ] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [x] I [linked to at least one issue that this PR fixes] in the description above. - [x] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [CLA]: https://cla.developers.google.com/ [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md [linked to at least one issue that this PR fixes]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [pub versioning philosophy]: https://dart.dev/tools/pub/versioning [exempt from version changes]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version [following repository CHANGELOG style]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog-style [exempt from CHANGELOG changes]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests

stuartmorgan requested a review from hellohuanlin as a code owner April 17, 2024 14:48

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

stuartmorgan commented Apr 17, 2024

View reviewed changes

stuartmorgan added 2 commits April 17, 2024 14:45

Switch to capture session thread

4713987

analyzer warning

e449624

hellohuanlin approved these changes Apr 17, 2024

View reviewed changes

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

auto-submit bot merged commit ba1e24b into flutter:main Apr 17, 2024
78 checks passed

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

Roll Packages from d39830e40c07 to 0e3809d995b6 (9 revisions) flutter/flutter#146992

Merged

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

ba1e24b [camera] Initial iOS Pigeon conversion (flutter/packages#6553)

448971c

emakar reviewed Apr 18, 2024

View reviewed changes

stuartmorgan deleted the camera-ios-pigeon branch April 18, 2024 18:29

stuartmorgan mentioned this pull request Sep 26, 2024

[camera] Convert to Pigeon flutter/flutter#117905

Closed

3 tasks

yaakovschectman mentioned this pull request Oct 1, 2024

[camera_android] Begin conversion to Pigeon #7755

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camera] Initial iOS Pigeon conversion #6553

[camera] Initial iOS Pigeon conversion #6553

stuartmorgan commented Apr 17, 2024

stuartmorgan Apr 17, 2024

stuartmorgan Apr 17, 2024

stuartmorgan Apr 17, 2024

stuartmorgan Apr 17, 2024

hellohuanlin Apr 17, 2024

stuartmorgan Apr 17, 2024

hellohuanlin Apr 17, 2024

stuartmorgan Apr 17, 2024

emakar Apr 18, 2024

stuartmorgan Apr 18, 2024

[camera] Initial iOS Pigeon conversion #6553

[camera] Initial iOS Pigeon conversion #6553

Conversation

stuartmorgan commented Apr 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment