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

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
42 changes: 42 additions & 0 deletions 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 @@ -561,6 +561,48 @@ void main() {
verify(recording.close());
verifyNoMoreInteractions(recording);
});

test('stopVideoRecording throws a camera exception if '
'no recording is in progress', () async {
final AndroidCameraCameraX camera = AndroidCameraCameraX();
const String videoOutputPath = '/test/output/path';

camera.recording = null;
camera.videoOutputPath = videoOutputPath;

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

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

camera.recording = recording;
camera.videoOutputPath = null;

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

test('calling stopVideoRecording twice stops the recording '
'and then throws a CameraException', () async {
final AndroidCameraCameraX camera = AndroidCameraCameraX();
final MockRecording recording = MockRecording();
final MockProcessCameraProvider processCameraProvider =
MockProcessCameraProvider();
final MockVideoCapture videoCapture = MockVideoCapture();
const String videoOutputPath = '/test/output/path';

camera.processCameraProvider = processCameraProvider;
camera.recording = recording;
camera.videoCapture = videoCapture;
camera.videoOutputPath = videoOutputPath;

final XFile file = await camera.stopVideoRecording(0);
assert(file.path == videoOutputPath);
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.

Should this be an expect statement instead of assert?

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.

Good catch! Changed

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.

It was actually in two places because I copied this test to make the test of "call start video recording twice", changed in both places now


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

test(
Expand Down