[camerax] Implement onCameraClosing #3878

camsim99 · 2023-05-01T23:57:45Z

Overview

Continuation of #3419.

Implements onCameraClosing that will send an event anytime the camera closes, as detected by observing the LiveData of the CameraState of the current camera being used in the plugin.

Fixes flutter/flutter#121162.

Highlights of what has changed since the latest reviews of the last PR:

Wrapped LiveData#getValue (docs) for the purposes of retrieving current value LiveData<ZoomState> to properly implement retrieving zoom information
Modified Dart Host API and FlutterAPI implementations' documentation on their binaryMessenger and instanceManager documentation to be clearer as per suggestion from comments on the original PR
Changed binaryMessenger and instanceManager fields to be private in Flutter API implementations
Removed cast method and added enum LiveDataSupportedType to pigeon file to use for safely casting the LiveData generic when received on the Dart or native side.
Moved CameraStateError code decoding to Dart side as this was written by me and not directly provided by CameraX.
Added tests for the conversion of objects.
The following issues were filed to capture feedback given but not addressed in this PR: [camerax] ☂️ Clean up wrapped native classes flutter#125926, [camerax] ☂️ Clean up Dart Tests flutter#125928.

Pre-launch Checklist

…imgcap

…o/flutter/plugins/camerax/ImageCaptureHostApiImpl.java Co-authored-by: Maurice Parrish <[email protected]>

Co-authored-by: Maurice Parrish <[email protected]>

bparrishMines · 2023-05-04T17:41:40Z

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

@@ -312,17 +332,33 @@ class AndroidCameraCameraX extends CameraPlatform {
  /// [cameraId] not used.
  @override
  Future<double> getMaxZoomLevel(int cameraId) async {
-    final ZoomState exposureState = await cameraInfo!.getZoomState();
-    return exposureState.maxZoomRatio;
+    final LiveData<ZoomState> liveZoomState = await cameraInfo!.getZoomState();


Would it not be better to have a local zoomState variable that is updated by an Observer added to liveZoomState? Same for getMinZoomLevel.

I don't think so, because I think it's safe to assume this won't be called often enough for us to want to set an Observer on it. If there was a Flutter camera method that suggested we'd want to observe changes in the zoom state, I would agree with you.

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

bparrishMines · 2023-05-04T17:54:25Z

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

+  ///    camera is in error state.
+  Observer<CameraState> _createCameraClosingObserver(int cameraId) {
+    // Callback method used to implement the behavior described above:
+    void onChanged(Object stateAsObject) {


Garbage collection for Dart is non-deterministic, so testing for it is a little difficult. I do have a test for leaking in the webview_flutter integration tests that verifies the WebView is garbage collected: https://github.com/flutter/packages/blob/main/packages/webview_flutter/webview_flutter_android/example/integration_test/webview_flutter_test.dart#L100

I don't think the implementation of the test should block this PR though. Leaks only prevent the release of the strong reference to the object in the Java InstanceManager. And the camera resources are explicitly released with the methods provided by the API.

I can also help with writing the test.

...ra/camera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/LiveDataTest.java

reidbaker · 2023-05-04T16:35:49Z

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

+
+    if (zoomState == null) {
+      throw CameraException(
+        'zoomStateNotSet',


I think this should be a constant. Both because it is used in multiple places and because callers that want to branch behavior on different CameraExceptions should be able to reference constants instead of strings.

Resolved but not a constant and no comment explaining why.

I think you're looking at outdated code?

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

reidbaker · 2023-05-04T16:37:44Z

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

@@ -447,11 +486,11 @@ class AndroidCameraCameraX extends CameraPlatform {
          height: imageProxy.height,
          width: imageProxy.width);

-      cameraImageDataStreamController?.add(cameraImageData);
+      weakThis.target!.cameraImageDataStreamController?.add(cameraImageData);


If the ? is ok for cameraImageDataStreamController why is it not ok for target?

I made them both ! because neither should be null!

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

reidbaker · 2023-05-04T17:53:54Z

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

+  Future<T?>? getValueFromInstances<T extends Object>(
+      LiveData<T> instance) async {
+    LiveDataSupportedTypeData? typeData;
+    switch (T) {


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

reidbaker · 2023-05-04T17:59:42Z

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

+
+  /// Adds specified [Observer] instance to instance manager and makes call
+  /// to native side to create the instance.
+  Future<void> createFromInstances<T>(


Should this be createFromInstance (singular)?

This was marked resolved but I dont see anything explaining why it should be plural. Non blocking but I am still curious for the answer.

I think you're also looking at outdated code here; I changed it to singular.

reidbaker · 2023-05-04T18:00:43Z

packages/camera/camera_android_camerax/test/observer_test.dart

+
+      expect(callbackParameter, value);
+    });
+  });


Can you add a test to make sure createFromInstances copies the values correctly?

Same for other classes if a similar method exists.

This is handled in the host API create tests. createFromInstances calls through to the mockApis so when we test that call, we ensure createFromInstances is handling the values correctly

bparrishMines

LGTM

reidbaker · 2023-05-05T02:45:16Z

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

+  ///
+  /// [shouldCreateDetachedObjectForTesting] is for testing only and thus,
+  /// is set to `false` by default.
+  AndroidCameraCameraX({this.shouldCreateDetachedObjectForTesting = false});


With this implementation is it possible to call AndroidCameraCameraX(shouldCreateDetachedObjectForTesting: true) from non testing code?

My gut says yes it is and that instead this should be something like

Suggested change

AndroidCameraCameraX({this.shouldCreateDetachedObjectForTesting = false});

AndroidCameraCameraX():this._shouldCreateDetachedObjectForTesting = false;

@visibleForTesting

AndroidCameraCameraX.forTesting(this._shouldCreateDetachedObjectForTesting );

Notice how the parameter is still final but now private and the value is protected by the constructor being visible for testing.

Yeah, it's still possible to change from non-testing code :/

How would I change this from testing code, though, with this structure? I need a way to set this to true for testing, so is there a way to do that without exposing it also to non-testing code? I would think using a setter, but that's still visible.

Here is a patch file with it working you can apply.
protected-testing-object.patch

reidbaker · 2023-05-05T02:49:59Z

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

@@ -404,7 +450,10 @@ class AndroidCameraCameraX extends CameraPlatform {

  /// Binds [preview] instance to the camera lifecycle controlled by the
  /// [processCameraProvider].
-  Future<void> _bindPreviewToLifecycle() async {
+  ///
+  /// [cameraId] used to properly update the live camera state for the camera


Non blocking: This still does not help me as someone who does not know the implementation what I should do as a caller.

For example [cameraId] the camera reference id that will have its [CameraState] updated when the process for preview is rebound (if required)

I don't actually know if that is true but that is sort of what I see when I read the code. It implies if you pass a bad camera id you might not get state updates but that everything else will probably work.

reidbaker · 2023-05-05T02:50:59Z

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

@@ -447,11 +499,14 @@ class AndroidCameraCameraX extends CameraPlatform {
          height: imageProxy.height,
          width: imageProxy.width);

-      cameraImageDataStreamController?.add(cameraImageData);
+      weakThis.target!.cameraImageDataStreamController!.add(cameraImageData);


It is likely that I just misunderstand how our weakThis works but wouldnt the point of a weak reference be that target can be null?

Second issue, best I can tell this is in _bindPreviewToLifecycle is there a promised relationship that this method is called after onStreamedFrameAvailable? if not then cameraImageDataStreamController I think can be null.

Reading this line of code I see 2 places where it should be expected that we cant set cameraImageData.

First issue: @bparrishMines am I misunderstanding how weakThis should be used? Should I allow for the target to be null to avoid leaks?

Second issue: Yes, this is a helper method used to set up the image streaming that is called from listening to the stream returned by onStreamedFrameAvailable, so it should never be called really any other time. cameraImageDataStreamController is set up before that, so it would never be null here.

I think the point of the weak reference is to ensure the callback method doesn't prevent garbage collection of the instance that contains it. The target COULD be null, but the ! is just a runtime check that can be used if you expect the target to always be available in the callback.

For example, if you expect the user to eventually ALWAYS call CameraController.dispose and you do some kind of cleanup in that method that stops image streaming callbacks. Then I think it is reasonable to expect this to always be nonnull. But a test would also be helpful for ensuring this and preventing a null pointer exception.

As a comparison, weview_flutter always uses weakThis.target?... because there is no WebViewController.dispose.

What you described is what we can expect because dispose must be called whenever users want to release camera resources. I didn't properly handle this for image streaming so I handled that and updated the test for dispose.

reidbaker · 2023-05-05T02:57:06Z

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

+  /// If a previous [liveCameraState] was stored, existing observers are
+  /// removed, as well.
+  Future<void> _updateLiveCameraState(int cameraId) async {
+    final CameraInfo cameraInfo = await camera!.getCameraInfo();


Non blocking. I still find this weird that we need to pass a cameraId (and use that name) when we have a reference to a camera object already and we dont use one to validate against the other. Is there an oppetunity to use more descriptive variable names here?

I think this gets at the bigger question as to why cameraId is used throughout the Flutter camera API to begin with. Really, cameraId is the ID of the Flutter surface texture used to render a preview to, and thus, is almost totally unrelated to the actual camera instance we use to do any camera functionality. The Flutter surface texture ID is returned by calls to createCamera(...), so I assume it is just an ID that folks using the plugin can mostly use to filter through CameraEvents added to the various camera event streams (because they are constructed with an ID) because otherwise, at least in the Android camera implementations, it goes mostly unused.

I'm on the fence about renaming cameraId, because while it would be more useful to name it something more like flutterSurfaceTextureId, it's not named that in the actual camera platform interface, so it could cause more confusion for folks trying to contribute to the plugin.

However, I am on board for bettering documentation in the implementation around what it is (always referring to the Flutter surface texture ID/createCamera), when it is actually being used in a method, and if it is being used, exactly how. I'll file an issue before landing this for that.

flutter/flutter#126270 filed for improving documentation

reidbaker · 2023-05-05T02:59:48Z

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

+    void onChanged(Object stateAsObject) {
+      final WeakReference<AndroidCameraCameraX> weakThis =
+          WeakReference<AndroidCameraCameraX>(this);
+      final CameraState state = stateAsObject as CameraState;


Consider adding a comment to that effect since one class is relying on the known implementation of another class and not on its api surface.

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

…camerax.dart Co-authored-by: Reid Baker <[email protected]>

…cc_3

### Overview Continuation of flutter#3419. Implements onCameraClosing that will send an event anytime the camera closes, as detected by observing the LiveData of the CameraState of the current camera being used in the plugin. Fixes flutter/flutter#121162. ### Highlights of what has changed since the latest reviews of the [last PR](flutter#3419): - Wrapped `LiveData#getValue` ([docs](https://developer.android.com/reference/androidx/lifecycle/LiveData#getValue())) for the purposes of retrieving current value `LiveData<ZoomState>` to properly implement retrieving zoom information - Modified Dart Host API and FlutterAPI implementations' documentation on their `binaryMessenger` and `instanceManager` documentation to be clearer as per suggestion from comments on the original PR - Changed `binaryMessenger` and `instanceManager` fields to be private in Flutter API implementations - Removed `cast` method and added enum `LiveDataSupportedType` to pigeon file to use for safely casting the `LiveData` generic when received on the Dart or native side. - Moved `CameraStateError` code decoding to Dart side as this was written by me and not directly provided by CameraX. - Added tests for the conversion of objects. - The following issues were filed to capture feedback given but not addressed in this PR: flutter/flutter#125926, flutter/flutter#125928.

camsim99 and others added 30 commits February 23, 2023 14:35

Recreating image capture impl from flutter/plugins

6125eca

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

5be1371

Fix integration tesT

b2fce3d

Merge branch 'main' into camx_imgcap

ed62a9a

Analyzer pt 2

2e3027b

Merge branch 'camx_imgcap' of github.com:camsim99/packages into camx_…

fbd3fe5

…imgcap

Personal cleanup

dfa477e

Add initialization of controller

a651b92

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

884fd6c

Modify some docs

d5f8b91

Address review

060b7e5

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

ff50c70

Fix analyze

fe6f1fc

Update packages/camera/camera_android_camerax/android/src/main/java/i…

e2643ae

…o/flutter/plugins/camerax/ImageCaptureHostApiImpl.java Co-authored-by: Maurice Parrish <[email protected]>

Update packages/camera/camera_android_camerax/lib/src/image_capture.dart

b7194f7

Co-authored-by: Maurice Parrish <[email protected]>

Start addressing review

dadc230

Finish addressing review

d15e1d4

Fix analyze

6dc2b77

Generate mocks on stable

f826b62

Fix/add tests

0f7f29a

Add isBound dart test

1e9739b

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

9ba20d0

Start dev

5bf6b17

Add cameraClosing impl

5e58b2c

Add tests

4c8c94d

Implement occ with camera state

6a0de08

Reimplement with live camera state

d3ed3e7

Start adding tests

d1c684f

Add java tests

c04d8c1

Formatting

b2322d2

camsim99 mentioned this pull request May 3, 2023

[camerax] ☂️ Clean up wrapped native classes flutter/flutter#125926

Open

6 tasks

Address maurice review

6f701a5

camsim99 requested review from reidbaker and bparrishMines May 3, 2023 23:26

bparrishMines reviewed May 4, 2023

View reviewed changes

reidbaker reviewed May 4, 2023

View reviewed changes

Address reviews

a636525

camsim99 requested review from reidbaker and bparrishMines May 4, 2023 19:42

bparrishMines approved these changes May 4, 2023

View reviewed changes

reidbaker approved these changes May 5, 2023

View reviewed changes

Improve comments

d5a21cb

reidbaker reviewed May 5, 2023

View reviewed changes

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

camsim99 and others added 4 commits May 8, 2023 08:05

Update packages/camera/camera_android_camerax/lib/src/android_camera_…

bc4ebc1

…camerax.dart Co-authored-by: Reid Baker <[email protected]>

Apply patch + fix dispose

491ab2c

Merge branch 'camx_occ_3' of github.com:camsim99/packages into camx_o…

bff96a9

…cc_3

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

ad40999

This was referenced May 9, 2023

[camerax] Publish plugin #3896

Merged

[camerax] Implement video capture for CameraX android camera re-write #3467

Merged

camsim99 added 2 commits May 9, 2023 11:04

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

a3667dd

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

c3732fe

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

auto-submit bot merged commit c62f6f7 into flutter:main May 9, 2023

reidbaker mentioned this pull request May 12, 2023

[url_launcher] Set broadcast reciever visability as required by target api 34 #3973

Merged

11 tasks

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

-  AndroidCameraCameraX({this.shouldCreateDetachedObjectForTesting = false});
+  AndroidCameraCameraX():this._shouldCreateDetachedObjectForTesting = false;
+  @visibleForTesting
+  AndroidCameraCameraX.forTesting(this._shouldCreateDetachedObjectForTesting );

[camerax] Implement onCameraClosing #3878

[camerax] Implement onCameraClosing #3878

Uh oh!

Conversation

camsim99 commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Highlights of what has changed since the latest reviews of the last PR:

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bparrishMines May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reidbaker May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camsim99 commented May 1, 2023 •

edited

Loading

bparrishMines May 4, 2023 •

edited

Loading

reidbaker May 5, 2023 •

edited

Loading