-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[camera_avfoundation] Implementation swift migration - part 4 #9219
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
base: main
Are you sure you want to change the base?
[camera_avfoundation] Implementation swift migration - part 4 #9219
Conversation
There was a problem hiding this 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. is this the last remaining work?
packages/camera/camera_avfoundation/example/ios/RunnerTests/CameraTestUtils.swift
Outdated
Show resolved
Hide resolved
|
||
// Import Objectice-C part of the implementation when SwiftPM is used. | ||
#if canImport(camera_avfoundation_objc) | ||
import camera_avfoundation_objc | ||
@testable import camera_avfoundation_objc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious does @testable import
have any effect on objc modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, TBH I haven't considered it. I will keep this one for consistency and we will be able to remove all of them shortly
var getMaximumAvailableZoomFactorStub: (() -> CGFloat)? | ||
final class MockFLTCam: NSObject, FLTCam { | ||
var setDartApiStub: ((FCPCameraEventApi?) -> Void)? | ||
var setOnFrameAvailableStub: (((() -> Void)?) -> Void)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these just re-ordered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes aside from some very minor renames e.g. lockCaptureStub
-> lockCaptureOrientationStub
because this method is finally is called properly in FLTCam
packages/camera/camera_avfoundation/example/ios/RunnerTests/PhotoCaptureTests.swift
Outdated
Show resolved
Hide resolved
packages/camera/camera_avfoundation/example/ios/RunnerTests/SampleBufferTests.swift
Outdated
Show resolved
Hide resolved
camera?.close() | ||
camera = newCamera | ||
|
||
FLTEnsureToRunOnMainQueue { [weak self] in | ||
guard let strongSelf = self else { return } | ||
completion(NSNumber(value: strongSelf.registry.register(newCamera)), nil) | ||
} | ||
} catch let error as NSError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the error originated from our own code or from AVFoundation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both:
AVCaptureDeviceInput deviceInputWithDevice
(AVFoundation)AVCaptureDevice lockForConfiguration
(AVFoundation)FLTDefaultCam.setCaptureSessionPreset
(our own code)
IMO the whole package deserves a clean up of errors in the future because there is absolutely no consistency in the error format
.../camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.swift
Outdated
Show resolved
Hide resolved
.../camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.swift
Outdated
Show resolved
Hide resolved
.../camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.swift
Outdated
Show resolved
Hide resolved
...avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTImageStreamHandler.swift
Outdated
Show resolved
Hide resolved
This is the main part but it's not the last. There will be a ~3 more PRs I think. ~2 with migration of all the protocols which is a formality and one with the switch to a Swift based pigeon which will include some more significant changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it's really really hard for me to review this PR due to its size and complexity of the logic. Is it possible for you to have separate PRs that break out smaller bits first (e.g. stateless helpers)?
static func createTestCamera(_ configuration: FLTCamConfiguration) -> FLTCam { | ||
return FLTCam(configuration: configuration, error: nil) | ||
static func createTestCamera(_ configuration: FLTCamConfiguration) -> DefaultCamera { | ||
let camera = try? DefaultCamera(configuration: configuration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you do a XCTUnwrap instead of !
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XCTUnwrap
is marked with throws
so it leaves me in the position as the DefaultCamera
init
|
||
// Import Objectice-C part of the implementation when SwiftPM is used. | ||
#if canImport(camera_avfoundation_objc) | ||
import camera_avfoundation_objc | ||
@testable import camera_avfoundation_objc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove @testable
and see if it compiles
get { | ||
return super.dartAPI | ||
preconditionFailure("Attempted to access unimplemented property: dartAPI") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same q for other proeprties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mock used to extend the FLTCam
class now it's implementing the Camera
protocol so there is no super
implementation to fall back to
|
||
// Add the audio input | ||
if mediaSettings.enableAudio { | ||
var acl = AudioChannelLayout() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: audioChannelLayout
917fe7e
to
d49c640
Compare
@hellohuanlin I've extracted whatever was possible to part 3.5. I will rebase this one on main once 3.5 is merged |
Migrates camera implementation as part of flutter/flutter#119109
This PR migrates the main
FLTCam
class to Swift. I know it's large, but I don't see any reasonable way to split it into smaller parts (MockFLTCam
clean up could be separated but it won't make the review simpler in any meaningful way).Since the goal of the PR is migration to Swift the code is mostly translated close to verbatim including the known issues in
setDescriptionWhileRecording
method (flutter/flutter#162376 (comment))There are quite a few cases of
error as NSError
casting because I didn't want to introduce a breaking change so I had do keep the error message structure exactly the same as in ObjC. The errors in ObjC implementation were very closely related toNSError
. I don't see a better way to handle it than castingError
back toNSError
but I'm open to suggestions.Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Footnotes
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