Skip to content

[camera_android_camerax] Fix video recording after backgrounding app#12145

Draft
camsim99 wants to merge 10 commits into
flutter:mainfrom
camsim99:cos_background
Draft

[camera_android_camerax] Fix video recording after backgrounding app#12145
camsim99 wants to merge 10 commits into
flutter:mainfrom
camsim99:cos_background

Conversation

@camsim99

@camsim99 camsim99 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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

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-assist bot 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

  1. 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

Comment thread packages/camera/camera_android_camerax/implementation_plan.md
dart run ../../../script/tool/bin/flutter_plugin_tools.dart integration-test --android --packages=camera_android_camerax
```

### Manual Verification

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we do this via an espresso test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea! Yes, definitely.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well wait a quick search makes it seem like we would need UiAutomator to get real camera recordings. We could mock it but I'll have to look into what that would look like.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread packages/camera/camera_android_camerax/implementation_plan.md
});
```

## Verification Plan

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That works for me

```

### Manual Verification
1. Run the example app (`example/lib/main.dart`) on an Android device.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 flutter build apk).

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#12066 is my actual proposal.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remember to remove this agents.md file from this pr.

Comment thread packages/camera/camera_android_camerax/implementation_plan.md
Comment thread packages/camera/camera_android_camerax/implementation_plan.md
dart run ../../../script/tool/bin/flutter_plugin_tools.dart integration-test --android --packages=camera_android_camerax
```

### Manual Verification

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

@reidbaker reidbaker Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it is expected to be done by the reviewer we should call the section "reviewer verification" and it should be required to be part of the pull request.

In my experience "manual verification" were steps that did not produce artifacts like automated tests that were done to ensure the code was correct and they were only done once near the end but not after all final edits.

Comment thread packages/camera/camera_android_camerax/implementation_plan.md Outdated
Comment thread packages/camera/camera_android_camerax/implementation_plan.md
```

#### [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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW It feels like a pattern that information about camera is in a bunch of variables that need their state cleared. Is there a better pattern for this that we can use. Maybe a CameraState object that we can call dispose on that handles all its own variables and makes the lifecycle more clear.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is a great suggestion, but I consider this out of scope for solving the video recording issue (I think it would be a medium to large scale change). Filed flutter/flutter#189599 so we keep that work on our radar.

Comment thread packages/camera/camera_android_camerax/implementation_plan.md
Comment thread packages/camera/camera_android_camerax/implementation_plan.md Outdated
Comment thread packages/camera/camera_android_camerax/implementation_plan.md Outdated
});
```

## Verification Plan

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 and Dart logic:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I dont think this is true but we can leave it alone for this pr if you want. If you want to make this agent run an integration test lets give it an emulator to launch and work together on how to document what emulators are expected and how to configure them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I say let's leave it alone for now. Before I kick off execution, I will give one more round of feedback to address https://github.com/flutter/packages/pull/12145/changes#r3546039722 and add the pre push skill explicitly into the verification plan + tell the agent that an emulator will be running on my device so it can validate the integration tests on its own. Let me know what you think!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline and decided to let this be for now.

@camsim99 camsim99 added the CICD Run CI/CD label Jul 21, 2026
@flutter-dashboard

Copy link
Copy Markdown

This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled.

@camsim99 camsim99 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The changes are pretty straightforward so this looks good at a high level to me! Mostly left nits and random thoughts.

Comment on lines +524 to +526
recording = null;
pendingRecording = null;
videoOutputPath = null;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A comment with some context would be nice.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@@ -1,3 +1,7 @@
## 0.7.4+2

* Fix NullPointerException when disposing camera during active video recording.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nit:

Suggested change
* Fix NullPointerException when disposing camera during active video recording.
* Fix `NullPointerException` when disposing camera during active video recording.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

deviceOrientationManager.stopListeningForDeviceOrientationChange(),
]);

recording = null;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 dispose is called and the app is resumed, an exception will be thrown (source).

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.

@camsim99
camsim99 requested a review from reidbaker July 21, 2026 21:51
@camsim99 camsim99 added CICD Run CI/CD and removed CICD Run CI/CD labels Jul 21, 2026

@reidbaker reidbaker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code looks good. Pr description needs to be updated and the plan deleted for me to approve.

I would like for the final version of the plan to be linked in the final pr description before the pr is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants