[camerax] Implement camera preview #7112

camsim99 · 2023-02-07T00:30:47Z

Implements Flutter camera API methods related building, pausing, and resuming a preview.

In order to implement this, I also needed to implement methods to create an initialize the camera, as well as callback methods to send information related to camera initialization, the device orientation changing, and native camera errors.

I also added back the example app used in the other plugins, as it will be needed for integration testing and easier to verify feature parity is ultimately reached.

Fixes flutter/flutter#112658.

Pre-launch Checklist

camsim99 · 2023-02-10T23:52:52Z

packages/camera/camera_android_camerax/example/lib/camera_controller.dart

@@ -0,0 +1,554 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


This is an exact copy from camera_android.

I'm assuming that camera_android just copied this from camera. Would it be better to use the latest copy of this from camera instead?

camsim99 · 2023-02-10T23:53:16Z

packages/camera/camera_android_camerax/example/lib/camera_preview.dart

@@ -0,0 +1,84 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


This is an exact copy from camera_android.

Same as above

camsim99 · 2023-02-10T23:54:38Z

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

@@ -2,43 +2,1003 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+import 'dart:async';


This is an exact copy from camera_android, except some functionality was deleted for currently unsupported operations. My thought is that we can just fill them back in as we implement things. I left TODOs in those spots.

Same as above.

camsim99 · 2023-02-11T00:02:20Z

packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart

  @visibleForTesting
-  CameraSelector? backCameraSelector = CameraSelector.getDefaultBackCamera();


@gmackall I ended up refactoring this, primarily for testing reasons. Initializing it like this would cause us to have to initialize this in every test, so I apologize for noting this as an option.

To do this, I modified availableCameras() to use a helper method to create CameraSelectors that is stubbed in the mock version of AndroidCameraCameraX that I created for testing. Let me know what you think!

I think that makes sense!

It means we will have to recreate those camera selectors for each call to availableCameras, but I don't think there is much reason to repeatedly make calls to availableCameras.

It also looks like it makes the testing much cleaner.

camsim99 · 2023-02-11T00:05:29Z

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart

-  ProcessCameraProvider,
-  CameraSelector,
-  CameraInfo,
+@GenerateNiceMocks(<MockSpec<Object>>[


@stuartmorgan @bparrishMines Generate mocks for BuildContext this way has caused me to run into this error: https://cirrus-ci.com/task/6299387630977024?logs=unit_test#L203. This passes for me locally, so I'm wondering if y'all have any insight as to what might be happening with the CI?

It looks like the error is about a typedef that was added on master recently, so it's not on stable. You can probably just generate the mocks on stable.

packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart

gmackall

LGTM!

The only change I would consider is changing the _bindPreviewToLifecycle (and unbind) into helpers that can handle all of the potential use cases (i.e. a _bindUseCaseToLifecycle).

But I think that change is probably easier to make once we have more use cases to work with and it is clearer what assertions are shared.

bparrishMines · 2023-02-14T20:48:45Z

packages/camera/camera_android_camerax/example/lib/camera_controller.dart

@@ -0,0 +1,554 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


I'm assuming that camera_android just copied this from camera. Would it be better to use the latest copy of this from camera instead?

bparrishMines · 2023-02-14T20:48:59Z

packages/camera/camera_android_camerax/example/lib/camera_preview.dart

@@ -0,0 +1,84 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


Same as above

bparrishMines · 2023-02-14T20:49:33Z

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

@@ -2,43 +2,1003 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+import 'dart:async';


Same as above.

packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart

packages/camera/camera_android_camerax/lib/src/camera_selector.dart

bparrishMines · 2023-02-14T21:41:18Z

packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart

+  /// The [Camera] instance returned by the [processCameraProvider] when a [UseCase] is
+  /// bound to the lifecycle of the camera it manages.
+  @visibleForTesting
+  Camera? camera;


nit: It may be worth moving all the @visibleForTesting fields and methods to a separate class and pass that class as an optional parameter in a constructor. Calling it something like AndroidCameraXProxy. This could help organize the code a bit. You can decide which is easier to use.

I would honestly find this kind of confusing, especially since this would involve some of the major use case instances. Are you open to reconsidering this later on? I could see it helping organizationally if there ends up being a lot of different fields (especially booleans and things that aren't necessarily CameraX objects) that need to be visible or there are a bunch more methods we'll need to create to mock.

camsim99 · 2023-02-14T22:53:31Z

The only change I would consider is changing the _bindPreviewToLifecycle (and unbind) into helpers that can handle all of the potential use cases (i.e. a _bindUseCaseToLifecycle).

But I think that change is probably easier to make once we have more use cases to work with and it is clearer what assertions are shared.

@gmackall I agree! I thought the same thing.

reidbaker · 2023-02-15T16:04:22Z

packages/camera/camera_android_camerax/example/lib/camera_image.dart

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// TODO(a14n): remove this import once Flutter 3.1 or later reaches stable (including flutter/flutter#104231)


fluter 3.7 is in stable and that pull request has landed.

reidbaker · 2023-02-15T16:08:29Z

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

+    try {
+      await cameraController.initialize();
+      await Future.wait(<Future<Object?>>[
+        // The exposure mode is currently not supported on the web.


Consider breaking this out into a method _isExposureModeSupported and using the method to encapsulate that logic.

reidbaker · 2023-02-15T16:15:35Z

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

+            .getMinZoomLevel()
+            .then((double value) => _minAvailableZoom = value),
+      ]);
+    } on CameraException catch (e) {


I think the above block has a subtle programming error related to how async code and thrown exceptions work. In my own dart programming I avoid then.
From reading this section https://dart.dev/guides/libraries/futures-error-handling#error-handling-within-then I think that if you get an error thrown during getMaxExposureOffset, getMaxZoomLevel or getMinZoom level it will not be caught by your code below if the async execution has left this method.

Instead you either need to ensure each of your then methods have their own catch error or alternatively you can use await. Also with the way this is currently structured I think there is a possibility that max available zoom level is not updated when the value is used.

reidbaker · 2023-02-15T16:16:12Z

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

+      ]);
+    } on CameraException catch (e) {
+      switch (e.code) {
+        case 'CameraAccessDenied':


Where do these cases come from? Can we use a static constant instead of inline strings?

The code is thrown from the platform side, and yes we can use a string constant instead of inline strings.

Though the latest recommendation is to translate the code into subclasses (e.g.CameraAccessDeniedException)

Though the latest recommendation is to translate the code into subclasses (e.g.CameraAccessDeniedException)

That's not what it says; the recommendation is to have a plugin-specific exception with some kind of structured definition of specific cases. Subclasses are a possible approach, but the examples it gives are enums or other constants.

Ah I read this part wrong:

plugin-specific Exception class

This is just referring to CameraException, not CameraAccessDeniedException.

Thanks for the correction!

reidbaker · 2023-02-15T16:18:05Z

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

+  }
+
+  void onFlashModeButtonPressed() {
+    if (_flashModeControlRowAnimationController.value == 1) {


if there are only 2 possible values for this and the below lines why are we comparing against an int instead of a boolean?

Oh I see. We can probably just check if the animation is finished or not instead.

I think code that is clearly about animation logic makes more sense than what I am reading now.

packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart

reidbaker · 2023-02-15T16:21:12Z

packages/camera/camera_android_camerax/lib/src/camera_selector.dart

-  static const int LENS_FACING_FRONT = 0;
+  ///
+  /// See https://developer.android.com/reference/androidx/camera/core/CameraSelector#LENS_FACING_FRONT().
+  static const int lensFacingFront = 0;


Why do not we have a copy of lens facing unknown?

We aren't using it now, but we will need that later on, so I added it!

camsim99 · 2023-02-15T20:03:10Z

@reidbaker Regarding the comments you left on the example app, I agree with them, so I plan to make a cleanup PR to update the camera example apps across the board. I just coped the one from camera/camera here, and I would like the examples to remain consistent.

* 3c7d54bba [camerax] Implement camera preview (flutter/plugins#7112) * 48f50b4f1 [image_picker] Update NSPhotoLibraryUsageDescription description in README (flutter/plugins#6589) * eea17c996 Migrate these tests to the "new" API. (flutter/plugins#7189) * 190c6d916 Roll Flutter from 298d8c7 to 0be7c3f (21 revisions) (flutter/plugins#7194) * c0d4e8041 [google_sign_in] Endorses next web package. (flutter/plugins#7191) * cc4eaba0f [google_maps]: Bump org.mockito:mockito-core (flutter/plugins#7099) * 717a8bfef [image_picker]: Bump org.mockito:mockito-core (flutter/plugins#7097) * 8a09d8c13 [lifecycle]: Bump org.mockito:mockito-core (flutter/plugins#7096) * 40377a12a [in_app_pur]: Bump org.mockito:mockito-core (flutter/plugins#7094) * 6a4bbf1df [url_launcher]: Bump org.mockito:mockito-core (flutter/plugins#7098) * 96ab5cd12 Update codeowners (flutter/plugins#7188) * 00d5855cc Add missing CODEOWNER (flutter/plugins#7016) * c3e9d1ba3 [webview_flutter] Adds examples of accessing platform-specific features for each class (flutter/plugins#7089) * 1f7b57917 Roll Flutter from 0be7c3f to 33e4d21 (5 revisions) (flutter/plugins#7196) * 1acaf55c2 [plugins] Disables the AndroidGradlePluginVersion issue ID in all android packages (flutter/plugins#7045) * 132d9c77d [espresso] Update some dependencies (flutter/plugins#7195)

Add base code from proof of concept

7894cc2

github-actions bot added the p: camera label Feb 7, 2023

camsim99 added 14 commits February 7, 2023 09:35

Fix analyzer

efca3ed

Add example app and tests

b4ae1db

Actually add tets and visible for testing annotation

7c8168b

Make methods private

93d442b

Merge remote-tracking branch 'upstream/main' into camx_preview

d99ef9c

Fix tests

bf1ca9a

Fix analyze:

b237d6e

Update changelog

a21429a

Formatting

dcff8bc

Merge remote-tracking branch 'upstream/main' into camx_preview

a183944

Update todos with links and modify camera controller

f163907

Format and add availableCameras

285659d

Fix mocks

cffc0ac

Try bumping mockito version

889802d

camsim99 commented Feb 10, 2023

View reviewed changes

Review documentation

24ee512

camsim99 commented Feb 11, 2023

View reviewed changes

camsim99 marked this pull request as ready for review February 11, 2023 00:03

camsim99 commented Feb 11, 2023

View reviewed changes

camsim99 requested review from gmackall, bparrishMines and reidbaker February 11, 2023 00:08

Re-generate mocks

573cf33

gmackall reviewed Feb 14, 2023

View reviewed changes

packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart Outdated Show resolved Hide resolved

Fix typo

d08c087

gmackall approved these changes Feb 14, 2023

View reviewed changes

Fix another typo

1e1dd8b

bparrishMines reviewed Feb 14, 2023

View reviewed changes

camsim99 added 2 commits February 14, 2023 14:48

Address review and fix bug

e1cea85

Merge remote-tracking branch 'upstream/main' into camx_preview

5ccc872

camsim99 added 4 commits February 14, 2023 14:57

Fix analyze

7129845

Merge remote-tracking branch 'upstream/main' into camx_preview

c54c547

Fix tests

5f30bc7

Foramtting

8fc71f9

camsim99 requested a review from bparrishMines February 14, 2023 23:58

Clear preview is paused

d50bd87

reidbaker suggested changes Feb 15, 2023

View reviewed changes

Add unknown lens

de2fc29

camsim99 requested a review from reidbaker February 15, 2023 20:03

reidbaker approved these changes Feb 17, 2023

View reviewed changes

camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 17, 2023

auto-submit bot merged commit 3c7d54b into flutter:main Feb 17, 2023

engine-flutter-autoroll mentioned this pull request Feb 20, 2023

Roll Plugins from 8f3419be5e0e to 132d9c77da4d (16 revisions) flutter/flutter#121099

Closed

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

3c7d54bba [camerax] Implement camera preview (flutter/plugins#7112)

da6b98a

engine-flutter-autoroll mentioned this pull request Feb 20, 2023

Roll Plugins from 8f3419be5e0e to 132d9c77da4d (16 revisions) flutter/flutter#121101

Merged

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

3c7d54bba [camerax] Implement camera preview (flutter/plugins#7112)

1c05f99

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camerax] Implement camera preview #7112

[camerax] Implement camera preview #7112

		@@ -0,0 +1,554 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

		@@ -0,0 +1,84 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

		@visibleForTesting
		CameraSelector? backCameraSelector = CameraSelector.getDefaultBackCamera();

[camerax] Implement camera preview #7112

[camerax] Implement camera preview #7112

Conversation

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

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

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