Skip to content

[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.

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 1 commit into
base: main
Choose a base branch
from
Open

[camera_avfoundation] Implementation swift migration - part 4 #9219

wants to merge 1 commit into from
+450 −334

Conversation

RobertOdrowaz
Copy link
Contributor

@RobertOdrowaz RobertOdrowaz commented May 7, 2025 •
edited
Loading

Migrates camera implementation as part of flutter/flutter#119109

This PR migrates a part of FLTCam class to Swift. To avoid migrating the whole huge class to Swift at once and a refactor that we don't have a budget for we are extending the FLTCam class in swift (DefaultCamera). We will now migrate the methods in smaller chunks.

This PR includes the first chunk:

  • captureOutput (from AVCaptureVideoDataOutputSampleBufferDelegate, AVCaptureAudioDataOutputSampleBufferDelegate protocols)
  • copyPixelBuffer (from FlutterTexture protocol)

Some properties of the FLTCam have to be temporarily made public so that they are accessible in DefaultCamera.

The DefaultCamera class conforms to the new Camera protocol. A few properties defined in FLTCam are now marked nullable to match the Camera protocol and the reality.

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

hellohuanlin
hellohuanlin reviewed May 7, 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. is this the last remaining work?

@RobertOdrowaz
Copy link
Contributor Author

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

@RobertOdrowaz RobertOdrowaz requested a review from hellohuanlin May 9, 2025 05:19
hellohuanlin
hellohuanlin reviewed May 14, 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.

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)?

@RobertOdrowaz RobertOdrowaz mentioned this pull request May 15, 2025
Merged
11 tasks
@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-implementation-swift-migration-part4 branch from 917fe7e to d49c640 Compare May 15, 2025 16:09
@RobertOdrowaz RobertOdrowaz marked this pull request as draft May 15, 2025 16:12
@RobertOdrowaz
Copy link
Contributor Author

@hellohuanlin I've extracted whatever was possible to part 3.5. I will rebase this one on main once 3.5 is merged

auto-submit bot pushed a commit that referenced this pull request May 28, 2025
@RobertOdrowaz
[camera_avfoundation] Implementation swift migration - part 3.5 (#9254)
69499c0 
Migrates camera implementation as part of flutter/flutter#119109

This PR consists of everything that was possible to extract from the [part 4](#9219) which includes:
* Adds `audioCaptureDeviceFactory` to `FLTCamConfiguration`.
* Renames the `lockCaptureOrientation` method of Objective-C type `FLTCam` when exported to Swift.
* Renames arguments of the `captureOutput` method of Objective-C type `FLTCam` when exported to Swift.
* Changes the `connection` argument type of the `captureOutput` method of the of `FLTCam` class to `AVCaptureConnection`.
* Makes `minimum/maximumAvailableZoomFactor` and `minimum/maximumExposureOffset` fields of `FLTCam` readonly.
* Remove `@testable` from `camera_avfoundation_objc` imports

## Pre-Review Checklist

[^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.
@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-implementation-swift-migration-part4 branch 3 times, most recently from 3ba84c1 to 52f0941 Compare May 28, 2025 15:39
@RobertOdrowaz RobertOdrowaz marked this pull request as ready for review May 28, 2025 15:58
@RobertOdrowaz
Copy link
Contributor Author

It's still big but at least a bit smaller now

hellohuanlin
hellohuanlin reviewed May 28, 2025
View reviewed changes
.../camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/DefaultCamera.swift Outdated
details: error.domain)
}

private static func createConnection(
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 try to further reduce the size of PR again? For example, static members can be moved out since it doesn't depend on camera states. For non-static members, some stateless logic can be factored out, and others can be reorganized into extensions.

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 don't think we can meaningfully reduce the size of this PR without a significant refactor, which I don't have a budget for. What I think we can do instead is try to migrate this class in smaller parts. Keep a "base" implementation in ObjC, subclass it in Swift, and move method implementation in smaller chunks from the "base" ObjC class to the Swift subclass. Let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

That'd be great. Smaller PRs will be much easier to review. Sorry about the extra work. I really want to make sure this class is done right, since it's the hardest one.

@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-implementation-swift-migration-part4 branch from 52f0941 to d0e1eb9 Compare June 1, 2025 19:43
@RobertOdrowaz RobertOdrowaz marked this pull request as draft June 1, 2025 19:44
@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-implementation-swift-migration-part4 branch from d0e1eb9 to 18fba53 Compare June 2, 2025 08:49
@RobertOdrowaz RobertOdrowaz marked this pull request as ready for review June 2, 2025 09:43
hellohuanlin
hellohuanlin approved these changes Jun 3, 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.

LGTM! Thanks for splitting it into smaller PRs.

packages/camera/camera_avfoundation/example/ios/RunnerTests/CameraTestUtils.swift
@@ -81,16 +81,16 @@ enum CameraTestUtils {
return configuration
}

static func createTestCamera(_ configuration: FLTCamConfiguration) -> FLTCam {
return FLTCam(configuration: configuration, error: nil)
static func createTestCamera(_ configuration: FLTCamConfiguration) -> DefaultCamera {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: -> Camera (same below)

packages/camera/camera_avfoundation/example/ios/RunnerTests/SampleBufferTests.swift
@@ -69,7 +69,7 @@ private class FakeMediaSettingsAVWrapper: FLTCamMediaSettingsAVWrapper {
/// Includes test cases related to sample buffer handling for FLTCam class.
final class CameraSampleBufferTests: XCTestCase {
private func createCamera() -> (
FLTCam,
DefaultCamera,
Copy link
Contributor

Choose a reason for hiding this comment

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

here too, and a few other places

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

@hellohuanlin hellohuanlin hellohuanlin approved these changes

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

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