[camera] android-rework part 3: Android exposure related features #3797

mvanbeusekom · 2021-04-08T13:11:06Z

Adds exposure related features containing the implementation to handle auto exposure, exposure lock, exposure offset and exposure point via the Android Camera2 API.

This is the third PR in a series of pull-requests which will gradually introduce changes from PR #3651, making it easier to review the code (as discussed with @stuartmorgan).

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

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

google-cla · 2021-04-08T13:11:25Z

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.

acoutts · 2021-04-08T14:48:49Z

@googlebot I consent.

Co-authored-by: Andrew Coutts <[email protected]>

stuartmorgan

Initial pass comments below; I haven't reviewed the tests yet.

stuartmorgan · 2021-04-20T16:55:59Z

...droid/src/main/java/io/flutter/plugins/camera/features/exposurelock/ExposureLockFeature.java

+      default:
+        requestBuilder.set(CaptureRequest.CONTROL_AE_LOCK, false);
+        break;
+    }


Is there a reason to write this as a switch rather than just: requestBuilder.set(CaptureRequest.CONTROL_AE_LOCK, currentSetting = ExposureMode.locked);?

No, no particular reason. I have adjusted the code.

stuartmorgan · 2021-04-20T18:02:23Z

...d/src/main/java/io/flutter/plugins/camera/features/exposureoffset/ExposureOffsetFeature.java

+  }
+
+  /**
+   * Return the minimum exposure offset double value.


Returns

s/double value/value/
No need to document things in comments that are expressly indicated by the types in the function signature.

stuartmorgan · 2021-04-20T18:03:18Z

...d/src/main/java/io/flutter/plugins/camera/features/exposureoffset/ExposureOffsetFeature.java

+  }
+
+  /**
+   * Return the max exposure offset double value.


stuartmorgan · 2021-04-20T18:06:05Z

...d/src/main/java/io/flutter/plugins/camera/features/exposureoffset/ExposureOffsetFeature.java

+
+  /**
+   * Returns the exposure offset step size. This is the smallest amount which the exposure offset
+   * can be changed.


This second sentence seems misleading; it implies that this value is a just minimum delta in exposure offset changes, rather than a multiplier.

stuartmorgan · 2021-04-20T18:07:02Z

...oid/src/main/java/io/flutter/plugins/camera/features/exposureoffset/ExposureOffsetValue.java

+ * This represents the exposure offset value. It holds the minimum and maximum values, as well as
+ * the current setting value.
+ */
+public class ExposureOffsetValue {


Why are min/max and current value the same object? That seems surprising since they are fundamentally different kinds of values.

You are correct. The ExposureOffsetValue is currently used to return all values at once to the Camera class. However it would make much more sense if the Camera class could just call the ExposureOffsetFeature.getMinExposureOffset or ExposureOffsetFeature.getMaxExposureOffset methods when it needs to returns these values to the Dart side.

If have removed the ExposureOffsetValue and replaced it with a simple double and made the getMinExposureOffset and getMaxExposureOffset public.

stuartmorgan · 2021-04-20T18:10:25Z

...android/src/main/java/io/flutter/plugins/camera/features/regionboundaries/CameraRegions.java

+  public MeteringRectangle getMeteringRectangleForPoint(Size maxBoundaries, double x, double y) {
+    assert (x >= 0 && x <= 1);
+    assert (y >= 0 && y <= 1);
+    if (maxBoundaries == null)


Add braces, per Google Java style.
https://google.github.io/styleguide/javaguide.html#s4.1-braces

stuartmorgan · 2021-04-20T18:11:37Z

...android/src/main/java/io/flutter/plugins/camera/features/regionboundaries/CameraRegions.java

+    // Interpolate the target coordinate
+    int targetX = (int) Math.round(x * ((double) (maxBoundaries.getWidth() - 1)));
+    int targetY = (int) Math.round(y * ((double) (maxBoundaries.getHeight() - 1)));
+    // Determine the dimensions of the metering triangle (10th of the viewport)


Where is 1/10th coming from? Is this arbitrarily chosen? A standard? Other? A comment would be helpful.

Added explanation in the method API documentation.

stuartmorgan · 2021-04-20T18:13:46Z

...c/main/java/io/flutter/plugins/camera/features/regionboundaries/RegionBoundariesFeature.java

+
+/**
+ * Holds the current region boundaries. When this is created, you must provide a
+ * CaptureRequestBuilder for which we can read the distortion correction settings from.


Who are "you" and "we"?

I have merged the RegionBoundariesFeature class with the CameraRegions class which makes this file obsolete. I made sure to include the feedback you provided in the new structure as well.

The RegionBoundariesFeature in fact was used as a factory to create an instance of the CameraRegions class. The actual implementation didn't represent a "Camera Feature" and was mis-used as a feature and consumed by other feature classes.

stuartmorgan · 2021-04-20T18:14:59Z

...c/main/java/io/flutter/plugins/camera/features/regionboundaries/RegionBoundariesFeature.java

+    super(cameraProperties);
+
+    // No distortion correction support
+    if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.P


Why not just make this check internal to supportsDistortionCorrection? Assuming false is always going to be the fallback, presumably.

This is to silence the linter, since the else construct contains code that is only available on API level P and higher.

Ah, I missed that this was a < check; usually it's the opposite. I think it would be clearer to flip the bodies and use

if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.P && supportsDistortionCorrection()) { // the distortion correction code } else { // the fallback code }

stuartmorgan · 2021-04-20T18:15:59Z

...c/main/java/io/flutter/plugins/camera/features/regionboundaries/RegionBoundariesFeature.java

+  private boolean supportsDistortionCorrection() {
+    int[] availableDistortionCorrectionModes =
+        cameraProperties.getDistortionCorrectionAvailableModes();
+    if (availableDistortionCorrectionModes == null) availableDistortionCorrectionModes = new int[0];


…posure_features

ghost · 2021-04-30T12:27:48Z

S.a

…posure_features

mvanbeusekom · 2021-05-18T11:49:31Z

@stuartmorgan thank you for the review. I have processed your feedback and would appreciate it if you could have another look.

stuartmorgan

Just a few small things left, otherwise looks good.

stuartmorgan · 2021-05-25T17:29:23Z

...c/main/java/io/flutter/plugins/camera/features/regionboundaries/RegionBoundariesFeature.java

+    super(cameraProperties);
+
+    // No distortion correction support
+    if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.P


Ah, I missed that this was a < check; usually it's the opposite. I think it would be clearer to flip the bodies and use

if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.P && supportsDistortionCorrection()) { // the distortion correction code } else { // the fallback code }

stuartmorgan · 2021-05-25T17:33:24Z

...d/src/main/java/io/flutter/plugins/camera/features/exposureoffset/ExposureOffsetFeature.java

  }

  /**
-   * Return the minimum exposure offset double value.
+   * Returns the minimum exposure offset value, in counts of {@link #getExposureOffsetStepSize}.


The return value is multiplied by stepSize, so this seems wrong.

I was strongly influenced by the documentation from the Android Camera2 API when I documented this method.

Here this mention:

Maximum and minimum exposure compensation values for android.control.aeExposureCompensation, in counts of android.control.aeCompensationStep, that are supported by this camera device.

I am not saying this would be necessarily correct or the best, just wanted to quickly verify with you before I update it.

I don't follow. You're pointing to the documentation for the exposure range, which is line 64 here. This method doesn't return that, it returns that multiplied by the step size (line 67). If you take step counts, and multiply by step sizes, you don't have step counts any more. You have the actual exposure value.

Yes you are right, I completely read the Android documentation wrong.

stuartmorgan · 2021-05-25T17:33:29Z

...d/src/main/java/io/flutter/plugins/camera/features/exposureoffset/ExposureOffsetFeature.java

    Range<Integer> range = cameraProperties.getControlAutoExposureCompensationRange();
    double minStepped = range == null ? 0 : range.getLower();
    double stepSize = getExposureOffsetStepSize();
    return minStepped * stepSize;
  }

  /**
-   * Return the max exposure offset double value.
+   * Returns the maximum exposure offset value, in counts of {@link #getExposureOffsetStepSize}.


…posure_features

stuartmorgan

LGTM

stuartmorgan · 2021-05-27T14:34:44Z

Something seems to have gone wrong with the Windows re-run; it either didn't schedule, or didn't update the status here. Maybe something related to the jobs changing right around the time I restarted it?

I'm just going to land this, as the Windows test isn't relevant to the change.

…ures (flutter/plugins#3797)

* master: (131 commits) [in_app_purchase] fix "autoConsume" param in "buyConsumable" (flutter#3957) [video_player_web] fix: set autoplay to false during initialization (flutter#3985) [multiple_web] Adapt web PlatformView widgets to work slotted. (flutter#3964) [url_launcher] Add iOS unit and UI tests (flutter#3987) [image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. (flutter#3956) [script/tool] Use 'dart pub' instead of deprecated 'pub' (flutter#3991) [video_player] Add iOS unit and UI tests (flutter#3986) Add pubspec convention checks (flutter#3984) Enable pubspec dependency sorting lint (flutter#3983) [image_picker] Migrate maven repo from jcenter to mavenCentral (flutter#3915) [video_player] Update README.md (flutter#3975) [script/tool] speed up the pub get portion of the analyze command (flutter#3982) Revert commit e742a7b (flutter#3976) Added support to request list of purchases (flutter#3944) [google_maps_flutter] Add iOS unit and UI tests (flutter#3978) Added Windows to the description (flutter#3936) use 'flutter pub get' for both dart and flutter packages (flutter#3973) [camera] android-rework part 3: Android exposure related features (flutter#3797) Remove exoplayer workaround from everything but video_player (flutter#3980) Allow reverts when checking versions (flutter#3981) ...

@stuartmorgan

…utter#3797) Adds exposure related features containing the implementation to handle auto exposure, exposure lock, exposure offset and exposure point via the Android Camera2 API. This is the third PR in a series of pull-requests which will gradually introduce changes from PR flutter#3651, making it easier to review the code (as discussed with @stuartmorgan).

mvanbeusekom requested a review from stuartmorgan April 8, 2021 13:11

mvanbeusekom requested a review from bparrishMines as a code owner April 8, 2021 13:11

google-cla bot added the cla: no label Apr 8, 2021

github-actions bot added p: camera platform-android labels Apr 8, 2021

google-cla bot added cla: yes and removed cla: no labels Apr 8, 2021

mvanbeusekom and others added 4 commits April 20, 2021 09:35

Base classes to support Android camera features

2b7aa9b

Co-authored-by: Andrew Coutts <[email protected]>

Fixed formatting

f780742

Applied feedback from PR

76bc5bd

Added Android exposure related features

461309a

Co-authored-by: Andrew Coutts <[email protected]>

mvanbeusekom force-pushed the camera-android/exposure_features branch from 7a37b86 to 461309a Compare April 20, 2021 11:48

stuartmorgan suggested changes Apr 20, 2021

View reviewed changes

mvanbeusekom added 2 commits April 21, 2021 14:04

Merge remote-tracking branch 'upstream/master' into camera-android/ex…

058805b

…posure_features

Added partial feedback from PR

7052b45

mvanbeusekom added 3 commits May 17, 2021 13:40

Merge remote-tracking branch 'upstream/master' into camera-android/ex…

8985a9c

…posure_features

Updated feedback from PR

7a0b41e

Fix formatting

6a50e99

mvanbeusekom requested a review from stuartmorgan May 18, 2021 11:49

stuartmorgan suggested changes May 25, 2021

View reviewed changes

mvanbeusekom added 4 commits May 25, 2021 19:48

Merge remote-tracking branch 'upstream/master' into camera-android/ex…

a3a4ee1

…posure_features

Apply feedback from PR

989ecfa

Correct documentation of min and max exposure offset

ebae3de

Fix formatting

81dafb3

stuartmorgan approved these changes May 26, 2021

View reviewed changes

stuartmorgan merged commit 2bfba7b into flutter:master May 27, 2021

engine-flutter-autoroll mentioned this pull request May 27, 2021

Roll Plugins from 246b493538d2 to df0361e2d994 (5 revisions) flutter/flutter#83490

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 27, 2021

2bfba7b [camera] android-rework part 3: Android exposure related feat…

52f696e

…ures (flutter/plugins#3797)

mvanbeusekom deleted the camera-android/exposure_features branch May 27, 2021 17:27

[camera] android-rework part 3: Android exposure related features #3797

[camera] android-rework part 3: Android exposure related features #3797

Conversation

mvanbeusekom commented Apr 8, 2021

Pre-launch Checklist

google-cla bot commented Apr 8, 2021

acoutts commented Apr 8, 2021

stuartmorgan 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

mvanbeusekom May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvanbeusekom May 18, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 30, 2021

mvanbeusekom commented May 18, 2021

stuartmorgan 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

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan commented May 27, 2021

mvanbeusekom May 18, 2021 •

edited

Loading

mvanbeusekom May 18, 2021 •

edited

Loading