Support Xcode 16 #4066
Support Xcode 16 #4066
Conversation
| <BuildActionEntry | ||
| buildForTesting = "YES" | ||
| buildForRunning = "YES" | ||
| buildForProfiling = "YES" | ||
| buildForArchiving = "YES" | ||
| buildForAnalyzing = "YES"> | ||
| <BuildableReference | ||
| BuildableIdentifier = "primary" | ||
| BlueprintIdentifier = "41D17A3F2C5A73A6007C6EE6" | ||
| BuildableName = "StripeConnect.framework" | ||
| BlueprintName = "StripeConnect" | ||
| ReferencedContainer = "container:StripeConnect/StripeConnect.xcodeproj"> | ||
| </BuildableReference> | ||
| </BuildActionEntry> | ||
| <BuildActionEntry | ||
| buildForTesting = "YES" | ||
| buildForRunning = "NO" | ||
| buildForProfiling = "NO" | ||
| buildForArchiving = "NO" | ||
| buildForAnalyzing = "NO"> | ||
| <BuildableReference | ||
| BuildableIdentifier = "primary" | ||
| BlueprintIdentifier = "41D17A492C5A73A7007C6EE6" | ||
| BuildableName = "StripeConnectTests.xctest" | ||
| BlueprintName = "StripeConnectTests" | ||
| ReferencedContainer = "container:StripeConnect/StripeConnect.xcodeproj"> | ||
| </BuildableReference> | ||
| </BuildActionEntry> |
There was a problem hiding this comment.
Adds StripeConnect to AllStripeFrameworks so we'll get CI failures when this module doesn't compile
| // Set beginning of scan session | ||
| ScanAnalyticsManager.shared.setScanSessionStartTime(time: Date()) | ||
| /// Check and log torch availability | ||
| // Check and log torch availability |
There was a problem hiding this comment.
Swiftlint fix
| if self.machineLearningSemaphore.wait(timeout: .now()) == .success { | ||
| ScanBaseViewController.machineLearningQueue.async { | ||
| self.captureOutputWork(sampleBuffer: sampleBuffer) | ||
| guard let pixelBuffer = CMSampleBufferGetImageBuffer(sampleBuffer), | ||
| let fullCameraImage = pixelBuffer.cgImage() else { | ||
| self.machineLearningSemaphore.signal() | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func captureOutputWork(sampleBuffer: CMSampleBuffer) { | ||
| guard let pixelBuffer = CMSampleBufferGetImageBuffer(sampleBuffer) else { | ||
| return | ||
| } | ||
|
|
||
| guard let fullCameraImage = pixelBuffer.cgImage() else { | ||
| return | ||
| ScanBaseViewController.machineLearningQueue.async { [weak self] in | ||
| self?.captureOutputWork(fullCameraImage: fullCameraImage) | ||
| self?.machineLearningSemaphore.signal() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Both CMSampleBuffer and CVImageBuffer are not Sendable, which I think means that they can only be modified on the main thread? Solution was to convert them to CIImage before passing them to the dispatch queue.
I'm unsure if this is the intended behavior – if it's expensive to convert CMSampleBuffer -> CVImageBuffer -> CIImage, then we're now doing that work on the main thread.
There was a problem hiding this comment.
Based on my memory from implementing the equivalent for Identity, captureOutput on the AVCaptureVideoDataOutputSampleBufferDelegate is always called off the main thread on its own dedicated thread, so I think this should be okay.
There was a problem hiding this comment.
- I think it might be a little expensive, but I agree that I don't think this will be called on the main thread
- I don't have the full logic downloaded into my brain atm, but is it okay to call the
machineLearningSemaphoreoutside the machineLearningQueue? That would free anyone elsewait-ing on it, right? And no one else should be waiting on it anyway, becausecaptureOutput()won't be called again until this function returns?
There was a problem hiding this comment.
is it okay to call the machineLearningSemaphore outside the machineLearningQueue?
Yes. In fact, this is what it's designed for. Generally the camera feed is faster than the ML model can process images, so the idea is that you want to block the thread that is reading from the camera in order to finish processing the image in CoreML before asking for more camera frames, otherwise you end queuing up so much of the video buffer that the displayed camera feed begins to drop frames and the app lags.
Here's an excerpt from the copy of the CoreML survival guide that gives a basic example:
https://drive.google.com/open?id=1gwW8pgJvg8t_EOkM2cgnDsQDI5Hc_f4o&disco=AAAAVxpjn24
Public link (page 400):
https://www.scribd.com/document/689236733/Core-ML-Survival-Guide
There was a problem hiding this comment.
So yeah, given that we're explicitly calling wait in order to pause this DispatchQueue, I think that it's inconsequential that converting CMSampleBuffer -> CIImage could be non-performant
| @@ -24,8 +24,7 @@ | |||
| "kind" : "remoteSourceControl", | |||
| "location" : "https://github.com/erikdoe/ocmock", | |||
| "state" : { | |||
| "branch" : "master", | |||
| "revision" : "05cbc9d934e84ad32530fa53fd7d29c7966d5828" | |||
| "revision" : "2c0bfd373289f4a7716db5d6db471640f91a6507" | |||
There was a problem hiding this comment.
This hash corresponds to the 3.9.4 release tag:
https://github.com/erikdoe/ocmock/releases/tag/v3.9.4
Swift Package manager doesn't allow OCMock unless you add it using a commit hash:
erikdoe/ocmock#500 (comment)
| - test-builds-xcode-16: {} | ||
| - test-builds-xcode-15-release: {} | ||
| - test-builds-xcode-16-release: {} |
There was a problem hiding this comment.
Adds tests to ensure the frameworks build on both Xcode 15 and 16
I tested CardScan and it seemed like it was behaving as expected, I wasn't seeing any noticeable camera lag (see video in PR description). I did find that switching between Xcode 15 and 16 was automatically making changes to the Package.resolved file, so I reverted back to the original |
mludowise-stripe
deleted the
mludowise/MXMOBILE-2808_compile_xcode16
branch
Summary
Note: OCMock doesn't work with SPM unless you add the dependency via commit hash as opposed to release version. We're using the commit hash that corresponds to v3.9.4:
Motivation
https://jira.corp.stripe.com/browse/MXMOBILE-2808
Testing
Manually tested CardScan example app:
RPReplay_Final1727495221.MP4