-
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_windows] Restore image streaming support #8234
base: main
Are you sure you want to change the base?
Conversation
…tform (flutter#7951)" This reverts commit b3a5e33.
The logic is synchronous, so there is no need to have the `@async` annotation here. Addresses: flutter#7067 (comment)
When image stream frames are being received from Media Foundation, it appears to use a separate thread when calling the `OnSample` callback, which results in this error from: The 'plugins.flutter.io/camera_windows/imageStream' channel sent a message from native to Flutter on a non-platform thread. Platform channel messages must be sent on the platform thread. Failure to do so may result in data loss or crashes, and must be fixed in the plugin or application code creating that channel. In order to ensure that we are only ever using the `EventChannel` from a platform thread, create a `TaskRunner` that is backed by a hidden window to submit the frames to the image stream. Based on the suggestion in this comment: flutter/flutter#134346 (comment)
Instead of a global `EventSink` that is re-used for each stream, create a dedicated `EventChannel` for each capture stream. The channel gets an unique name like `plugins.flutter.io/camera_android/imageStream/<cameraId>` for each camera instance. Addresses: flutter#7067 (comment)
We don’t really need to keep track of the subscription and controller in instance variables.
Given that subscribing to the `EventChannel` tells the capture controller to start the image stream and cancelling the subscription tells it to stop, we don’t actually need a separate method in `CameraPlatform` for stopping the stream.
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!
I haven't reviewed the entire PR yet, but one high-level note about the overall PR structure, carried over from my comment on the original PR.
/// 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 Android, iOS and | ||
/// Windows (other platforms won't be supported in current setup). |
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.
See #7220 (comment). This PR should include adding an API to query support, rather than modifying the legacy approach of hard-coding platforms.
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.
As far as I can tell, Windows is the sole outlier without image streaming support. Perhaps we could just eliminate the check altogether?
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.
camera_web
does not support streaming. There may also be third-party, unendorsed implementation that don't support streaming.
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’ve been looking into this, but it’s not quite clear how to deal with the versioning. If I add a supportsImageStreaming
method to camera_platform_interface
and bump the version to 2.9.0, then the platform packages won’t be able to depend on that, since they resolve the dependency from pub.dev.
So, as I understand the situation, we should:
- first create a PR with just that method in the platform interface,
- release platform interface 2.9.0 and then
- implement that method in the platform packages by updating their camera_platform_interface dependency to
^2.9.0
Is this right at all, or am I missing something?
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.
Opened #8250 to get started.
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.
Restores support for streaming images from the camera(s) in Windows.
I reverted #7951 as a starting point and attempted to resolve the issues that were raised:
startImageStream
andstopImageStream
methods are no longerasync
.EventSink
for the images. Each camera gets a dedicatedEventChannel
for the image data.In addition, the images are submitted to the
EventChannel
s from a dedicated hidden window -based task runner, based on flutter/engine#24232.Implemented with assistance from @jokerttu.
Fixes flutter/flutter#97542.
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.