[camera] Finish converting iOS to Pigeon #6601

stuartmorgan-g · 2024-04-23T20:29:41Z

Converts all remaining Dart->host communication in the iOS implementation to use Pigeon. Given the boilerplate nature of many of the changes, it seemed easiest to just do the remaining calls all at once now that the structure is in place.

Some high-level notes:

Many methods used to send the cameraId without it ever being used on the native side, so the Pigeon versions do not send them.
ThreadSafeTextureRegistry is removed because I discovered that it was masking a bug, so was more trouble than it was worth (see inline comments in PR).
A number of enums have been removed in favor of using the Pigeon-generated enums to pass data from the plugin class to FLTCam.
In many cases where the completion callback (previously result) was being passed to FLTCam in a call, only to have it always just call result(nil), that's now done in the plugin class since it's easier to reason about completions being called when they aren't passed around. (Long term we should consider moving almost all of the rest out, and using FlutterError* out params that the plugin class passes to completion, but that is more surgery than I wanted to do in this PR.)

Completes the iOS portion of flutter/flutter#117905

Pre-launch Checklist

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

stuartmorgan-g · 2024-04-23T20:31:34Z

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

-      [_camera setDescriptionWhileRecording:(call.arguments[@"cameraName"]) result:result];
-    } else if ([@"setImageFileFormat" isEqualToString:call.method]) {
-      NSString *fileFormat = call.arguments[@"fileFormat"];
-      [_camera setImageFileFormat:FCPGetFileFormatFromString(fileFormat)];


This was never calling result (and it doesn't pass it in to the implementation), so that wasn't good :|

stuartmorgan-g · 2024-04-23T20:38:57Z

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

+
+- (void)disposeCamera:(NSInteger)cameraId
+           completion:(nonnull void (^)(FlutterError *_Nullable))completion {
+  [_registry unregisterTexture:cameraId];


This is what made me remove the thread abstraction around the registry—this was one of two meaningful uses of the wrapper, and it was wrong. The old code was calling this in the async handler, which means it was effectively in the captureSessionQueue dispatch block below. But it was internally moving the call back to the main thread, which while correct, was not awaited before continuing. That meant that unregistering the texture—which ensures that the engine won't keep asking for textures—was not reliably being done before tearing down the camera on the session queue.

It may have worked out later due to the way thread redispatch is done when actually doing the copy, but I didn't try to unwind that since it's clearly better to do in this order.

stuartmorgan-g · 2024-04-23T20:39:56Z

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

+- (void)resumeVideoRecording;
+- (void)lockCaptureOrientation:(FCPPlatformDeviceOrientation)orientation;
+- (void)unlockCaptureOrientation;
+- (void)setFlashMode:(FCPPlatformFlashMode)mode


I fixed a bunch of weird/unidiomatic naming while touching these signatures.

stuartmorgan-g · 2024-04-23T20:40:30Z

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

+
+/// Sets the exposure point, in a (0,1) coordinate system.
+///
+/// If @c point is nil, the exposure point will reset to the center.


This logic was in CameraPlugin, but I moved it into this class now that it was easy to pass nil through using the new class.

stuartmorgan-g · 2024-04-23T20:41:25Z

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

-      result(@(_camera.captureDevice.minExposureTargetBias));
-    } else if ([@"getMaxExposureOffset" isEqualToString:call.method]) {
-      result(@(_camera.captureDevice.maxExposureTargetBias));
-    } else if ([@"getExposureOffsetStepSize" isEqualToString:call.method]) {


This method does not exist in the Pigeon version because hard-coding 0 as the return value is trivial to do in Dart.

hellohuanlin

I went through a pretty thorough review and everything looks very clean

hellohuanlin · 2024-04-24T18:18:47Z

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

+
+- (void)disposeCamera:(NSInteger)cameraId
+           completion:(nonnull void (^)(FlutterError *_Nullable))completion {
+  [_registry unregisterTexture:cameraId];


flutter/packages@cf6d280...fde908d 2024-04-25 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.3 to 4.1.4 (flutter/packages#6609) 2024-04-24 [email protected] [go_router] Add `GoRouterState state` parameter to `GoRouterData.onExit` (flutter/packages#6495) 2024-04-24 [email protected] Add CI steps to test iOS and macOS plugins with both CocoaPods and Swift Package Manager (flutter/packages#6557) 2024-04-24 [email protected] Roll Flutter from 77043ba to dba4f77 (30 revisions) (flutter/packages#6607) 2024-04-24 [email protected] [camera] Finish converting iOS to Pigeon (flutter/packages#6601) 2024-04-24 [email protected] [go_router] Fixes an issue where route future does not complete when â�¦ (flutter/packages#6596) 2024-04-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Bump legacy all_packages project AGP version to 7.0.0, Gradle version to 7.0.2 (#6591)" (flutter/packages#6605) 2024-04-23 [email protected] [in_app_purchase_android] Readme update for Alternative billing (flutter/packages#6578) 2024-04-23 [email protected] Bump legacy all_packages project AGP version to 7.0.0, Gradle version to 7.0.2 (flutter/packages#6591) 2024-04-23 [email protected] Roll Flutter from 140edb9 to 77043ba (21 revisions) (flutter/packages#6599) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.1 to 3.25.2 (flutter/packages#6597) 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 all remaining Dart->host communication in the iOS implementation to use Pigeon. Given the boilerplate nature of many of the changes, it seemed easiest to just do the remaining calls all at once now that the structure is in place. Some high-level notes: - Many methods used to send the `cameraId` without it ever being used on the native side, so the Pigeon versions do not send them. - `ThreadSafeTextureRegistry` is removed because I discovered that it was masking a bug, so was more trouble than it was worth (see inline comments in PR). - A number of enums have been removed in favor of using the Pigeon-generated enums to pass data from the plugin class to `FLTCam`. - In many cases where the completion callback (previously `result`) was being passed to `FLTCam` in a call, only to have it always just call `result(nil)`, that's now done in the plugin class since it's easier to reason about completions being called when they aren't passed around. (Long term we should consider moving almost all of the rest out, and using `FlutterError*` out params that the plugin class passes to `completion`, but that is more surgery than I wanted to do in this PR.) Completes the iOS portion of flutter/flutter#117905

Add structured types and convert implementations to use typesafe method calls for platform methods from Dart to native side. Android analog of #6601 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-g added 4 commits April 19, 2024 11:55

Convert create call

803a1a7

migrate initialize

22c1b07

Migrate all remaining calls

24e90af

Version bump

f54d881

stuartmorgan-g requested a review from hellohuanlin as a code owner April 23, 2024 20:29

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

stuartmorgan-g commented Apr 23, 2024

View reviewed changes

stuartmorgan-g changed the title ~~[pigeon] Finish converting iOS to Pigeon~~ [camera] Finish converting iOS to Pigeon Apr 24, 2024

hellohuanlin approved these changes Apr 24, 2024

View reviewed changes

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

auto-submit bot merged commit 59916a9 into flutter:main Apr 24, 2024
78 checks passed

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

Roll Packages from cf6d2803d93e to 59916a9bceb6 (7 revisions) flutter/flutter#147324

Closed

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

59916a9 [camera] Finish converting iOS to Pigeon (flutter/packages#6601)

15af789

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

Roll Packages from cf6d2803d93e to fde908d13896 (11 revisions) flutter/flutter#147375

Merged

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

59916a9 [camera] Finish converting iOS to Pigeon (flutter/packages#6601)

bf79328

reidbaker mentioned this pull request Jul 2, 2024

i151188 update espresso dependencies v2 reidbaker/packages#6

Closed

11 tasks

stuartmorgan-g mentioned this pull request Jul 9, 2024

Camera throws error when flash has been turned on in IOS flutter/flutter#151453

Closed

stuartmorgan-g mentioned this pull request Sep 26, 2024

[camera] Convert to Pigeon flutter/flutter#117905

Closed

3 tasks

yaakovschectman mentioned this pull request Oct 14, 2024

[camera_android] Convert Dart to native calls to use Pigeon #7874

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[camera] Finish converting iOS to Pigeon #6601

[camera] Finish converting iOS to Pigeon #6601

Uh oh!

stuartmorgan-g commented Apr 23, 2024 •

edited

Loading

Uh oh!

stuartmorgan-g Apr 23, 2024

Uh oh!

stuartmorgan-g Apr 23, 2024

Uh oh!

hellohuanlin Apr 24, 2024

Uh oh!

stuartmorgan-g Apr 23, 2024

Uh oh!

stuartmorgan-g Apr 23, 2024

Uh oh!

stuartmorgan-g Apr 23, 2024

Uh oh!

hellohuanlin left a comment

Uh oh!

hellohuanlin Apr 24, 2024

Uh oh!

Uh oh!

Uh oh!

[camera] Finish converting iOS to Pigeon #6601

[camera] Finish converting iOS to Pigeon #6601

Uh oh!

Conversation

stuartmorgan-g commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g commented Apr 23, 2024 •

edited

Loading