[camera] Add `camera_platform_interface` package #3253

mvanbeusekom · 2020-11-06T16:22:20Z

Description

Adds CameraPlatform which can be overridden to implement camera on other platforms (which may not want to use MethodChannels).

The PR is marked as draft so it can serve as a starting point on discussing the platform interface for the camera plugin. I have followed the article "How To Write a Flutter Web Plugin: Part 2" as a guide on which steps to take to migrate the camera plugin to the federated architecture. Below is an overview of steps:

Move the camera into its own subdirectory ("camera/camera");
Create the camera_platform_interface package;
- Copy the LICENSE file from the camera package;
- Add a CHANGELOG.md file;
- Define a pubspec.yaml file;
- Create a README.md file;
Define the camera platform interface;
Implement the default implementation in method_channel_camera.dart;
Refactor package:camera to use the camera platform interface;

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

ditman · 2020-11-07T00:15:26Z

Before I go into any detail, I normally like to just do the first move (from camera to camera/camera) as a proper PR and get that merged ASAP, so the development of the platform_interface can happen "in isolation".

Moving to camera/camera is a very simple PR (only "moved files without changes" or URL/link/pubspec path changes), and has the immediate benefit of being able to land any incoming fixes (if any) without having to worry about merge conflicts or git getting confused with changes + moves later on.

Then, the PR for the platform_interface will be quite smaller!

ditman

I've dumped some of the things we discussed over email before, and some advice after federating the google maps plugin that might be useful here too!

I'm not a firm believer in any of my opinions, so discussions are welcome! (I'll also be available on VC for further clarifications, if needed!)

packages/camera/camera_platform_interface/CHANGELOG.md

packages/camera/camera_platform_interface/lib/src/method_channel/method_channel_camera.dart

packages/camera/camera_platform_interface/lib/src/platform_interface/camera_platform.dart

packages/camera/camera_platform_interface/lib/src/types/callbacks.dart

packages/camera/camera_platform_interface/test/camera_platform_interface_test.dart

packages/camera/camera_platform_interface/lib/src/platform_interface/camera_platform.dart

packages/camera/camera_platform_interface/lib/src/types/camera_event.dart

packages/camera/camera_platform_interface/lib/src/platform_interface/camera_platform.dart

mvanbeusekom · 2020-11-09T15:43:12Z

Before I go into any detail, I normally like to just do the first move (from camera to camera/camera) as a proper PR and get that merged ASAP, so the development of the platform_interface can happen "in isolation".

Moving to camera/camera is a very simple PR (only "moved files without changes" or URL/link/pubspec path changes), and has the immediate benefit of being able to land any incoming fixes (if any) without having to worry about merge conflicts or git getting confused with changes + moves later on.

Then, the PR for the platform_interface will be quite smaller!

Clear and perfect point, luckily I made a separate commit of the move so I could easily create a new branch based on this commit and make a new PR which is ready to be reviewed (and hopefully merged): #3258.

ditman · 2020-11-09T19:52:35Z

make a new PR which is ready to be reviewed (and hopefully merged): #3258.

@mvanbeusekom yes, I left a couple of silly comments there (pls update the changelog and pubspec version) but I think that one can be merged pretty quickly.

google-cla · 2020-11-13T13:13:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

BeMacized · 2020-11-13T13:33:30Z

@googlebot I fixed it.

google-cla · 2020-11-13T13:33:46Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

mvanbeusekom · 2020-11-13T13:34:01Z

@googlebot I consent.

google-cla · 2020-11-13T13:34:20Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

BeMacized · 2020-11-13T13:35:17Z

@googlebot I consent.

ditman

This is looking really cool, I'm starting to run out of comments :P

ditman · 2020-11-16T20:43:10Z

packages/camera/camera_platform_interface/lib/src/platform_interface/camera_platform.dart

+  /// The file can be read as soon as [stopVideoRecording] returns.
+  Future<XFile> startVideoRecording(int cameraId) {


The file can be read as soon as [stopVideoRecording] returns. Should stopVideoRecording return the XFile instead? (It doesn't really matter much, but since this is a Future, it's a little bit more clunky to use. For example, I don't think we could use await with this future, because that'd probably block the program (so stopVideoRecording would never be called))

Having stopVideoRecording return the XFile indeed sounds like the more logical approach to me.

The only case I could argue for startVideoRecording to return the XFile as well, is if there is any reason for users to access the file before stopping the recording. Currently I cannot seem to come up with any though.

The only case I could argue for startVideoRecording to return the XFile as well, is if there is any reason for users to access the file before stopping the recording. Currently I cannot seem to come up with any though.

I think the case where we'd want to be able to have a future returned from startVideoRecording is if we wanted to use some sort of UI widget, with a FutureBuilder or similar; but in the example app that is not done at all.

packages/camera/camera_platform_interface/pubspec.yaml

packages/camera/camera_platform_interface/lib/src/events/camera_event.dart

bparrishMines · 2020-11-19T18:04:52Z

packages/camera/camera_platform_interface/lib/src/platform_interface/camera_platform.dart

+  }
+
+  /// Returns a widget showing a live camera preview.
+  Widget buildView(int cameraId) {


nit: I think buildPreview or buildWidget would be more appropriate since they aren't Views in Flutter.

Great catch, I agree buildPreview is a much better name, as such I renamed the method to buildPreview.

packages/camera/camera_platform_interface/lib/src/types/resolution_preset.dart

ditman · 2020-11-19T22:04:56Z

Quick comment: package: cross_file is now a thing:

https://pub.dev/packages/cross_file

mvanbeusekom · 2020-11-19T23:56:08Z

Quick comment: package: cross_file is now a thing:

https://pub.dev/packages/cross_file

Awesome, I will create a quick PR to update the file_selector plugin.

google-cla · 2020-11-27T18:26:18Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-11-27T18:28:16Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-11-27T20:24:18Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

BeMacized · 2020-11-30T07:55:13Z

@googlebot I consent.

google-cla · 2020-11-30T07:56:31Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

danielroek · 2020-11-30T08:16:38Z

@googlebot i consent

mvanbeusekom · 2020-11-30T17:33:18Z

@ditman, @bparrishMines, I just removed the draft label from this PR.

I this I have processed all earlier feedback and we (me and the team) migrated the Android and iOS implementations as well as the App Facing interface package to work with the new camera_platform_interface.

One open "to do" is to update the dependency on the camera_platform_interface in the camera package to a reference from pub.dev. This is not yet possible since the camera_platform_interface package is not yet published. For now I have marked this version of the camera package with the publish_to: none attribute and left a comment in the pubspec.yaml file to remind us to update the dependency.

Would love to hear your feedback.

packages/camera/camera/example/lib/main.dart

ditman

This is looking great. The biggest issue that this PR has now, is that it contains changes both to camera and camera_platform_interface, so it's not publishable (we should push only the platform_interface, and then do the backwards incompatible change to the core plugin).

Anyway, minor things. TL;DR:

Should the camera_image.dart file live in the platform_interface? If so, can it have Constructors that don't rely in a JSON/Map data structure to create them? Web is not going to have those maps handy!
Do we need to use part directives to structure the library? Can we use import/export?
Can we make the MethodChannel implementation class API simpler, by moving helper methods to outside helper functions, instead of having methods within the class?

ditman · 2020-12-02T19:58:19Z

packages/camera/camera/lib/src/camera_preview.dart

+    return controller.value.isInitialized
+        ? Texture(textureId: controller._cameraId)
+        : Container();


Aha, this is the trick. For this to work across platforms, the implementation must be:

return controller.value.isInitialized ? platform.buildPreview(cameraId: controller._cameraId) : Container();

The Texture implementation should be defined in the MethodChannel implementation (mobile version), and the web can return something based in HtmlViewElements or similar :)

Done ;)

I do wonder if the CameraPreview widget can actually become a cross platform widget since it relies on the CameraController which is a Android / iOS implementation of the camera_platform_interface. I think we can also make the CameraController cross platform but not sure if this will work for the web (looking at the google_maps_flutter I also see separate controllers and widgets). What do you think @ditman?

Everything that is exposed in the camera/camera must be completely cross-platform, and if it needs to do calls to the native side, it needs to use a platform interface call. I thought the controller was already doing that? Re-checking!

The current implementation of the camera controller is using a method channel directly, but I think all the methods called there are already represented in the platform_interface?

The current camera controller is using the method channel directly because I reverted the changes in the camera/camera plugin. Changes for the camera/camera plugin are now part of PR #3302, here all calls to the native side go through the platform_interface (except for the startImageStream and stopImageStream).

packages/camera/camera/lib/src/camera_image.dart

packages/camera/camera/lib/camera.dart

packages/camera/camera/example/integration_test/camera_test.dart

ditman · 2020-12-02T20:17:29Z

packages/camera/camera/example/lib/main.dart

-      await controller.startVideoRecording(filePath);
+      await controller.startVideoRecording();


Looks like we didn't care much about the start recording future before either :/

packages/camera/camera_platform_interface/lib/src/types/camera_exception.dart

packages/camera/camera_platform_interface/lib/src/types/resolution_preset.dart

packages/camera/camera_platform_interface/test/types/resolution_preset_test.dart

ditman · 2020-12-03T02:47:25Z

packages/camera/camera/lib/src/camera_controller.dart

+
+part of camera;
+
+final MethodChannel _channel = const MethodChannel('plugins.flutter.io/camera');


This should be calling the methdos from the interface, but I guess this is a similar case to the CameraWidget that hasn't been migrated yet?

This channel is used by the startImageStream and stopImageStream methods. As it is the Google team is not very happy with these methods and their implementation, because they don't perform very well. So we decided in an offline meeting (between Amir, Maurice, Chris and myself) that for now we will keep these methods only available for the Android and iOS so not to break existing users until we have a better alternative. Meaning we don't add them to the platform_interface as of yet.

Ahhh, ok, those methods would need some assert(!kIsWeb) or similar so people are aware that the web implementation will not handle them (at all), instead of getting a runtime error of "method channel not registered for message blah" which is more cryptic, and sounds like a bug!

packages/camera/camera/lib/src/camera_image.dart

mvanbeusekom · 2020-12-03T15:56:16Z

This is looking great. The biggest issue that this PR has now, is that it contains changes both to camera and camera_platform_interface, so it's not publishable (we should push only the platform_interface, and then do the backwards incompatible change to the core plugin).

Anyway, minor things. TL;DR:
* Should the camera_image.dart file live in the platform_interface? If so, can it have Constructors that don't rely in a JSON/Map `data` structure to create them? Web is not going to have those maps handy!

* Do we need to use `part` directives to structure the library? Can we use `import`/`export`?

* Can we make the MethodChannel implementation class API simpler, by moving helper methods to outside helper functions, instead of having methods within the class?

@ditman, thank you for this detailed review. I have gone through all of your remarks and made sure everything has been either fixed or I left explanation.

I have updated this PR to only include the addition off the camera_platform_interface and removed all changes to the camera plugin, so we can move this along. Currently CI is failing because of a deprecation warning (which is treated as error). A PR to fix this is already on it's way (see #3299).

A second PR (#3302) now contains the implementation of the camera_platform_interface for the camera package (by my colleague @BeMacized).

I did have two points I wanted your opinion on, maybe you could have a look at them:

One is regarding the CameraPreview widget, see comment here;
And the second one is regarding the handleMethodCall method, see comment here

ditman · 2020-12-03T21:59:25Z

It seems #3299 has been merged; do you mind rebasing this branch so all tests go green? I'll take a final quick look at the PR right after!

mvanbeusekom · 2020-12-03T22:13:16Z

It seems #3299 has been merged; do you mind rebasing this branch so all tests go green? I'll take a final quick look at the PR right after!

I have just rebased, CI is running ;) Thanks David.

ditman

LGTM! Maybe have @bparrishMines wants to take a look again, but I don't have much else to say! Thanks for the effort @mvanbeusekom!

* Make sure only camera_platform_interface has updates * add dev dependency to async package * refactored import to package import * Fix formatting issues

…m-changes * upstream-share-final-null-release: (233 commits) [q-w] Update Flutter SDK constraint (flutter#3323) [i-p] Update Flutter SDK constraint (flutter#3322) [d-g] Update Flutter SDK constraint (flutter#3321) [a-c] Update Flutter SDK constraint (flutter#3320) [image_picker_platform_interface] Pass Uri to package:http APIs (flutter#3309) Exclude null-safe plugins from testing on stable (flutter#3318) [documentation] [url_launcher] fix for readme code sample (flutter#3308) [camera] Add zoom support to platform interface (flutter#3312) update analysis options for nnbd (flutter#3319) [camera] Suppress unchecked cast warning in java test (flutter#3316) [image_picker] [integration_test] Fixes to make the tree green (flutter#3317) [camera] Expanded platform interface to support setting flash mode (flutter#3313) [Espresso] Android Code Inspection and Clean up (flutter#3111) [camera] Add `camera_platform_interface` package (flutter#3253) [camera] Support Android 30 (flutter#3299) bump integration test to 1.0.0 (flutter#3295) [android_alarm_manager] fix AndroidManifest.xml for android lint issue "XML tag has empty body" (flutter#3288) Use testWidgets instead of test to fix failures not surfacing on CI (flutter#3279) [file_selector_platform_interface] Migrate to cross_file package (flutter#3286) Fix broken link (flutter#3280) ...

google-cla bot added the cla: yes label Nov 6, 2020

ditman reviewed Nov 7, 2020

View reviewed changes

mvanbeusekom mentioned this pull request Nov 9, 2020

[camera] Move camera to camera/camera #3258

Merged

13 tasks

mvanbeusekom force-pushed the camera_federated branch from 009df94 to fdac99f Compare November 9, 2020 07:55

mvanbeusekom mentioned this pull request Nov 10, 2020

[cross_file] An abstraction to allow working with files across multiple platforms. #3260

Merged

13 tasks

mvanbeusekom force-pushed the camera_federated branch from abfb158 to 69f573b Compare November 12, 2020 10:38

google-cla bot added cla: no and removed cla: yes labels Nov 13, 2020

google-cla bot added cla: yes and removed cla: no labels Nov 13, 2020

ditman reviewed Nov 16, 2020

View reviewed changes

bparrishMines reviewed Nov 19, 2020

View reviewed changes

google-cla bot added cla: no and removed cla: yes labels Nov 27, 2020

google-cla bot added cla: yes and removed cla: no labels Nov 30, 2020

mvanbeusekom marked this pull request as ready for review November 30, 2020 17:26

mvanbeusekom requested a review from cyanglaz as a code owner November 30, 2020 17:26

ditman reviewed Nov 30, 2020

View reviewed changes

packages/camera/camera/example/lib/main.dart Outdated Show resolved Hide resolved

ditman reviewed Dec 3, 2020

View reviewed changes

mvanbeusekom force-pushed the camera_federated branch from 22ec1d7 to 5a0f51e Compare December 3, 2020 14:15

BeMacized mentioned this pull request Dec 3, 2020

[camera] Add implementations for camera_platform_interface package. #3302

Merged

13 tasks

mvanbeusekom added 4 commits December 3, 2020 23:10

Make sure only camera_platform_interface has updates

8d8f0b8

add dev dependency to async package

1c7e368

refactored import to package import

21572ee

Fix formatting issues

e1bd5f2

mvanbeusekom force-pushed the camera_federated branch from cf364c0 to e1bd5f2 Compare December 3, 2020 22:11

ditman approved these changes Dec 4, 2020

View reviewed changes

mvanbeusekom merged commit 9fef1c7 into flutter:master Dec 4, 2020

mvanbeusekom deleted the camera_federated branch December 10, 2020 09:35

stuartmorgan mentioned this pull request Mar 22, 2022

[camera] Rework image streaming flutter/flutter#100555

Closed

		/// The file can be read as soon as [stopVideoRecording] returns.
		Future<XFile> startVideoRecording(int cameraId) {

		await controller.startVideoRecording(filePath);
		await controller.startVideoRecording();


		part of camera;

		final MethodChannel _channel = const MethodChannel('plugins.flutter.io/camera');

[camera] Add camera_platform_interface package #3253

[camera] Add camera_platform_interface package #3253

Conversation

mvanbeusekom commented Nov 6, 2020 • edited Loading

Description

Checklist

Breaking Change

ditman commented Nov 7, 2020

ditman left a comment

Choose a reason for hiding this comment

mvanbeusekom commented Nov 9, 2020

ditman commented Nov 9, 2020

google-cla bot commented Nov 13, 2020

BeMacized commented Nov 13, 2020

google-cla bot commented Nov 13, 2020

mvanbeusekom commented Nov 13, 2020

google-cla bot commented Nov 13, 2020

BeMacized commented Nov 13, 2020

ditman left a comment • edited Loading

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

ditman commented Nov 19, 2020

mvanbeusekom commented Nov 19, 2020

google-cla bot commented Nov 27, 2020

google-cla bot commented Nov 27, 2020

google-cla bot commented Nov 27, 2020

BeMacized commented Nov 30, 2020

google-cla bot commented Nov 30, 2020

danielroek commented Nov 30, 2020

mvanbeusekom commented Nov 30, 2020

ditman 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

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

mvanbeusekom commented Dec 3, 2020 • edited Loading

ditman commented Dec 3, 2020

mvanbeusekom commented Dec 3, 2020 • edited Loading

ditman left a comment

Choose a reason for hiding this comment

[camera] Add `camera_platform_interface` package #3253

[camera] Add `camera_platform_interface` package #3253

mvanbeusekom commented Nov 6, 2020 •

edited

Loading

ditman left a comment •

edited

Loading

mvanbeusekom commented Dec 3, 2020 •

edited

Loading

mvanbeusekom commented Dec 3, 2020 •

edited

Loading