-
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
[camera] Add API support query for image streaming #8250
base: main
Are you sure you want to change the base?
Conversation
3d16044
to
4d88109
Compare
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.
Thanks for the contribution!
@@ -173,6 +173,11 @@ abstract class CameraPlatform extends PlatformInterface { | |||
throw UnimplementedError('resumeVideoRecording() is not implemented.'); | |||
} | |||
|
|||
/// Check whether this platform supports image streaming via [onStreamedFrameAvailable]. | |||
bool supportsImageStreaming() { | |||
throw UnimplementedError('supportsImageStreaming() is not implemented.'); |
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.
The default should be false, not to throw, so that existing implementations that don't support it will automatically get the right behavior.
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.
Changed.
@@ -661,6 +661,9 @@ class CameraPlugin extends CameraPlatform { | |||
); | |||
} | |||
|
|||
@override | |||
bool supportsImageStreaming() => false; |
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.
The web and Windows package changes won't be necessary once the base implementation is changed.
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.
Removed those changes.
/// The `startImageStream` method is only available on Android and iOS (other | ||
/// platforms won't be supported in current setup). | ||
/// The `startImageStream` method is only available on platforms that | ||
/// report support for image streaming via [CameraPlatform.supportsImageStreaming]. |
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 needs to be wrapped in a method at this layer; clients are not expected to use the platform interface directly.
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.
Added a method.
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.
Android changes LGTM!
4d88109
to
b87babe
Compare
Android build is failing in Firebase Test Lab, but I don’t seem to have permission to view the results. |
The
|
2927697
to
ead8c41
Compare
I am unable to reproduce the failure locally with an emulator or with Firebase Test Lab. I tried the integration tests with Firebase Test Lab both in $ pushd android
$ ./gradlew app:assembleAndroidTest
$ ./gradlew app:assembleDebug -Ptarget=integration_test/camera_test.dart
$ popd
$ gcloud firebase test android run --type instrumentation --app build/app/outputs/apk/debug/app-debug.apk --test build/app/outputs/apk/androidTest/debug/app-debug-androidTest.apk --timeout 7m --device model=panther,version=33 But I get successful results: camera_android
camera
and similar results with the emulator, except for this:
in the “Capture specific image resolutions” test. Any ideas as to what’s going on? |
Re-running the check seems to have solved the problem 🎉 |
ead8c41
to
247846d
Compare
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.
The API changes look good to me; this is ready to split out the first sub-PR for landing.
I left notes about missing tests, but those can be addressed in the sub PRs.
@@ -173,6 +173,9 @@ abstract class CameraPlatform extends PlatformInterface { | |||
throw UnimplementedError('resumeVideoRecording() is not implemented.'); | |||
} | |||
|
|||
/// Check whether this platform supports image streaming via [onStreamedFrameAvailable]. | |||
bool supportsImageStreaming() => false; |
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 need a Dart unit test.
@@ -237,6 +237,9 @@ class AVFoundationCamera extends CameraPlatform { | |||
await _hostApi.resumeVideoRecording(); | |||
} | |||
|
|||
@override | |||
bool supportsImageStreaming() => true; |
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 need a Dart unit test.
@@ -1152,6 +1152,9 @@ class AndroidCameraCameraX extends CameraPlatform { | |||
} | |||
} | |||
|
|||
@override | |||
bool supportsImageStreaming() => true; |
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 need a Dart unit test.
@@ -228,6 +228,9 @@ class AndroidCamera extends CameraPlatform { | |||
Future<void> resumeVideoRecording(int cameraId) => | |||
_hostApi.resumeVideoRecording(); | |||
|
|||
@override | |||
bool supportsImageStreaming() => true; |
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 need a Dart unit test.
Add API support query,
supportsImageStreaming
for checking if the camera platform supports image streaming.As requested on this comment: #8234 (comment)
Attempting to follow the contribution guide wrt. changing federated plugins.
There is no issue to link to, but should I create one?
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, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.