Skip to content
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

Lay the foundation for a Swift migration #547

Merged
merged 20 commits into from
Jul 12, 2023

Conversation

DavidBertet
Copy link
Contributor

Summary

Laying the foundation for a Swift migration.

It should be easier to read/write, thus maintain, and new devs are more used to Swift than ObjC.

Is this something you would be interested in?

How did you test this change?

This is more of a PoC at this stage, that hasn't been fully tested

Built on iOS device & simulator and made sure examples worked the same.

@scarlac
Copy link
Collaborator

scarlac commented Jun 12, 2023

Yes, @DavidBertet, a Swift conversion/migration would be very welcome. We'd have to thoroughly test it on hardware devices as well, obviously. I have merged a few of your other PRs which may have caused the conflict you see.
If you are up for doing it, we very much welcome a Swift migration. Please proceed :)

@DavidBertet
Copy link
Contributor Author

Thanks @scarlac for the merges! I'll rebase and move forward on the Swift conversion.

Agreed on the test, that will definitely be a big one.

Would you prefer multiple PRs that should be easier to review, or one with everything included?

@scarlac
Copy link
Collaborator

scarlac commented Jun 12, 2023

I think one big one is fine unless it’s more work for you that way.

@DavidBertet
Copy link
Contributor Author

Sounds good, let's do one 👍

@DavidBertet
Copy link
Contributor Author

Here we are @scarlac. Swift conversion/rewrite.

I extracted the logic that's unrelated to the actual camera into CameraView (focus, zoom, overlay, ...), which embeds either RealCamera or SimulatorCamera. That way, we can test much more things on the simulator.

The iOS barcode scanner was checking we aren't scanning twice the same value, I replaced it by a throttle configurable with scanThrottleDelay. In some use cases it seems legit for a user to scan twice the same in a row. Could easily be implemented on Android if you are aligned with the idea.

I removed setTorchMode which shouldn't have been exposed to React. It is redundant with the torchMode props. I'll remove it on Android if you are aligned.

I updated AVCaptureStillImageOutput that's deprecated to AVCapturePhotoOutput, and did a lot of things here and there.

I fixed a bunch of small issues here and there. Let me know how I can help further, that's definitely a big one.

@scarlac scarlac self-requested a review July 1, 2023 05:55
@DavidBertet
Copy link
Contributor Author

DavidBertet commented Jul 2, 2023

Here is a side by side recording of ObjC vs Swift implementation

test-swift.mp4
(custom ratio overlay color doesn't work on master, which is why it doesn't appear on ObjC)

Perf are very very similar (milliseconds one another, with Objc or Swift randomly being first). I'm still testing thoroughly the feature set.
I found one issue around default focusMode that should be on while not set from React. I'll check all default values.

@scarlac
Copy link
Collaborator

scarlac commented Jul 3, 2023

Excellent, @DavidBertet - I've also been testing it and it generally seems good. I am however removing the CameraScreen component as well as updating the tests to use functional components. I'll commit to your branch as well.

I don't know that there is a use case for the throttled barcode scanner, but I think it's an OK for this PR.
We need a follow up PR that has the ability to toggle that behavior, for example an optional ignoreDuplicateScans={false} and then throttleScans={2000}.
I think it is most likely that people are not interested in scanning the same code twice. From experience, you either want to scan immediately, or scan multiple items, in which case you want to ignore items that you already scanned. Since the rate of scanning differs on each platform, spamming the RN bridge is not a good default due to performance issues.
As an example, QR codes, and multiple barcodes can often appear close to each other on products.

Using hooks/functional components
Fixed zoom on second pinch
Fixed orientation issues
Added support for new ultra wide camera (fix blurry close ups)
Added support for thumbnails (for later)
Fixed default zoom to be wide angle camera instead of ultra wide
Remove cameraType from the class, use local variable only to avoid unwanted side effects
Move thumbnail creation into delegate, to be consistent and return only data
Make `counterRotatedCaptureVideoOrientationFrom` private
Refactor slightly `update(pinchVelocity`
Remove `setTorchMode`, add `scanThrottleDelay` from React types
Copy link
Contributor Author

@DavidBertet DavidBertet left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for your updates @scarlac!! I pushed a few things, and added some questions inlined

About throttling barcodes, my use case is to let users scan barcode of products they bought. iOS is so quick that pointing a product returns immediately it multiple times, but they might have bought the same product twice. Throttling permits to let the user move to the next barcode, that might happen to be the same value.

I get your use case about ignoreDuplicateScans. Today it was only "the last". So a product with multiple barcodes/qrCode would still spam randomly React, depending what it read last. You are thinking about ignoring duplicates for the whole session? (then let's switch that discussion to an issue :))

if orientationNew == self.deviceOrientation {
return
}
self.deviceOrientation = orientationNew
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the accelerometer is a great addition!

2 feedbacks

  • I would have kept that in CameraView as this code isn't related to the actual AV camera. It could benefit simulator implementation (and futur tests, or others). Then the computed orientation is forwarded to the camera implementation be taken into account.
  • Shouldn't we update self.cameraPreview.previewLayer.connection?.videoOrientation here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. CMMotionManager won't work in the simulator, so it needs to be in the specific implmenentation. The rotation you can do with the simulator does not cause "actual" motion.
  2. I tried that. No, videoOrientation mustn't always be updated based on motion. Only after a significant change in orientation, or only taking a picture. Otherwise, UI orientation and motion may counter-indicate, causing video to be landscape, while UI is portrait or vice versa, which is technically correct but confusing to the user.
    Please keep it as is, as I spent a significant amount of time testing all the use cases. You need to consider UI portrait (locked + unlocked), UI landscape left+right (not recommended but iPad UIs may need it), and every 3D rotation of the phone as well (portrait, landscape left, landscape right, upside down, 45 degree angle (=meaning no change from past orientation). If you compare the native iOS camera, it has the same behavior - where UI elements indicate portrait, but the captured photo is landscape. And that is what you'd want it to.

@@ -149,21 +159,20 @@ class RealCamera: NSObject, CameraProtocol, AVCaptureMetadataOutputObjectsDelega
}
}

func update(zoomScale: CGFloat) {
guard !zoomScale.isNaN else { return }
func update(pinchVelocity: CGFloat, pinchScale: CGFloat) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you tried here? It seems exact same code as mine, with dividerFactor being 10 instead of 20. I rollbacked the code a bit, kept 10.

  • 10 is very snappy on my iPhone, barely usable
  • pinchScale isn't used. is it intended?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I had re-written it to use scale without velocity (so it's always linear to pinch size, and you can zoom far in and out at the cost of accuracy). That's why you see it. I then realized that the native camera uses velocity and tried to match it, however velocity is very finicky and as you zoom further in, it becomes harder to zoom out, esp if the phone is sluggish. Not clear what math the native camera uses.
I'm fine with going with 20 for now, but it needs to be well tested, otherwise we should use the scale-based zooming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What makes you feel they are using velocity?
Native zoom seems to be capped at x10 which is roughly a zoomFactor of 20 on my iPhone (while videoMaxZoomFactor is like 180). Image at 20 is already kinda bad, not sure why they allow 180

Using scale + capping at 20 feels fairly similar to the native app. Miles better than all attempts I did with velocity. I pushed it so you can try. Let me know

PS: resetting pinchRecognizer.scale to 1.0 every time value changes is key to make the algorithm work

Copy link
Collaborator

@scarlac scarlac Jul 5, 2023

Choose a reason for hiding this comment

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

Yeah maybe. At first it looked like it was using velocity but after further inspection and using the grid on the native camera to align my fingers, it looks like there's no velocity involved.
In that case, we do need the pinchRecognizer.scale and the original code I wrote to use scale to zoom (which I unfortunately never committed).

if pinchRecognizer.state == .changed {
camera.update(zoomScale: zoomScale)
camera.update(pinchVelocity: pinchRecognizer.velocity, pinchScale: pinchRecognizer.scale)
Copy link
Contributor Author

@DavidBertet DavidBertet Jul 4, 2023

Choose a reason for hiding this comment

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

If we do want to use pinchRecognizer.scale here (seems not used)

  • Should put the line pinchRecognizer.scale = 1.0 first?
  • Or move it when we set it up?
  • Well, not sure what it does :D and it's late...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't matter a ton to me. I was just playing with various algorithms to get an improved behavior.

@@ -244,36 +253,33 @@ class RealCamera: NSObject, CameraProtocol, AVCaptureMetadataOutputObjectsDelega
self.update(torchMode: self.torchMode)
}
}


func counterRotatedCaptureVideoOrientationFrom(deviceOrientation: UIInterfaceOrientation) -> AVCaptureVideoOrientation? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by counterRotate? seems like just a type conversion (at least as of now)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was very tired and had to finish up. It was because UI and hardware orientation are opposed to each other in landscape mode (device is rotated left, so UI must rotate right to be readable).
The switch was indeed counter rotating until the end.
I think it's because initializeMotionManager is already counter-rotating the device orientation, which is a bug.

So it may be worth checking if we should:

  1. Keep the name the same
  2. Change initializeMotionManager so it doesn't counter-rotate
  3. Change the initial value of self.deviceOrientation when it's initialized to the UI orientation such that it also counter-rotates back (so if UI is in landscape-left, device orientation must be assumed to be landscape-right).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I'm currently looking at 2.

Copy link
Contributor Author

@DavidBertet DavidBertet Jul 4, 2023

Choose a reason for hiding this comment

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

@scarlac I refined the orientation implementation. Let me know what you think - feel free to revert if you preferred yours

Every time UIDeviceOrientation is converted to AVCaptureVideoOrientation, we counter rotate it
It applies to

  • Accelerometer, which is now setting deviceOrientation (UIDeviceOrientation), not counter rotated
  • uiOrientationChanged, by accessing device.orientation

(requires a yarn bootstrap)

Copy link
Collaborator

@scarlac scarlac Jul 5, 2023

Choose a reason for hiding this comment

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

UIDeviceOrientation and AVCAptureVideoOrientation are not the same thing. They are conceptually different and so you cannot use them as the same value.
When you orient the device left, the UI must rotate right. The counter-rotation function is necessary.
Can we undo those changes? The code is necessary. You cannot simplify it to the same value. We must have the actual device orientation, both for onOrientationChange() events and for selectively updating the video stream orientation. Both are needed, separately, for separate purposes.

Copy link
Contributor Author

@DavidBertet DavidBertet Jul 5, 2023

Choose a reason for hiding this comment

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

Yes, I think we are aligned.

Current code does counter rotate when converting device to interface/video. Everything is exact same as yours. I only fixed the bug so initializeMotionManager doesn't counter rotate deviceOrientation, BUT happens later while used.

What's misleading is the name UIDeviceOrientation which contains UI & Device

  • UIDeviceOrientation is physical / device
  • UIInterfaceOrientation is interface
  • AVCAptureVideoOrientation is video, synced with interface

Converting UIDeviceOrientation to UIInterfaceOrientation/AVCAptureVideoOrientation requires counter rotation left/right.
That counter rotation is abstracted in the conversion extension, so as a consumer they don't have to know about those subtleties

  • Asking for .videoOrientation from a UIInterfaceOrientation IS NOT counter rotated
  • Asking for .videoOrientation from a UIDeviceOrientation IS counter rotated

I'm fine using methods, it's mainly code style.

@@ -262,15 +262,21 @@ class CameraView: UIView {
}
}

private func writeCaptured(imageData: Data,
private func writeCaptured(imageData: Data,
thumbnailData: Data?,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you getting a thumbnail? Seems like we have to explicitly enable it

If you requested a preview image by specifying the previewPhotoFormat property of your photo settings when requesting capture, this property offers access to the resulting preview image pixel data

https://developer.apple.com/documentation/avfoundation/avcapturephoto/2873984-previewpixelbuffer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it's just laying some preparation for later changes. I had it working locally but have not committed enabling it since it needs several other features enabled and must be an opt-in feature. We can leave it in for now.

the main thread and session configuration is done on the session queue.
*/
DispatchQueue.main.async {
var videoPreviewLayerOrientation = self.counterRotatedCaptureVideoOrientationFrom(deviceOrientation: self.deviceOrientation) ?? self.cameraPreview.previewLayer.connection?.videoOrientation
Copy link
Contributor Author

@DavidBertet DavidBertet Jul 4, 2023

Choose a reason for hiding this comment

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

The variable self.deviceOrientation seems clashing with self.cameraPreview.previewLayer.connection?.videoOrientation

Sticking to one source of truth might help to understand the code, and avoid cases where they are un-synced

If we want to prioritize accelerometer orientation against interface orientation we should make that clear, and sooner

  • Everytime accelerometerOrientation or interfaceOrientation changes, we should do
    previewLayer.connection?.videoOrientation = accelerometerOrientation ?? interfaceOrientation
  • When we take the picture, we just use previewLayer.connection?.videoOrientation

Copy link
Collaborator

Choose a reason for hiding this comment

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

As explained in other comments - no. videoOrientation is not the same as device orientation. You do not always want them to be similar, unfortunately. It comes back to the fact that UIs can rotate, which cause a disconnect between physical orientation and what we know is the right orientation. We do want this disconnect, but we don't want the UI to show it.
If you use the native camera, you'll see that it has the same behavior. UI elements may not rotate, and video stream wont, but captured picture does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Thanks for the explanation, makes sense now.

Could we make it extra clear by renaming deviceOrientation with physicalOrientation / orientationFromAccelerometer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think deviceOrientation is accurate. It's the orientation of the device. The only confusion is that UI orientation, video orientation, and device orientation are all 3 separate concepts that we need to juggle.

@scarlac
Copy link
Collaborator

scarlac commented Jul 4, 2023

If you remove setTorchMode then be careful since it's also implemented in Android and was requested by a few users already:
#523

I honestly don't think setTorchMode is something we should have imperatively - that's what props are for, so if we remove it we need to remove on both platforms and be give an example of what to do instead in a changelog.

@DavidBertet
Copy link
Contributor Author

If you remove setTorchMode then be careful since it's also implemented in Android and was requested by a few users already: #523

I honestly don't think setTorchMode is something we should have imperatively - that's what props are for, so if we remove it we need to remove on both platforms and be give an example of what to do instead in a changelog.

Fully aligned. I wanted your insight before to put it back / remove it from Android.

It comes from a bug, where defaulting torchMode to 'on' doesn't work.
The right fix is to address the timing issue like I did on iOS. Torch should be delayed until camera is ready
The workaround should be to delay setting torchMode by a few ms, than creating another way of doing it.
I'll take a look after this PR being merge.

Make initializeMotionManager private
Use extensions for conversions
Handle accelerometer updates on background thread
Use UIDeviceOrientation for deviceOrientation
Converting UIDeviceOrientation to Interface/VideoOrientation counter-rotate it
@DavidBertet
Copy link
Contributor Author

While looking for leaks, I noticed the CKCameraManager is retained when we go back to the main screen
image
It is released/deinit as soon as we open a new one.

I put that here just in case you have any idea why, might be a React Native thing

@scarlac
Copy link
Collaborator

scarlac commented Jul 5, 2023

As a general comment, try to avoid making extensions. Extensions are non-obvious and global. They can come from anywhere. It makes code hard to read and understand for newcomers so it should be limited to use cases where we cannot do it in a more obvious way.

(I noticed that scaleZoomFactor was moved into the extension, which is unnecessary)

@scarlac
Copy link
Collaborator

scarlac commented Jul 5, 2023

If you remove setTorchMode then be careful since it's also implemented in Android and was requested by a few users already: #523
I honestly don't think setTorchMode is something we should have imperatively - that's what props are for, so if we remove it we need to remove on both platforms and be give an example of what to do instead in a changelog.

Fully aligned. I wanted your insight before to put it back / remove it from Android.

It comes from a bug, where defaulting torchMode to 'on' doesn't work. The right fix is to address the timing issue like I did on iOS. Torch should be delayed until camera is ready The workaround should be to delay setting torchMode by a few ms, than creating another way of doing it. I'll take a look after this PR being merge.

Cool. Let's get rid of setTorchMode on both platforms then.

On a separate note, the code doesn't compile right now. Please make sure I can also run the latest commit that's pushed so I can verify we don't create a regression.

@DavidBertet
Copy link
Contributor Author

On a separate note, the code doesn't compile right now. Please make sure I can also run the latest commit that's pushed so I can verify we don't create a regression.

Did you run yarn bootstrap? I added a file, which requires a pod install

@DavidBertet
Copy link
Contributor Author

As a general comment, try to avoid making extensions. Extensions are non-obvious and global. They can come from anywhere. It makes code hard to read and understand for newcomers so it should be limited to use cases where we cannot do it in a more obvious way.

(I noticed that scaleZoomFactor was moved into the extension, which is unnecessary)

Extensions became fairly common in mobile native world, but agree might be non-obvious developers used to other paradigms.

First, I'm aligned on not spreading logic around.

I used them here only to extract uninteresting boilerplate, not related to the actual camera logic. Like locking configuration to update torchMode / zoomFactor (which can be easily missed), or converting one type to another.
Benefit of having them here is we can easily unit test them, contrary to private methods in consumer class.

However, as you said, if bypassed/duplicated because unknown, it's not any better :)

I can update the code, what do you recommend? My only advice would be to write code that could be unit tested, because there is quite a few subtle things that could be broken fairly easily

@scarlac
Copy link
Collaborator

scarlac commented Jul 6, 2023

As a general comment, try to avoid making extensions. Extensions are non-obvious and global. They can come from anywhere. It makes code hard to read and understand for newcomers so it should be limited to use cases where we cannot do it in a more obvious way.

(I noticed that scaleZoomFactor was moved into the extension, which is unnecessary)

Extensions became fairly common in mobile native world, but agree might be non-obvious developers used to other paradigms.

First, I'm aligned on not spreading logic around.

I used them here only to extract uninteresting boilerplate, not related to the actual camera logic. Like locking configuration to update torchMode / zoomFactor (which can be easily missed), or converting one type to another.

Benefit of having them here is we can easily unit test them, contrary to private methods in consumer class.

However, as you said, if bypassed/duplicated because unknown, it's not any better :)

I can update the code, what do you recommend? My only advice would be to write code that could be unit tested, because there is quite a few subtle things that could be broken fairly easily

Normal functions or public functions can be unit tested. We needn’t unit test every single method though.
Private methods in this context do not help. They are a way to hide methods that we don’t want exposed but we aren’t writing a Swift API so there is no contract to keep or hide, so if it helps, feel free to make it a public method or expose any other methods too.

@scarlac
Copy link
Collaborator

scarlac commented Jul 6, 2023

While looking for leaks, I noticed the CKCameraManager is retained when we go back to the main screen

image

It is released/deinit as soon as we open a new one.

I put that here just in case you have any idea why, might be a React Native thing

RN modules are kept in memory unless we actively delete our stuff. We probably can’t rely on deinit but must use viewDidHide (or whatever the equivalent might be that actually works here).
That’s my only thought right now.
It would be nice to fix although I’d say it’s not important to getting this particular PR merged. It’s likely an old issue if my theory holds up.

@DavidBertet
Copy link
Contributor Author

@scarlac I pushed the things we talked about. Let me know if I forgot anything.

FYI I noticed the examples are broken on Android. Bottom bar appears just below top bar

@scarlac
Copy link
Collaborator

scarlac commented Jul 7, 2023

@scarlac I pushed the things we talked about. Let me know if I forgot anything.

FYI I noticed the examples are broken on Android. Bottom bar appears just below top bar

Ah yes, I'll try to have a look at that if you don't. I made some adjustments to the flexbox sizing of the elements and the viewport on Android has 0 height by default whereas iOS viewport is larger.

@DavidBertet
Copy link
Contributor Author

DavidBertet commented Jul 7, 2023

Ah yes, I'll try to have a look at that if you don't. I made some adjustments to the flexbox sizing of the elements and the viewport on Android has 0 height by default whereas iOS viewport is larger.

👍. I can look at it, I just need to know what's the expected behavior.

On the existing CameraScreen the preview seems to be full screen on Android, while "centered" on iOS. But it probably means Android camera preview is cropped left/right?

Same, what do you expect for the preview. Full screen or centered.

I would go centered for both platform/use case to simplify the example, but let me know what you expected

@scarlac
Copy link
Collaborator

scarlac commented Jul 7, 2023

Ah yes, I'll try to have a look at that if you don't. I made some adjustments to the flexbox sizing of the elements and the viewport on Android has 0 height by default whereas iOS viewport is larger.

👍. I can look at it, I just need to know what's the expected behavior.

On the existing CameraScreen the preview seems to be full screen on Android, while "centered" on iOS. But it probably means Android camera preview is cropped left/right?

Same, what do you expect for the preview. Full screen or centered.

I would go centered for both platform/use case to simplify the example, but let me know what you expected

Centered, yes. 4:3 locked format for this example, as you've probably noticed. It's for a future update to somehow detect camera dimensions but the majority of camera/sensors are 4:3.
The challenge is that even 4:3 may be too large on some small/slim screens, so even centered can be a challenge. For the example project we shouldn't spend too much time optimizing the UI. In the end, it's just an example, not a "perfect" implementation.

@DavidBertet
Copy link
Contributor Author

I pushed a first shot.

iOS Android
Zones
Real

My Android camera is cropped a bit, but might be acceptable.

What's not great is the Android scan target isn't centered anymore (but crop is, camera is cropped same size top/bottom)

Something to look at later

@scarlac
Copy link
Collaborator

scarlac commented Jul 10, 2023

Thanks David. I'll fix the frame on Android.

@scarlac scarlac merged commit 6e05414 into teslamotors:master Jul 12, 2023
0 of 3 checks passed
@scarlac
Copy link
Collaborator

scarlac commented Jul 12, 2023

Thank you so much @DavidBertet, this was extremely helpful! Hopefully this will encourage further high quality contributions now that we're mainly using Swift.
🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants