Skip to content

[camera_avfoundation] Implementation swift migration - part 2 #9007

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[camera_avfoundation] Implementation swift migration - part 2 #9007

wants to merge 11 commits into from
+627 −674

Conversation

RobertOdrowaz
Copy link
Contributor

@RobertOdrowaz RobertOdrowaz commented Apr 5, 2025 •
edited
Loading

Migrates camera implementation as part of flutter/flutter#119109

This PR migrates CameraPlugin class.

Pre-Review Checklist

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

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 ↩3

@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-implementation-swift-migration-part2 branch 3 times, most recently from aa4269d to 8be6136 Compare April 16, 2025 10:43
@RobertOdrowaz RobertOdrowaz marked this pull request as ready for review April 16, 2025 11:26
@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-implementation-swift-migration-part2 branch from 8be6136 to 859ad13 Compare April 16, 2025 11:32
hellohuanlin
hellohuanlin reviewed Apr 22, 2025
View reviewed changes
Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

not a complete review, left a few questions

@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-implementation-swift-migration-part2 branch from 859ad13 to 63292b4 Compare April 23, 2025 07:03
hellohuanlin
hellohuanlin reviewed Apr 29, 2025
View reviewed changes
...amera_avfoundation/example/ios/RunnerTests/CameraCaptureSessionQueueRaceConditionTests.swift
@@ -55,6 +60,6 @@ final class CameraCaptureSessionQueueRaceConditionTests: XCTestCase {
// `captureSessionQueue` passed into `AVCaptureVideoDataOutput::setSampleBufferDelegate:queue:`
// API will cause a crash.
XCTAssertNotNil(
cameraPlugin.captureSessionQueue, "captureSessionQueue must not be nil after create method.")
captureSessionQueue, "captureSessionQueue must not be nil after create method.")
Copy link
Contributor

Choose a reason for hiding this comment

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

is it because you are making captureSessionQueue private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

...a/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/CameraPlugin.swift
import Flutter
import ObjectiveC

// Import Objectice-C part of the implementation when SwiftPM is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add more details to this? (e.g. things like 2 modules for swift pm but one module for cocoapods, due to swift pm's language restriction)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can, but then I’d have to update all 44 files that include it. We should’ve done this in the previous PR. TBH I don’t think it’s necessary since the conditional imports are temporary

...a/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/CameraPlugin.swift Outdated
import camera_avfoundation_objc
#endif

typealias CaptureNamedDeviceFactory = (String) -> FLTCaptureDevice
Copy link
Contributor

Choose a reason for hiding this comment

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

what's "named device"? is there an "unnamed" device?

maybe just CaptureDeviceFactory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an "unnamed" factory CaptureDeviceFactory which doesn't receive a device name as an argument. It should be removed in the future because it is causing this issue but at the moment there are 2 factories. I will change the order of words because CaptureNamedDeviceFactory makes little sense

...a/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/CameraPlugin.swift Outdated
}
}

camera?.dartAPI = FCPCameraEventApi(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is strange, why is camera still optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean regarding the guard above? That guard is in the onFrameAvailable lambda and this is outside of it. I will add a guard outside

@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-implementation-swift-migration-part2 branch from 63292b4 to 2898d01 Compare April 30, 2025 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellohuanlin hellohuanlin hellohuanlin left review comments

@stuartmorgan-g stuartmorgan-g Awaiting requested review from stuartmorgan-g

@vashworth vashworth Awaiting requested review from vashworth vashworth is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
p: camera platform-ios platform-macos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RobertOdrowaz @hellohuanlin