[camera] Fix for CameraAccessException that prevents image capture on certain devices running Android 7/8

[carman247]

Fix for flutter/flutter#88775 CameraAccessException that affects certain devices running Android 7/8

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[carman247]

Apologies if anything is incorrect/incomplete but this is my first PR.

It was suggested I create one for this bug.

Any help would be greatly appreciated as although the changes I've made are minimal I've never done this before.

[stuartmorgan-g]

Apologies if anything is incorrect/incomplete but this is my first PR.

Thanks for the contribution! It mostly looks good, except for the item in the checklist that you missed; please see the linked documents about testing requirements.

[stuartmorgan-g]

Why is this line duplicated instead of outside of the if/else?

[stuartmorgan-g]

There needs to be a comment explaining why this is conditional.

[subzero911]

I can confirm that this line causes troubles on some old Huawei/Xiaomi devices.

[jgoyvaerts]

Actually, I'm pretty sure the captureSession.abortCaptures(); can be removed completely. I was about to make a PR for this myself and stumbled upon this issue.

According to the documentation it should only be called before a new session is created, however, calling .capture() does not create a new session.

The .abortCaptures call also adds a significant delay between when the take picture button is pressed and when the shutter actually takes a picture (around 200ms on my Samsung S10), removing this line causes the delay of the shutter to dissappear.

I have not noticed any negative side effects from removing this line completely, git blame doesn't give any reason why it exists in the first place.

[carman247]

@jgoyvaerts Hi, so do you think the code can simply be captureSession.stopRepeating(); and exclude the if statement altogether?

I'm also noticing some issues on specific devices (Alcatel 1C, and Pixel 6 Pro so far) that don't seem to be able to capture at all ...

I'm wondering if it's something related to the .abortCaptures(); ?

Captures don't seem to complete, nor do they throw an error. And following attempts to capture get caught on:

if (_cameraController.value.isTakingPicture) {
  return;
}

[jgoyvaerts]

I'm just saying based on my tests, the .abortCaptures call

• Has no effect on being able to take a picture on my device, I can take a picture just fine when the call is removed
• Introduces an arbitrary delay of 150-200ms when taking a picture
• The documentation doesn't say why this should be called before taking a picture, only when you want to abort a picture capture process in progress (like a long shutter exposure or something).

Based on these reasons, I'm inclined to believe the line can be removed, especially if you have noticed it causes issues on certain Android versions.

Maybe there's an edge case we're missing why it needs to be there, but then that edge case should probably be fixed in a different way, since the happy flow of taking a picture should not have an unneccessary 150-200ms delay.

[carman247]

@jgoyvaerts Ok. I will test on the three different devices I have, and potentially test on some cloud devices as well.

Also I've just realised it can't be the cause of the issues on those devices I mentioned, as they are both higher versions than Android 8.0 (VERSION_CODES.O)

If I find no problems removing it then I'm tempted to test it out in the wild although certainly not the best way to test whether it's worked or not.

[jgoyvaerts]

@carman247 Can you just adjust the PR so it removes the line captureSession.abortCaptures(); completely?

[carman247]

@jgoyvaerts no problem, will get that updated in a moment.

[carman247]

Can anyone explain why the checks are failing? Not sure what I'm doing wrong there.

Also, I haven't added any new "tests" for this PR .. I'm assuming the current ones will still be fine.

[stuartmorgan-g]

Can anyone explain why the checks are failing? Not sure what I'm doing wrong there.

The repository keys are rotated periodically; you likely just need to merge in the latest changes from main.

Also, I haven't added any new "tests" for this PR .. I'm assuming the current ones will still be fine.

This PR doesn't meet any of the test exemption criteria (see the link in that checkbox item), so this needs tests. See also the comment from the bot above. If having this call breaks image capture on certain devices, then there need to be a test that the call isn't being made in this flow.

[jgoyvaerts]

This PR doesn't meet any of the test exemption criteria (see the link in that checkbox item), so this needs tests. See also the comment from the bot above. If having this call breaks image capture on certain devices, then there need to be a test that the call isn't being made in this flow.

The PR has been changed to completely delete the line, since it serves no purpose AND causes issues on certain devices.

I thought delete only PRs did not require tests?

[stuartmorgan-g]

I thought delete only PRs did not require tests?

I missed that it was delete-only now; my comment was based on the previous state.

That said, this should still have a test, for the reason described in the bot comment above: someone added this once, thinking it was a good idea. If someone does so again, do you want all of the currently affected devices to silently regress again?

[stuartmorgan-g]

@mvanbeusekom You added this line in #3383; do you remember why by any chance?

@carman247 Have you verified that this doesn't regress the fix in that PR?

[stuartmorgan-g]

On the testing side: I would expect it would be straightforward to add a test to https://github.com/flutter/plugins/blob/main/packages/camera/camera/android/src/test/java/io/flutter/plugins/camera/CameraTest.java that uses the mock capture session to ensure that this isn't being called.

I took a look at this in more detail, and was not as straightforward as I thought since CameraDevice is not easily mockable since it's partially abstract and has a private constructor. I inserted a wrapper that allows mocking it, and tested using that.

@blasten Please review closely, since I'm very out of practice with Java.

Tests added

[stuartmorgan-g]

@BeMacized Could you also take a look since you were in this code quite a bit?

[carman247]

@stuartmorgan thanks for sorting the test! Glad you tackled it as would've taken me ages to get in done and would probably be wrong anyway 😅

[blasten]

LGTM

[fluttergithubbot]

This pull request is not suitable for automatic merging in its current state.

• Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[stuartmorgan-g]

Explicitly approving the non-test parts (i.e., the parts I didn't write) for the bot.