[camerax] Implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` #6059

camsim99 · 2024-02-05T19:38:09Z

This PR implements setFocusPoint, setExposurePoint, setExposureOffset and makes some small fixes here and there, each of which I have left a comment about for context.

Part of flutter/flutter#120468 & flutter/flutter#120467.

~~NOTE: Should land after #6068.~~ done :)

Pre-launch Checklist

camsim99 · 2024-02-05T21:22:35Z

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

@@ -446,13 +465,67 @@ class AndroidCameraCameraX extends CameraPlatform {

  /// Gets the supported step size for exposure offset for the selected camera in EV units.
  ///
-  /// Returns 0 when exposure compensation is not supported.
-  ///


This explicitly went against the platform interface; 0 should be returned when step sizes are not supported. So, this is an unrelated fix for that fact.

...droid_camerax/android/src/main/java/io/flutter/plugins/camerax/CameraControlHostApiImpl.java

camsim99 · 2024-02-07T17:26:56Z

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

+      // supported for the device.
+      throw CameraException(exposureCompensationNotSupported,
+          'Exposure compensation not supported for the device.');
+    }


Looking for feedback on throwing an exception here/how to handle exposure compensation not being supported on the device.

What would the other options be? Another specific return value for this case, like -1?

Yeah something like that! Returning a negative number or throwing an exception are really the only two options I can think of honestly, since we cannot return null.

I suppose I lean towards a special negative return value, because it isn't an actual error (or at least CameraX doesn't think it is). But this is a weak preference, if you have any reason for preferring throwing an error instead.

Do you know how the other platforms handle this? I'm not familiar with how exposureOffsetStepSize works, but it seems the app-facing package indicates that a zero should be returned when it is not supported: https://pub.dev/documentation/camera/latest/camera/CameraController/getExposureOffsetStepSize.html

I think we should just return 0 if the other platforms are doing this as well.

Thanks for the research!

I changed it to return 0 since that is consistent with iOS and CameraX and added a note in the docs.

After talking again with @gmackall, decided to change to -1 and modify the platform interface. I think it will be helpful to have a defined return value for unsupported exposure compensation going forward and help differentiate it from the other platforms, since iOS seems to not support step sizes by always returning 0 and it doesn't seem that exposure control is implemented on web or Windows.

Edit: will need to update the platform interface in another PR it seems

@stuartmorgan let us know if you have any concerns about modifying the interface, though I believe this addition will help future-proof it. ~~Windows could also, then, return -1.~~ This is confusing because Windows just doesn't support it at all.

Going to land this for now since it's blocking me, but I can revisit this if needed and otherwise, will proceed with a PR for the platform interface.

@stuartmorgan let us know if you have any concerns about modifying the interface, though I believe this addition will help future-proof it.

What do you mean by "modify the platform interface"? If it's just a docs change that sounds reasonable given the discussion above, if it's a code change that's complicated.

Just a docs change :) #6182

camsim99 · 2024-02-07T17:28:41Z

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

+          'setExposureOffsetFailed',
+          e.message ??
+              'Setting the camera exposure compensation index failed.');
+    }


Looking for feedback on throwing an exception here and forwarding the exception from CameraControl in order to conform with the platform interface, in the sense that the method

Throws a CameraException when an illegal offset is supplied.

as per its documentation. Another option is explicitly checking in this method, but was worried that such an asynchronous call could impact performance.

@bparrishMines no pressure on a review, but if you have time to take a look, would appreciate your feedback on this decision!

I'm fine with throwing an exception here since a user is explicitly warned about it in the app-facing package.

Although this does highlight the need for a way to check if a feature is supported without using a try/catch for every method. Something we will consider when the camera platform interface is updated.

camsim99 · 2024-02-07T17:36:35Z

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

          cameraControlIdentifier, actionIdentifier);
+      if (focusMeteringResultId == null) {


Part of previously mentioned fix.

camsim99 · 2024-02-07T17:36:41Z

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

-      return setExposureCompensationIndex(identifier, index);
+      final int? exposureCompensationIndex =
+          await setExposureCompensationIndex(identifier, index);
+      if (exposureCompensationIndex == null) {


Part of previously mentioned fix.

camsim99 · 2024-02-07T17:37:23Z

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

-      return Future<int?>.value();
+      // Surfacing error to plugin layer to maintain consistency of
+      // setExposureOffset implementation across platform implementations.
+      rethrow;


Looking for feedback on this as per https://github.com/flutter/packages/pull/6059/files#r1481841622.

camsim99 · 2024-02-07T17:40:06Z

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

@@ -69,6 +73,9 @@ class AndroidCameraCameraX extends CameraPlatform {
  @visibleForTesting
  CameraInfo? cameraInfo;

+  /// The [CameraControl] instance that corresponds to the [camera] instance.
+  CameraControl? cameraControl;
+


Decided to save this whenever the related camera is updated versus making async calls to retrieve it every time that it's needed.

camsim99 · 2024-02-08T18:23:21Z

...droid_camerax/android/src/main/java/io/flutter/plugins/camerax/MeteringPointHostApiImpl.java

+      DisplayOrientedMeteringPointFactory factory =
+          getDisplayOrientedMeteringPointFactory(display, 1f, 1f, cameraInfo);


A slightly larger fix: I originally implemented this host API with a different MeteringPointFactory, which was not technically wrong but would have required that the plugin manually handle changes in camera and device orientation and can be tricky. This factory automatically does that based on the current camera's CameraInfo and the current display. So, I made changes in this PR to the MeteringPoint wrapper to accommodate that fact.

Fixes small issues I noticed in starting/stopping video capture while working on #6059, notably: - Change all remaining `unawaited` calls to `await` to avoid any racy behavior. - Update `camera` info after `VideoCapture` use case is bound to the lifecycle of the plugin's `ProcessCameraProvider` to make sure it is up to date. - ~Unbind `VideoCapture` use case when video recording stops since it was suggested to lazily load it for performance reasons (open to pushback on this).~ this would require potentially more changes than I originally thought - Make tests checking that async methods throw exceptions actually wait for those exceptions as this may cause flaky test behavior. Fixes flutter/flutter#132499 as this PR removes any remaining `unawaited` calls.

gmackall

LGTM outside of the decision on how to handle the "Exposure compensation not supported for the device" situation

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart

…erax_test.dart Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>

bparrishMines

LGTM

I'll leave it to @gmackall for the final review.

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

bparrishMines · 2024-02-21T03:54:43Z

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

+      // supported for the device.
+      throw CameraException(exposureCompensationNotSupported,
+          'Exposure compensation not supported for the device.');
+    }


Looking at other platforms:

iOS returns 0: https://github.com/flutter/packages/blob/main/packages/camera/camera_avfoundation/ios/Classes/CameraPlugin.m#L233

Web throws an exception: https://github.com/flutter/packages/blob/main/packages/camera/camera_web/lib/src/camera_web.dart#L557

Windows returns 1. Although, I'm not sure why that is: https://github.com/flutter/packages/blob/main/packages/camera/camera_windows/lib/camera_windows.dart#L299

And as you mentioned above, camera_android states that all devices have support. So it looks like every platform handles it differently 😞.

I think returning 0 or throwing an exception should be fine. Returning a -1 would make it consistent that each platform handles it uniquely though haha

…odes and the ability to set focus points for each (#6109) Modifies `CameraInitializedEvent` to 1. Set the initial exposure and focus modes to auto, since CameraX defaults to auto exposure mode and only supports auto focus mode without Camera2 interop 2. Sets the ability to set focus and exposure points to be true, since CameraX supports these by default ~Should land after #6059 so that these values reflect what is actually implemented on our end~ Done :)

…tExposureOffset` (flutter/packages#6059)

flutter/packages@48048f6...078c2a3 2024-02-22 engine-flutter-autoroll@skia.org Manual roll Flutter from 5129806 to efee280 (47 revisions) (flutter/packages#6180) 2024-02-22 engine-flutter-autoroll@skia.org Manual roll Flutter (stable) from bae5e49 to abb292a (1 revision) (flutter/packages#6179) 2024-02-21 louisehsu@google.com [in_app_purchase] Convert refreshReceipt(), startObservingPaymentQueue(), stopObservingPaymentQueue(), registerPaymentQueueDelegate(), removePaymentQueueDelegate(), showPriceConsentIfNeeded() to Pigeon (flutter/packages#6165) 2024-02-21 43054281+camsim99@users.noreply.github.com [camerax] Modifies initialized camera info to reflect default AF/AE modes and the ability to set focus points for each (flutter/packages#6109) 2024-02-21 43054281+camsim99@users.noreply.github.com [camerax] Implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` (flutter/packages#6059) 2024-02-21 stuartmorgan@google.com [various] Commit Windows build migrations (flutter/packages#6175) 2024-02-21 kevmoo@users.noreply.github.com [flutter_markdown] Support wasm (flutter/packages#6168) 2024-02-21 737941+loic-sharma@users.noreply.github.com [ci] Run Windows Arm64 build tests post-submit (flutter/packages#6166) 2024-02-21 stuartmorgan@google.com [url_launcher] Remove `renderView` usage (flutter/packages#6137) 2024-02-21 b@sdevaan.nl [camera_android_camerax] Fix typo in readme (flutter/packages#6143) 2024-02-21 stuartmorgan@google.com [local_auth] Switch iOS endorsement to `local_auth_darwin` (flutter/packages#6107) 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 flutter-ecosystem@google.com,rmistry@google.com 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

) Documents `getExposureOffsetStepSize` to return -1 if the device does not support exposure compensation. Helps account for `camera_android_camerax` since CameraX does not return a step size if exposure compensation is not supported. A follow up to [this discussion](#6059 (comment)).

Implements `setExposureMode`. Fixes flutter/flutter#120468. ~To be landed after (1) #6059 then (2) #6109 Done :)

…utter#6182) Documents `getExposureOffsetStepSize` to return -1 if the device does not support exposure compensation. Helps account for `camera_android_camerax` since CameraX does not return a step size if exposure compensation is not supported. A follow up to [this discussion](flutter#6059 (comment)).

Implements `setExposureMode`. Fixes flutter/flutter#120468. ~To be landed after (1) flutter#6059 then (2) flutter#6109 Done :)

Fixes small issues I noticed in starting/stopping video capture while working on flutter#6059, notably: - Change all remaining `unawaited` calls to `await` to avoid any racy behavior. - Update `camera` info after `VideoCapture` use case is bound to the lifecycle of the plugin's `ProcessCameraProvider` to make sure it is up to date. - ~Unbind `VideoCapture` use case when video recording stops since it was suggested to lazily load it for performance reasons (open to pushback on this).~ this would require potentially more changes than I originally thought - Make tests checking that async methods throw exceptions actually wait for those exceptions as this may cause flaky test behavior. Fixes flutter/flutter#132499 as this PR removes any remaining `unawaited` calls.

…eOffset` (flutter#6059) This PR implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` and makes some small fixes here and there, each of which I have left a comment about for context. Part of flutter/flutter#120468 & flutter/flutter#120467. ~NOTE: Should land after flutter#6068 done :)

…odes and the ability to set focus points for each (flutter#6109) Modifies `CameraInitializedEvent` to 1. Set the initial exposure and focus modes to auto, since CameraX defaults to auto exposure mode and only supports auto focus mode without Camera2 interop 2. Sets the ability to set focus and exposure points to be true, since CameraX supports these by default ~Should land after flutter#6059 so that these values reflect what is actually implemented on our end~ Done :)

…utter#6182) Documents `getExposureOffsetStepSize` to return -1 if the device does not support exposure compensation. Helps account for `camera_android_camerax` since CameraX does not return a step size if exposure compensation is not supported. A follow up to [this discussion](flutter#6059 (comment)).

Implements `setExposureMode`. Fixes flutter/flutter#120468. ~To be landed after (1) flutter#6059 then (2) flutter#6109 Done :)

camsim99 added 2 commits February 2, 2024 10:41

Start implementation of setting points + offset

b6729ac

Finish adding intial impls

14d1024

github-actions bot added the p: camera label Feb 5, 2024

Fix crashes

975806b

github-actions bot added the platform-android label Feb 5, 2024

camsim99 commented Feb 5, 2024

View reviewed changes

camsim99 added 2 commits February 6, 2024 10:59

Cleanup pt1

a7181b2

Cleanup pt2

ea5f15f

camsim99 mentioned this pull request Feb 6, 2024

[camerax] Small fixes to starting/stopping video capture #6068

Merged

11 tasks

camsim99 added 5 commits February 6, 2024 13:39

Self review

5fe2976

Add tests

e41a825

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

9736287

Format

38e88a8

Update readme + bump version

2843626

camsim99 commented Feb 7, 2024

View reviewed changes

...droid_camerax/android/src/main/java/io/flutter/plugins/camerax/CameraControlHostApiImpl.java Show resolved Hide resolved

camsim99 commented Feb 7, 2024

View reviewed changes

camsim99 added 2 commits February 7, 2024 09:46

Improve docs

35ebf13

Change metering point factory

c2f75ec

camsim99 commented Feb 8, 2024

View reviewed changes

camsim99 added 2 commits February 8, 2024 10:29

Self review

b8526e1

Nits + lints

928b4cd

camsim99 added 2 commits February 9, 2024 10:06

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

3527d9e

Update video cap test

1d4d37e

camsim99 marked this pull request as ready for review February 9, 2024 18:18

camsim99 requested a review from gmackall February 14, 2024 18:46

gmackall reviewed Feb 20, 2024

View reviewed changes

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart Outdated Show resolved Hide resolved

camsim99 and others added 3 commits February 20, 2024 11:16

Update packages/camera/camera_android_camerax/test/android_camera_cam…

2c3c9ab

…erax_test.dart Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>

Use absolute paths

6652171

Merge remote-tracking branch 'refs/remotes/origin/camx_fe' into camx_fe

a08eed6

bparrishMines reviewed Feb 21, 2024

View reviewed changes

camsim99 added 2 commits February 21, 2024 09:41

Address nit + change getExposureOffsetStepSize to return 0

c35fa15

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

fac3ff6

camsim99 requested a review from gmackall February 21, 2024 18:21

Change to -1

077ee37

gmackall approved these changes Feb 21, 2024

View reviewed changes

camsim99 added 2 commits February 21, 2024 10:36

Update wording

ff3fc39

Undo platform interface changes

fc60e77

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

auto-submit bot merged commit 83d7fc6 into flutter:main Feb 21, 2024
78 checks passed

engine-flutter-autoroll mentioned this pull request Feb 22, 2024

Roll Packages from 48048f6bc779 to 078c2a304a01 (11 revisions) flutter/flutter#143935

Merged

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

83d7fc6 [camerax] Implements setFocusPoint, setExposurePoint, `se…

071eef9

…tExposureOffset` (flutter/packages#6059)

camsim99 mentioned this pull request Feb 22, 2024

[camera] Document -1 return value for getExposureOffsetStepSize #6182

Merged

11 tasks

auto-submit bot pushed a commit that referenced this pull request Feb 27, 2024

[camerax] Implements setExposureMode (#6110)

e07eb50

Implements `setExposureMode`. Fixes flutter/flutter#120468. ~To be landed after (1) #6059 then (2) #6109 Done :)

LouiseHsu pushed a commit to LouiseHsu/packages that referenced this pull request Mar 7, 2024

[camerax] Implements setExposureMode (flutter#6110)

f9c0961

Implements `setExposureMode`. Fixes flutter/flutter#120468. ~To be landed after (1) flutter#6059 then (2) flutter#6109 Done :)

reidbaker mentioned this pull request Apr 1, 2024

[camera_android_camerax] Move integration_test dependency to tests reidbaker/packages#4

Closed

11 tasks

arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024

[camerax] Implements setExposureMode (flutter#6110)

19eb2d0

Implements `setExposureMode`. Fixes flutter/flutter#120468. ~To be landed after (1) flutter#6059 then (2) flutter#6109 Done :)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camerax] Implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` #6059

[camerax] Implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` #6059

camsim99 commented Feb 5, 2024 •

edited

Loading

camsim99 Feb 5, 2024

camsim99 Feb 7, 2024

gmackall Feb 13, 2024

camsim99 Feb 14, 2024

gmackall Feb 20, 2024

bparrishMines Feb 20, 2024

camsim99 Feb 21, 2024

camsim99 Feb 21, 2024 •

edited

Loading

camsim99 Feb 21, 2024

stuartmorgan Feb 22, 2024

camsim99 Feb 22, 2024

camsim99 Feb 7, 2024

camsim99 Feb 9, 2024

bparrishMines Feb 20, 2024

camsim99 Feb 7, 2024

camsim99 Feb 7, 2024

camsim99 Feb 7, 2024

camsim99 Feb 7, 2024

camsim99 Feb 8, 2024

gmackall left a comment

bparrishMines left a comment

bparrishMines Feb 21, 2024

		cameraControlIdentifier, actionIdentifier);
		if (focusMeteringResultId == null) {

		DisplayOrientedMeteringPointFactory factory =
		getDisplayOrientedMeteringPointFactory(display, 1f, 1f, cameraInfo);

[camerax] Implements setFocusPoint, setExposurePoint, setExposureOffset #6059

[camerax] Implements setFocusPoint, setExposurePoint, setExposureOffset #6059

Conversation

camsim99 commented Feb 5, 2024 • edited Loading

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

camsim99 Feb 21, 2024 • 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

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

gmackall left a comment

Choose a reason for hiding this comment

bparrishMines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[camerax] Implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` #6059

[camerax] Implements `setFocusPoint`, `setExposurePoint`, `setExposureOffset` #6059

camsim99 commented Feb 5, 2024 •

edited

Loading

camsim99 Feb 21, 2024 •

edited

Loading