[camera] Add ability to concurrently record and stream video #6290

adam-harwood · 2022-08-18T06:30:11Z

This PR adds the ability to optionally also enable streaming when starting recording of a video. This feature is useful, for example, in applications that use image recognition. The customer will want some immediate feedback as the video is being recorded (where an ML model might be run on the captured images), as well as to upload the video to a server at the end for final server-side processing.

Functionally, the new optional named parameter on startVideoRecording will combine the existing implementation with that of startImageStream. Besides this, all functionality remains the same. For existing code, that does not use the new parameter, there is no change to the code path.

I have added unit tests to the higher-level dart abstractions, added integration tests for the underlying android and iOS implementations, and tested manually on Android and iOS devices. I have updated camera_web to throw an exception if the user attempts to enable streaming when recording (given that streaming is not yet supported).

Closes flutter/flutter#83634

Pre-launch Checklist

google-cla · 2022-08-18T06:30:14Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

adam-harwood · 2022-08-18T06:42:32Z

Looks like I need to update camera_windows as well. Will do.

adam-harwood · 2022-08-19T00:02:04Z

One of the checks is failing because it doesn't want the PR to include both changes to camera_platform_interface and the implementations. Can you confirm if that's the case (and that you're happy with the changes in camera_platform_interface)? If so then I'll split this out into 2 PRs.

packages/camera/camera_windows/lib/camera_windows.dart

hellohuanlin

I only reviewed the iOS part

packages/camera/camera_avfoundation/CHANGELOG.md

packages/camera/camera_avfoundation/ios/Classes/FLTCam.h

hellohuanlin · 2022-08-23T18:08:40Z

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

+    return _frameStreamController!.stream;
+  }
+
+  void _installStreamController({Function()? onListen}) {


In startVideoRecording Can we directly pass streamCallback into _installStreamController() and make it nonNull? A no-op onListen seems a bad state to me.

Not sure I totally understand. streamCallback is the callback which receives each frame that is processed. onListen is different, it's a callback invoked once when the stream is listened on. In the case of onStreamedFrameAvailable it needs to call the native startImageStream to begin the stream, but on the startVideoRecording the camera stream has already been initialised during the call to the native startVideoRecording. Therefore, in the case of startVideoRecording, onListen should be a no-op.

I could make onListen non-null, and have it up to the caller to default it to a no-op function, but that seems to put more burden on the caller. Is that preferable?

I meant do we want to have onListen for both use cases? It's a bit odd to have one streaming API with onListen, but the other streaming API without.

onListen isn't part of the public API though. It's an implementation detail inside this class. Nobody using the camera plugin will have to deal with it at all. The only reason that onListen is nullable is that, in the case where they want to both stream and record, the initialization is another section of the (internal) code. StreamController itself is a class external to the camera plugin and can't be easily modified.

Given that the method is private and internal to the class, is the difference still a concern? If it is, could you maybe code a quick mockup of what you're after, just so that I can understand?

packages/camera/camera_avfoundation/lib/src/avfoundation_camera.dart

hellohuanlin · 2022-08-23T18:10:27Z

packages/camera/camera_avfoundation/lib/src/avfoundation_camera.dart

    _frameStreamController = StreamController<CameraImageData>(
-      onListen: _onFrameStreamListen,
+      onListen: onListen ?? () {},


when do we want a no-op onListen

Hopefully addressed in this comment, but let me know if not.

adam-harwood · 2022-08-24T01:37:53Z

Thanks for the feedback @hellohuanlin , I'll wait for some feedback on the other modules as well and then address everything you raised.

camsim99

Reviewed Android part!

packages/camera/camera/CHANGELOG.md

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

packages/camera/camera_android/CHANGELOG.md

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

camsim99 · 2022-08-26T20:02:25Z

packages/camera/camera_android/lib/src/android_camera.dart

    _frameStreamController = StreamController<CameraImageData>(
-      onListen: _onFrameStreamListen,
+      onListen: onListen ?? () {},


Consider https://github.com/flutter/plugins/pull/6290/files#r952963757 here as well.

Replied in this comment, I don't think it applies unless I'm misunderstanding something.

packages/camera/camera_windows/CHANGELOG.md

hellohuanlin · 2022-09-06T18:16:00Z

Saw a few updates in my email. Just double check - let me know when it's ready for a second review.

adam-harwood · 2022-12-05T22:31:24Z

@stuartmorgan this is ready for re-review.

stuartmorgan

LGTM!

flutter/plugins#6290)

* 63f1b8fb6 [camera] Handle empty grantResults on permission request (flutter/plugins#6758) * 1579f2f33 Roll Flutter from 33f3e13 to 30fc993 (6 revisions) (flutter/plugins#6791) * 345bddbd8 [gh_actions]: Bump github/codeql-action from 2.1.27 to 2.1.35 (flutter/plugins#6788) * 374e59804 [camera] Add ability to concurrently record and stream video (flutter/plugins#6290)

…6290)" This reverts commit 374e598.

stuartmorgan · 2022-12-06T16:07:32Z

We'll need to revert this; the new Android integration test is failing. See #6796 for details. I should have pushed a trivial change to this PR to trigger the Android tests, but forgot.

@adam-harwood Are you able to reproduce that failure locally to debug it for an updated version of the PR?

…6290)" (#6796) This reverts commit 374e598.

adam-harwood · 2022-12-07T00:25:12Z

@adam-harwood Are you able to reproduce that failure locally to debug it for an updated version of the PR?

I ran them just now on an Android phone and cannot replicate the error you posted. I did get a different error though:

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
Guarded function conflict.
You must use "await" with all Future-returning test APIs.
The guarded method "pump" from class LiveTestWidgetsFlutterBinding was called from
package:flutter_test/src/binding.dart on line 841.
Then, the "expectLater" function was called from
file:///home/adam/dev/flutter_plugins/packages/camera/camera_android/example/integration_test/camera_test.dart
on line 275.
The first method (LiveTestWidgetsFlutterBinding.pump) had not yet finished executing at the time
that the second function (expectLater) was called. Since both are guarded, and the second was not a
nested call inside the first, the first must complete its execution before the second can be called.
Typically, this is achieved by putting an "await" statement in front of the call to the first.

When the first method (LiveTestWidgetsFlutterBinding.pump) was called, this was the stack:
#0      TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:69:54)
#1      LiveTestWidgetsFlutterBinding.pump (package:flutter_test/src/binding.dart:1653:27)
#2      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:841:13)
<asynchronous suspension>

When the exception was thrown, this was the stack:
#0      TestAsyncUtils.guardSync (package:flutter_test/src/test_async_utils.dart:267:5)
#1      expectLater (package:flutter_test/src/widget_tester.dart:491:18)
#2      main.<anonymous closure>.<anonymous closure> (file:///home/adam/dev/flutter_plugins/packages/camera/camera_android/example/integration_test/camera_test.dart:275:9)
(elided 24 frames from dart:async and package:stack_trace)

The test description was:
  recording with image stream
════════════════════════════════════════════════════════════════════════════════════════════════════
01:46 +2 ~2 -1: recording with image stream [E]                                                                                                                            
  Test failed. See exception logs above.
  The test description was: recording with image stream
  
  'package:flutter_test/src/binding.dart': Failed assertion: line 1736 pos 12: '!_expectingFrame': is not true.
  dart:core                                                                  _AssertionError._throwNew
  package:flutter_test/src/binding.dart 1736:12                              LiveTestWidgetsFlutterBinding.postTest
  ===== asynchronous gap ===========================
  package:stream_channel                                                     _GuaranteeSink.add
  tmp/flutter_tools.TJHMNR/flutter_test_listener.FNKBUV/listener.dart 53:22  main.<fn>
  tmp/flutter_tools.TJHMNR/flutter_test_listener.FNKBUV/listener.dart 52:20  main.<fn>

which I can fix, but I think it's unlikely it will solve the problem you posted. Applying the fix locally, all tests are then green. I'm testing this on an Android device, what device do the automatic runs execute on @stuartmorgan ?

For reference, I'm running the tests with:

flutter_plugins/packages/camera/camera_android/example$ flutter test integration_test/camera_test.dart

Which I assume is correct.

…#116591) * 63f1b8fb6 [camera] Handle empty grantResults on permission request (flutter/plugins#6758) * 1579f2f33 Roll Flutter from 33f3e13 to 30fc993 (6 revisions) (flutter/plugins#6791) * 345bddbd8 [gh_actions]: Bump github/codeql-action from 2.1.27 to 2.1.35 (flutter/plugins#6788) * 374e59804 [camera] Add ability to concurrently record and stream video (flutter/plugins#6290)

stuartmorgan · 2022-12-07T14:44:24Z

which I can fix, but I think it's unlikely it will solve the problem you posted. Applying the fix locally, all tests are then green.

Could you post a new PR that starts as a revert of the revert, and then has a commit that fixes the test issue you found locally (so we can easily review just the diff)? I can then push a commit to that PR to trigger the tests and we can see how it looks in CI.

I'm testing this on an Android device, what device do the automatic runs execute on @stuartmorgan ?

It looks like it's a Samsung Galaxy S9 and a Pixel 5.

For reference, I'm running the tests with:

That should run the same tests. Unfortunately there's no easy way to run them exactly the same way as the CI runs them (we hope to add simulator tests at some point to avoid this disconnect), but that's usually not an issue.

This re-commits the content from flutter#6290. Will make a subsequent commit to try and fix the broken integ tests.

adam-harwood · 2022-12-08T00:27:56Z

Could you post a new PR that starts as a revert of the revert, and then has a commit that fixes the test issue you found locally (so we can easily review just the diff)?

Sure @stuartmorgan : #6808

…6808) * Re-enable stream and record This re-commits the content from #6290. Will make a subsequent commit to try and fix the broken integ tests. * Fix broken integration test for streaming The `whenComplete` call was sometimes causing a race condition. It is also isn't needed for the test (to reset the value of isDetecting, given it's local), so removing it makes the test reliable. * Trivial CHANGELOG change to trigger full CI tests Co-authored-by: stuartmorgan <stuartmorgan@google.com>

…#116591) * 63f1b8fb6 [camera] Handle empty grantResults on permission request (flutter/plugins#6758) * 1579f2f33 Roll Flutter from 33f3e13 to 30fc993 (6 revisions) (flutter/plugins#6791) * 345bddbd8 [gh_actions]: Bump github/codeql-action from 2.1.27 to 2.1.35 (flutter/plugins#6788) * 374e59804 [camera] Add ability to concurrently record and stream video (flutter/plugins#6290)

…#6290) * Implement interface methods to allow concurrent stream and record There will be a subsequent change to the `camera` package to make use of these implementations. * Fix android test * Format android_camera_test.dart * Resolve analyze failures * Fix version bumps * Fix MethodChannelCameraTest * Fix comment on FLTCam * Add tests to confirm can't stream on windows or web * CHANGELOG updates * Fix analyze errors * Fix dart analyze warnings for web * Formatted

…lutter#6290)" (flutter#6796) This reverts commit 374e598.

…lutter#6808) * Re-enable stream and record This re-commits the content from flutter#6290. Will make a subsequent commit to try and fix the broken integ tests. * Fix broken integration test for streaming The `whenComplete` call was sometimes causing a race condition. It is also isn't needed for the test (to reset the value of isDetecting, given it's local), so removing it makes the test reliable. * Trivial CHANGELOG change to trigger full CI tests Co-authored-by: stuartmorgan <stuartmorgan@google.com>

…lutter#6808) * Re-enable stream and record This re-commits the content from flutter/plugins#6290. Will make a subsequent commit to try and fix the broken integ tests. * Fix broken integration test for streaming The `whenComplete` call was sometimes causing a race condition. It is also isn't needed for the test (to reset the value of isDetecting, given it's local), so removing it makes the test reliable. * Trivial CHANGELOG change to trigger full CI tests Co-authored-by: stuartmorgan <stuartmorgan@google.com>

adam-harwood requested review from hellohuanlin, camsim99, ditman and bparrishMines as code owners August 18, 2022 06:30

github-actions bot added p: camera platform-android platform-ios platform-web labels Aug 18, 2022

adam-harwood force-pushed the aharwood/enable_stream_and_record branch from 084ea88 to a33f148 Compare August 18, 2022 06:35

adam-harwood requested a review from cbracken as a code owner August 18, 2022 23:18

github-actions bot added the platform-windows label Aug 18, 2022

cbracken requested a review from loic-sharma August 19, 2022 00:31

loic-sharma reviewed Aug 19, 2022

View reviewed changes

packages/camera/camera_windows/lib/camera_windows.dart Outdated Show resolved Hide resolved

hellohuanlin reviewed Aug 23, 2022

View reviewed changes

camsim99 reviewed Aug 26, 2022

View reviewed changes

adam-harwood force-pushed the aharwood/enable_stream_and_record branch from 082b493 to 837ddcd Compare August 30, 2022 03:02

adam-harwood requested review from hellohuanlin and camsim99 and removed request for cbracken, ditman, bparrishMines, hellohuanlin and camsim99 August 30, 2022 03:07

loic-sharma reviewed Sep 2, 2022

View reviewed changes

packages/camera/camera_windows/CHANGELOG.md Outdated Show resolved Hide resolved

adam-harwood requested review from stuartmorgan and removed request for GaryQian, gaaclarke and tarrinneal December 5, 2022 22:31

adam-harwood added 3 commits December 6, 2022 08:54

Fix analyze errors

9433844

Fix dart analyze warnings for web

cb0c094

Formatted

3c514d4

stuartmorgan approved these changes Dec 6, 2022

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 6, 2022

auto-submit bot merged commit 374e598 into flutter:main Dec 6, 2022

engine-flutter-autoroll mentioned this pull request Dec 6, 2022

Roll Plugins from 2a330bc0afd5 to 374e59804a6c (4 revisions) flutter/flutter#116591

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2022

374e59804 [camera] Add ability to concurrently record and stream video (

af2d8a9

flutter/plugins#6290)

stuartmorgan added a commit that referenced this pull request Dec 6, 2022

Revert "[camera] Add ability to concurrently record and stream video (#…

13d6192

…6290)" This reverts commit 374e598.

stuartmorgan mentioned this pull request Dec 6, 2022

Revert "[camera] Add ability to concurrently record and stream video" #6796

Merged

stuartmorgan added a commit that referenced this pull request Dec 6, 2022

Revert "[camera] Add ability to concurrently record and stream video (#…

610370e

…6290)" (#6796) This reverts commit 374e598.

adam-harwood added a commit to adam-harwood/flutter_plugins that referenced this pull request Dec 8, 2022

Re-enable stream and record

af89cf4

This re-commits the content from flutter#6290. Will make a subsequent commit to try and fix the broken integ tests.

adam-harwood mentioned this pull request Dec 8, 2022

[camera] Re-enable ability to concurrently record and stream video #6808

Merged

11 tasks

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

Revert "[camera] Add ability to concurrently record and stream video (f…

18f5a01

…lutter#6290)" (flutter#6796) This reverts commit 374e598.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camera] Add ability to concurrently record and stream video #6290

[camera] Add ability to concurrently record and stream video #6290

adam-harwood commented Aug 18, 2022

google-cla bot commented Aug 18, 2022

adam-harwood commented Aug 18, 2022

adam-harwood commented Aug 19, 2022

hellohuanlin left a comment

hellohuanlin Aug 23, 2022

adam-harwood Aug 30, 2022

hellohuanlin Aug 30, 2022

adam-harwood Aug 30, 2022 •

edited

hellohuanlin Aug 23, 2022

adam-harwood Aug 30, 2022

adam-harwood commented Aug 24, 2022

camsim99 left a comment

camsim99 Aug 26, 2022

adam-harwood Aug 30, 2022

hellohuanlin commented Sep 6, 2022

adam-harwood commented Dec 5, 2022

stuartmorgan left a comment

stuartmorgan commented Dec 6, 2022

adam-harwood commented Dec 7, 2022

stuartmorgan commented Dec 7, 2022

adam-harwood commented Dec 8, 2022

[camera] Add ability to concurrently record and stream video #6290

[camera] Add ability to concurrently record and stream video #6290

Conversation

adam-harwood commented Aug 18, 2022

Pre-launch Checklist

google-cla bot commented Aug 18, 2022

adam-harwood commented Aug 18, 2022

adam-harwood commented Aug 19, 2022

hellohuanlin left a comment

Choose a reason for hiding this comment

hellohuanlin Aug 23, 2022

Choose a reason for hiding this comment

adam-harwood Aug 30, 2022

Choose a reason for hiding this comment

hellohuanlin Aug 30, 2022

Choose a reason for hiding this comment

adam-harwood Aug 30, 2022 • edited

Choose a reason for hiding this comment

hellohuanlin Aug 23, 2022

Choose a reason for hiding this comment

adam-harwood Aug 30, 2022

Choose a reason for hiding this comment

adam-harwood commented Aug 24, 2022

camsim99 left a comment

Choose a reason for hiding this comment

camsim99 Aug 26, 2022

Choose a reason for hiding this comment

adam-harwood Aug 30, 2022

Choose a reason for hiding this comment

hellohuanlin commented Sep 6, 2022

adam-harwood commented Dec 5, 2022

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan commented Dec 6, 2022

adam-harwood commented Dec 7, 2022

stuartmorgan commented Dec 7, 2022

adam-harwood commented Dec 8, 2022

adam-harwood Aug 30, 2022 •

edited