Skip to content
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.

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

[camerax] Fixes unawaited_futures violations #4337

Merged
merged 24 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
dec3d69
Merge remote-tracking branch 'upstream/main' into camx_occ
camsim99 May 1, 2023
0e0333b
Merge remote-tracking branch 'upstream/main'
camsim99 May 2, 2023
bd7ac99
Merge remote-tracking branch 'upstream/main'
camsim99 May 3, 2023
5c3363b
Merge remote-tracking branch 'upstream/main'
camsim99 May 10, 2023
fed9621
Undo changes
camsim99 May 10, 2023
5aabe34
Merge remote-tracking branch 'upstream/main'
camsim99 May 12, 2023
2b9a352
Merge remote-tracking branch 'upstream/main'
camsim99 May 25, 2023
a1173da
Merge remote-tracking branch 'upstream/main'
camsim99 May 30, 2023
cbc3d6b
Merge remote-tracking branch 'upstream/main'
camsim99 May 30, 2023
cae5a4c
Merge remote-tracking branch 'upstream/main'
camsim99 Jun 1, 2023
72283db
Merge remote-tracking branch 'upstream/main'
camsim99 Jun 5, 2023
166a77c
Merge remote-tracking branch 'upstream/main'
camsim99 Jun 5, 2023
399780e
Merge remote-tracking branch 'upstream/main'
camsim99 Jun 14, 2023
8d5d0e7
Merge remote-tracking branch 'upstream/main'
camsim99 Jun 26, 2023
ccc0715
Add initial fixes
camsim99 Jun 28, 2023
0351c29
Merge remote-tracking branch 'upstream/main' into unawaited_futures_c…
camsim99 Jun 28, 2023
6b88aed
Bump version
camsim99 Jun 28, 2023
52b8faa
Make some modifications
camsim99 Jun 28, 2023
9d1d004
One last modification
camsim99 Jun 28, 2023
c0f6f52
Remove exception
camsim99 Jun 28, 2023
3dd2855
Merge branch 'main' into unawaited_futures_camerax
camsim99 Jun 28, 2023
c64b72d
revisions
camsim99 Jun 30, 2023
8c78b95
Merge branch 'unawaited_futures_camerax' of github.com:camsim99/packa…
camsim99 Jun 30, 2023
8913126
Merge remote-tracking branch 'upstream/main' into unawaited_futures_c…
camsim99 Jun 30, 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
4 changes: 4 additions & 0 deletions packages/camera/camera_android_camerax/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.5.0+8

* Fixes unawaited_futures violations.

## 0.5.0+7

* Updates Guava version to 32.0.1.
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ class CameraController extends ValueNotifier<CameraValue> {
}

if (value.isStreamingImages) {
stopImageStream();
await stopImageStream();
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the await here will make it take longer to actually begin the process of stopping recording on the native side; do we want that? An alternative would be to store the future in a local variable, and then await it just before the return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we want this...I'll remove the await.

}

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,9 @@ class AndroidCameraCameraX extends CameraPlatform {
@override
Future<void> dispose(int cameraId) async {
preview?.releaseFlutterSurfaceTexture();
liveCameraState?.removeObservers();
unawaited(liveCameraState?.removeObservers());
processCameraProvider?.unbindAll();
imageAnalysis?.clearAnalyzer();
unawaited(imageAnalysis?.clearAnalyzer());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bparrishMines this makes me wonder if I did some wrapping wrong. Should all of the methods I wrapped that were void been wrapped as Future<void>?

}

/// The camera has been initialized.
Expand Down Expand Up @@ -423,7 +423,7 @@ class AndroidCameraCameraX extends CameraPlatform {
/// [cameraId] not used.
@override
Future<void> pausePreview(int cameraId) async {
_unbindUseCaseFromLifecycle(preview!);
await _unbindUseCaseFromLifecycle(preview!);
_previewIsPaused = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the mistake I made in maps: you're making a field update asynchronously instead of synchronously. You probably want to reverse the statement order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay yes I'll reverse the order!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was also the case in resumePreview, so I modified it there, too.

}

Expand Down Expand Up @@ -507,7 +507,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.
recording!.close();
unawaited(recording!.close());
recording = null;
pendingRecording = null;
throw CameraException(
Expand All @@ -516,7 +516,7 @@ class AndroidCameraCameraX extends CameraPlatform {
'while reporting success. The platform should always '
'return a valid path or report an error.');
}
recording!.close();
unawaited(recording!.close());
recording = null;
pendingRecording = null;
return XFile(videoOutputPath!);
Expand All @@ -526,15 +526,15 @@ class AndroidCameraCameraX extends CameraPlatform {
@override
Future<void> pauseVideoRecording(int cameraId) async {
if (recording != null) {
recording!.pause();
await recording!.pause();
}
}

/// Resume the current video recording if it is not null.
@override
Future<void> resumeVideoRecording(int cameraId) async {
if (recording != null) {
recording!.resume();
await recording!.resume();
}
}

Expand Down Expand Up @@ -614,7 +614,7 @@ class AndroidCameraCameraX extends CameraPlatform {
width: imageProxy.width);

weakThis.target!.cameraImageDataStreamController!.add(cameraImageData);
imageProxy.close();
unawaited(imageProxy.close());
}

// shouldCreateDetachedObjectForTesting is used to create an Analyzer
Expand All @@ -627,7 +627,7 @@ class AndroidCameraCameraX extends CameraPlatform {
// TODO(camsim99): Support resolution configuration.
// Defaults to YUV_420_888 image format.
imageAnalysis = createImageAnalysis(null);
imageAnalysis!.setAnalyzer(analyzer);
unawaited(imageAnalysis!.setAnalyzer(analyzer));

// TODO(camsim99): Reset live camera state observers here when
// https://github.com/flutter/packages/pull/3419 lands.
Expand All @@ -652,7 +652,7 @@ class AndroidCameraCameraX extends CameraPlatform {
/// The [onListen] callback for the stream controller used for image
/// streaming.
Future<void> _onFrameStreamListen() async {
_configureAndBindImageAnalysisToLifecycle();
await _configureAndBindImageAnalysisToLifecycle();
}

/// The [onCancel] callback for the stream controller used for image
Expand All @@ -661,7 +661,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 {
imageAnalysis!.clearAnalyzer();
unawaited(imageAnalysis!.clearAnalyzer());
}

/// Converts between Android ImageFormat constants and [ImageFormatGroup]s.
Expand All @@ -687,7 +687,7 @@ class AndroidCameraCameraX extends CameraPlatform {
/// removed, as well.
Future<void> _updateLiveCameraState(int cameraId) async {
final CameraInfo cameraInfo = await camera!.getCameraInfo();
liveCameraState?.removeObservers();
await liveCameraState?.removeObservers();
liveCameraState = await cameraInfo.getCameraState();
await liveCameraState!.observe(_createCameraClosingObserver(cameraId));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class ImageCaptureHostApiImpl extends ImageCaptureHostApi {
assert(identifier != null,
'No ImageCapture has the identifer of that requested to get the resolution information for.');

setFlashMode(identifier!, flashMode);
await setFlashMode(identifier!, flashMode);
}

/// Takes a picture with the specified [ImageCapture] instance.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,22 +67,22 @@ class RecordingHostApiImpl extends RecordingHostApi {

/// Closes the specified recording instance.
Future<void> closeFromInstance(Recording recording) async {
close(instanceManager.getIdentifier(recording)!);
await close(instanceManager.getIdentifier(recording)!);
}

/// Pauses the specified recording instance if active.
Future<void> pauseFromInstance(Recording recording) async {
pause(instanceManager.getIdentifier(recording)!);
await pause(instanceManager.getIdentifier(recording)!);
}

/// Resumes the specified recording instance if paused.
Future<void> resumeFromInstance(Recording recording) async {
resume(instanceManager.getIdentifier(recording)!);
await resume(instanceManager.getIdentifier(recording)!);
}

/// Stops the specified recording instance, as if calling closeFromInstance().
Future<void> stopFromInstance(Recording recording) async {
stop(instanceManager.getIdentifier(recording)!);
await stop(instanceManager.getIdentifier(recording)!);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera_android_camerax/pubspec.yaml
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+7
version: 0.5.0+8

environment:
sdk: ">=2.19.0 <4.0.0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ void main() {
camera.liveCameraState = MockLiveCameraState();
camera.imageAnalysis = MockImageAnalysis();

camera.dispose(3);
await camera.dispose(3);

verify(camera.preview!.releaseFlutterSurfaceTexture());
verify(camera.liveCameraState!.removeObservers());
Expand Down Expand Up @@ -689,7 +689,7 @@ void main() {
final AndroidCameraCameraX camera = AndroidCameraCameraX();
final MockRecording recording = MockRecording();
camera.recording = recording;
camera.pauseVideoRecording(0);
await camera.pauseVideoRecording(0);
verify(recording.pause());
verifyNoMoreInteractions(recording);
});
Expand All @@ -698,7 +698,7 @@ void main() {
final AndroidCameraCameraX camera = AndroidCameraCameraX();
final MockRecording recording = MockRecording();
camera.recording = recording;
camera.resumeVideoRecording(0);
await camera.resumeVideoRecording(0);
verify(recording.resume());
verifyNoMoreInteractions(recording);
});
Expand Down Expand Up @@ -974,7 +974,7 @@ void main() {
// Verify camera and cameraInfo were properly updated.
expect(camera.camera, equals(mockCamera));
expect(camera.cameraInfo, equals(mockCameraInfo));
onStreamedFrameAvailableSubscription.cancel();
await onStreamedFrameAvailableSubscription.cancel();
});

test(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ void main() {
instanceManager.addHostCreatedInstance(recording, recordingId,
onCopy: (_) => Recording.detached(instanceManager: instanceManager));

recording.close();
await recording.close();

verify(mockApi.close(recordingId));
});
Expand All @@ -57,7 +57,7 @@ void main() {
instanceManager.addHostCreatedInstance(recording, recordingId,
onCopy: (_) => Recording.detached(instanceManager: instanceManager));

recording.pause();
await recording.pause();

verify(mockApi.pause(recordingId));
});
Expand All @@ -77,7 +77,7 @@ void main() {
instanceManager.addHostCreatedInstance(recording, recordingId,
onCopy: (_) => Recording.detached(instanceManager: instanceManager));

recording.resume();
await recording.resume();

verify(mockApi.resume(recordingId));
});
Expand All @@ -97,7 +97,7 @@ void main() {
instanceManager.addHostCreatedInstance(recording, recordingId,
onCopy: (_) => Recording.detached(instanceManager: instanceManager));

recording.stop();
await recording.stop();

verify(mockApi.stop(recordingId));
});
Expand Down
1 change: 0 additions & 1 deletion script/configs/custom_analysis.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

# Temporary opt-outs of unawaited_futures; see
# https://github.com/flutter/flutter/issues/127323
- camera/camera_android_camerax
- webview_flutter/webview_flutter
- webview_flutter/webview_flutter_android
- webview_flutter/webview_flutter_wkwebview
Expand Down
Loading