Skip to content
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

[camera] Permissions request are not well-awaiting on iOS #97199

Closed
AlexV525 opened this issue Jan 25, 2022 · 16 comments
Closed

[camera] Permissions request are not well-awaiting on iOS #97199

AlexV525 opened this issue Jan 25, 2022 · 16 comments
Assignees
Labels
c: regression It was better in the past than it is now has reproducible steps The issue has been confirmed reproducible and is ready to work on p: camera The camera plugin P1 High-priority issues at the top of the work list package flutter/packages repository. See also p: labels.

Comments

@AlexV525
Copy link
Member

AlexV525 commented Jan 25, 2022

Related to #96429. It's a serious bug that breaks lifecycle control flow on iOS. This issue could be introduced by flutter/plugins#4140.

Steps to Reproduce

  1. Run the example of the camera plugin like the first install (uninstall first if it's installed).
  2. Request for a camera instance.
  3. Exception is thrown.
======== Exception caught by widgets library =======================================================
The following assertion was thrown building CameraPreview:
A CameraController was used after being disposed.

Once you have called dispose() on a CameraController, it can no longer be used.

Details

From the screenshot, we can recognize that the initialize method completes before permissions requests are done. This could be affected by the thread model changing of implementations on iOS.
image

Once the new controller has been given to the field, the lifecycle listener is triggered a bit later, which will dispose the controller in a short time, causing a race condition issue.

image

And after the app is resumed, the controller will initialize again.

image

Environment

camera: 0.9.4+6

@AlexV525 AlexV525 added c: regression It was better in the past than it is now p: camera The camera plugin labels Jan 25, 2022
@AlexV525
Copy link
Member Author

/cc @renefloor @hellohuanlin

@hellohuanlin
Copy link
Contributor

I did a bisect and verified that this was introduced by flutter/plugins#4140 (v0.9.4+3).

@hellohuanlin
Copy link
Contributor

hellohuanlin commented Jan 25, 2022

I did some initial investigation:

Comparing with apple's AVCam sample project, looks like we are missing the logic that explicitly requests permission. The current permission alert is triggered implicitly by some of the AVFoundation calls which does not have completion callbacks (e.g. AVCaptureDeviceInput::deviceInputWithDevice:error:). This happened to work fine previously because this call blocked the main thread (which wasn't ideal either), and flutter/plugins#4140 that moves this operation off the main thread introduced the regression.

So in conclusion, I think a good solution is to explicitly request permission with a completion callback like what Apple did in the sample project. I will re-visit this after other threading works are done in #96429

@AlexV525
Copy link
Member Author

So in conclusion, I think a good solution is to explicitly request permission with a completion callback like what Apple did in the sample project. I will re-visit this after other threading works are done in #96429

Thanks for the clarification. Though I would suggest a fix for a higher priority since it breaks most cases with the lifecycle hook.

@AlexV525 AlexV525 added has reproducible steps The issue has been confirmed reproducible and is ready to work on plugin labels Jan 26, 2022
@stuartmorgan stuartmorgan added the P1 High-priority issues at the top of the work list label Jan 27, 2022
@hellohuanlin
Copy link
Contributor

Update: I recently wrapped up the thread model effort (#96429), and will be working on this permission issue now.

@hellohuanlin
Copy link
Contributor

Update: There are API changes that may involve other platforms. I wrote up a draft proposal. It's still in progress and there are more investigation to be done.

@AlexV525
Copy link
Member Author

I wrote up a draft proposal.

The proposal looks detailed and fluent overall even it's incompleted, great work!

@hellohuanlin
Copy link
Contributor

Update: Just finished investigating all the platforms (iOS/Android/Web/Windows) and finalized the proposal.

@AlexV525
Copy link
Member Author

AlexV525 commented May 15, 2022

@hellohuanlin I've verified that the PR has fixed the problem with the initialize method. However, it was still not well-awaited when I was calling the prepareForVideoRecording method. I think camera permissions are well-handled, but not with others.

Also, the example is still requesting twice initialization when selecting one of the controllers, I'll update the example later with a separate PR.

Step to reproduce

  1. Run the example of wechat_camera_picker as the first installation.
  2. Click the FAB.
  3. Allow the camera permission, but hold on for seconds before allowing the microphone permission.
  4. The lifecycle callback will run again which calls a new initialization and widgets will throw exceptions.

See the screenshot which might explain the behavior:
image
I've updated the new implementation (fluttercandies/flutter_wechat_camera_picker/pull/93) which can reproduce this issue when the example was installed the first time.

@hellohuanlin
Copy link
Contributor

hellohuanlin commented May 15, 2022

@AlexV525 Strange. I remember when I tried it a few months ago, the camera permission request already includes microphone, so this separate microphone request is a surprise to me. (though I might be on iOS 14 back then).

@AlexV525
Copy link
Member Author

@AlexV525 Strange. I remember when I tried it a few months ago, the camera permission request already includes microphone, so this separate microphone request is a surprise for me. (though I might be on iOS 14 back then).

I didn't notice this too when I was tested with the official example, it seems there are a couple of combinations with audio and video recording configurations, which might be related?

@hellohuanlin
Copy link
Contributor

@AlexV525 I just tried out the example project again and it did ask for microphone separately now (which was not the case previously).

I think we just need to add similar request logic here, but for AVMediaTypeAudio instead of AVMediaTypeVideo.

https://github.com/flutter/plugins/blob/486071d7f9bb948e0d8cfeb0abceaf3d36e2374f/packages/camera/camera/ios/Classes/CameraPlugin.m#L197

Let me try out your project and take a look

@AlexV525
Copy link
Member Author

I think we just need to add similar request logic here, but for AVMediaTypeAudio instead of AVMediaTypeVideo.

It might be better if they can be requested at the same time, or I think there will be another resume state between permission requests.

Let me try out your project and take a look

Thanks! Please use the PR version for further investigations, and you can suggest changes if there are any problems with the implementation.

@hellohuanlin
Copy link
Contributor

@AlexV525 I was able to fix your project by explicitly requesting the audio permission (similar to what I did for video permission). Will be sending a PR next week.

@AlexV525
Copy link
Member Author

All incorrect behaviors are well-addressed in 0.9.6. Thanks @hellohuanlin ! Closing.

@github-actions
Copy link

github-actions bot commented Jun 1, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2022
@flutter-triage-bot flutter-triage-bot bot added the package flutter/packages repository. See also p: labels. label Jul 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: regression It was better in the past than it is now has reproducible steps The issue has been confirmed reproducible and is ready to work on p: camera The camera plugin P1 High-priority issues at the top of the work list package flutter/packages repository. See also p: labels.
Projects
None yet
Development

No branches or pull requests

3 participants