[camera_web] Recording Video #4210

ABausG · 2021-07-31T21:31:24Z

Adds ability to record webm of the camera platform interface.

Implements calls startVideoRecording, pauseVideoRecording, resumeVideoRecording and stopVideoRecording

Part of flutter/flutter#45297.

Pre-launch Checklist

ABausG · 2021-07-31T21:32:47Z

@bselwe @felangel @ditman I saw that you did some work on camera_web recently. Could you take a look at this?

ditman

Thanks @ABausG for the contribution, this is great!

I've left some questions about the current approach (mostly about cross-browser compat).

BTW, @bselwe is currently OOTO (until the end of the week), but he was working in normalizing the exceptions thrown from the code.

In this PR, for example, we're manually throwing DomExceptions from the low-level Camera object, but I think @bselwe was working on getting some standardization in what types of exceptions to throw from where. The guiding lines here would be the core camera plugin, seeing what types of Exceptions are expected there, so everything works transparently.

I'll defer to @bselwe for more comments in that regard!

Other than that, this is looking amazing! Thanks again for the contribution!

packages/camera/camera_web/lib/src/camera.dart

ditman · 2021-08-03T21:40:53Z

packages/camera/camera_web/lib/src/camera.dart

+  /// /// Throws a [html.DomException.INVALID_STATE] if there already is an active Recording
+  Future<void> startVideoRecording({Duration? maxVideoDuration}) async {
+    if (_mediaRecorder != null && _mediaRecorder!.state != 'inactive') {
+      throw html.DomException.INVALID_STATE;


(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.

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 MethodChannelCamera invoke methods on the method channel - this may throw a PlatformException. For consistency, any error that is related to an invalid state/execution of the camera during video recording should throw a PlatformException as well.

We should rather avoid throwing a CameraException in internal classes (not exposed to the end user). If we throw platform exceptions, they are usually caught and mapped to camera exceptions in the CameraController.

Throwing a PlatformException now and not checking fo the state of the mediaRecorder`

ditman · 2021-08-03T21:46:01Z

packages/camera/camera_web/lib/src/camera.dart

+    if (maxVideoDuration != null) {
+      _mediaRecorder!.addEventListener('dataavailable', (event) {
+        final blob = (event as html.BlobEvent).data;
+        final file = XFile(html.Url.createObjectUrl(blob));


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.

Added mimeType and Filename. For now using the hashcode of the blob as filename

ditman · 2021-08-03T21:55:35Z

packages/camera/camera_web/lib/src/camera.dart

+  /// Pauses the current video Recording
+  /// Throws a [html.DomException.INVALID_STATE] if there is no active Recording
+  Future<void> pauseVideoRecording() async {
+    if (_mediaRecorder == null || _mediaRecorder!.state == 'inactive') {


According to the docs, the _mediaRecorder state check is performed by the browser?

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.

See above. Now throwing PlatformExceptions

ditman · 2021-08-03T22:01:53Z

packages/camera/camera_web/lib/src/camera.dart

+    final availableData = Completer<XFile>();
+    _mediaRecorder!.addEventListener('dataavailable', (event) {
+      final blob = (event as html.BlobEvent).data;
+      availableData.complete(XFile(html.Url.createObjectUrl(blob)));
+    });
+    _mediaRecorder?.stop();


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?

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 XFile in stopVideoRecording

remove the listener

stop the MediaRecorder

This stop is necessary to only get data once if a maxVideoDuration is provided as MediaRecorder only takes in splices instead of a maxDuration

ditman · 2021-08-03T22:02:59Z

packages/camera/camera_web/lib/src/camera.dart

+            .add(VideoRecordedEvent(this.textureId, file, maxVideoDuration));
+        _mediaRecorder!.stop();
+      });
+      _mediaRecorder!.start(maxVideoDuration.inMilliseconds);


What happens if maxVideoDuration is 0 milliseconds? Should that be disallowed?

It could throw a DomException with NotSupportedError that would be caught in the camera platform here.

Throwing a PlatformException

ditman · 2021-08-03T22:04:08Z

packages/camera/camera_web/lib/src/camera.dart

+    if (_mediaRecorder == null || _mediaRecorder!.state == 'inactive') {
+      throw html.DomException.INVALID_STATE;
+    }


(These exceptions are already thrown by the browser, no need to assert for _mediaRecorder.state)

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.

bselwe

Awesome, thanks for the contribution @ABausG and your comments @ditman! 🙌

I've added some clarification regarding which exceptions to throw and when. The general rule would be to try to match the exceptions thrown in MethodChannelCamera (an implementation of the camera platform used for iOS and Android platforms) and handled in CameraController. Let me know if there's anything unclear about this approach.

I think it would also be nice to include Camera tests. That could include:

For startVideoRecording:
- Throws an exception when the media recorder is in an invalid state.
- Initializes a media recorder instance from the video element stream.
- Uses an appropriate mime type for the video recording based on MediaRecorder.isTypeSupported().
- Uses maxVideoDuration to record a video.
- Emits a VideoRecordedEvent when the video recording has been stopped.
For pauseVideoRecording:
- Throws an exception when the media recorder is null.
- Calls pause on the media recorder.
And similar tests for other methods.

Thanks again!

bselwe · 2021-08-05T07:30:14Z

packages/camera/camera_web/lib/src/camera.dart

+  final StreamController<VideoRecordedEvent> _videoRecorderController =
+      StreamController();
+
+  /// Returns a Stream that emits when a video Recodring with a defined maxVideoDuration was created


Suggested change

/// 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.

bselwe · 2021-08-05T08:21:06Z

packages/camera/camera_web/lib/src/camera.dart

+  /// /// Throws a [html.DomException.INVALID_STATE] if there already is an active Recording
+  Future<void> startVideoRecording({Duration? maxVideoDuration}) async {
+    if (_mediaRecorder != null && _mediaRecorder!.state != 'inactive') {
+      throw html.DomException.INVALID_STATE;


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 MethodChannelCamera invoke methods on the method channel - this may throw a PlatformException. For consistency, any error that is related to an invalid state/execution of the camera during video recording should throw a PlatformException as well.

We should rather avoid throwing a CameraException in internal classes (not exposed to the end user). If we throw platform exceptions, they are usually caught and mapped to camera exceptions in the CameraController.

bselwe · 2021-08-05T08:27:25Z

packages/camera/camera_web/lib/src/camera.dart

+    if (_mediaRecorder == null || _mediaRecorder!.state == 'inactive') {
+      throw html.DomException.INVALID_STATE;
+    }


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.

bselwe · 2021-08-05T08:30:34Z

packages/camera/camera_web/lib/src/camera.dart

+  /// Pauses the current video Recording
+  /// Throws a [html.DomException.INVALID_STATE] if there is no active Recording
+  Future<void> pauseVideoRecording() async {
+    if (_mediaRecorder == null || _mediaRecorder!.state == 'inactive') {


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.

packages/camera/camera_web/lib/src/camera.dart

bselwe · 2021-08-05T08:38:38Z

packages/camera/camera_web/lib/src/camera.dart

+            .add(VideoRecordedEvent(this.textureId, file, maxVideoDuration));
+        _mediaRecorder!.stop();
+      });
+      _mediaRecorder!.start(maxVideoDuration.inMilliseconds);


It could throw a DomException with NotSupportedError that would be caught in the camera platform here.

bselwe · 2021-08-05T08:40:46Z

packages/camera/camera_web/lib/src/camera.dart

+        videoElement.captureStream(), {'mimeType': 'video/webm'});
+
+    if (maxVideoDuration != null) {
+      _mediaRecorder!.addEventListener('dataavailable', (event) {


Would it make sense to remove the event listener (removeEventListener) when the video recording is stopped?

Yes. Removing it now

bselwe · 2021-08-05T08:59:42Z

packages/camera/camera_web/lib/src/camera.dart

+      StreamController();
+
+  /// Returns a Stream that emits when a video Recodring with a defined maxVideoDuration was created
+  Stream<VideoRecordedEvent> get onVideoRecordedEventStream =>


WDYT about renaming it to onVideoRecorded for consistency with other stream names in the camera plugin (onCameraInitialized, onCameraResolutionChanged, etc.)?

I renamed it to onVideoRecordedEvent to match the naming of the getter in the camera_plugin_interface

bselwe

Should we maybe remove throwing an unimplemented error in prepareForVideoRecording?

Match naming from camera_plugin_interface

ABausG · 2021-08-05T14:49:02Z

Should we maybe remove throwing an unimplemented error in [prepareForVideoRecording]

I was thinking about that method as well. I'm seeing two options:

Make it no-op
Perform initialisation in the actual camera implementation like setting up the media recorder.

Some issues with removing listener and stopping the mediaRecorder

packages/camera/camera_web/example/integration_test/camera_test.dart

MediaRecorder.start with a Timeslice does not seem to fire in the test case

ABausG · 2021-08-07T12:04:08Z

packages/camera/camera_web/example/integration_test/camera_test.dart

+      testWidgets(
+          'starts a video recording with a given maxDuration '
+          'emits a VideoRecordedEvent', (tester) async {
+        // TODO: MediaRecorder with a Timeslice does not seem to call the dataavailable Listener in a Test


I could not get this test to work. As it does not seem to trigger the datavailable event of the media recorder.
I was able to manually test the behaviour with setting a max duration (I tested with 4 seconds) however this test seems to timeout waiting for the first element of the event stream.
Could you take a look at what's might be going on?
Also is there a (nice) way to debug the integration tests?

I will have a look at this test.

There is a way to debug integration tests in the browser. You'll need to execute this in camera_web/example:

flutter run -d web-server --target integration_test/camera_test.dart --debug

It will produce a link that you should open in the browser, open inspection tools (CMD+Option+J in Chrome), find a dart file you want to debug in the "Source" tab and mark a breakpoint. Refresh the web page and it should stop at the breakpoint.

bselwe · 2021-08-09T13:09:44Z

packages/camera/camera_web/example/integration_test/camera_test.dart

+
+        await camera.startVideoRecording();
+
+        expect('recording', camera.mediaRecorder!.state);


Suggested change

expect('recording', camera.mediaRecorder!.state);

expect(camera.mediaRecorder!.state, equals('recording`));

bselwe · 2021-08-09T13:18:03Z

packages/camera/camera_web/example/integration_test/camera_test.dart

+            throwsA(predicate<PlatformException>(
+                (ex) => ex.code == CameraErrorCode.notSupported.toString())));


nit

Suggested change

throwsA(predicate<PlatformException>(

(ex) => ex.code == CameraErrorCode.notSupported.toString())));

throwsA(predicate<PlatformException>(

(ex) => ex.code == CameraErrorCode.notSupported.toString(),),),);

bselwe · 2021-08-09T13:18:18Z

packages/camera/camera_web/example/integration_test/camera_test.dart

+
+        await camera.pauseVideoRecording();
+
+        expect('paused', camera.mediaRecorder!.state);


Suggested change

expect('paused', camera.mediaRecorder!.state);

expect(camera.mediaRecorder!.state, equals('paused'));

bselwe · 2021-08-09T13:18:37Z

packages/camera/camera_web/example/integration_test/camera_test.dart

+
+        await camera.resumeVideoRecording();
+
+        expect('recording', camera.mediaRecorder!.state);


Suggested change

expect('recording', camera.mediaRecorder!.state);

expect(camera.mediaRecorder!.state, equals('recording'));

ditman · 2021-08-31T16:41:27Z

Updating _videoRecorderController to a broadcast stream controller fixes this issue.

Ahhh, I didn't notice that, very good catch there @bselwe!

ditman

After reading the PR again, I don't understand very well how these features are currently implemented:

How does this respect the max length of a video recording passed by a user? I don't think we ever check or use the value of max length (only null checks).
How does this combine several ondataavailable events with video "chunks" to produce one single, large video file that can be used later? It seems this code emits the File as soon as the first ondataavailable event fires.
- See the how they use ondataavailable and stop events here (it's for audio, but the API is the same).
- Is this even supported by XFile, do we need to change anything there? Do we need to be able to create a XFile from an html.File, or a bunch of Blobs or something other than "bytes"?
@bselwe noticed that video on Chrome/Edge is missing some metadata, and the duration is listed as Infinity; have you noticed this? It seems to be this bug. SO1, SO2. (Is this something that can be fixed when putting together the XFile from all the "chunks"? Does the mp4 codec have this problem?)

For now, I'm going to publish the camera_web plugin with what we currently have, and when this PR matures a little bit more, we can release it in a new version. This is a great start, but I'm a little concerned about the comments above :S

ditman · 2021-09-03T19:41:49Z

How does this respect the max length of a video recording passed by a user? I don't think we ever check or use the value of max length (only null checks).

We discussed this, and it seems that we're leveraging the timeslice feature (that I had misunderstood), and we only use one blob of video, until we call stop, or the timeslice is done.

How does this combine several ondataavailable events with video "chunks" to produce one single, large video file

This is already achieved by the behavior of the ondataavailable event, and @bselwe just made it so that we use the stop event to collect the recorded video, closer to the example.

google-cla · 2021-09-16T16:45:58Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

ditman · 2021-09-16T16:58:56Z

@googlebot I consent.

ditman

LGTM! All tests pass locally, let's go with this one!

…ns#4210)

* feat: Add Support for Video Recording in Camera Web * docs: add video recording documentation Co-authored-by: Bartosz Selwesiuk <[email protected]>

ABausG added 5 commits July 31, 2021 22:35

Add Support for Video Recording in Camera Web

0c6fac4

Add Documentation

65b2718

Adding Tests

4bd1173

Adding Error Handling

ce42759

Formatting

fdbe131

ABausG requested a review from bparrishMines as a code owner July 31, 2021 21:31

google-cla bot added the cla: yes label Jul 31, 2021

github-actions bot added p: camera platform-web labels Jul 31, 2021

ABausG changed the title ~~[camera_web] Recording~~ [camera_web] Recording Video Jul 31, 2021

Add missing call to get RecordedEvent Stream

4b1ae4c

ditman reviewed Aug 3, 2021

View reviewed changes

bselwe reviewed Aug 5, 2021

View reviewed changes

Rename onVideoRecordedEventStream

2e6dfec

Match naming from camera_plugin_interface

bselwe mentioned this pull request Aug 5, 2021

Add support for video recording VGVentures/photobooth#19

Open

Unify logic from startVideoRecording and stopVideoRecording

f252ffc

ABausG added 3 commits August 5, 2021 16:53

throw PlatformExceptions

d8114ab

Use srcObject

39adc96

Add Camera Tests

4db7748

Some issues with removing listener and stopping the mediaRecorder

ditman reviewed Aug 7, 2021

View reviewed changes

packages/camera/camera_web/example/integration_test/camera_test.dart Outdated Show resolved Hide resolved

ABausG added 2 commits August 7, 2021 14:00

Fix Test and Stopping Video Recording

287bd4a

Clarify maxVideoDuration Test Issue

8f0f2fd

MediaRecorder.start with a Timeslice does not seem to fire in the test case

ABausG commented Aug 7, 2021

View reviewed changes

bselwe reviewed Aug 9, 2021

View reviewed changes

Merge branch 'upstream/master' into camera_web_recording

1d3906d

ditman suggested changes Sep 2, 2021

View reviewed changes

bselwe added 2 commits September 3, 2021 19:50

feat: combine ondataavailable video blobs into a single video blob

3c74e9e

test: update video recording tests for multiple video blobs

f5863e9

bselwe added 7 commits September 13, 2021 14:41

docs: add video recording documentation

4552b01

feat: add Camera onVideoRecordingError stream

cd7f80d

test: add Camera onVideoRecordingError stream tests

01b3210

feat: emit a CameraErrorEvent on video recording error

c03fea6

test: emit a CameraErrorEvent on video recording error tests

194db74

Merge branch 'upstream/master' into camera_web_recording

24ab126

feat: make prepareForVideoRecording a no-op

787ee6b

ditman removed the request for review from bparrishMines September 14, 2021 16:25

ditman mentioned this pull request Sep 16, 2021

[camera_web] Release the camera stream used to request video and audio permissions #4342

Merged

11 tasks

ditman added 2 commits September 16, 2021 09:43

Merge branch 'master' into camera_web_recording

bc9c72a

Update CHANGELOG and pubspec

114cc46

google-cla bot added cla: no and removed cla: yes labels Sep 16, 2021

google-cla bot added cla: yes and removed cla: no labels Sep 16, 2021

ditman self-requested a review September 16, 2021 17:15

ditman approved these changes Sep 16, 2021

View reviewed changes

ditman merged commit aa69736 into flutter:master Sep 16, 2021

engine-flutter-autoroll mentioned this pull request Sep 18, 2021

Roll Plugins from 5f1c83e3933e to 1fe19f1d4946 (22 revisions) flutter/flutter#90301

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 18, 2021

aa69736 [camera_web] Add video recording functionality (flutter/plugi…

d589f8c

…ns#4210)

	/// 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.


		await camera.startVideoRecording();

		expect('recording', camera.mediaRecorder!.state);

	expect('recording', camera.mediaRecorder!.state);
	expect(camera.mediaRecorder!.state, equals('recording`));

		throwsA(predicate<PlatformException>(
		(ex) => ex.code == CameraErrorCode.notSupported.toString())));


		await camera.pauseVideoRecording();

		expect('paused', camera.mediaRecorder!.state);


		await camera.resumeVideoRecording();

		expect('recording', camera.mediaRecorder!.state);

[camera_web] Recording Video #4210

[camera_web] Recording Video #4210

Conversation

ABausG commented Jul 31, 2021

Pre-launch Checklist

ABausG commented Jul 31, 2021

ditman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bselwe Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bselwe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bselwe Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bselwe left a comment • edited Loading

Choose a reason for hiding this comment

ABausG commented Aug 5, 2021

Choose a reason for hiding this comment

bselwe Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman commented Aug 31, 2021 • edited Loading

ditman left a comment • edited Loading

Choose a reason for hiding this comment

ditman commented Sep 3, 2021

google-cla bot commented Sep 16, 2021

ditman commented Sep 16, 2021

ditman left a comment

Choose a reason for hiding this comment

bselwe Aug 5, 2021 •

edited

Loading

bselwe Aug 5, 2021 •

edited

Loading

bselwe left a comment •

edited

Loading

bselwe Aug 9, 2021 •

edited

Loading

ditman commented Aug 31, 2021 •

edited

Loading

ditman left a comment •

edited

Loading