[camera_android_camerax] Fix video recording after backgrounding app#12145
[camera_android_camerax] Fix video recording after backgrounding app#12145camsim99 wants to merge 2 commits into
Conversation
| ```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. | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
I would prefer to see sample code here versus plain English.
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart integration-test --android --packages=camera_android_camerax | ||
| ``` | ||
|
|
||
| ### Manual Verification |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Could we do this via an espresso test?
| ```diff | ||
| /// Releases the resources of the accessed camera with ID [cameraId]. | ||
| @override | ||
| Future<void> dispose(int cameraId) async { |
There was a problem hiding this comment.
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.
| }); | ||
| ``` | ||
|
|
||
| ## Verification Plan |
There was a problem hiding this comment.
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.
| ``` | ||
|
|
||
| ### Manual Verification | ||
| 1. Run the example app (`example/lib/main.dart`) on an Android device. |
There was a problem hiding this comment.
At the very least it can run the example app and see if it builds (or at least try a flutter build apk).
There was a problem hiding this comment.
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.
|
|
||
| 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`. |
There was a problem hiding this comment.
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 recording started before backgrounding the app gets closed/taken care of automatically, but just want to make sure there's no native Recording instances, corrupted files, etc. lying around. The plan should clarify that.
| await deviceOrientationManager.stopListeningForDeviceOrientationChange(); | ||
| + recording = null; | ||
| + pendingRecording = null; | ||
| + videoOutputPath = null; |
There was a problem hiding this comment.
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.
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart integration-test --android --packages=camera_android_camerax | ||
| ``` | ||
|
|
||
| ### Manual Verification |
There was a problem hiding this comment.
Could we do this via an espresso test?
| dart run ../../../script/tool/bin/flutter_plugin_tools.dart integration-test --android --packages=camera_android_camerax | ||
| ``` | ||
|
|
||
| ### Manual Verification |
There was a problem hiding this comment.
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?
2 participants
WIP towards fixing flutter/flutter#183880.
As of now, reviewers focus on issues
implemenation_plan.md.Normal pull request information above the line.
This pull request is an attempt to "oneshot" fixing flutter/flutter#183880. Using antigravity to execute a plan that is collaboratively worked on. The "rules" are to not allow human authored code to camera_android_camerax and to not rely on human code review feedback for code iteration.
We can add documentation, skills, mcp servers, presubmit test etc. Basically any "support infrastructure" for development is allowed but the package changes must be generated as a single pass.
When we discover that the plan is not good enough we will blow the package changes away and either modify the plan or add more support infrastructure and try again.
For more information see the working doc in go/flutter-project-one-shot
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2