[camera] Limit video length with maxVideoDuration on startVideoRecording #3403

danielroek · 2021-01-11T09:08:32Z

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

Adds implementation for defining a maximum video duration.

List which issues are fixed by this PR. You must list at least one issue.

TODO

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

TODO

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Co-authored-by: Maurits van Beusekom <[email protected]>

# Conflicts: # packages/camera/camera/pubspec.yaml # packages/camera/camera_platform_interface/CHANGELOG.md # packages/camera/camera_platform_interface/lib/src/events/camera_event.dart # packages/camera/camera_platform_interface/lib/src/platform_interface/camera_platform.dart # packages/camera/camera_platform_interface/pubspec.yaml

packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/Camera.java

packages/camera/camera/example/lib/main.dart

packages/camera/camera/lib/src/camera_controller.dart

danielroek · 2021-01-11T11:15:25Z

packages/camera/camera/pubspec.yaml

+  camera_platform_interface: #^1.5.0
+    path: ../camera_platform_interface


Remove path

BeMacized

Overall this already looks pretty good! 😄
I've left a few comments below.

Additionally:

Unit tests are still missing for your changes.
iOS implementations are still missing.

packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/Camera.java

BeMacized · 2021-01-11T13:52:26Z

packages/camera/camera/lib/src/camera_controller.dart

  /// Throws a [CameraException] if the capture fails.
-  Future<void> startVideoRecording() async {
+  ///
+  /// TODO: Documentation: when maxVideoDuration listen to Stream with CameraTimeLimitReachedEvent


Remaining TODO (PR is in draft state, but marking these just in case 🙂 )

BeMacized · 2021-01-11T13:54:34Z

packages/camera/camera/lib/src/camera_controller.dart

    }
  }

+  /// TODO: Documentation


Remaining TODO

packages/camera/camera/lib/src/camera_controller.dart

BeMacized · 2021-01-11T14:08:39Z

packages/camera/camera/lib/src/camera_controller.dart

+  void onCameraTimeLimitReachedEvent({onCameraTimeLimitReached}) {
+    if (!value.isInitialized || _isDisposed) {
+      throw CameraException(
+        'Uninitialized CameraController',
+        'cameraTimeLimitReachedEventStream was called on uninitialized CameraController',
+      );
+    }
+    if (!value.isRecordingVideo) {
+      throw CameraException(
+        'No video is recording',
+        'cameraTimeLimitReachedEventStream was called when no video is recording.',
+      );
+    }
+
+    CameraPlatform.instance
+        .onCameraTimeLimitReached(_cameraId)
+        .first
+        .then((event) {
+      value = value.copyWith(isRecordingVideo: false);
+      onCameraTimeLimitReached(event.path);
+    });
+  }


One issue I see with this, is that if the user sets a maxVideoDuration, but never calls onCameraTimeLimitReachedEvent to handle the callback, value will never be updated, causing isRecordingVideo to still report true, while it has in fact already stopped.

Ideally, the camera controller should always be listening for CameraPlatform.instance.onCameraTimeLimitReached to update the CameraValue, even when the user has not provided a callback, so that the state is always correct.

Additionally, I think I would personally prefer seeing a method that returns a stream of these events (Like how the platform interface exposes events) rather than a method that allows for setting a callback, as that would imo offer a bit more flexibility. What do you think?

I agree, this would be easier for users to manage if we can use this as a Stream<XFile> and simply listen for a file to arrive.

We can even extend the stopVideoRecording method to add the final video file to the same stream. This way developers simply listen to the stream to get informed that a new video file is available (no matter if the end-user pressed the stop button or if the max time limit has been reached). Of course this would mean the name of the event and the stream should be updated a to reflect this change. Something like:

Stream<VideoRecordedEvent> onVideoRecorded(int cameraId);

This might also solve possible issues with recording video's and stopping them for unusual reasons, like full storage

packages/camera/camera_platform_interface/lib/src/events/camera_event.dart

BeMacized · 2021-01-11T14:14:26Z

packages/camera/camera_platform_interface/lib/src/platform_interface/camera_platform.dart

+  @override
+  Stream<CameraTimeLimitReachedEvent> onCameraTimeLimitReached(int cameraId) {
+    throw UnimplementedError('onCameraTimeLimitReached() is not implemented.');
+  }


This change to the platform interface should probably be split off to a separate PR, so that it can be published to pub.dev before the implementations are merged to master.

I think it would be better to rename this to onVideoRecordingTimeLimitReached, so it is clear this is directly related to the video recording feature. Of course if you accept my earlier comment about also emitting the recorded video on this stream when the user calls stopVideoRecording we should rename it to something more generic like onVideoRecorded.

packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/Camera.java

mvanbeusekom · 2021-01-11T16:09:46Z

packages/camera/camera/lib/src/camera_controller.dart

+  void onCameraTimeLimitReachedEvent({onCameraTimeLimitReached}) {
+    if (!value.isInitialized || _isDisposed) {
+      throw CameraException(
+        'Uninitialized CameraController',
+        'cameraTimeLimitReachedEventStream was called on uninitialized CameraController',
+      );
+    }
+    if (!value.isRecordingVideo) {
+      throw CameraException(
+        'No video is recording',
+        'cameraTimeLimitReachedEventStream was called when no video is recording.',
+      );
+    }
+
+    CameraPlatform.instance
+        .onCameraTimeLimitReached(_cameraId)
+        .first
+        .then((event) {
+      value = value.copyWith(isRecordingVideo: false);
+      onCameraTimeLimitReached(event.path);
+    });
+  }


I agree, this would be easier for users to manage if we can use this as a Stream<XFile> and simply listen for a file to arrive.

We can even extend the stopVideoRecording method to add the final video file to the same stream. This way developers simply listen to the stream to get informed that a new video file is available (no matter if the end-user pressed the stop button or if the max time limit has been reached). Of course this would mean the name of the event and the stream should be updated a to reflect this change. Something like:

Stream<VideoRecordedEvent> onVideoRecorded(int cameraId);

mvanbeusekom · 2021-01-11T16:10:24Z

packages/camera/camera_platform_interface/lib/src/events/camera_event.dart

 }
+
+class CameraTimeLimitReachedEvent extends CameraEvent {
+  final XFile path;


nit: Rename to file instead of path:

Suggested change

final XFile path;

final XFile file;

mvanbeusekom · 2021-01-11T16:13:55Z

packages/camera/camera_platform_interface/lib/src/method_channel/method_channel_camera.dart

  }

+  @override
+  Stream<CameraTimeLimitReachedEvent> onCameraTimeLimitReached(int cameraId) {


I think it would be better to rename this to onVideoRecordingTimeLimitReached, so it is clear this is directly related to the video recording feature. Of course if you accept my earlier comment about also emitting the recorded video on this stream when the user calls stopVideoRecording we should rename it to something more generic like onVideoRecorded.

mvanbeusekom · 2021-01-11T16:14:41Z

packages/camera/camera_platform_interface/lib/src/platform_interface/camera_platform.dart

+  @override
+  Stream<CameraTimeLimitReachedEvent> onCameraTimeLimitReached(int cameraId) {
+    throw UnimplementedError('onCameraTimeLimitReached() is not implemented.');
+  }


I think it would be better to rename this to onVideoRecordingTimeLimitReached, so it is clear this is directly related to the video recording feature. Of course if you accept my earlier comment about also emitting the recorded video on this stream when the user calls stopVideoRecording we should rename it to something more generic like onVideoRecorded.

mvanbeusekom · 2021-01-11T16:17:10Z

packages/camera/camera/README.md

+As of version [0.6.5](https://github.com/flutter/plugins/blob/master/packages/camera/camera/CHANGELOG.md#065) the startVideoRecording method can be used with the maxVideoDuration. To do this the result of the recording needs to be retrieved by calling controller.onCameraTimeLimitReachedEvent which accepts a callback to retrieve the XFile result. Like so:
+
+```dart
+    Future<void> startVideoRecording() async {
+      if (!controller.value.isInitialized) {
+        showInSnackBar('Error: select a camera first.');
+        return;
+      }
+  
+      if (controller.value.isRecordingVideo) {
+        // A recording is already started, do nothing.
+        return;
+      }
+  
+      try {
+        await controller.startVideoRecording(
+            maxVideoDuration: const Duration(milliseconds: 5000),
+        );
+        controller.onCameraTimeLimitReachedEvent(onCameraTimeLimitReached: (XFile file) {
+          //Handle the XFile
+        });
+      } on CameraException catch (e) {
+        _showCameraException(e);
+        return;
+      }
+    }
+```


Really like that you didn't forget to updated the documentation in the README.md. But this should probably also be updated according the the discussion @BeMacized started on using a stream instead of registering a callback method.

# Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/pubspec.yaml

kiaxseventh · 2021-08-07T14:27:58Z

add a full example please

danielroek and others added 10 commits December 23, 2020 11:49

Added maxVideoDuration to startVideoRecording

c7a749a

updated documentation

131918d

Co-authored-by: Maurits van Beusekom <[email protected]>

updated documentation

9b3ae14

Co-authored-by: Maurits van Beusekom <[email protected]>

Fixed long line in docs

3ca25df

Formatting

5e626b9

Started implementation for Android

02811b7

WIP: Started implementation of stream when time limit is reached

b8b07e2

Merge remote-tracking branch 'origin/master' into limit_video_length

532cbb0

Initial working implementation

50edc53

google-cla bot added the cla: yes label Jan 11, 2021

Android implementation works

6e31d7c

danielroek commented Jan 11, 2021

View reviewed changes

danielroek added 5 commits January 11, 2021 12:36

Improved implementation

d3bab02

Updated README order

ae7365c

removed debugPrints

ae1b47c

Fixed url in README.md

4037b5e

Formatting

36acc32

BeMacized reviewed Jan 11, 2021

View reviewed changes

mvanbeusekom suggested changes Jan 11, 2021

View reviewed changes

danielroek added 5 commits January 13, 2021 09:55

Merge remote-tracking branch 'origin/master' into limit_video_length

5a78233

# Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/pubspec.yaml

Implemented Java feedback

09dad16

Implemented Event and Stream to notify about videoRecording

771f116

stopVideoRecording now listens to VideoRecordedEvent

16e43fb

Fixed future returning xFile

7cb73ef

danielroek requested a review from mvanbeusekom January 13, 2021 14:41

stuartmorgan-g added the p: camera label Jan 29, 2021

Daniel Roek added 3 commits February 3, 2021 14:03

finished iOS implementation

759f163

Fixed formatting

0b4270d

fixed formatting

12231da

danielroek mentioned this pull request Feb 3, 2021

[camera_platform_interface] Added VideoRecordedEvent #3504

Closed

11 tasks

Reverted platform_interface changes

3b8d6ed

danielroek closed this Feb 5, 2021

mvanbeusekom deleted the limit_video_length branch September 21, 2021 09:31

		camera_platform_interface: #^1.5.0
		path: ../camera_platform_interface

[camera] Limit video length with maxVideoDuration on startVideoRecording #3403

[camera] Limit video length with maxVideoDuration on startVideoRecording #3403

Uh oh!

Conversation

danielroek commented Jan 11, 2021

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeMacized left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BeMacized Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BeMacized Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BeMacized Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiaxseventh commented Aug 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BeMacized Jan 11, 2021 •

edited

Loading

BeMacized Jan 11, 2021 •

edited

Loading

BeMacized Jan 11, 2021 •

edited

Loading