Skip to content

[camera_avfoundation] Abstract UIKit dependencies for macOS support (Part 2) - #12286

Draft
Mairramer wants to merge 3 commits into
flutter:mainfrom
Mairramer:camera-macos-support-part2
Draft

[camera_avfoundation] Abstract UIKit dependencies for macOS support (Part 2)#12286
Mairramer wants to merge 3 commits into
flutter:mainfrom
Mairramer:camera-macos-support-part2

Conversation

@Mairramer

Copy link
Copy Markdown
Contributor

Part 2 of the ongoing effort to add macOS support to the camera_avfoundation plugin.

Part of flutter/flutter#41708

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

@Mairramer

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot 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 Review

This pull request refactors the camera_avfoundation package to conditionally compile iOS-specific device orientation APIs (UIDevice, UIDeviceOrientation) using #if os(iOS) blocks, and transitions the internal representation of device orientation to PlatformDeviceOrientation. Feedback suggests optimizing the non-iOS fallback logic for setting exposure and focus points of interest in DefaultCamera.swift by using optional chaining instead of repeatedly instantiating PlatformPoint.

@Mairramer

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot 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 Review

This pull request refactors the camera AVFoundation implementation to support non-iOS platforms by wrapping iOS-specific device orientation and UIDevice dependencies in #if os(iOS) conditional compilation blocks, and migrating orientation properties to use the platform-agnostic PlatformDeviceOrientation type. The review feedback identifies potential issues in DefaultCamera.swift where try? is used with lockForConfiguration() during exposure and focus point updates, which could lead to silent failures or crashes, and suggests handling these errors using do-catch blocks.

Comment on lines +908 to 921
#if os(iOS)
let orientation = UIDevice.current.orientation
try? captureDevice.lockForConfiguration()
// A nil point resets to the center.
let exposurePoint = cgPoint(
for: point ?? PlatformPoint(x: 0.5, y: 0.5), withOrientation: orientation)
captureDevice.exposurePointOfInterest = exposurePoint
#else
try? captureDevice.lockForConfiguration()
captureDevice.exposurePointOfInterest = CGPoint(
x: point?.x ?? 0.5,
y: point?.y ?? 0.5)
#endif
captureDevice.unlockForConfiguration()

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.

high

Using try? with lockForConfiguration() can lead to silent failures and potential runtime crashes. If the lock cannot be acquired, try? returns nil and execution continues, attempting to set exposurePointOfInterest without holding the lock, and then calling unlockForConfiguration() which will fail or crash. Instead, we should properly handle the error by propagating it via the completion handler and only unlocking if the lock was successfully acquired.

    do {
      try captureDevice.lockForConfiguration()
    } catch {
      completion(.failure(error))
      return
    }
    #if os(iOS)
      let orientation = UIDevice.current.orientation
      // A nil point resets to the center.
      let exposurePoint = cgPoint(
        for: point ?? PlatformPoint(x: 0.5, y: 0.5), withOrientation: orientation)
      captureDevice.exposurePointOfInterest = exposurePoint
    #else
      captureDevice.exposurePointOfInterest = CGPoint(
        x: point?.x ?? 0.5,
        y: point?.y ?? 0.5)
    #endif
    captureDevice.unlockForConfiguration()

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 code already exists; it was simply wrapped.

Comment on lines +945 to 960
#if os(iOS)
let orientation = deviceOrientationProvider.orientation
try? captureDevice.lockForConfiguration()
// A nil point resets to the center.
captureDevice.focusPointOfInterest =
cgPoint(
for: point ?? PlatformPoint(x: 0.5, y: 0.5),
withOrientation: orientation)
#else
try? captureDevice.lockForConfiguration()
captureDevice.focusPointOfInterest = CGPoint(
x: point?.x ?? 0.5,
y: point?.y ?? 0.5)
#endif

captureDevice.unlockForConfiguration()

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.

high

Using try? with lockForConfiguration() can lead to silent failures and potential runtime crashes. If the lock cannot be acquired, try? returns nil and execution continues, attempting to set focusPointOfInterest without holding the lock, and then calling unlockForConfiguration() which will fail or crash. Instead, we should properly handle the error by propagating it via the completion handler and only unlocking if the lock was successfully acquired.

Suggested change
#if os(iOS)
let orientation = deviceOrientationProvider.orientation
try? captureDevice.lockForConfiguration()
// A nil point resets to the center.
captureDevice.focusPointOfInterest =
cgPoint(
for: point ?? PlatformPoint(x: 0.5, y: 0.5),
withOrientation: orientation)
#else
try? captureDevice.lockForConfiguration()
captureDevice.focusPointOfInterest = CGPoint(
x: point?.x ?? 0.5,
y: point?.y ?? 0.5)
#endif
captureDevice.unlockForConfiguration()
do {
try captureDevice.lockForConfiguration()
} catch {
completion(.failure(error))
return
}
#if os(iOS)
let orientation = deviceOrientationProvider.orientation
// A nil point resets to the center.
captureDevice.focusPointOfInterest =
cgPoint(
for: point ?? PlatformPoint(x: 0.5, y: 0.5),
withOrientation: orientation)
#else
captureDevice.focusPointOfInterest = CGPoint(
x: point?.x ?? 0.5,
y: point?.y ?? 0.5)
#endif
captureDevice.unlockForConfiguration()

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 code already exists; it was simply wrapped.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant