Skip to content

Commit

Permalink
[camerax] Small fixes to starting/stopping video capture (flutter#6068)
Browse files Browse the repository at this point in the history 
Fixes small issues I noticed in starting/stopping video capture while working on flutter#6059, notably:

- Change all remaining `unawaited` calls to `await` to avoid any racy behavior.
- Update `camera` info after `VideoCapture` use case is bound to the lifecycle of the plugin's `ProcessCameraProvider` to make sure it is up to date.
- ~Unbind `VideoCapture` use case when video recording stops since it was suggested to lazily load it for performance reasons (open to pushback on this).~ this would require potentially more changes than I originally thought
- Make tests checking that async methods throw exceptions actually wait for those exceptions as this may cause flaky test behavior.

Fixes flutter/flutter#132499 as this PR removes any remaining `unawaited` calls.
  • Loading branch information
@camsim99
camsim99 authored Feb 9, 2024
1 parent 316bde0 commit cd45100
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 44 deletions.
5 changes: 5 additions & 0 deletions packages/camera/camera_android_camerax/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.5.0+32

* Removes all remaining `unawaited` calls to fix potential race conditions and updates the
camera state when video capture starts.

## 0.5.0+31

* Wraps CameraX classes needed to set capture request options, which is needed to implement setting the exposure mode.
Expand Down
11 changes: 6 additions & 5 deletions packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,9 @@ class AndroidCameraCameraX extends CameraPlatform {
@override
Future<void> dispose(int cameraId) async {
preview?.releaseFlutterSurfaceTexture();
unawaited(liveCameraState?.removeObservers());
await liveCameraState?.removeObservers();
processCameraProvider?.unbindAll();
unawaited(imageAnalysis?.clearAnalyzer());
await imageAnalysis?.clearAnalyzer();
}

/// The camera has been initialized.
Expand Down Expand Up @@ -645,6 +645,7 @@ class AndroidCameraCameraX extends CameraPlatform {
if (!(await processCameraProvider!.isBound(videoCapture!))) {
camera = await processCameraProvider!
.bindToLifecycle(cameraSelector!, <UseCase>[videoCapture!]);
await _updateCameraInfoAndLiveCameraState(options.cameraId);
}

// Set target rotation to default CameraX rotation only if capture
Expand Down Expand Up @@ -681,7 +682,7 @@ class AndroidCameraCameraX extends CameraPlatform {
if (videoOutputPath == null) {
// Stop the current active recording as we will be unable to complete it
// in this error case.
unawaited(recording!.close());
await recording!.close();
recording = null;
pendingRecording = null;
throw CameraException(
Expand All @@ -690,7 +691,7 @@ class AndroidCameraCameraX extends CameraPlatform {
'while reporting success. The platform should always '
'return a valid path or report an error.');
}
unawaited(recording!.close());
await recording!.close();
recording = null;
pendingRecording = null;
return XFile(videoOutputPath!);
Expand Down Expand Up @@ -813,7 +814,7 @@ class AndroidCameraCameraX extends CameraPlatform {
/// Removes the previously set analyzer on the [imageAnalysis] instance, since
/// image information should no longer be streamed.
FutureOr<void> _onFrameStreamCancel() async {
unawaited(imageAnalysis!.clearAnalyzer());
await imageAnalysis!.clearAnalyzer();
}

/// Converts between Android ImageFormat constants and [ImageFormatGroup]s.
Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera_android_camerax/pubspec.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: camera_android_camerax
description: Android implementation of the camera plugin using the CameraX library.
repository: https://github.com/flutter/packages/tree/main/packages/camera/camera_android_camerax
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22
version: 0.5.0+31
version: 0.5.0+32

environment:
sdk: ">=3.0.0 <4.0.0"
Expand Down
133 changes: 95 additions & 38 deletions packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,9 @@ void main() {
'initializeCamera throws a CameraException when createCamera has not been called before initializedCamera',
() async {
final AndroidCameraCameraX camera = AndroidCameraCameraX();
expect(() => camera.initializeCamera(3), throwsA(isA<CameraException>()));
await expectLater(() async {
await camera.initializeCamera(3);
}, throwsA(isA<CameraException>()));
});

test('initializeCamera sends expected CameraInitializedEvent', () async {
Expand Down Expand Up @@ -1006,26 +1008,37 @@ void main() {

group('video recording', () {
test(
'startVideoCapturing binds video capture use case and starts the recording',
'startVideoCapturing binds video capture use case, updates saved camera instance and its properties, and starts the recording',
() async {
// Set up mocks and constants.
final AndroidCameraCameraX camera = AndroidCameraCameraX();
final MockPendingRecording mockPendingRecording = MockPendingRecording();
final MockRecording mockRecording = MockRecording();
final MockCamera mockCamera = MockCamera();
final MockCamera newMockCamera = MockCamera();
final MockCameraInfo mockCameraInfo = MockCameraInfo();
final MockLiveCameraState mockLiveCameraState = MockLiveCameraState();
final MockLiveCameraState newMockLiveCameraState = MockLiveCameraState();
final TestSystemServicesHostApi mockSystemServicesApi =
MockTestSystemServicesHostApi();
TestSystemServicesHostApi.setup(mockSystemServicesApi);

// Set directly for test versus calling createCamera.
camera.processCameraProvider = MockProcessCameraProvider();
camera.camera = MockCamera();
camera.camera = mockCamera;
camera.recorder = MockRecorder();
camera.videoCapture = MockVideoCapture();
camera.cameraSelector = MockCameraSelector();
camera.liveCameraState = mockLiveCameraState;

// Ignore setting target rotation for this test; tested seprately.
camera.captureOrientationLocked = true;

// Tell plugin to create detached Observer when camera info updated.
camera.proxy = CameraXProxy(
createCameraStateObserver: (void Function(Object) onChanged) =>
Observer<CameraState>.detached(onChanged: onChanged));

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

Expand All @@ -1039,12 +1052,30 @@ void main() {
.thenAnswer((_) async => false);
when(camera.processCameraProvider!.bindToLifecycle(
camera.cameraSelector!, <UseCase>[camera.videoCapture!]))
.thenAnswer((_) async => camera.camera!);
.thenAnswer((_) async => newMockCamera);
when(newMockCamera.getCameraInfo())
.thenAnswer((_) async => mockCameraInfo);
when(mockCameraInfo.getCameraState())
.thenAnswer((_) async => newMockLiveCameraState);

await camera.startVideoCapturing(const VideoCaptureOptions(cameraId));

// Verify VideoCapture UseCase is bound and camera & its properties
// are updated.
verify(camera.processCameraProvider!.bindToLifecycle(
camera.cameraSelector!, <UseCase>[camera.videoCapture!]));
expect(camera.camera, equals(newMockCamera));
expect(camera.cameraInfo, equals(mockCameraInfo));
verify(mockLiveCameraState.removeObservers());
expect(
await testCameraClosingObserver(
camera,
cameraId,
verify(newMockLiveCameraState.observe(captureAny)).captured.single
as Observer<dynamic>),
isTrue);

// Verify recording is started.
expect(camera.pendingRecording, equals(mockPendingRecording));
expect(camera.recording, mockRecording);
});
Expand All @@ -1056,6 +1087,8 @@ void main() {
final AndroidCameraCameraX camera = AndroidCameraCameraX();
final MockPendingRecording mockPendingRecording = MockPendingRecording();
final MockRecording mockRecording = MockRecording();
final MockCamera mockCamera = MockCamera();
final MockCameraInfo mockCameraInfo = MockCameraInfo();
final TestSystemServicesHostApi mockSystemServicesApi =
MockTestSystemServicesHostApi();
TestSystemServicesHostApi.setup(mockSystemServicesApi);
Expand All @@ -1069,6 +1102,11 @@ void main() {
// Ignore setting target rotation for this test; tested seprately.
camera.captureOrientationLocked = true;

// Tell plugin to create detached Observer when camera info updated.
camera.proxy = CameraXProxy(
createCameraStateObserver: (void Function(Object) onChanged) =>
Observer<CameraState>.detached(onChanged: onChanged));

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

Expand All @@ -1082,7 +1120,11 @@ void main() {
.thenAnswer((_) async => false);
when(camera.processCameraProvider!.bindToLifecycle(
camera.cameraSelector!, <UseCase>[camera.videoCapture!]))
.thenAnswer((_) async => MockCamera());
.thenAnswer((_) async => mockCamera);
when(mockCamera.getCameraInfo())
.thenAnswer((_) => Future<CameraInfo>.value(mockCameraInfo));
when(mockCameraInfo.getCameraState())
.thenAnswer((_) async => MockLiveCameraState());

await camera.startVideoCapturing(const VideoCaptureOptions(cameraId));

Expand Down Expand Up @@ -1267,9 +1309,14 @@ void main() {
camera.videoCapture = videoCapture;
camera.videoOutputPath = videoOutputPath;

// Tell plugin that videoCapture use case was bound to start recording.
when(camera.processCameraProvider!.isBound(videoCapture))
.thenAnswer((_) async => true);

final XFile file = await camera.stopVideoRecording(0);
expect(file.path, videoOutputPath);

// Verify that recording stops.
verify(recording.close());
verifyNoMoreInteractions(recording);
});
Expand All @@ -1284,47 +1331,57 @@ void main() {
camera.recording = null;
camera.videoOutputPath = videoOutputPath;

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

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

// Set directly for test versus calling startVideoCapturing.
camera.recording = recording;
camera.videoOutputPath = null;
// Set directly for test versus calling startVideoCapturing.
camera.processCameraProvider = MockProcessCameraProvider();
camera.recording = mockRecording;
camera.videoOutputPath = null;
camera.videoCapture = mockVideoCapture;

expect(
() => camera.stopVideoRecording(0), throwsA(isA<CameraException>()));
expect(camera.recording, null);
});
// Tell plugin that videoCapture use case was bound to start recording.
when(camera.processCameraProvider!.isBound(mockVideoCapture))
.thenAnswer((_) async => true);

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';
await expectLater(() async {
await camera.stopVideoRecording(0);
}, throwsA(isA<CameraException>()));
expect(camera.recording, null);
});

// Set directly for test versus calling createCamera and startVideoCapturing.
camera.processCameraProvider = processCameraProvider;
camera.recording = recording;
camera.videoCapture = videoCapture;
camera.videoOutputPath = videoOutputPath;
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';

final XFile file = await camera.stopVideoRecording(0);
expect(file.path, videoOutputPath);
// Set directly for test versus calling createCamera and startVideoCapturing.
camera.processCameraProvider = processCameraProvider;
camera.recording = recording;
camera.videoCapture = videoCapture;
camera.videoOutputPath = videoOutputPath;

expect(
() => camera.stopVideoRecording(0), throwsA(isA<CameraException>()));
});
final XFile file = await camera.stopVideoRecording(0);
expect(file.path, videoOutputPath);

await expectLater(() async {
await camera.stopVideoRecording(0);
}, throwsA(isA<CameraException>()));
});

test(
Expand Down

0 comments on commit cd45100

Please sign in to comment.