Skip to content

[camerax] Implement video capture for CameraX android camera re-write #3467

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.

Sign up for GitHub

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

Merged
camsim99 merged 19 commits into from
May 9, 2023
Merged

[camerax] Implement video capture for CameraX android camera re-write #3467

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f876e73
remaking video capture pr to fix CLA issue
May 3, 2023
ca2a353
accepting documentation change to createTempFile
May 3, 2023
22eea7a
making video recording tests into a group, and random typo
May 3, 2023
77dfba1
Merge branch 'main' into video_capture
gmackall May 4, 2023
daf385b
Merge branch 'flutter:main' into video_capture
gmackall May 8, 2023
167f9c2
documentation changes from review comments
May 8, 2023
23ec03e
changing test name
May 8, 2023
034a508
Merge branch 'flutter:main' into video_capture
gmackall May 8, 2023
d5affdd
changing a use of RuntimeException to a FlutterError
May 8, 2023
9797067
format
May 8, 2023
c76d1a4
adding tests for error cases in stopVideoRecording
May 8, 2023
3141b02
formatting
May 8, 2023
92a51b8
Making explicit that we do nothing on pressing record twice, and addi…
May 8, 2023
5e776a5
formatting new tests
May 8, 2023
0ba0dfc
review comments around documentation and an assert
May 8, 2023
89d7936
removing use of the word 'we' per the style guide
May 8, 2023
9eecbed
removing second 'assert' that was copied over from first instance of …
May 8, 2023
f8eee31
Merge branch 'main' into video_capture
gmackall May 9, 2023
fb4770b
Merge branch 'main' into video_capture
gmackall May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,11 @@ class AndroidCameraCameraX extends CameraPlatform {
assert(cameraSelector != null);
assert(processCameraProvider != null);

if (recording != null) {
// There is currently an active recording, so do not start a new one.
Copy link
Contributor

@camsim99 camsim99 May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a statement to the doc for this method that this is the expected behavior for trying to start a new recording?

Copy link
Member Author

@gmackall gmackall May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note that we silently return without doing anything if there is an active recording.

return;
}

if (!(await processCameraProvider!.isBound(videoCapture!))) {
camera = await processCameraProvider!
.bindToLifecycle(cameraSelector!, <UseCase>[videoCapture!]);
Expand All @@ -444,6 +449,11 @@ class AndroidCameraCameraX extends CameraPlatform {
'video recording while no recording is in progress.');
}
if (videoOutputPath == null) {
// Stop the current active recording as we will be unable to complete it
// in this error case.
recording!.close();
recording = null;
pendingRecording = null;
Copy link
Contributor

@camsim99 camsim99 May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about adding it to the doc

Copy link
Member Author

@gmackall gmackall May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note on the cleanup of the recording objects in the doc

throw CameraException(
'INVALID_PATH',
'The platform did not return a path '
Expand Down
52 changes: 51 additions & 1 deletion packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart
View file
Open in desktop
Copy link
Contributor

@reidbaker reidbaker May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there error cases around recording that should be tested. For example calling stop twice in a row?

Copy link
Contributor

@camsim99 camsim99 May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort of testing is good, and likely the sort we should use for several other methods tested here. Not sure this should block this PR, though, so if you don't get to it, please add it here: flutter/flutter#125928

Copy link
Contributor

@reidbaker reidbaker May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: I do think that some error testing should be required before merging even if the test is that we we swallow the error and don't crash (or that we crash and the crash is part of the documented behavior)

Copy link
Member Author

@gmackall gmackall May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example calling stop twice in a row?

This action currently throws a CameraException. I'll add a test for it.

Copy link
Member Author

@gmackall gmackall May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test for

  1. if the recording is null (we haven't started a recording). In this case we throw a CameraException
  2. if the videoOutputPath is null (we shouldn't generally reach this case). In this case we throw a CameraException
  3. We call stop on a valid recording, and then we call it again (in this case we test everything performs normally for the first call to stop, and then we throw a CameraException on the second call).

Copy link
Member Author

@gmackall gmackall May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added a logic and a test for calling recording twice (we silently do nothing in this case, based on a check of if the class variable recording is null or not).

Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,55 @@ void main() {
expect(camera.recording, mockRecording);
});

test(
'startVideoRecording binds video capture use case and starts the recording'
' on first call, and does nothing on second call',
() async {
//Set up mocks and constants.
final MockAndroidCameraCameraX camera = MockAndroidCameraCameraX();
camera.processCameraProvider = MockProcessCameraProvider();
camera.cameraSelector = MockCameraSelector();
camera.recorder = camera.testRecorder;
camera.videoCapture = camera.testVideoCapture;
camera.camera = MockCamera();
final MockPendingRecording mockPendingRecording = MockPendingRecording();
final MockRecording mockRecording = MockRecording();
final TestSystemServicesHostApi mockSystemServicesApi =
MockTestSystemServicesHostApi();
TestSystemServicesHostApi.setup(mockSystemServicesApi);

const int cameraId = 17;
const String outputPath = '/temp/MOV123.temp';

// Mock method calls.
when(mockSystemServicesApi.getTempFilePath(camera.videoPrefix, '.temp'))
.thenReturn(outputPath);
when(camera.testRecorder.prepareRecording(outputPath))
.thenAnswer((_) async => mockPendingRecording);
when(mockPendingRecording.start()).thenAnswer((_) async => mockRecording);
when(camera.processCameraProvider!.isBound(camera.videoCapture!))
.thenAnswer((_) async => false);
when(camera.processCameraProvider!.bindToLifecycle(
camera.cameraSelector!, <UseCase>[camera.videoCapture!]))
.thenAnswer((_) async => camera.camera!);

await camera.startVideoRecording(cameraId);

verify(camera.processCameraProvider!.bindToLifecycle(
camera.cameraSelector!, <UseCase>[camera.videoCapture!]));
expect(camera.pendingRecording, equals(mockPendingRecording));
expect(camera.recording, mockRecording);

await camera.startVideoRecording(cameraId);
// Verify that each of these calls happened only once.
verify(mockSystemServicesApi.getTempFilePath(camera.videoPrefix, '.temp')).called(1);
verifyNoMoreInteractions(mockSystemServicesApi);
verify(camera.testRecorder.prepareRecording(outputPath)).called(1);
verifyNoMoreInteractions(camera.testRecorder);
verify(mockPendingRecording.start()).called(1);
verifyNoMoreInteractions(mockPendingRecording);
});

test('pauseVideoRecording pauses the recording', () async {
final AndroidCameraCameraX camera = AndroidCameraCameraX();
final MockRecording recording = MockRecording();
Expand Down Expand Up @@ -577,7 +626,7 @@ void main() {

test(
'stopVideoRecording throws a camera exception if '
'videoOutputPath is null', () async {
'videoOutputPath is null, and sets recording to null', () async {
final AndroidCameraCameraX camera = AndroidCameraCameraX();
final MockRecording recording = MockRecording();

Expand All @@ -586,6 +635,7 @@ void main() {

expect(
() => camera.stopVideoRecording(0), throwsA(isA<CameraException>()));
expect(camera.recording, null);
});

test(
Expand Down