[camerax] Wrap classes to implement resolution configuration for image capture, image analysis, and preview #4523

camsim99 · 2023-07-19T17:01:16Z

Wraps classes to implement resolution configuration for image capture, image analysis, and preview. Also bumps CameraX version to latest and removes the deprecated classes used previously.

No functionality changes. Also thanks to @bparrishMines who did majority of the work here!

Part of flutter/flutter#120462

Pre-launch Checklist

camsim99 · 2023-07-19T19:40:10Z

packages/camera/camera_android_camerax/lib/src/camerax_library.g.dart

+///  * Update `LiveDataHostApiImpl#getValue` is updated to properly return
+///    identifiers for instances of type S.
+///  * Update `ObserverFlutterApiWrapper#onChanged` to properly handle receiving
+///    calls with instances of type S if a LiveData<S> instance is observed.


This is unrelated. Pigeon must have carried over my comment I left in camerax_library.dart in a previous PR.

Should this be removed?

No, pigeon will just keep adding it back since it's part of the source file

...camerax/android/src/main/java/io/flutter/plugins/camerax/AspectRatioStrategyHostApiImpl.java

gmackall · 2023-07-21T18:33:27Z

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

+  /// this fallback rule is used.
+  ///
+  /// See https://developer.android.com/reference/androidx/camera/core/resolutionselector/AspectRatioStrategy#FALLBACK_RULE_NONE().
+  static const int fallbackRuleNone = 0;


Not a blocker for this PR, but I did want to ask if it would be better for us to make a getter for these constants.

We are essentially relying on the camerax implementations to never change these values, and if they did it might not get caught by our testing because they could still be valid values, but just not actually correspond to what they are named. I know we are doing this in existing camerax and webview wrappings though.

My initial thought is that changing them to getters wouldn't be a significant improvement. If the values change, we likely will be quick to find out. Plus, there's the risk that they change the name and a getter would fail there. Not sure which case is more likely but I think it's something to consider. @bparrishMines any thoughts on this?

It's hard to change them to getters since it would require an async call. Which would make it unintuitive to use. You could also add a Java unit test that verifies that these constants stay the same. However, this would still be susceptible to an instance where the constant values are different for different Android versions. Although this case is probably extremely rare.

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

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

packages/camera/camera_android_camerax/test/aspect_ratio_strategy_test.dart

packages/camera/camera_android_camerax/test/resolution_selector_test.dart

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

gmackall · 2023-07-21T19:46:14Z

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

+@immutable
+class ResolutionStrategy extends JavaObject {
+  /// Construct a [ResolutionStrategy].
+  ResolutionStrategy({


What do you think about making the arguments to this constructor non-null (as they are for the android class) and instead using a private named constructor instead for the creation of the highest available strategy?

I love this idea. Changed it and added tests.

I believe as is, it is still nullable (it is required but someone can still pass null).

I think this can be changed to required Size this.boundSize, though, to keep it as an initializing formal but just with a stricter (non-nullable) type.

See the "NOTE: Also note that it is possible to enforce a type that is stricter"... in https://dart.dev/tools/linter-rules/prefer_initializing_formals (can't link sections unfortunately).

gmackall · 2023-07-21T19:56:52Z

Left some comments but mostly looks good to me! Main things I'm wondering about are the comment about the constructor for resolution strategy and the question about how we are handling named constants.

Other than that it's just comment nits and requests for detached constructor tests

gmackall

Just one more comment about the ResolutionStrategy constructor, and a couple of comment nits!

...camerax/android/src/main/java/io/flutter/plugins/camerax/AspectRatioStrategyHostApiImpl.java

gmackall · 2023-07-26T00:27:19Z

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

+@immutable
+class ResolutionStrategy extends JavaObject {
+  /// Construct a [ResolutionStrategy].
+  ResolutionStrategy({


I believe as is, it is still nullable (it is required but someone can still pass null).

I think this can be changed to required Size this.boundSize, though, to keep it as an initializing formal but just with a stricter (non-nullable) type.

See the "NOTE: Also note that it is possible to enforce a type that is stricter"... in https://dart.dev/tools/linter-rules/prefer_initializing_formals (can't link sections unfortunately).

bparrishMines

LGTM

reidbaker · 2023-07-27T20:22:55Z

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

@@ -116,10 +116,6 @@ class AndroidCameraCameraX extends CameraPlatform {
  @visibleForTesting
  CameraSelector? cameraSelector;

-  /// The resolution preset used to create a camera that should be used for


Is this a breaking change?

No, it wasn't used before

reidbaker · 2023-07-27T20:25:31Z

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

+  /// options.
+  ///
+  /// See https://developer.android.com/reference/androidx/camera/core/resolutionselector/AspectRatioStrategy#FALLBACK_RULE_AUTO().
+  static const int fallbackRuleAuto = 1;


Consider bumping these constants to the top of the class for easier discoverability.

This would go against the pattern I've been following but if you have strong opinions I can follow this up with another PR moving all static constants to the top of their respective classes

reidbaker · 2023-07-27T20:29:04Z

packages/camera/camera_android_camerax/lib/src/camerax_library.g.dart

+///  * Update `LiveDataHostApiImpl#getValue` is updated to properly return
+///    identifiers for instances of type S.
+///  * Update `ObserverFlutterApiWrapper#onChanged` to properly handle receiving
+///    calls with instances of type S if a LiveData<S> instance is observed.


Should this be removed?

…for image capture, image analysis, and preview (flutter/packages#4523)

flutter/packages@f4ae933...10aab44 2023-07-28 [email protected] remove unneeded imports (flutter/packages#4579) 2023-07-27 [email protected] [camerax] Wrap classes to implement resolution configuration for image capture, image analysis, and preview (flutter/packages#4523) 2023-07-27 [email protected] [camera_android] Provides a default exposure point if null. (flutter/packages#3851) 2023-07-27 [email protected] Roll Flutter from 61fd11d to dd9764e (5 revisions) (flutter/packages#4577) 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 [email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

camsim99 added 20 commits May 1, 2023 16:46

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

dec3d69

Merge remote-tracking branch 'upstream/main'

0e0333b

Merge remote-tracking branch 'upstream/main'

bd7ac99

Merge remote-tracking branch 'upstream/main'

5c3363b

Undo changes

fed9621

Merge remote-tracking branch 'upstream/main'

5aabe34

Merge remote-tracking branch 'upstream/main'

2b9a352

Merge remote-tracking branch 'upstream/main'

a1173da

Merge remote-tracking branch 'upstream/main'

cbc3d6b

Merge remote-tracking branch 'upstream/main'

cae5a4c

Merge remote-tracking branch 'upstream/main'

72283db

Merge remote-tracking branch 'upstream/main'

166a77c

Merge remote-tracking branch 'upstream/main'

399780e

Merge remote-tracking branch 'upstream/main'

8d5d0e7

Merge remote-tracking branch 'upstream/main'

084d960

Merge remote-tracking branch 'upstream/main'

d2a59ac

Merge remote-tracking branch 'upstream/main'

a1422bf

Merge remote-tracking branch 'upstream/main'

bdd87a6

Merge remote-tracking branch 'upstream/main'

137a28b

Add wrapped classes, tests, and remove deprecated apis

c78e0b8

github-actions bot added p: camera platform-android labels Jul 19, 2023

camsim99 mentioned this pull request Jul 19, 2023

[camerax] Implement resolution configuration #3799

Merged

11 tasks

camsim99 added 5 commits July 19, 2023 10:19

Modify null check to fix integration test

3cd06e8

Cleanup

a383e8f

Add missing import

725a4ec

Fix some spelling errors

bdfb634

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

fc7c977

camsim99 marked this pull request as ready for review July 19, 2023 19:39

camsim99 commented Jul 19, 2023

View reviewed changes

camsim99 requested review from bparrishMines, gmackall and a team July 19, 2023 19:40

Mark new classes as immutable

41fdabc

gmackall reviewed Jul 21, 2023

View reviewed changes

...camerax/android/src/main/java/io/flutter/plugins/camerax/AspectRatioStrategyHostApiImpl.java Outdated Show resolved Hide resolved

gmackall reviewed Jul 21, 2023

View reviewed changes

Address review

996ce03

camsim99 requested a review from gmackall July 24, 2023 21:49

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

d9ab6c3

gmackall reviewed Jul 26, 2023

View reviewed changes

bparrishMines approved these changes Jul 27, 2023

View reviewed changes

reidbaker approved these changes Jul 27, 2023

View reviewed changes

Address review

98e1035

gmackall approved these changes Jul 27, 2023

View reviewed changes

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

auto-submit bot merged commit 897f695 into flutter:main Jul 27, 2023

engine-flutter-autoroll mentioned this pull request Jul 28, 2023

Roll Packages from f4ae9336ebb7 to 10aab445d985 (4 revisions) flutter/flutter#131483

Merged

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

897f695 [camerax] Wrap classes to implement resolution configuration …

7fb5d17

…for image capture, image analysis, and preview (flutter/packages#4523)

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camerax] Wrap classes to implement resolution configuration for image capture, image analysis, and preview #4523

[camerax] Wrap classes to implement resolution configuration for image capture, image analysis, and preview #4523

camsim99 commented Jul 19, 2023 •

edited

Loading

camsim99 Jul 19, 2023

reidbaker Jul 27, 2023

camsim99 Jul 27, 2023

gmackall Jul 21, 2023

camsim99 Jul 24, 2023

bparrishMines Jul 27, 2023

gmackall Jul 21, 2023

camsim99 Jul 24, 2023

gmackall Jul 26, 2023

gmackall commented Jul 21, 2023

gmackall left a comment

gmackall Jul 26, 2023

bparrishMines left a comment

reidbaker Jul 27, 2023

camsim99 Jul 27, 2023

reidbaker Jul 27, 2023

camsim99 Jul 27, 2023

reidbaker Jul 27, 2023

[camerax] Wrap classes to implement resolution configuration for image capture, image analysis, and preview #4523

[camerax] Wrap classes to implement resolution configuration for image capture, image analysis, and preview #4523

Conversation

camsim99 commented Jul 19, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmackall commented Jul 21, 2023

gmackall left a comment

Choose a reason for hiding this 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

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 commented Jul 19, 2023 •

edited

Loading