-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[camera_web] Recording Video #4210
Changes from 6 commits
0c6fac4
65b2718
4bd1173
ce42759
fdbe131
4b1ae4c
2e6dfec
f252ffc
d8114ab
39adc96
4db7748
287bd4a
8f0f2fd
0e1d690
d38978f
ce5c1ca
a517757
2d1022f
01a6999
4df6796
1d3906d
3c74e9e
f5863e9
4552b01
cd7f80d
01b3210
c03fea6
194db74
24ab126
787ee6b
bc9c72a
114cc46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |||||
| // Use of this source code is governed by a BSD-style license that can be | ||||||
| // found in the LICENSE file. | ||||||
|
|
||||||
| import 'dart:async'; | ||||||
| import 'dart:html' as html; | ||||||
| import 'dart:ui'; | ||||||
|
|
||||||
|
|
@@ -155,6 +156,8 @@ class Camera { | |||||
| /// Stop the camera stream. | ||||||
| stop(); | ||||||
|
|
||||||
| _videoRecorderController.close(); | ||||||
|
|
||||||
| /// Reset the [videoElement] to its initial state. | ||||||
| videoElement | ||||||
| ..srcObject = null | ||||||
|
|
@@ -171,4 +174,69 @@ class Camera { | |||||
| ..objectFit = 'cover' | ||||||
| ..transform = 'scaleX(-1)'; | ||||||
| } | ||||||
|
|
||||||
| html.MediaRecorder? _mediaRecorder; | ||||||
| final StreamController<VideoRecordedEvent> _videoRecorderController = | ||||||
| StreamController(); | ||||||
|
|
||||||
| /// Returns a Stream that emits when a video Recodring with a defined maxVideoDuration was created | ||||||
|
||||||
| /// Returns a Stream that emits when a video Recodring with a defined maxVideoDuration was created | |
| /// Returns a Stream that emits when a video recording with a defined maxVideoDuration was created. |
Outdated
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.
WDYT about renaming it to onVideoRecorded for consistency with other stream names in the camera plugin (onCameraInitialized, onCameraResolutionChanged, etc.)?
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 renamed it to onVideoRecordedEvent to match the naming of the getter in the camera_plugin_interface
Outdated
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 my general comment about the exception style within the plugin. @bselwe was overhauling this, and I don't think we'd want to throw a raw DomException, even from the low-level Camera object.
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.
Yeah, AFAIK, it is not possible to create an instance of html.DomException (the class has only a private factory). It also seems that html.DomException.INVALID_STATE is a constant string rather than an exception so we would not be able to provide both code and description for the error.
I think the general rule would be to follow MethodChannelCamera (an implementation of the camera platform used for iOS and Android platforms) and CameraController:
- Video recording methods in
MethodChannelCamerainvoke methods on the method channel - this may throw aPlatformException. For consistency, any error that is related to an invalid state/execution of the camera during video recording should throw aPlatformExceptionas well. - We should rather avoid throwing a
CameraExceptionin internal classes (not exposed to the end user). If we throw platform exceptions, they are usually caught and mapped to camera exceptions in theCameraController.
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.
Throwing a PlatformException now and not checking fo the state of the mediaRecorder`
ditman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
Would it make sense to remove the event listener (removeEventListener) when the video recording is stopped?
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.
Yes. Removing it now
Outdated
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 file needs some extra data, like name and mimeType to be initialized at this point. This will help later when the user wants to save it to disk.
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 mimeType and Filename. For now using the hashcode of the blob as filename
Outdated
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.
What happens if maxVideoDuration is 0 milliseconds? Should that be disallowed?
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.
It could throw a DomException with NotSupportedError that would be caught in the camera platform 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.
Throwing a PlatformException
Outdated
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.
According to the docs, the _mediaRecorder state check is performed by the browser?
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.
It would be probably enough to throw a PlatformException with an appropriate code and description if _mediaRecorder is null. Any exception thrown by MediaRecorder.pause would then be caught in the camera platform 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.
See above. Now throwing PlatformExceptions
Outdated
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.
(These exceptions are already thrown by the browser, no need to assert for _mediaRecorder.state)
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.
True, same as above. It would be probably enough to throw a PlatformException with an appropriate code and description if _mediaRecorder is null. Any exception thrown by MediaRecorder.resume would then be caught in the camera platform 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.
See above
Outdated
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 seems to be some code duplication here vs startVideoRecording (with a max length).
Could you unify these two codepaths so the implementation of thedataavailable event on startVideoRecording uses the same logic as "stop"?
I also wonder why the start with max length emits a VideoRecordedEvent, but stopVideoRecording doesn't?
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.
Unified it by registering the listener on dataavailable only in the startVideoRecording.
If the listener is triggered it will do the following things:
- emit a
VideoRecordedEvent - Complete the Completer used to obtain the
XFileinstopVideoRecording - remove the listener
- stop the MediaRecorder
- This stop is necessary to only get data once if a
maxVideoDurationis provided asMediaRecorderonly takes in splices instead of a maxDuration
- This stop is necessary to only get data once if a
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.
nit