-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
…o/flutter/plugins/camerax/ImageCaptureHostApiImpl.java Co-authored-by: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com>
Co-authored-by: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com>
...ndroid_camerax/android/src/main/java/io/flutter/plugins/camerax/GeneratedCameraXLibrary.java
Show resolved
Hide resolved
/** Constructor is private to enforce null safety; use Builder. */ | ||
private ResolutionInfo() {} | ||
/** Constructor is non-public to enforce null safety; use Builder. */ | ||
ResolutionInfo() {} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generated.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
...ndroid_camerax/android/src/main/java/io/flutter/plugins/camerax/GeneratedCameraXLibrary.java
Show resolved
Hide resolved
...ndroid_camerax/android/src/main/java/io/flutter/plugins/camerax/GeneratedCameraXLibrary.java
Show resolved
Hide resolved
...ra/camera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/AnalyzerTest.java
Outdated
Show resolved
Hide resolved
.../android/app/src/androidTest/java/io/flutter/plugins/cameraxexample/InstanceManagerTest.java
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Show resolved
Hide resolved
late final _PlaneProxyHostApiImpl _api; | ||
|
||
/// Returns the pixels buffer containing frame data. | ||
Future<Uint8List> getBuffer() { |
There was a problem hiding this comment.
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.
packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart
Show resolved
Hide resolved
There was a problem hiding this 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
.../android/app/src/androidTest/java/io/flutter/plugins/cameraxexample/InstanceManagerTest.java
Outdated
Show resolved
Hide resolved
@@ -252,7 +266,8 @@ class AndroidCameraCameraX extends CameraPlatform { | |||
/// [cameraId] not used. | |||
@override | |||
Future<void> pausePreview(int cameraId) async { | |||
_unbindPreviewFromLifecycle(); | |||
assert(preview != null); |
There was a problem hiding this comment.
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.
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
|
||
/// Unbinds [useCase] from camera lifecycle controlled by the | ||
/// [processCameraProvider]. | ||
Future<void> _unbindUseCaseFromLifecycle(UseCase useCase) async { |
There was a problem hiding this comment.
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]. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
||
/// Unbinds [useCase] from camera lifecycle controlled by the | ||
/// [processCameraProvider]. | ||
Future<void> _unbindUseCaseFromLifecycle(UseCase useCase) async { | ||
assert(processCameraProvider != null); |
There was a problem hiding this comment.
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 !.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
* 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) ...
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 ...
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!
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
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).