-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[camera_web] Re: Support for camera stream on web #7950
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
base: main
Are you sure you want to change the base?
Conversation
…d before expecting mock response
…d before expecting mock response
…d before expecting mock response
…d before expecting mock response
…d before expecting mock response
…d before expecting mock response
…m, and used window.animationFrames
…le height and width in takeFrame
…seOffScreenCanvas
I'll re-review this tomorrow! |
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 @TecHaxter! This is getting close. I left some comments that should improve the performance of the frame capture (maybe hit 60fps!) and the accuracy of the image data reported.
Have you used this code in any demo app compiled for the web?
* Updates minimum supported SDK version to Flutter 3.22/Dart 3.4. | ||
- Supporting camera image stream on web. | ||
- Updates minimum supported SDK version to Flutter 3.22/Dart 3.4. |
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 probably an automatic update/fix, but please don't change all the early changelog entries of this file, stick to the *
for lists. This should simplify this file greatly.
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 fixed, can be marked as resolved?
@@ -106,6 +106,10 @@ void main() { | |||
).thenAnswer( | |||
(_) => Future<MediaStream>.value(canvasElement.captureStream())); | |||
|
|||
when( | |||
() => cameraService.hasPropertyOffScreenCanvas(), |
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.
Do we need to mock this? The code should be running on Chrome, which AFAIK has an offscreen canvas. What happens to this test if we revert this change?
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 cameraService
is a mock class object itself.
If we don't stub hasPropertyOffScreenCanvas
, it throws an error - "TypeError: null: type 'Null' is not a subtype of type 'bool'
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 we mark this resolved?
await completer.future; | ||
final CameraImageData cameraImageData = cameraService.takeFrame( | ||
videoElement, | ||
canUseOffscreenCanvas: 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.
We need a test for canUseOffscreenCanvas: 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 is fixed, can be marked as resolved?
when( | ||
() => cameraService.hasPropertyOffScreenCanvas(), | ||
).thenAnswer((_) => 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.
Same comment as above, we need a test where offscreen canvas is available, and one where it isn't, so we test both code paths.
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 fixed, can be marked as resolved?
)..videoElement = videoElement; | ||
|
||
when( | ||
() => cameraService.takeFrame( |
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.
We're already mocking the videoElement, why are we mocking the cameraService too?
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 cameraService
is a mock class object itself.
If we don't stub hasPropertyOffScreenCanvas
, it throws an error - "TypeError: null: type 'Null' is not a subtype of type 'bool'
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 we mark this resolved?
@@ -6,7 +6,6 @@ import 'dart:async'; | |||
import 'dart:js_interop'; | |||
import 'dart:ui'; | |||
import 'dart:ui_web' as ui_web; | |||
|
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.
Do not delete this empty line please :)
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 fixed, can be marked as resolved?
Completer<void> completer = Completer<void>(); | ||
completer.complete(); |
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.
Why do we need this? Remove?
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 fixed, can be marked as resolved?
return; | ||
} | ||
|
||
if (!completer.isCompleted) { |
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 always false, isn't it? The completer
is being .complete()
d unconditionally in line 677?
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 have changed the implementation of _triggerAnimationFramesLoop
in upcoming commits.
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 fixed, can be marked as resolved?
if (!_cameraFrameStreamController.hasListener) { | ||
return; | ||
} |
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.
Do not control the stop of the animation like this. What you need to do is to keep the return value of window.requestAnimationFrame
(which is an int
), and then when the stream controller ceases to have any listeners (see StreamController.onCancel), call window.cancelAnimationFrame
with the last ID returned by rAF.
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 fixed, can be marked as resolved?
format: const CameraImageFormat( | ||
ImageFormatGroup.jpeg, | ||
raw: '', | ||
), | ||
planes: <CameraImagePlane>[ | ||
CameraImagePlane( | ||
bytes: byteBuffer.asUint8List(), | ||
bytesPerRow: 0, | ||
), | ||
], |
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.
Is this CameraImageData
correctly constructed?
First it says that this is a jpeg
(which it isn't), and then puts it in the first plane, but we say it's "0 bytesPerRow".
My suggestions are:
- Make the format
ImageFormatGroup.unknown
(the image coming from the canvas is actually RGBA with 4 bytes per-pixel, arranged in rows, but that's not an expected ImageFormatGroup for the plugin). (Maybe called RGBA32 elsewhere?) - set a
raw
value that doesn't clash with anything possible with Android or iOS. "unknown" seems to be "0" on android, so we could use that too. - The
bytesPerRow
of the browser'sgetImageData
seems to bewidth * 4
.
More docs here.
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 found FLEX_RGBA_8888
in Android and kCVPixelFormatType_32RGBA
in iOS which looks promising for compatibility with RGBA 4 bytes per-pixel arranged image coming from ImageData
of Canvas (Although I haven't tested it yet)
Also found a comment in \camera\camera_android_camerax\lib\src\android_camera_camerax.dart
/// [imageFormatGroup] is used to specify the image format used for image
/// streaming, but CameraX currently only supports YUV_420_888 (supported by
/// Flutter) and RGBA (not supported by Flutter). CameraX uses YUV_420_888
/// by default, so [imageFormatGroup] is not used.
Seeing this, I went with the values you suggested
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 have tested this in the example app of google_mediapipe_face_detection (code not pushed yet). It works like charm ✨✨
Can we mark this resolved?
…into camera_web_stream
…Group, 0 as raw value and fixed the bytes per row value
…nFrame for cancelAnimationFrame
…p and improved readability
…enCanvasRenderingContext2D
…alculation on high resolution timestamp
…m takeFrame function of camera service, using the function directly
…or instead of intialize()
…t OffscreenCanvas in camera service
…t OffscreenCanvas in camera test
5721618
to
d7b7d5c
Compare
Hi @ditman The PR is ready for another review, Thanks :) |
From triage: Ping @ditman on this re-review. |
Update from triage: we're working through trying to find a web reviewer with availability to look at this. Apologies for the delay. |
@yjbanov can you suggest a reviewer for this change? It has been blocked on a web review. |
This PR aims to provide support for strartImageStream and stopImageStream on Web.
#92460
Based on #6944 from the archived plugins repository
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].///
).POV: my first PR on a public repo
Contains required commits from the PR #6443, resolves unnecessary 202 commits