[camera] android-rework part 5: Android FPS range, resolution and sensor orientation features #3799

mvanbeusekom · 2021-04-08T14:09:00Z

Adds FPS range, resolution and sensor orientation features containing the implementation to handle FPS range, resolution and sensor orientation via the Android Camera2 API.

This is the fifth 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-08T14:09:06Z

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:49:04Z

@googlebot I consent.

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

…s_resolution_sensor_features

...amera/android/src/main/java/io/flutter/plugins/camera/features/fpsrange/FpsRangeFeature.java

stuartmorgan-g · 2021-05-25T17:51:32Z

...amera/android/src/main/java/io/flutter/plugins/camera/features/fpsrange/FpsRangeFeature.java

+      }
+    } catch (Exception e) {
+      // TODO: maybe just send a dart error back
+      //            pictureCaptureRequest.error("cameraAccess", e.getMessage(), null);


Why is this a TODO? It seems like having some kind of error handling is important here, whether that's reporting it back, or setting a default if that would make more sense.

After some refactoring the need for error handling is removed. I have removed the obsolete try...catch statement.

stuartmorgan-g · 2021-05-25T17:54:12Z

...a/android/src/test/java/io/flutter/plugins/camera/features/fpsrange/FpsRangeFeatureTest.java

+
+    verify(mockBuilder).set(eq(CaptureRequest.CONTROL_AE_TARGET_FPS_RANGE), any(Range.class));
+  }
+


The error codepath, whatever handling ends up going there, should be tested as well.

Has become obsolete

stuartmorgan-g · 2021-05-25T17:56:34Z

...a/android/src/main/java/io/flutter/plugins/camera/features/resolution/ResolutionFeature.java

+
+  public static CamcorderProfile getBestAvailableCamcorderProfileForResolutionPreset(
+      String cameraName, ResolutionPreset preset) {
+    int cameraId = Integer.parseInt(cameraName);


If what's actually needed is an integer ID, why does the method take a string?

Refactored the code to only accept an integer.

stuartmorgan-g · 2021-05-25T18:00:19Z

...main/java/io/flutter/plugins/camera/features/sensororientation/DeviceOrientationManager.java

+        angle = 270;
+        break;
+    }
+    if (isFrontFacing) angle *= -1;


stuartmorgan-g · 2021-05-25T18:00:34Z

...main/java/io/flutter/plugins/camera/features/sensororientation/DeviceOrientationManager.java

+  }
+
+  private void startUIListener() {
+    if (broadcastReceiver != null) return;


stuartmorgan-g · 2021-05-25T18:00:42Z

...main/java/io/flutter/plugins/camera/features/sensororientation/DeviceOrientationManager.java

+  }
+
+  private void stopSensorListener() {
+    if (orientationEventListener == null) return;


stuartmorgan-g · 2021-05-25T18:01:00Z

...main/java/io/flutter/plugins/camera/features/sensororientation/DeviceOrientationManager.java

+  }
+
+  private void stopUIListener() {
+    if (broadcastReceiver == null) return;


stuartmorgan-g · 2021-05-25T18:03:26Z

...main/java/io/flutter/plugins/camera/features/sensororientation/DeviceOrientationManager.java

+  }
+
+  public void start() {
+    startSensorListener();


Could you add a comment here explaining why we listen to both? It's not clear to me why we need to compute the orientation two different ways. Is it a case of needing a fallback if one isn't available?

stuartmorgan-g · 2021-05-25T18:07:31Z

...main/java/io/flutter/plugins/camera/features/sensororientation/DeviceOrientationManager.java

+  private Display getDisplay() {
+    return ((WindowManager) activity.getSystemService(Context.WINDOW_SERVICE)).getDefaultDisplay();
+  }
+}


This class has a lot of non-trivial logic, but no tests.

…s_resolution_sensor_features

mvanbeusekom · 2021-05-31T14:44:40Z

@stuartmorgan thank you for reviewing this PR, I have processed all feedback and would appreciate it if you could have another look.

stuartmorgan-g

Looking good, just some small things

...amera/android/src/main/java/io/flutter/plugins/camera/features/fpsrange/FpsRangeFeature.java

...a/android/src/main/java/io/flutter/plugins/camera/features/resolution/ResolutionFeature.java

...main/java/io/flutter/plugins/camera/features/sensororientation/DeviceOrientationManager.java

mvanbeusekom · 2021-06-16T07:13:36Z

@stuartmorgan I have processed your feedback and would appreciate it if you can have another look. Also I am not sure why the cla/google step is failing. The cla: yes label is correctly applied since the 8th of April and no other users but me have worked on this PR since.

stuartmorgan-g · 2021-06-16T16:44:03Z

You landed another co-authored commit after #3799 (comment), maybe it needs another consent message? I'm not sure exactly how that check is implemented internally.

...a/android/src/main/java/io/flutter/plugins/camera/features/resolution/ResolutionFeature.java

mvanbeusekom · 2021-06-16T17:19:14Z

You landed another co-authored commit after #3799 (comment), maybe it needs another consent message? I'm not sure exactly how that check is implemented internally.

Strange after pushing the changes you requested (removed the obsolete comment) the CLA is happy again. Anyway would appreciate it if you could have another look.

stuartmorgan-g

LGTM

…resolution_sensor_features

… and sensor orientation features (flutter/plugins#3799)

…sor orientation features (flutter#3799)

mvanbeusekom requested a review from stuartmorgan-g April 8, 2021 14:09

mvanbeusekom requested a review from bparrishMines as a code owner April 8, 2021 14:09

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

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

mvanbeusekom changed the title ~~[camera] android-rework part 5: Android flash and zoom features~~ [camera] android-rework part 5: Android FPS range, resolution and sensor orientation features Apr 8, 2021

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

mvanbeusekom and others added 5 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 FPS range, resolution and sensor orientation features

0bbed99

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

Use mockito-inline

1ba738d

mvanbeusekom force-pushed the camera-android/fps_resolution_sensor_features branch from a8f0e9f to 1ba738d Compare April 20, 2021 11:47

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

de4e70f

…s_resolution_sensor_features

stuartmorgan-g suggested changes May 25, 2021

View reviewed changes

mvanbeusekom added 5 commits May 26, 2021 15:22

Fix issue Pixel 4A

728346a

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

c014fe3

…s_resolution_sensor_features

Added API documentation

84f5e73

Processed feedback on PR

f763f77

Fix formatting

4a7c73a

mvanbeusekom requested a review from stuartmorgan-g May 31, 2021 14:44

Fix formatting

a890919

stuartmorgan-g suggested changes Jun 2, 2021

View reviewed changes

mvanbeusekom added 3 commits June 8, 2021 11:35

Only exclude 60 FPS limit for Pixel 4a

55a6702

Removed redundant empty line

cd53321

Fixed comment

35831d3

mvanbeusekom added 5 commits June 8, 2021 14:30

Test Pixel 4a workaround

a9f3142

Add tests for orientation updates

551800e

Fix formatting

68cbc56

Fix formatting

1b137c2

Added missing license header

6514a00

mvanbeusekom added 2 commits June 16, 2021 17:06

Accept cameraName as String

7f0180e

Format

24af367

stuartmorgan-g reviewed Jun 16, 2021

View reviewed changes

Removed obsolete comment

8313dd0

mvanbeusekom requested a review from stuartmorgan-g June 16, 2021 17:19

stuartmorgan-g approved these changes Jun 16, 2021

View reviewed changes

mvanbeusekom added 2 commits June 16, 2021 19:34

update method structure in class to a more logical order

a39c2e1

Merge remote-tracking branch 'origin/master' into camera-android/fps_…

3eecfe9

…resolution_sensor_features

mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jun 22, 2021

Fix formatting

7299b1d

fluttergithubbot merged commit 0232846 into flutter:master Jun 22, 2021

engine-flutter-autoroll mentioned this pull request Jun 22, 2021

Roll Plugins from 67885c5955ec to d91b0087d48d (3 revisions) flutter/flutter#85063

Closed

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

0232846 [camera] android-rework part 5: Android FPS range, resolution…

75fafba

… and sensor orientation features (flutter/plugins#3799)

engine-flutter-autoroll mentioned this pull request Jun 22, 2021

Roll Plugins from 67885c5955ec to b921089cf93a (5 revisions) flutter/flutter#85067

Closed

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

0232846 [camera] android-rework part 5: Android FPS range, resolution…

c3f7add

… and sensor orientation features (flutter/plugins#3799)

engine-flutter-autoroll mentioned this pull request Jun 22, 2021

Roll Plugins from 67885c5955ec to 9a90a9d0c61d (7 revisions) flutter/flutter#85073

Merged

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

0232846 [camera] android-rework part 5: Android FPS range, resolution…

d274179

… and sensor orientation features (flutter/plugins#3799)

amantoux pushed a commit to amantoux/plugins that referenced this pull request Jul 10, 2021

[camera] android-rework part 5: Android FPS range, resolution and sen…

3a40a72

…sor orientation features (flutter#3799)

fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021

[camera] android-rework part 5: Android FPS range, resolution and sen…

801b8e2

…sor orientation features (flutter#3799)

mvanbeusekom deleted the camera-android/fps_resolution_sensor_features branch September 21, 2021 09:36


		verify(mockBuilder).set(eq(CaptureRequest.CONTROL_AE_TARGET_FPS_RANGE), any(Range.class));
		}

[camera] android-rework part 5: Android FPS range, resolution and sensor orientation features #3799

[camera] android-rework part 5: Android FPS range, resolution and sensor orientation features #3799

Uh oh!

Conversation

mvanbeusekom commented Apr 8, 2021

Pre-launch Checklist

Uh oh!

google-cla bot commented Apr 8, 2021

Uh oh!

acoutts commented Apr 8, 2021

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!

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!

Choose a reason for hiding this comment

Uh oh!

mvanbeusekom commented May 31, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvanbeusekom commented Jun 16, 2021

Uh oh!

stuartmorgan-g commented Jun 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvanbeusekom commented Jun 16, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!