Add auto rotate, auto scan, and Vision support #41

julianschiavo · 2018-07-27T08:03:37Z

This pull request adds support for auto rotating images to Portrait on iOS 10 and above, and uses Vision instead of CoreImage to detect rectangles on iOS 11 and above. It also adds iPad support to the sample project.

Auto rotation, auto scanning, and Vision support have been fully tested by me, but I'd appreciate any feedback and improvements.

…t tested yet)

julianschiavo · 2018-07-27T12:14:48Z

I've just pushed a commit to actually send the rotated image to didFinishScanningWithResults.

AvdLee

Thanks for your contribution, really great!! 💪

I got a few remarks, let me know your thoughts on them.
Also, would it be possible to write some tests for the added code? I feel like some parts are at least easy to test.

Further, you mentioned Vision support may not work yet . Can you elaborate a bit more on that? Does that mean we're merging in non-working code here? I might lack a bit of knowledge here, but I'm happy to learn from you :)

AvdLee · 2018-07-27T13:28:00Z

WeScan/Common/RectangleDetector.swift

+
+                    } else { completion(nil) }
+                })
+                rectDetectRequest.minimumConfidence = 0.7 // Be confident.


The Be confident. doesn't really add any value and could be removed

This wasn't my contribution.

No worries, let's not blame anyone. Either way, would you mind removing it within these code changes? @Boris-Em what are your thoughts here?

I've removed it with the commit 6d5573f.

AvdLee · 2018-07-27T13:30:03Z

WeScan/Common/RectangleDetector.swift

-
-        guard let rectangleFeatures = rectangleDetector?.features(in: image) as? [CIRectangleFeature] else {
-            return nil
+    static func rectangle(forImage image: CIImage, completion: @escaping ((Quadrilateral?) -> ())) {


It would be great if we can use the Result enum here for completion, including the Quadrilateral if it succeeded and the occurred error if it failed

It's quite late here, I'll look at implementing that tomorrow.

AvdLee · 2018-07-27T13:32:45Z

WeScan/Common/RectangleDetector.swift

+            let biggestRectangle = Quadrilateral(rectangleFeature: biggest)
+
+            completion(biggestRectangle)
+


We more or less have two detectors here, one using CoreImage and one using Vision. Can we make these two separated methods or even classes for readability, ending up with code like for example:

if #available(iOS 11.0, *) { VisionRectangleDetector.detectRectangle(in: image, completion: ..) } else { CIRectangleDetector.detectRectangle(in: image, completion: ..) }

Keeping the code separated also makes it easier for us to write tests 🎉

It's quite late here, I'll look at implementing that tomorrow.

I've separated the detectors with commit e7b07ac.

AvdLee · 2018-07-27T13:33:46Z

WeScan/Scan/CaptureSessionManager.swift

 import AVFoundation

+var editImageOrientation: CGImagePropertyOrientation = .downMirrored


For what reason are we defining this property as a global? Can't we solve this in a different way?

This is to rotate the image when it's being passed to the ReviewVC, so that it'll work if the person rotates their phone after scanning a picture.

I understand, but why do we define it as a global parameter and not just as a property within a class for example?

The CaptureSessionManager class is initialized from the ScannerViewController, while the ReviewViewController is initialized from the EditScanViewController (so it's hard for them to access each other's properties). I think it would be best to keep this as a global property, but if anyone knows a better way of doing this feel free to suggest it and i'll push it.

Would it be possible to dependency inject this value through the view controllers? We might even want to create a wrapper class CaptureSession or thinking about it, a singleton class for this case?

CaptureSession.current which always keeps the current capture session.

@Boris-Em can you think with us here, I feel like this is getting to be an important change for the library.

I'm in favor of using a Singleton in this case.
There can only be one orientation and one capture session at any given time, and they're relevant during the whole life of the scanner.
I would use CaptureSession.current.

This has finally been implemented in 9083131 using CaptureSession.current.

AvdLee · 2018-07-27T13:34:40Z

WeScan/Scan/RectangleFeaturesFunnel.swift

@@ -90,7 +91,7 @@ final class RectangleFeaturesFunnel {
            bestRectangle.rectangleFeature.isWithin(matchingThreshold, ofRectangleFeature: previousRectangle) {
        } else if bestRectangle.matchingScore >= minNumberOfMatches {
            completion(bestRectangle.rectangleFeature)
-        }
+        }*/


If we don't need this code anymore, can we get rid of it?

Which code?

AvdLee is referring to the dead code (code commented out).

Ah, I understand. Not using it seems to work on iOS 10, but again, I haven't had a chance to test on iOS 11. As soon as I do, i'll either leave it in/take it out.

Update on this: the commented out code makes the rectangles very jerky on all versions of iOS. I'm looking at reimplementing this code soon.

Edit: Re-adding the commented out code makes the rectangles very inaccurate, but commenting it out makes it very jerky. I'm working on making it better but it might take some time.

Re-added with some changes. See commit f1e01c1.

julianschiavo · 2018-07-27T13:56:58Z

Vision scanning may not work because I implemented it a few weeks ago and haven't had time to test it, while i've tested auto rotate code. I don't think this PR should be merged yet, it's here so people (and me) can test it and suggest things 😄

jcampbell05 · 2018-07-27T14:19:10Z

Works better with some of the more complicated documents on our iOS 11 device which previously failed under the old code

The only piece of feedback we have is we have to be slow when scanning as the rectangle jumps around a bit. Not sure if that's a bug with this framework or the way this library processes the incoming information but we need to improve the snapping so that it's instant and consistent :)
2018-07-27 15.18.01.mp4.zip

I've attached a video for you, @Boris-Em and @AvdLee to see as an example

julianschiavo · 2018-07-27T14:39:41Z

That's probably because of the commented out code (see AvdLee's requested changes), it happens on iOS 10 as well. I'll work on it tomorrow 😄

Also, to fix the rotation issue at the end of your video, hold the phone up more so it's obvious to the accelerometer that you're in portrait.

jcampbell05 · 2018-07-27T15:27:12Z

Okay what the rotation issue exactly?

…rs for cleaner code

julianschiavo · 2018-07-28T05:42:08Z

I've fixed some of the issues raised in the review and am working on the others. Let me know if there's anything else I should change/add 👍 😄

… auto scanning (setting for it coming soon), and other changes

julianschiavo · 2018-07-28T09:16:39Z

I've just pushed a big commit with auto scanning support (setting for it coming soon) as well as more accurate Vision scanning and iPad support on the sample project.

Edit: Just pushed a commit to add a setting for auto scan.

…shutter button

…anner vc) with a flash and auto-scan setting, to resemble Apple's Notes document scanner

AvdLee

Great to see the changes, the VisionRectangleDetector and CIRectangleDetector are exactly what I meant! Awesome :)

Also great to see you've pushed even more features, but I do want to ask you to create a separated PR next time to prevent big PRs.

Big PRs slows down reviews in general and makes it harder for the creator to keep push needed changes.

For this PR, would it be possible to write some tests? Quite some code is testable and should be tested for future maintenance.

Thanks again!

AvdLee · 2018-07-30T07:16:21Z

WeScan/Scan/CaptureSessionManager.swift

 import AVFoundation

+var editImageOrientation: CGImagePropertyOrientation = .downMirrored


Would it be possible to dependency inject this value through the view controllers? We might even want to create a wrapper class CaptureSession or thinking about it, a singleton class for this case?

CaptureSession.current which always keeps the current capture session.

@Boris-Em can you think with us here, I feel like this is getting to be an important change for the library.

AvdLee · 2018-07-30T07:17:16Z

WeScan/Scan/CaptureSessionManager.swift

+        if let rectangle = rectangle {
+
+            self.noRectangleCount = 0
+            self.rectangleFunnel.add(rectangle, currentlyDisplayedRectangle: self.displayedRectangleResult?.rectangle) { (rectangle, shouldAutoScan) in


If this method is asynchronous, we should capture self weak here. It might be good to in general capture self weakly to prevent us from unwanted retains.

AvdLee · 2018-07-30T07:18:51Z

WeScan/Scan/RectangleFeaturesFunnel.swift

+            if thresholdPassed > 30 {
+                thresholdPassed = 0
+                completion(bestRectangle.rectangleFeature, true)
+                // Scan the rectangle automatically by passing True to the completion


the fact that we need this documentation here implies a bad code pattern. Code should try to be self explaining as much as possible.

Can we replace the completion callback with something more describing letting the code explain that we're scanning the rectangle automatically or not?

Done: I replaced the Bool with a new enum, AddResult, that describes whether we're auto scanning or only showing the rectangle.

AvdLee · 2018-07-30T07:19:16Z

WeScan/Scan/ScannerViewController.swift

@@ -9,6 +9,9 @@
 import UIKit
 import AVFoundation

+/// Whether auto scan is enabled or not (has to be public to be seen by CaptureSessionManager)
+var autoScanEnabled = true


@Boris-Em this would also be something for the currentSession I mentioned before.

Yep. Same thing here.

AvdLee · 2018-07-30T07:23:28Z

WeScan/Scan/ScannerViewController.swift

+    func toggleTorch(shouldTurnOn: Bool) {
+        guard let device = AVCaptureDevice.default(for: AVMediaType.video) else { return }
+
+        if device.hasTorch {


This can be part of the guard

AvdLee · 2018-07-30T07:24:20Z

WeScan/Scan/ScannerViewController.swift

+                device.torchMode = shouldTurnOn ? .on : .off
+                device.unlockForConfiguration()
+            } catch {
+                return


If we don't use the catch as we do here, it might be better to use try? instead and get rid of the do catch

…ed code to clean up the project

julianschiavo · 2018-07-30T09:38:38Z

Sorry for the big PR! I've resolved all the issues except for the global variable, where I'm waiting for @Boris-Em's input.

Edit: As for the tests, I only started learning Swift 4-5 months ago and have no experience with testing. If anyone would like to contribute tests, they can, but I'm not able to.

Boris-Em

Thanks a lot for the contribution!

WeScan/Common/Quadrilateral.swift

Boris-Em · 2018-07-30T10:03:37Z

WeScan/Scan/CaptureSessionManager.swift

 import AVFoundation

+var editImageOrientation: CGImagePropertyOrientation = .downMirrored


I'm in favor of using a Singleton in this case.
There can only be one orientation and one capture session at any given time, and they're relevant during the whole life of the scanner.
I would use CaptureSession.current.

Boris-Em · 2018-07-30T10:07:06Z

WeScan/Scan/CaptureSessionManager.swift

+        var motion: CMMotionManager!
+        motion = CMMotionManager()
+        motion.accelerometerUpdateInterval = 0.2
+        motion.startAccelerometerUpdates(to: OperationQueue()) { p, _ in


I'd be in favor of using descriptive variable names here instead of a and p.

Done (bb2db9d)

Boris-Em · 2018-07-30T10:08:08Z

WeScan/Scan/CaptureSessionManager.swift

+        }
+    }
+
+    func processRectangle(rectangle: Quadrilateral?, imageSize: CGSize) {


Can't this function be private?

Done (bb2db9d)

Boris-Em · 2018-07-30T10:10:35Z

WeScan/Scan/ScannerViewController.swift

@@ -9,6 +9,9 @@
 import UIKit
 import AVFoundation

+/// Whether auto scan is enabled or not (has to be public to be seen by CaptureSessionManager)
+var autoScanEnabled = true


Yep. Same thing here.

jcampbell05 · 2018-10-30T14:37:08Z

Unfortunately, I found it incredibly hard to reproduce as it happened randomly. I think we could just add a comment explaining the fix?

Yeah I guess it must be eliminating false negatives by resetting the rectangles.

julianschiavo · 2018-10-30T14:40:36Z

Yeah

P.S. Interesting use of Apple event time 👀

jcampbell05 · 2018-10-30T14:49:25Z

P.S. Interesting use of Apple event time 👀

Usually iit's at 6pm in the UK :) so I didn't realise

jcampbell05 · 2018-11-01T12:27:40Z

@Boris-Em can you re-review ? :)

Boris-Em · 2018-11-01T14:11:29Z

Sorry guys for the delay. I'll have time to help and review the PRs tomorrow!

Boris-Em

Great work! Thanks for all of the changes =)

jcampbell05 · 2018-11-05T17:41:08Z

Woop :) 🎂Time to merge!

AvdLee · 2018-11-06T09:18:22Z

@justjs if you can merge in develop for the latest changes, CI will probably succeed. After that, you can merge the PR in. Let me know if you have any questions

julianschiavo · 2018-11-06T12:36:49Z

Uhm I’m kinda in Japan atm (no computer). If someOne makes a PR I’ll merge it, otherwise I’ll get back in a week or so.

jcampbell05 · 2018-11-06T13:46:02Z

I'll see what I can do :)

ghost · 2018-11-10T01:39:12Z

WeScan/Scan/CaptureSessionManager.swift

 }

 extension CaptureSessionManager: AVCapturePhotoCaptureDelegate {
-
+    
    func photoOutput(_ captureOutput: AVCapturePhotoOutput, didFinishProcessingPhoto photoSampleBuffer: CMSampleBuffer?, previewPhoto previewPhotoSampleBuffer: CMSampleBuffer?, resolvedSettings: AVCaptureResolvedPhotoSettings, bracketSettings: AVCaptureBracketedStillImageSettings?, error: Error?) {


⚠️ Function should have 5 parameters or less: it currently has 6

WeScanTests/CIRectangleDetectorTests.swift

julianschiavo · 2018-11-10T01:41:36Z

This should be good to go! I just have one last commit to push to update the tests for iOS 12.1, which will silence the bot.

julianschiavo · 2018-11-10T01:54:37Z

Done. Ready to merge @jcampbell05 @AvdLee @Boris-Em

jcampbell05 · 2018-11-12T00:46:59Z

All green :) @Boris-Em and @AvdLee

julianschiavo · 2018-11-12T11:22:51Z

🎉 🎊 We finally made it! It's taken ages but i'm pretty happy with the final result 😅

jcampbell05 · 2018-11-12T11:32:31Z

@justjs really great work :)

AvdLee · 2018-11-12T13:17:26Z

Great work @justjs @jcampbell05, thanks for all the work and positive energy to work on our feedback. Highly appreciated!

julianschiavo · 2018-11-12T13:19:20Z

Thanks again everyone for the feedback + help with tests 😄

Boris-Em · 2018-11-13T10:56:14Z

@justjs - You should be, this is really great!

adityativ-issart · 2018-12-28T07:11:29Z

@justjs how can I add auto capture? It doesn't work to me right now

julianschiavo · 2018-12-28T09:28:53Z

It’s available on the latest develop branch (maybe master too). Make sure you’re on Auto not Manual and it only works if the rectangle is very accurate.

…

Sent from my iPhone

On Dec 28, 2018, at 3:11 PM, adityativ-issart ***@***.***> wrote: @justjs how can I add auto capture? It doesn't work to me right now — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Auto rotate images to portrait (fully tested), add vision support (no…

927c7c3

…t tested yet)

julianschiavo mentioned this pull request Jul 27, 2018

OpenCV for document edge detection #22

Closed

Boris-Em requested review from Boris-Em, AvdLee and renssies July 27, 2018 11:59

julianschiavo added 3 commits July 27, 2018 20:11

Actually send the rotated image to didFinishScanningWithResults

46c701a

Merge remote-tracking branch 'refs/remotes/origin/develop'

3d4f4e3

(Again) Actually send the rotated image to didFinishScanningWithResults

497c587

AvdLee requested changes Jul 27, 2018

View reviewed changes

julianschiavo added 2 commits July 28, 2018 13:27

Remove the useless "be confident" comment

6d5573f

Separate the iOS 11 (vision) and iOS 10 (CoreImage) rectangle detecto…

e7b07ac

…rs for cleaner code

julianschiavo mentioned this pull request Jul 28, 2018

Rotation Support on Edit Screen #39

Closed

julianschiavo added 2 commits July 28, 2018 15:28

Add iPad support for the sample project

483177b

Fix Vision's rectangle detector scaling (makes it more accurate), add…

f1e01c1

… auto scanning (setting for it coming soon), and other changes

julianschiavo changed the title ~~Add auto rotate and Vision support~~ Add auto rotate, auto scan, and Vision support Jul 28, 2018

julianschiavo added 3 commits July 28, 2018 18:12

Add a button and variable to enable or disable auto scan next to the …

b978dc6

…shutter button

Switch the icon states on the auto scan button

812771b

This commit replaces the nav bar with a toolbar (only while in the sc…

8e9fcf7

…anner vc) with a flash and auto-scan setting, to resemble Apple's Notes document scanner

AvdLee requested changes Jul 30, 2018

View reviewed changes

This commit fixes most issues raised by @AvdLee, and removes old unus…

556c8ed

…ed code to clean up the project

Boris-Em requested changes Jul 30, 2018

View reviewed changes

C.

21bf8b7

This was referenced Oct 23, 2018

Filter the cropped image as black and white or even better based on user selection! #72

Closed

Updated our CI implementation to prevent unneeded force unwraps #73

Merged

Boris-Em approved these changes Nov 5, 2018

View reviewed changes

julianschiavo added 2 commits November 10, 2018 09:32

OK

547db28

t push origin developMerge branch 'WeTransfer-develop' into develop

6a066ec

ghost reviewed Nov 10, 2018

View reviewed changes

WeScanTests/CIRectangleDetectorTests.swift Show resolved Hide resolved

ghost reviewed Nov 10, 2018

View reviewed changes

WeScanTests/CIRectangleDetectorTests.swift Show resolved Hide resolved

Add reference image for iOS 12.1

565bf09

Boris-Em merged commit ea0a658 into WeTransfer:develop Nov 12, 2018

julianschiavo mentioned this pull request Nov 12, 2018

Swift 4.2 support for iOS 12 SDK #77

Closed

		let biggestRectangle = Quadrilateral(rectangleFeature: biggest)

		completion(biggestRectangle)

		import AVFoundation

		var editImageOrientation: CGImagePropertyOrientation = .downMirrored

Add auto rotate, auto scan, and Vision support #41

Add auto rotate, auto scan, and Vision support #41

Conversation

julianschiavo commented Jul 27, 2018 • edited

julianschiavo commented Jul 27, 2018

AvdLee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianschiavo Jul 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianschiavo Jul 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianschiavo Jul 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianschiavo commented Jul 27, 2018

jcampbell05 commented Jul 27, 2018 • edited

julianschiavo commented Jul 27, 2018 • edited

jcampbell05 commented Jul 27, 2018

julianschiavo commented Jul 28, 2018

julianschiavo commented Jul 28, 2018 • edited

AvdLee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianschiavo commented Jul 30, 2018 • edited

Boris-Em left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcampbell05 commented Oct 30, 2018

julianschiavo commented Oct 30, 2018

jcampbell05 commented Oct 30, 2018

jcampbell05 commented Nov 1, 2018

Boris-Em commented Nov 1, 2018

Boris-Em left a comment

Choose a reason for hiding this comment

jcampbell05 commented Nov 5, 2018

AvdLee commented Nov 6, 2018

julianschiavo commented Nov 6, 2018

jcampbell05 commented Nov 6, 2018

ghost Nov 10, 2018 • edited by ghost

Choose a reason for hiding this comment

julianschiavo commented Nov 10, 2018

julianschiavo commented Nov 10, 2018

jcampbell05 commented Nov 12, 2018

julianschiavo commented Nov 12, 2018

jcampbell05 commented Nov 12, 2018

AvdLee commented Nov 12, 2018

julianschiavo commented Nov 12, 2018

julianschiavo commented Jul 27, 2018 •

edited

julianschiavo Jul 28, 2018 •

edited

julianschiavo Jul 28, 2018 •

edited

julianschiavo Jul 28, 2018 •

edited

jcampbell05 commented Jul 27, 2018 •

edited

julianschiavo commented Jul 27, 2018 •

edited

julianschiavo commented Jul 28, 2018 •

edited

julianschiavo commented Jul 30, 2018 •

edited

ghost Nov 10, 2018 •

edited by ghost