-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[camera_android_camerax] Fix video recording after backgrounding app #12145
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
base: main
Are you sure you want to change the base?
Changes from all commits
8c98295
7874903
8e6b053
2fa34ae
93949c9
e5f6c95
8d928b7
c82b347
a435216
c23ac8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reviewers: This is the logs from the agent that I got when "proceeding" with the implementation plan in implementation_plan.md.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing in here looks particularly concerning to me. We never specified anywhere in the harness to use test driven development, so the agent writing tests after making changes is not surprising, but we could change that in the future. Might be helpful for more complex bugs. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| Viewed android_camera_camerax.dart:540-580 | ||
| Searched for "dispose(int cameraId)" | ||
| Viewed android_camera_camerax.dart:510-525 | ||
| Edited android_camera_camerax.dart | ||
| Searched for "dispose releases Flutter surface texture" | ||
| Viewed android_camera_camerax_test.dart:1910-1945 | ||
| Edited android_camera_camerax_test.dart | ||
| Searched for "video recording" | ||
| Viewed integration_test.dart:200-247 | ||
| Edited integration_test.dart | ||
| Ran command: `dart run ../../../script/tool/bin/flutter_plugin_tools.dart dart-test --packages=camera_android_camerax` | ||
|
|
||
| I've implemented the changes according to our plan: | ||
| 1. Updated `dispose` in `android_camera_camerax.dart` to use `Future.wait` and clear the `recording`, `pendingRecording`, and `videoOutputPath`. | ||
| 2. Updated the `dispose` test in `android_camera_camerax_test.dart` to verify these states are nullified. | ||
| 3. Added the integration test to `example/integration_test/integration_test.dart` that tests recording after `dispose`. | ||
|
|
||
| I am currently running the Dart unit tests in the background to ensure we haven't broken any existing behavior. I'll continue with the pre-push formatting, analysis, and publish checks as soon as the tests pass. | ||
|
|
||
| Ran command: `dart run ../../../script/tool/bin/flutter_plugin_tools.dart format --packages=camera_android_camerax && dart run ../../../script/tool/bin/flutter_plugin_tools.dart analyze --packages=camera_android_camerax && dart run ../../../script/tool/bin/flutter_plugin_tools.dart publish-check --packages=camera_android_camerax` | ||
|
|
||
| The Dart unit tests have successfully passed! | ||
|
|
||
| I've now kicked off the pre-push checks (`format`, `analyze`, and `publish-check`). Once those finish, I will automatically bump the version and update the `CHANGELOG.md` using the repository's `update-release-info` tool. I'll let you know as soon as everything is complete. | ||
|
|
||
| Viewed integration_test.dart:245-282 | ||
| Edited integration_test.dart | ||
| Ran command: `dart run ../../../script/tool/bin/flutter_plugin_tools.dart format --packages=camera_android_camerax && dart run ../../../script/tool/bin/flutter_plugin_tools.dart analyze --packages=camera_android_camerax && dart run ../../../script/tool/bin/flutter_plugin_tools.dart publish-check --packages=camera_android_camerax` | ||
|
|
||
| I found a few minor analyzer errors related to my integration test: I passed `ResolutionPreset.low` as a positional argument instead of a named argument, and I didn't omit some obvious type annotations as the linter prefers. | ||
|
|
||
| I've fixed those and re-triggered the pre-push checks! | ||
|
|
||
| Searched for "class CameraController" | ||
| Viewed camera_controller.dart:228-250 | ||
| Edited integration_test.dart | ||
| Ran command: `dart run ../../../script/tool/bin/flutter_plugin_tools.dart format --packages=camera_android_camerax && dart run ../../../script/tool/bin/flutter_plugin_tools.dart analyze --packages=camera_android_camerax && dart run ../../../script/tool/bin/flutter_plugin_tools.dart publish-check --packages=camera_android_camerax` | ||
|
|
||
| I realized the `CameraController` API in the example app requires a `MediaSettings` object rather than just the raw `ResolutionPreset`. I've updated the test code to match the API and re-started the pre-push checks! | ||
|
|
||
| Ran command: `git add example/integration_test/integration_test.dart lib/src/android_camera_camerax.dart test/android_camera_camerax_test.dart && git commit -m "Fix NPE on backgrounding during active video recording"` | ||
| Ran command: `dart run ../../../script/tool/bin/flutter_plugin_tools.dart update-release-info --version=minimal --base-branch=upstream/main --changelog="Fix NullPointerException when disposing camera during active video recording."` | ||
| Ran command: `git add CHANGELOG.md pubspec.yaml && git commit -m "Bump version and update CHANGELOG for NPE fix during video recording"` | ||
| Ran command: `dart run ../../../script/tool/bin/flutter_plugin_tools.dart publish-check --packages=camera_android_camerax` | ||
|
|
||
| All pre-push checks, including the `publish-check`, have now passed successfully! | ||
|
|
||
| I have automatically run `update-release-info` to bump the version and update the `CHANGELOG.md`, and then committed everything to your local branch `cos_background`. | ||
|
|
||
| The git working tree is now clean, and you are fully ready to push this branch and open your Pull Request! Remember to include the `Reviewer Verification` instructions from our plan in the PR description so the reviewer knows how to manually verify the integration flow. | ||
|
|
||
| Let me know if there's anything else you need! |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| ## Fix NullPointerException on backgrounding during active video recording | ||
| The `NullPointerException` occurs because the Flutter-side state representing an active recording becomes desynced from the Native CameraX state when the app goes into the background. | ||
|
|
||
| When an app using the `camera` plugin is backgrounded, the `CameraController` automatically calls `dispose()` to tear down the camera resources. This invokes `processCameraProvider?.unbindAll()` in the `camera_android_camerax` plugin. | ||
|
|
||
| **Native Cleanup Clarity:** | ||
| When `processCameraProvider?.unbindAll()` is called natively, CameraX unbinds the `VideoCapture` use case. This action inherently stops any active recording on the native side. CameraX gracefully finalizes the recording and saves the video file to the disk without corrupting it. No hanging `Recording` instances are left behind natively. | ||
|
|
||
| However, the `dispose` method on the Dart side does not clear the `recording` and `pendingRecording` objects. When the app is resumed, the singleton `AndroidCameraCameraX` still thinks the previous recording is active (`recording != null`). When the user tries to start a new recording, `startVideoCapturing` returns silently. When they click the "Stop" button, `stopVideoRecording` attempts to stop the old recording by calling `await recording!.close()`. Since the Native CameraX `Recorder` was already finalized, this throws a `java.lang.NullPointerException`. | ||
|
camsim99 marked this conversation as resolved.
|
||
|
|
||
| ## User Review Required | ||
| No major architectural shifts or breaking changes are introduced. This is a straightforward bug fix to clean up internal state during teardown. | ||
|
|
||
| ## Open Questions | ||
| None. | ||
|
|
||
| --- | ||
|
|
||
| ## Proposed Changes | ||
|
|
||
| ### camera_android_camerax package | ||
| We will update `dispose()` to clean up the video recording state so it correctly aligns with native behavior upon teardown. We will also optimize the cleanup by running the tear down methods concurrently. | ||
|
|
||
| **Why clear state in `dispose()` vs listening to Native events?** | ||
| The native CameraX library *does* emit a `VideoRecordEvent.Finalize` event when the recording is stopped natively (e.g. by `dispose` unbinding the use cases). However, proactively nullifying the `recording` state in the event listener would introduce a race condition with the `stopVideoRecording` method, which actively awaits the `Finalize` event to return the video file path. Because `dispose()` explicitly triggers the `unbindAll()` action that forcefully finalizes the recording, clearing the state directly inside `dispose()` correctly mirrors the native teardown without requiring a complex refactor of the existing event queues and state management. | ||
|
|
||
| #### [MODIFY] android_camera_camerax.dart | ||
| Add state cleanup for `recording`, `pendingRecording`, and `videoOutputPath` in the `dispose` method, and execute the teardown futures concurrently using `Future.wait`. | ||
|
|
||
| ```diff | ||
| /// Releases the resources of the accessed camera with ID [cameraId]. | ||
| @override | ||
| Future<void> dispose(int cameraId) async { | ||
|
camsim99 marked this conversation as resolved.
|
||
| - await preview?.releaseSurfaceProvider(); | ||
| - await liveCameraState?.removeObservers(); | ||
| - await processCameraProvider?.unbindAll(); | ||
| - await imageAnalysis?.clearAnalyzer(); | ||
| - await deviceOrientationManager.stopListeningForDeviceOrientationChange(); | ||
| + await Future.wait(<Future<void>>[ | ||
| + if (preview != null) preview!.releaseSurfaceProvider(), | ||
| + if (liveCameraState != null) liveCameraState!.removeObservers(), | ||
| + if (processCameraProvider != null) processCameraProvider!.unbindAll(), | ||
| + if (imageAnalysis != null) imageAnalysis!.clearAnalyzer(), | ||
| + deviceOrientationManager.stopListeningForDeviceOrientationChange(), | ||
| + ]); | ||
| + | ||
| + recording = null; | ||
| + pendingRecording = null; | ||
| + videoOutputPath = null; | ||
|
camsim99 marked this conversation as resolved.
reidbaker marked this conversation as resolved.
|
||
| } | ||
| ``` | ||
| *(Note: Because of recent changes in your local branch, I will ensure it matches the current state of `dispose()`, e.g., using `preview?.setSurfaceProvider(null)` if that replaced `releaseSurfaceProvider`.)* | ||
|
|
||
| #### [MODIFY] android_camera_camerax_test.dart | ||
| Add assertions in the `dispose` test to ensure that the recording state variables are properly nullified. Since this is a Dart unit test using mock objects (and no real files are written), we cannot verify the video was saved to disk here. We will verify that in the integration test. | ||
|
|
||
| ```diff | ||
| test( | ||
| 'dispose releases Flutter surface texture, removes camera state observers, and unbinds all use cases', | ||
| () async { | ||
| + // Setup mock recording state | ||
| + camera.recording = MockRecording(); | ||
| + camera.pendingRecording = MockPendingRecording(); | ||
| + camera.videoOutputPath = 'test/path.mp4'; | ||
| + | ||
| await camera.dispose(3); | ||
|
|
||
| verify(mockPreview.releaseSurfaceProvider()); | ||
| verify(mockLiveCameraState.removeObservers()); | ||
| verify(mockProcessCameraProvider.unbindAll()); | ||
| verify(mockImageAnalysis.clearAnalyzer()); | ||
| verify(mockDeviceOrientationManager | ||
| .stopListeningForDeviceOrientationChange()); | ||
| + | ||
| + // Verify state is cleared | ||
|
camsim99 marked this conversation as resolved.
|
||
| + expect(camera.recording, isNull); | ||
| + expect(camera.pendingRecording, isNull); | ||
| + expect(camera.videoOutputPath, isNull); | ||
| }); | ||
| ``` | ||
|
|
||
| #### [MODIFY] example/integration_test/integration_test.dart | ||
| Add a Flutter integration test mimicking the app lifecycle pause/resume while recording. This test *will* verify that the video is successfully saved to disk and retrievable. | ||
|
|
||
| ```dart | ||
| testWidgets( | ||
| 'video recording state is cleared after camera is disposed', | ||
| (WidgetTester tester) async { | ||
| final CameraController cameraController = CameraController( | ||
| cameras[0], | ||
| ResolutionPreset.low, | ||
| ); | ||
| await cameraController.initialize(); | ||
| await cameraController.startVideoRecording(); | ||
|
|
||
| // Dispose the controller, which simulates what the example app does | ||
| // when the AppLifecycleState becomes inactive (e.g. backgrounding). | ||
| await cameraController.dispose(); | ||
|
|
||
| // Create a new controller (simulating app resume) | ||
| final CameraController newController = CameraController( | ||
| cameras[0], | ||
| ResolutionPreset.low, | ||
| ); | ||
| await newController.initialize(); | ||
|
|
||
| // Attempt to start a new recording. This should not throw or silently fail. | ||
| await newController.startVideoRecording(); | ||
|
|
||
| // Stop it, ensuring no NPE is thrown by the native side and the file is valid. | ||
| final XFile file = await newController.stopVideoRecording(); | ||
| expect(file, isNotNull); | ||
|
|
||
| // Ensure the video was saved correctly | ||
| final File videoFile = File(file.path); | ||
| expect(videoFile.existsSync(), isTrue); | ||
| expect(videoFile.lengthSync(), greaterThan(0)); | ||
|
|
||
| await newController.dispose(); | ||
| }); | ||
| ``` | ||
|
camsim99 marked this conversation as resolved.
|
||
|
|
||
| ## Verification Plan | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In our first plan, we had it explicitly list out the steps it would take to make sure that the code is high quality. I'm ok with that not being explicitly included because it will be in our AGENTS.md and the skills, but just noting in case other reviewers do not agree.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for now I think the verification steps should be listed especially if there are no artifacts of that verification. For a human, this would be something like, I ran it on a pixel device and swapped between front facing and rear facing camera and it the example app did not crash.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somehow I missed this comment and will include in on the next iteration. But question: thoughts on the step just be to run the pre-push skill? Most of those steps are part of the skill (the only one not included is the gradle check which honestly would be a good addition).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That works for me |
||
| Verification ensures the app gracefully handles backgrounding during recordings and cleanly starts new recordings upon resume. | ||
|
|
||
| ### Automated Tests (What I can do autonomously) | ||
| As an AI agent, I am unable to launch a local Android Emulator or connect to a physical Android device, meaning I cannot run integration tests (`integration-test`). I will run the following commands to verify compilation, Dart logic, and PR readiness: | ||
|
|
||
| ```bash | ||
| # Verify the example APK builds successfully using the flutter tool | ||
| cd example && flutter build apk | ||
|
|
||
| # Run unit tests to verify the Dart logic works as expected | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart dart-test --packages=camera_android_camerax | ||
|
|
||
| # Verify PR readiness using the pre-push-skill | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart format --packages=camera_android_camerax | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart analyze --packages=camera_android_camerax | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart publish-check --packages=camera_android_camerax | ||
| ``` | ||
| *(I will leverage the repository's `.agents/skills/pre-push-skill/SKILL.md` to ensure all checks pass before claiming the work is complete.)* | ||
|
|
||
| ### Reviewer Verification | ||
| *Note: This must be explicitly requested in the pull request description.* | ||
|
|
||
| Because I do not have access to an emulator or physical device, a human reviewer is needed to run the integration tests and verify the UI on a device. | ||
|
|
||
| 1. Run the integration test on an attached device: | ||
| ```bash | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart integration-test --android --packages=camera_android_camerax | ||
| ``` | ||
| 2. Run the example app (`example/lib/main.dart`) on an Android device. | ||
| 3. Click the video camera icon to start recording a new video. | ||
| 4. Background the app (e.g., navigate to the home screen). | ||
| 5. Resume the app. | ||
| 6. Click the video camera icon to start recording a new video. | ||
| 7. Click the stop icon to stop recording. | ||
| 8. Verify the app does not crash, the recording preview is displayed, and the second video is successfully saved to the device. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -513,11 +513,17 @@ class AndroidCameraCameraX extends CameraPlatform { | |
| /// Releases the resources of the accessed camera with ID [cameraId]. | ||
| @override | ||
| Future<void> dispose(int cameraId) async { | ||
| await preview?.releaseSurfaceProvider(); | ||
| await liveCameraState?.removeObservers(); | ||
| await processCameraProvider?.unbindAll(); | ||
| await imageAnalysis?.clearAnalyzer(); | ||
| await deviceOrientationManager.stopListeningForDeviceOrientationChange(); | ||
| await Future.wait(<Future<void>>[ | ||
| if (preview != null) preview!.releaseSurfaceProvider(), | ||
| if (liveCameraState != null) liveCameraState!.removeObservers(), | ||
| if (processCameraProvider != null) processCameraProvider!.unbindAll(), | ||
| if (imageAnalysis != null) imageAnalysis!.clearAnalyzer(), | ||
| deviceOrientationManager.stopListeningForDeviceOrientationChange(), | ||
| ]); | ||
|
|
||
| recording = null; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing I probably should have pointed out before...if the developer attempts to stop the recording after For the record, I think that is fine; I think this exception clearly describes what happens and makes it easier for developers to handle vs still trying to attempt to stop a non-existent recording. Just wanted to point out the impact of this for reviewers. |
||
| pendingRecording = null; | ||
| videoOutputPath = null; | ||
|
Comment on lines
+524
to
+526
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A comment with some context would be nice.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
| } | ||
|
|
||
| /// The camera with ID [cameraId] has been initialized. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#12277
flutter/flutter#189922