Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 28 additions & 22 deletions packages/camera/camera/lib/src/camera_controller.dart
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,9 @@ class CameraController extends ValueNotifier<CameraValue> {
_deviceOrientationSubscription ??= CameraPlatform.instance
.onDeviceOrientationChanged()
.listen((DeviceOrientationChangedEvent event) {
value = value.copyWith(deviceOrientation: event.orientation);
if (!_isDisposed) {
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.

Could these use early returns? That would reduce the indent changes in code.

Just a personal preference though.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Early returns would definitely improve the code.

However, this PR is still in draft. @mbcorona from the Flutter team recently
discovered a deeper issue: ImageReader fails to drain pending frames on dispose,
flooding the queue with 290+ orphaned callbacks
.
flutter/flutter#184959 (comment)

My current fix addresses the symptom (guarding state updates). but the root cause is the improper ImageReader cleanup at the native
level. So I'm currently reinvestigating the issue

Once I resolve the frame leakage issue, I'll:

  • Apply your early return suggestion
  • Update the fix comprehensively
  • Mark it ready for review

Thanks again for your suggestion!

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.

Before marking it ready for review, please update the PR to use and follow our actual checklist, rather than an AI-hallucinated checklist.

value = value.copyWith(deviceOrientation: event.orientation);
}
});

_cameraId = await CameraPlatform.instance.createCameraWithSettings(
Expand All @@ -361,7 +363,9 @@ class CameraController extends ValueNotifier<CameraValue> {
CameraPlatform.instance.onCameraError(_cameraId).first.then((
CameraErrorEvent event,
) {
value = value.copyWith(errorDescription: event.description);
if (!_isDisposed) {
value = value.copyWith(errorDescription: event.description);
}
}),
);

Expand All @@ -370,26 +374,28 @@ class CameraController extends ValueNotifier<CameraValue> {
imageFormatGroup: imageFormatGroup ?? ImageFormatGroup.unknown,
);

value = value.copyWith(
isInitialized: true,
description: description,
previewSize: await initializeCompleter.future.then(
(CameraInitializedEvent event) =>
Size(event.previewWidth, event.previewHeight),
),
exposureMode: await initializeCompleter.future.then(
(CameraInitializedEvent event) => event.exposureMode,
),
focusMode: await initializeCompleter.future.then(
(CameraInitializedEvent event) => event.focusMode,
),
exposurePointSupported: await initializeCompleter.future.then(
(CameraInitializedEvent event) => event.exposurePointSupported,
),
focusPointSupported: await initializeCompleter.future.then(
(CameraInitializedEvent event) => event.focusPointSupported,
),
);
if (!_isDisposed) {
value = value.copyWith(
isInitialized: true,
description: description,
previewSize: await initializeCompleter.future.then(
(CameraInitializedEvent event) =>
Size(event.previewWidth, event.previewHeight),
),
exposureMode: await initializeCompleter.future.then(
(CameraInitializedEvent event) => event.exposureMode,
),
focusMode: await initializeCompleter.future.then(
(CameraInitializedEvent event) => event.focusMode,
),
exposurePointSupported: await initializeCompleter.future.then(
(CameraInitializedEvent event) => event.exposurePointSupported,
),
focusPointSupported: await initializeCompleter.future.then(
(CameraInitializedEvent event) => event.focusPointSupported,
),
);
}
Comment on lines +377 to +398
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The if (!_isDisposed) check is performed before the await expressions. Since await suspends execution, dispose() could be called while waiting for the initializeCompleter.future to complete. When execution resumes, the assignment to value will proceed regardless of the disposal state, which will trigger the 'used after being disposed' crash because notifyListeners() is called on a disposed object. The check should be performed after the future completes but before the assignment to value. Additionally, awaiting the same future multiple times is inefficient.

      final CameraInitializedEvent event = await initializeCompleter.future;
      if (!_isDisposed) {
        value = value.copyWith(
          isInitialized: true,
          description: description,
          previewSize: Size(event.previewWidth, event.previewHeight),
          exposureMode: event.exposureMode,
          focusMode: event.focusMode,
          exposurePointSupported: event.exposurePointSupported,
          focusPointSupported: event.focusPointSupported,
        );
      }

} on PlatformException catch (e) {
throw CameraException(e.code, e.message);
} finally {
Expand Down
Loading