[camera] Dispose resources correctly on setDescription #4003

BradenBagby · 2023-05-15T19:45:15Z

setDescription for CameraController was not correctly handling resources. It would recreate the _deviceOrientationSubscription each time it was called, which caused the subscription to not be disposed correctly. It also was not disposing of the previous device camera

List which issues are fixed by this PR. You must list at least one issue.

Issue seen here: flutter/flutter#126823

Pre-launch Checklist

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

…en cameras, does not recreate _deviceOrientationSubscription each time

flutter-dashboard · 2023-05-15T19:45:18Z

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

jellynoone · 2023-05-17T07:30:06Z

Hi, I was the one that reported the issue. Thank you for fixing on this!

Are you sure doing await _initCalled actually does anything, from the looks of the class, _initCalled might be marked as FutureOr? but its only ever bool or null. (I think this fact also puts into question the dispose method of this class which uses the same logic, the _initCalled is never really awaited for since its never a Future, so if the initialisation process is going on it will be null not Future)

Additionally, what if a camera preview is being displayed and the camera is disposed before the new one is initialised? I don't know this is why I'm asking.

BradenBagby · 2023-05-17T15:00:05Z

Hi, I was the one that reported the issue. Thank you for fixing on this!

Are you sure doing await _initCalled actually does anything, from the looks of the class, _initCalled might be marked as FutureOr? but its only ever bool or null. (I think this fact also puts into question the dispose method of this class which uses the same logic, the _initCalled is never really awaited for since its never a Future, so if the initialisation process is going on it will be null not Future)

Additionally, what if a camera preview is being displayed and the camera is disposed before the new one is initialised? I don't know this is why I'm asking.

Yeah thanks again! The fix was exactly what you laid out in your issue report.

Those are some good points. There probably should be some future for initializing that can be awaited so we don't accidentally overlap two initialization calls. It does look like _initCalled is never actually used as a future as you point out.

I think the CameraController class definitely needs some improvements. Feel free to add to this PR or submit another one, I will also dive into it when I get the chance!

bparrishMines · 2023-05-23T17:41:41Z

packages/camera/camera/lib/src/camera_controller.dart

+    // dispose resources from previous camera description
+    if (_initCalled != null) {
+      await _initCalled;
+      _unawaited(CameraPlatform.instance.dispose(_cameraId));


I don't like the idea that the CameraController calls dispose and is still remains usable. This doesn't seem like intended behavior. And now that I look at setDescription, is it possible to call setDescription instead of initialize() to initialize a camera? This also doesn't seem intended.

I think it makes more sense to put this line in setDescription and not have this conditional.

Ill change and test it with that under setDescription.

I agree its weird that you could call setDescription to completely initialize the controller. Since setDescription is really just handling what you used to have to do manually when changing cameras -> re-initialize a controller with a new description. We could throw calls to setDescription if initialize has never been called?

I agree. I think we should add the _throwIfNotInitialized method to the top of setDescription.

Are you going to add _throwIfNotInitialized to the beginning of the method?

stuartmorgan-g · 2023-06-13T19:56:20Z

@BradenBagby Are you still planning on updating this PR based on the feedback above?

BradenBagby · 2023-06-13T21:20:20Z

Ive moved it to setDescription @bparrishMines . Let me know if you want setDescription to throw an error if its not already initialized instead of just initializing it for you. If thats the case, wouldn't need the dispose call within the if block, but we would probably still want to await _initCalled incase the user is spamming the switch camera button

bparrishMines

I added a couple more comments.

As a side note, I didn't realize before that setDescription reinitializes the CameraController. I don't think the class was designed to be able to handle this, so we are starting to see bugs like this pop up. It may be worth evaluating whether the setDescription should only be used while recording for the short term. This doesn't block this PR from fixing these bugs though.

packages/camera/camera/lib/src/camera_controller.dart

packages/camera/camera/CHANGELOG.md

BradenBagby · 2023-06-23T13:08:51Z

Yes I also think it should just be setDescriptionWhileRecording. I should hopefully be able to get to fixing this pr in the next couple days

bparrishMines

This looks good to me except a missing test. Specifically, you could test that dispose is called in setDescription.

packages/camera/camera/CHANGELOG.md

Co-authored-by: Maurice Parrish <[email protected]>

stuartmorgan-g · 2023-07-27T18:59:12Z

Is this ready for a second review? (It looks like it's failing analysis, but it's not clear to me from discussion above if it's ready for a second look more generally.)

stuartmorgan-g · 2023-09-07T18:58:00Z

Update from triage: @bparrishMines will add tests to this when time allows.

…scription-resources

bparrishMines · 2023-09-22T20:07:44Z

@stuartmorgan I added the tests and also updated _initCalled with a field that actually waits for initialization.

stuartmorgan-g

LGTM

…scription-resources

auto-submit · 2023-09-29T19:29:06Z

auto label is removed for flutter/packages/4003, due to This PR has not met approval requirements for merging. You are not a member of flutter-hackers and need 1 more review(s) in order to merge this PR.

Merge guidelines: You need at least one approved review if you are already part of flutter-hackers or two member reviews if you are not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

bparrishMines

LGTM

…er/packages#4003)

flutter/packages@d0e9a0e...e2ac440 2023-10-01 [email protected] Roll Flutter from d3df8f6 to d42313c (4 revisions) (flutter/packages#5044) 2023-10-01 [email protected] [webview_flutter] Adds app facing implementation to override console log (flutter/packages#4705) 2023-09-30 [email protected] Roll Flutter from 57b5c3c to d3df8f6 (24 revisions) (flutter/packages#5043) 2023-09-30 [email protected] [webview_flutter] Add a method for getting the user agent (flutter/packages#4472) 2023-09-29 [email protected] [webview_flutter_android] Fix race condition in flaky test (flutter/packages#5037) 2023-09-29 [email protected] [ci] Wait for LUCI test checkin in `release` (flutter/packages#4911) 2023-09-29 [email protected] [webview_flutter_android][webview_flutter_wkwebview] Adds support for `getUserAgent` for `webview_flutter` platform implementations (flutter/packages#4927) 2023-09-29 [email protected] [ci] Disable maps tests in Android emulator (flutter/packages#5003) 2023-09-29 [email protected] [camera] Dispose resources correctly on setDescription (flutter/packages#4003) 2023-09-29 [email protected] [webview_flutter_android] Adds Android implementation to override console log (flutter/packages#4702) 2023-09-29 [email protected] [camera] Remove `@throw` from iOS implementation (flutter/packages#5034) 2023-09-29 [email protected] Roll Flutter from ff4a0f6 to 57b5c3c (47 revisions) (flutter/packages#5036) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

BradenBagby added 2 commits May 15, 2023 13:39

correctly disposes previous cameraId from device when switching betwe…

5874083

…en cameras, does not recreate _deviceOrientationSubscription each time

version and changelog

adbb45c

BradenBagby requested a review from bparrishMines as a code owner May 15, 2023 19:45

flutter-dashboard bot added the needs tests label May 15, 2023

github-actions bot added the p: camera label May 15, 2023

BradenBagby and others added 2 commits May 15, 2023 14:28

Merge branch 'main' into camera-set-description-resources

c9237cb

fixes

85de90b

bparrishMines reviewed May 23, 2023

View reviewed changes

moved to setDescription

51986e0

bparrishMines reviewed Jun 15, 2023

View reviewed changes

packages/camera/camera/lib/src/camera_controller.dart Outdated Show resolved Hide resolved

packages/camera/camera/CHANGELOG.md Outdated Show resolved Hide resolved

BradenBagby added 3 commits June 26, 2023 18:59

awaits dispose

5c2a72a

fixes readme

f85fcaf

Merge branch 'main' into camera-set-description-resources

4b926ef

BradenBagby requested a review from bparrishMines June 27, 2023 01:21

bparrishMines reviewed Jul 6, 2023

View reviewed changes

packages/camera/camera/CHANGELOG.md Outdated Show resolved Hide resolved

Update packages/camera/camera/CHANGELOG.md

f2487fb

Co-authored-by: Maurice Parrish <[email protected]>

bparrishMines added 3 commits September 21, 2023 12:38

Merge branch 'main' of github.com:flutter/packages into camera-set-de…

f17e964

…scription-resources

add a test

873e3bf

Merge branch 'main' of github.com:flutter/packages into camera-set-de…

7bc4bd8

…scription-resources

bparrishMines requested a review from stuartmorgan-g September 22, 2023 20:06

stuartmorgan-g approved these changes Sep 28, 2023

View reviewed changes

Merge branch 'main' of github.com:flutter/packages into camera-set-de…

ec396fa

…scription-resources

bparrishMines added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Sep 29, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 29, 2023

bparrishMines approved these changes Sep 29, 2023

View reviewed changes

bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 29, 2023

auto-submit bot merged commit 8483ac2 into flutter:main Sep 29, 2023

engine-flutter-autoroll mentioned this pull request Oct 2, 2023

Roll Packages from d0e9a0e1b3ac to e2ac440a547a (12 revisions) flutter/flutter#135859

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 2, 2023

8483ac2 [camera] Dispose resources correctly on setDescription (flutt…

4fa914e

…er/packages#4003)

[camera] Dispose resources correctly on setDescription #4003

[camera] Dispose resources correctly on setDescription #4003

Uh oh!

Conversation

BradenBagby commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented May 15, 2023

Uh oh!

jellynoone commented May 17, 2023

Uh oh!

BradenBagby commented May 17, 2023

Uh oh!

bparrishMines May 23, 2023

Choose a reason for hiding this comment

Uh oh!

BradenBagby Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

bparrishMines Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

bparrishMines Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jun 13, 2023

Uh oh!

BradenBagby commented Jun 13, 2023

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BradenBagby commented Jun 23, 2023

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g commented Jul 27, 2023

Uh oh!

stuartmorgan-g commented Sep 7, 2023

Uh oh!

bparrishMines commented Sep 22, 2023

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Sep 29, 2023

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BradenBagby commented May 15, 2023 •

edited

Loading