-
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # Agent Guide for camera_android_camerax | ||
|
|
||
| This document provides context, architectural details, and core workflows for making high quality contributions to the `camera_android_camerax` package. | ||
|
|
||
| ## Architectural Overview | ||
|
|
||
| The `camera_android_camerax` package is the Android platform implementation of the `camera` plugin, using the **Android Jetpack CameraX** library. | ||
|
|
||
| ### ProxyApi Binding System | ||
| This plugin utilizes **Pigeon's ProxyApi** to bind Dart objects directly to native Android CameraX Java/Kotlin objects. | ||
| - **Pigeon Definition**: The API surfaces are defined in [pigeons/camerax_library.dart](pigeons/camerax_library.dart). | ||
| - **Generated Code**: | ||
| - Dart proxy classes: [lib/src/camerax_library.g.dart](lib/src/camerax_library.g.dart) | ||
| - Kotlin proxy classes: [android/src/main/java/io/flutter/plugins/camerax/CameraXLibrary.g.kt](android/src/main/java/io/flutter/plugins/camerax/CameraXLibrary.g.kt) | ||
| - **Plugin Entry Point**: [lib/src/android_camera_camerax.dart](lib/src/android_camera_camerax.dart) implements `CameraPlatform` using the generated proxy classes. | ||
|
|
||
| ## Required Steps Before Making Any Changes | ||
|
|
||
| Before making any changes, run the the [check-readiness skill](.agents/skills/check-readiness/SKILL.md) to ensure your environment has all the required tooling. | ||
|
|
||
| ## Required Steps After Making Any Changes | ||
|
|
||
| 1. **Regenerate Code (if applicable)**: | ||
| - If you modified [pigeons/camerax_library.dart](pigeons/camerax_library.dart), run the Pigeon generation command under [Code Generation](#code-generation). | ||
| - If you modified any classes that are mocked or added new mocks, run the Mockito generation command under [Code Generation](#code-generation). | ||
| 2. **Verify Tests**: | ||
| - Ensure you have added or updated unit tests to cover your changes. | ||
| - Run and pass all tests (see [Running Tests](#running-tests) for details): | ||
| - Dart unit tests. | ||
| - Android native unit tests. | ||
| - Integration tests. | ||
| 3. **Static Analysis**: | ||
| - Do not rely on ad-hoc or generic commands to check for formatting or lint errors. Always use the [dart-run-static-analysis skill](.agents/skills/dart-run-static-analysis/SKILL.md) during development to analyze the code and apply automated fixes. | ||
|
|
||
| ## Code Generation | ||
|
|
||
| When you modify the Pigeon API [pigeons/camerax_library.dart](pigeons/camerax_library.dart), you must regenerate the respective code to update the Dart proxy class [lib/src/camerax_library.g.dart](lib/src/camerax_library.g.dart) and the Kotlin proxy class [android/src/main/java/io/flutter/plugins/camerax/CameraXLibrary.g.kt](android/src/main/java/io/flutter/plugins/camerax/CameraXLibrary.g.kt). Run from this directory: | ||
|
|
||
| ```bash | ||
| dart run pigeon --input pigeons/camerax_library.dart | ||
| ``` | ||
|
|
||
| Because test in this package use `mockito` for mocking, you must also regenerate the Mockito mocks that are used for unit testing any changes you make. So, also run from this directory: | ||
|
|
||
| ```bash | ||
| dart run build_runner build -d | ||
| ``` | ||
|
|
||
| ## Running Tests | ||
|
|
||
| When you make a change, add a test if you can (either a Dart unit test, Android native unit test, or Flutter integration tests). Regardless of if tests are added or not, all tests must pass after you make changes. How to run the tests: | ||
|
|
||
| ### Dart Unit Tests | ||
| Dart unit tests are located in [test/](./test/). | ||
| To run them from this directory: | ||
| ```bash | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart dart-test --packages=camera_android_camerax | ||
| ``` | ||
|
|
||
| ### Android Native Unit Tests | ||
| Android unit tests are located in [android/src/test/](./android/src/test/) and run using Robolectric. | ||
| To run them from this directory: | ||
| ```bash | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart native-test --android --packages=camera_android_camerax | ||
| ``` | ||
|
|
||
| ### Flutter Integration Tests | ||
| Integration tests are located in [example/integration_test/integration_test.dart](./example/integration_test/integration_test.dart). | ||
| With an emulator or physical device connected, run from this directory: | ||
| ```bash | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart integration-test --android --packages=camera_android_camerax | ||
| ``` | ||
|
|
||
| ## Required Steps Before Pushing | ||
|
|
||
| You MUST read and follow the [pre-push skill](.agents/skills/pre-push-skill/SKILL.md) immediately whenever: | ||
| - The user asks to push changes. | ||
| - The user asks if you or they are ready to push. | ||
| - The user wants to validate that local changes are ready to become a pull request. | ||
| - It is the final step when you are ready to make a PR and consider an issue solved. | ||
|
|
||
| This skill will execute all the required pre-push checks (e.g., tests, publish checks, license formatting) for the `flutter/packages` repository. | ||
|
|
||
| ## Agent Coding and Review Guidelines | ||
|
|
||
| - **Communication and Code Review**: When receiving code review feedback, DO NOT be overly accommodating or blindly agree with the user if the feedback seems technically questionable. Instead, use the [receiving-code-review skill](.agents/skills/receiving-code-review/SKILL.md) to apply technical rigor and verify the suggestions. Be direct if you believe the feedback is incorrect. | ||
| - **Code Quality and Complexity**: Do not produce low-quality or overly complex code. For complex features, propose using the `/grill-me` or `/plan` slash commands to create a design plan before writing code. Enforce strict minimum test coverage and cognitive complexity standards. Use the [dart-add-unit-test skill](.agents/skills/dart-add-unit-test/SKILL.md) and [dart-collect-coverage skill](.agents/skills/dart-collect-coverage/SKILL.md) to ensure high coverage. | ||
| - **Duplicate Code**: Avoid duplicating code, especially constant strings. Instead of duplicating, look for existing patterns in adjacent code and extract shared values into constants. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| ## 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. Unbinding the `VideoCapture` use case natively finalizes any ongoing video recording. | ||
|
|
||
| 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`. | ||
|
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'm not clear on is whether or not this leaves room for objects to not be cleaned up as expected. This explanation suggests that the |
||
|
|
||
| ## 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. The root cause and fix are well understood. | ||
|
|
||
| --- | ||
|
|
||
| ## 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'll also add a unit test and an integration test to prevent regressions. | ||
|
|
||
| #### [MODIFY] android_camera_camerax.dart | ||
| Add state cleanup for `recording`, `pendingRecording`, and `videoOutputPath` in the `dispose` method. | ||
|
|
||
| ```diff | ||
| /// Releases the resources of the accessed camera with ID [cameraId]. | ||
| @override | ||
| Future<void> dispose(int cameraId) async { | ||
|
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. Does dispose get called any other time that would make this fix incorrect? I think this plan should be clear about that and add tests as necessary to ensure that is no the case, even if it is a manual verification step. |
||
| await preview?.releaseSurfaceProvider(); | ||
| await liveCameraState?.removeObservers(); | ||
| await processCameraProvider?.unbindAll(); | ||
| await imageAnalysis?.clearAnalyzer(); | ||
| await deviceOrientationManager.stopListeningForDeviceOrientationChange(); | ||
| + recording = null; | ||
| + pendingRecording = null; | ||
| + videoOutputPath = null; | ||
|
Collaborator
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. I'm not familiar with the architecture of camera_android_camerax; is it expected that calling dispose for a specific camera instance clears a bunch of singleton state and never uses the camera ID? That seems surprising.
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. That is expected. Using CameraX, we are expected to manage the different camera use cases (image capture, video capture, etc.) vs managing the active camera(s) themselves, like Camera2 moreso expects. So, the plugin actually doesn't use the camera ID in a meaningful way besides to tell the users relevant information about the active camera via |
||
| } | ||
| ``` | ||
|
|
||
| #### [MODIFY] android_camera_camerax_test.dart | ||
| Add assertions in the `dispose` test to ensure that the recording state variables are properly nullified when `dispose()` completes. | ||
|
|
||
| ```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 | ||
| + expect(camera.recording, isNull); | ||
| + expect(camera.pendingRecording, isNull); | ||
| + expect(camera.videoOutputPath, isNull); | ||
| }); | ||
| ``` | ||
|
|
||
| #### [MODIFY] example/integration_test/integration_test.dart | ||
| Add a test mimicking the app lifecycle pause/resume while recording. | ||
|
|
||
| ```dart | ||
| testWidgets( | ||
| 'video recording state is cleared after camera is disposed (simulating backgrounding)', | ||
| (WidgetTester tester) async { | ||
| // 1. Initialize camera | ||
| // 2. Start video recording | ||
| // 3. Call cameraController.dispose() (this happens when app backgrounds) | ||
| // 4. Re-initialize a new cameraController (this happens when app resumes) | ||
| // 5. Start a new video recording | ||
| // 6. Stop the new video recording | ||
| // 7. Assert that no exceptions are thrown and the XFile is returned. | ||
| }); | ||
| ``` | ||
|
Comment on lines
+70
to
+82
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. I would prefer to see sample code here versus plain English. |
||
|
|
||
| ## 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. |
||
| Verification ensures the app gracefully handles backgrounding during recordings and cleanly starts new recordings upon resume. | ||
|
|
||
| ### Automated Tests | ||
| Run the following commands to ensure all tests pass: | ||
| ```bash | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart dart-test --packages=camera_android_camerax | ||
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart integration-test --android --packages=camera_android_camerax | ||
| ``` | ||
|
|
||
| ### Manual Verification | ||
|
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 an ideal world, it would know how to launch the emulator and test but I actually don't know if the agent would have any way of interacting with the emulator after that in a meaningful way (even if it were a more minimal app than the example).
Collaborator
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. Could we do this via an espresso test?
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. That's a great idea! Yes, definitely.
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. Well wait a quick search makes it seem like we would need
Collaborator
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. What does it mean for a one-shot plan to have a manual verification step? Does that mean that the reviewer is expected to read and carry out this section as part of the code review?
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. Yes. In both of our one-shot plan attempts, we did not ask for this section specifically, but I assume it's generated because the agent assumes it can't verify the solution itself. I do wonder if that's true, though. It can definitely launch an emulator and run the app, but I need to verify it could interact with it as we'd expect. |
||
| 1. Run the example app (`example/lib/main.dart`) on an Android device. | ||
|
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. At the very least it can run the example app and see if it builds (or at least try a |
||
| 2. Click the video camera icon to start recording a new video. | ||
| 3. Background the app (e.g., navigate to the home screen). | ||
| 4. Resume the app. | ||
| 5. Click the video camera icon to start recording a new video. | ||
| 6. Click the stop icon to stop recording. | ||
| 7. Verify the app does not crash, the recording preview is displayed, and the second video is successfully saved to the device. | ||
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.
This might not be the final AGENTS.md we use in the plugin, but included my draft for now in case some of the core information was useful for creating the implementation plan.