Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[camerax] Implement Image Streaming #3454

Merged
merged 73 commits into from
Apr 28, 2023
Merged

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented Mar 13, 2023

Implements image streaming with all required image data, see here.

Fixes flutter/flutter#120463.
Fixes flutter/flutter#113792 along the way.

Special shoutout to @reidbaker for pair programming on this!

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

/** Constructor is private to enforce null safety; use Builder. */
private ResolutionInfo() {}
/** Constructor is non-public to enforce null safety; use Builder. */
ResolutionInfo() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java behaves differently to dart non stated means package private which means that customer code can call it if they put their code in the right path. I think this should be protected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is generated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you set the scope on the class it is generated from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so

late final _PlaneProxyHostApiImpl _api;

/// Returns the pixels buffer containing frame data.
Future<Uint8List> getBuffer() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you lay out pros/cons or maybe use cases that are harder/easier for getters vs the current implementation.

Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few comments and a question about the InstanceManager test

@@ -252,7 +266,8 @@ class AndroidCameraCameraX extends CameraPlatform {
/// [cameraId] not used.
@override
Future<void> pausePreview(int cameraId) async {
_unbindPreviewFromLifecycle();
assert(preview != null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will call _unbindUseCase with a null preview in release builds. I suggest you either make this a hard check on preview nullability or remove this line.


/// Unbinds [useCase] from camera lifecycle controlled by the
/// [processCameraProvider].
Future<void> _unbindUseCaseFromLifecycle(UseCase useCase) async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bparrishMines does the ecosystem team have a guidance for code that is not used but might be useful? Most (but not all) of my past teams encouraged removing code like this.

Future<void> clearAnalyzer() => _api.clearAnalyzerfromInstances(this);
}

/// Host API implementation of [ImageAnalysis].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the dart doc to describe when you expect callers to use a global and when you expect them to specify instanceManager?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bparrishMines can you verify my understanding? Added at line 64

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should only be one scenario when we pass an InstanceManager to the HostApi. The summary could be shortened:

An [instanceManager] is typically passed when a copy of an instance contained by an `InstanceManager` is being created.

@camsim99 camsim99 requested a review from reidbaker April 27, 2023 21:26

/// Unbinds [useCase] from camera lifecycle controlled by the
/// [processCameraProvider].
Future<void> _unbindUseCaseFromLifecycle(UseCase useCase) async {
assert(processCameraProvider != null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI you have another assert here followed by a use with !.

Copy link
Contributor Author

@camsim99 camsim99 Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to remove all asserts in this PR.

Edit: all assert statements in the plugin API implementation (android_camera_camerax.dart). Done in this commit

Copy link
Contributor

@reidbaker reidbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking this on my review anymore. Please file issues or have some other way to track any unresolved comments.

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2023
@auto-submit auto-submit bot merged commit 1a90429 into flutter:main Apr 28, 2023
sybrands-place pushed a commit to sybrands-place/packages that referenced this pull request May 1, 2023
* main: (46 commits)
  [go_router] Cleans up route match API and introduces dart fix (flutter#3819)
  [camerax] Add `LifecycleOwner` Proxy (flutter#3837)
  [file_selector] Add getDirectoryPaths implementation for Windows (flutter#3704)
  [various] Update Android example min SDKs (flutter#3847)
  Bump github/codeql-action from 2.2.12 to 2.3.2 (flutter#3838)
  [camerax] Implement Image Streaming (flutter#3454)
  [various] update agp and gradle for all examples in packages (flutter#3822)
  Update xcode to 14c18 (flutter#3774)
  [camera_android] Add NV21 as an image stream format flutter#3277 (flutter#3639)
  [go_router] Remove unused navigator keys (flutter#3708)
  [image_picker] Move I/O operations to a separate thread (flutter#3506)
  [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 1.8.20 to 1.8.21 in /packages/pigeon/platform_tests/test_plugin/android (flutter#3824)
  [pigeon] Reland: Add an initial example app (flutter#3832)
  Roll Flutter from c9004ff to 66fa4c5 (68 revisions) (flutter#3830)
  [various] Conditionalize the namespace in all Android plugins (flutter#3836)
  [in_app_pur]: Bump com.android.billingclient:billing from 5.1.0 to 5.2.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter#3672)
  [auick_action_ios] Retries multiple times to not fail ci when there is a flake (flutter#3823)
  Swap some iOS package CODEOWNERS (flutter#3793)
  [various] Add `targetCompatibility` to build.gradle (flutter#3825)
  [various] Set cmake_policy versions (flutter#3828)
  ...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 1, 2023
Roll Packages from 7e3f5da to de6131d (41 revisions)

flutter/packages@7e3f5da...de6131d

2023-05-01 reidbaker@google.com I122213 update non examples (flutter/packages#3846)
2023-04-29 47866232+chunhtai@users.noreply.github.com [go_router] Cleans up route match API and introduces dart fix (flutter/packages#3819)
2023-04-29 43054281+camsim99@users.noreply.github.com [camerax] Add `LifecycleOwner` Proxy (flutter/packages#3837)
2023-04-29 stuartmorgan@google.com [file_selector] Add getDirectoryPaths implementation for Windows (flutter/packages#3704)
2023-04-29 stuartmorgan@google.com [various] Update Android example min SDKs (flutter/packages#3847)
2023-04-28 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.2.12 to 2.3.2 (flutter/packages#3838)
2023-04-28 43054281+camsim99@users.noreply.github.com [camerax] Implement Image Streaming (flutter/packages#3454)
2023-04-28 reidbaker@google.com [various] update agp and gradle for all examples in packages (flutter/packages#3822)
2023-04-28 54558023+keyonghan@users.noreply.github.com Update xcode to 14c18 (flutter/packages#3774)
2023-04-28 andrewjohncoutts@gmail.com [camera_android] Add NV21 as an image stream format #3277 (flutter/packages#3639)
2023-04-28 gautier.bayzelon@gmail.com [go_router] Remove unused navigator keys (flutter/packages#3708)
2023-04-28 JeroenWeener@users.noreply.github.com [image_picker] Move I/O operations to a separate thread (flutter/packages#3506)
2023-04-28 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 1.8.20 to 1.8.21 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#3824)
2023-04-28 stuartmorgan@google.com [pigeon] Reland: Add an initial example app (flutter/packages#3832)
2023-04-28 engine-flutter-autoroll@skia.org Roll Flutter from c9004ff to 66fa4c5 (68 revisions) (flutter/packages#3830)
2023-04-28 stuartmorgan@google.com [various] Conditionalize the namespace in all Android plugins (flutter/packages#3836)
2023-04-27 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump com.android.billingclient:billing from 5.1.0 to 5.2.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#3672)
2023-04-27 ychris@google.com [auick_action_ios] Retries multiple times to not fail ci when there is a flake (flutter/packages#3823)
2023-04-26 magder@google.com Swap some iOS package CODEOWNERS (flutter/packages#3793)
2023-04-26 stuartmorgan@google.com [various] Add `targetCompatibility` to build.gradle (flutter/packages#3825)
2023-04-26 stuartmorgan@google.com [various] Set cmake_policy versions (flutter/packages#3828)
2023-04-26 stuartmorgan@google.com [path_provider] Allow `win32` up to version 4.x (flutter/packages#3820)
2023-04-26 stuartmorgan@google.com [tool] Move Android lint checks (flutter/packages#3816)
2023-04-25 evace93@gmail.com [google_maps_flutter_android] Fix Android lint warnings (flutter/packages#3751)
2023-04-25 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.annotation:annotation from 1.5.0 to 1.6.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#3381)
2023-04-25 jason-simmons@users.noreply.github.com Update test golden images for the latest Skia roll (flutter/packages#3787)
2023-04-25 stuartmorgan@google.com [various] Adds Android namespace (flutter/packages#3791)
2023-04-25 reidbaker@google.com [shared_preferences] Update gradle/agp in example apps (flutter/packages#3809)
2023-04-24 43640732+dancamdev@users.noreply.github.com [go_router] Adds name to TypedGoRoute (flutter/packages#3702)
2023-04-22 10687576+bparrishMines@users.noreply.github.com [webview_flutter] Adds support to receive permission requests (flutter/packages#3543)
2023-04-21 stuartmorgan@google.com [google_sign_in] Fix Android Java warnings (flutter/packages#3762)
2023-04-21 stuartmorgan@google.com [local_auth] Fix enum return on Android (flutter/packages#3780)
2023-04-21 stuartmorgan@google.com [pigeon] Warn when trying to use enums in collections (flutter/packages#3782)
2023-04-21 10687576+bparrishMines@users.noreply.github.com [webview_flutter_android] [webview_flutter_wkwebview] Platform implementations for supporting permission requests (flutter/packages#3792)
2023-04-21 scheglov@google.com [pigeon] Update for compatibility with a future change to the analyzer. (flutter/packages#3789)
2023-04-21 10687576+bparrishMines@users.noreply.github.com [camera_android] Fix Android lint warnings  (flutter/packages#3716)
2023-04-21 10687576+bparrishMines@users.noreply.github.com [webview_flutter_platform_interface] Adds method to receive permission requests (flutter/packages#3767)
2023-04-21 magder@google.com [image_picker_ios] In unit test write and read kCGImagePropertyExifUserComment property (flutter/packages#3783)
2023-04-21 widrans@gmail.com [go_router_builder] Fixed the return value of the generated push method (flutter/packages#3650)
2023-04-21 JeroenWeener@users.noreply.github.com [image_picker] Mention `launchMode: singleInstance` in README (flutter/packages#3759)
2023-04-21 stuartmorgan@google.com Revert "[pigeon] Add an initial example app" (flutter/packages#3785)

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
...
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
Implements image streaming with all required image data, see [here](flutter/flutter#113792).

Fixes flutter/flutter#120463.
Fixes flutter/flutter#113792 along the way.

Special shoutout to @reidbaker for pair programming on this!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[camerax] Implement image streaming [camera] Add ImageAnalysis use case to CameraX plugin
5 participants