search

opencv / opencv

Open Source Computer Vision Library
https://opencv.org
Apache License 2.0
75.95k stars 55.62k forks source link

Objc binding #17165

Closed komakai closed 3 years ago

komakai commented 4 years ago

Pull Request Readiness Checklist

This Pull Request solves #17164

This Pull-Request consists:

New build options: “legacy_build” - if specified will build the old opencv2 framework without the Objective-C/Swift wrapper as if this pull request never existed “framework_name” - defaults to OpenCV (to match the capitalization of other popular Apple frameworks like OpenGL,OpenAI etc) but can be set to an arbitrary value (for example opencv2 for backwards compatibility)

More details here regarding the motivation for this contribution: https://medium.com/@gilespayne/opencv-4-4-lands-with-out-of-the-box-support-for-swift-and-objective-c-cdf3904066f1

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • [x] I agree to contribute to the project under OpenCV (BSD) License.
  • [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • [x] The PR is proposed to proper branch
  • [x] There is reference to original bug report and related work
  • [x] The feature is well documented and contains samples and tests
force_builders_only=Mac,iOS,Custom Mac,Docs
build_image:Custom Mac=osx_framework
buildworker:Custom Mac=macosx-2
buildworker:iOS=macosx-1
build_upload:iOS=ON
komakai commented 4 years ago

@alalek Would it be possible to upgrade the cmake on the Mac test machines to version 3.16 ? Otherwise we get caught by this issue https://gitlab.kitware.com/cmake/cmake/issues/19699 (this would only be required for the Objective-C/Swift wrapper support - other builds could still work with the older cmake)

alalek commented 4 years ago

Installed CMake 3.17.1 on macosx-2 build worker.

Currently build fails on:

build-armv7-iphoneos/lib/Release/OpenCV.framework/Headers/ArrayUtil.h:9:1: error: unknown type name 'NSMutableArray'

komakai commented 4 years ago

Installed CMake 3.17.1 on macosx-2 build worker.

Thanks! All errors and warnings should be fixed now.

komakai commented 4 years ago

@alalek the build is OK now - currently it is failing on the packaging stage because I have changed the framework name from opencv2 to OpenCV. There are two reasons I wanted to make this change, firstly because the 2 on the end doesn't make a lot of sense (current version is actually 4 but I don't see any real reason to have the version in the framework name). Second reason is to copy the capitalization of other common Mac/iOS frameworks like OpenGL, OpenCL, OpenAL. If you are OK with the change please let me know how to proceed so that the packaging stage can pass. If not it's no problem to change the name back to the old name.

alalek commented 4 years ago

I just enabled full pipeline to avoid surprises on nightly validation. I'm OK with renaming in general, we just need to update infrastructure scripts (perhaps after infra shutdown during next week).

@vpisarev Please take a look on renaming process and high-level overview of this PR.

komakai commented 4 years ago

we just need to update infrastructure scripts (perhaps after infra shutdown during next week).

Great - could we also add new tasks to run the test scripts I've added? The command lines to run the tests are output at the end of the macOS and iOS build scripts

komakai commented 4 years ago

@alalek I have another PR ready with auto-generation of documentation here: https://github.com/komakai/opencv/tree/objc-binding-doc but since we're still waiting to update the build scripts maybe I can just merge it in here?

(For info the generated docs will look like this: http://xtravision.stars.ne.jp/opencv-objc-doc-test/docs/index.html )

vpisarev commented 3 years ago

@komakai, thank you for the contribution! And sorry for very long delay with the review. I've just started it, but hopefully, I can do it reasonably fast.

I have some first questions about your patch.

  1. I see that the big part of your patch consists of manually written wrappers for some basic OpenCV data types, such as Point, Rect, Mat, etc. – overall it's more than 100 files in opencv/modules/core/misc/objc/common. I wonder whether it's possible to generate most of these wrappers on-fly as a custom build step and put somewhere to CMAKE_BUILD_DIR?

  2. More general question about data structures. I see that Swift has tuple data type. In Python we just use tuple for points, rectangles and other such structures. Whether it makes sense to follow this approach with Swift?

  3. Can you please put description somewhere (maybe here) on how to run the regression tests. I generated OpenCV.framework successfully and it seems to have objc bindings built-in. How do I use the run_tests.py script properly?

komakai commented 3 years ago

@vpisarev thanks for your questions!

I wonder whether it's possible to generate most of these wrappers on-fly as a custom build step

I mostly just copied the way the Java wrapper works - which also has a larger number of hand-coded classes. I have had a look at the hand-coded classes and found a few that aren't actually needed {Int,Float,Double}Out (I found a way to do handle out parameters without these special wrappers) and some others that are all very similar {Int,Float,Double}{n} which could be easily generated. I'll try and add a commit to reduce the number of manually coded classes in the next week or so.

I see that Swift has tuple data type.

Unfortunately we have to bridge to Swift via Objective-C which does not have tuple support. I plan at some point to generate Swift extensions to handle cases where there is a more natural Swift way of doing things - so I'll look at using tuples when I'm doing that.

How do I use the run_tests.py script properly?

If you get it to build successfully then you should see a line like this at the end of the build output:

To run tests call: ./opencv/platforms/osx/run_tests.py --framework_dir=/Users/gpayne/ContribNew/mactest25 /Users/gpayne/ContribNew/mactest25/build/build-x86_64-macosx/modules/objc/test

the --framework_dir parameter should point to the folder in which the built framework is located. The /Users/gpayne/ContribNew/mactest25/build/build-x86_64-macosx/modules/objc/test refers to a folder that is created during the build that contains the actual tests themselves, a CMakeLists.txt file for generating the Xcode test project and some dummy files needed to get the test project to build successfully.

Hope that helps - let me know if you have any more questions

vpisarev commented 3 years ago

@komakai, thank you! I ran the tests successfully. It looks like only core and imgproc are covered; which is fine for the first pull request, of course. I think, it would be nice to reduce this data types wrapper code, especially since you already working on it. Then the pull request can be merged, I think

alalek commented 3 years ago

@vpisarev Regarding manual basic data types please take a look on Java bindings. Example. Perhaps ObjC bindings are based on this approach. Number of basic data types is limited, so it should not be a real problem with scalability.

I see that Swift has tuple data type. In Python we just use tuple for points, rectangles and other such structures.

Current Python practice shows that "tuple everywhere" approach is very error prone without any reliable controls or checks. Also this approach requires heavy logic on bindings side.

vpisarev commented 3 years ago

@alalek, I think, I agree on the second item. Using specialized types is more safe. When you work with Python for a very long time, you start thinking in terms of tuples, but with languages like Swift, where structures are pretty efficient, it's better to use them.

On the first item. Well, of course, it's the implementation detail. I just thought that if we need to extend the types somehow, e.g. for some future versions of Swift, it could be more convenient to do it in the single place, in the script, rather than edit 100+ files manually. But you are right, it can wait.

So, let's make buildbot "all-green" and then it can be merged

komakai commented 3 years ago

@alalek thanks for your comments! I think there may still be some use for Swift tuples - but let's look at that later. @vpisarev thanks for approving! I have two more commits I want to push

alalek commented 3 years ago

@komakai Great Job!

Is it necessary to change framework name (opencv2 => OpenCV)?

Usually it is used for automatic handling of include names (OpenCV uses includes in this form "opencv2/core.hpp"). So changing framework name would break already existed Users code (which we can't fix).

Apple guidelines: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Tasks/IncludingFrameworks.html

Related code in the patch:

import <OpenCV/OpenCV.h>

Migration on OpenCV.framework can be done if @vpisarev approves this. But probably we should do this "breaking change" for the next major release (OpenCV 5.0). For current release series OpenCV 4.x I would suggest to keep previous behavior by default (and enable new one through flag - like inverse variant of legacy_build).

opencv.hpp is deprecated header. It is better to include used modules directly (similar to C++ code without extra changes).

samples/swift/ios/FaceDetection/FaceDetection/lbpcascade_frontalface.xml

Do we need to make a copy of this file?

vpisarev commented 3 years ago

@komakai, I agree with @alalek, we should postpone renaming opencv2 => OpenCV till the next release, at least, otherwise it will break C++/Objective C++ apps that use the framework.

On the planned commits:

komakai commented 3 years ago

@alalek @vpisarev Regarding postponing the name change 👍 @vpisarev the new out parameter handling and the extra wrappers are already merged @alalek the samples really need to be self contained - having a link to a file somewhere outside the sample directory would be really troublesome I think

Latest changes:

Still to do:

OpenCV 4.4 code freeze is around June 9th

OK - understood I will make the above changes in plenty of time for the 4.4 code freeze

komakai commented 3 years ago

OK - should be ready to go now. Latest commits:  - changed the default framework name back to opencv2  - fixed iOS Swift samples  - fixed randomly failing test  - added converter functions to same level as the Java Converter util class @alalek About including "opencv.hpp": On Apple platforms, because of the Detected Apple 'NO' macro definition, it can cause conflicts problem, it turns out you basically have to pull in all of OpenCV before you include any Apple headers. Including "opencv.hpp" is the best way to do this so I have left that part as is.

vpisarev commented 3 years ago

@komakai, I checked it on my machine – compiles fine and the tests ran successfully. I see, however, that the patch produces several warnings in iOS build. Could you please look at it?

alalek commented 3 years ago

Warning note:

warning: 'finalize' is deprecated CV_DEPRECATED_EXTERNAL

Please ensure that __OPENCV_BUILD=1 definition is passed to bindings compiler.

komakai commented 3 years ago

Latest changes:

@vpisarev For the documentation generation can you install Jazzy on the build machine? gem install jazzy Reference: https://github.com/realm/jazzy

Then I'll make one more commit to fix the remaining warning and we should be good

vpisarev commented 3 years ago

@komakai, I've installed jazzy and successfully built the docs. It looks great, sometimes better than the "native" C++ documentation!

The only two complaints I have (or rather ideas for possible improvements in the future updates) are:

  1. modules are put into "Classes" category, which is not very convenient
  2. I do not see search function. I see quite big search.js script in the root "docs" directory, but it's not exposed anywhere.

In any case, it's definitely good to go. Just, please, resolve the problems with iOS builder in order to squeeze this patch into OpenCV 4.4

komakai commented 3 years ago

@vpisarev I checked why the search feature wasn't working on Jazzy - turns out it is not currently supported for the default theme. I have created a PR against Jazzy that fixes this here: https://github.com/realm/jazzy/pull/1201 Unfortunately in Objective-C and Swift there is no real way to differentiate between Modules and Classes. What I can do is generate documentation comments at the top of each class of the form "This class belongs to the XXX module" and at the top of each module of the form "The XXX module is comprised of classes AAA, BBB, CCC, ...". That should make it easier to navigate between classes in the same module. (This would be a separate PR later) @alalek Looks like all builds are finally passing - hopefully we're good to go now

komakai commented 3 years ago

I updated the documentation at http://xtravision.stars.ne.jp/opencv-objc-doc-test/docs/index.html to show what it will look like with the search feature working

komakai commented 3 years ago

Observed on Mac Mini with OSX 10.14, Xcode 10.0.1.10010046 and CMake 3.15

@alalek can you test again with the latest commit on the above configuration? If it works then we can lower the required cmake version to 3.15

alalek commented 3 years ago

@komakai Changed iOS builder. This patch builds on mentioned configuration (please ignore BUILD_PRECOMMIT change - it is not needed)

komakai commented 3 years ago

@alalek @vpisarev Awesome - thanks for your help in getting this merged. Now we just need to tell everyone about it and get them using it!

vpisarev commented 3 years ago

@komakai, thank you too for the excellent work! I think, it will be one of the major highlights of OpenCV 4.4 release! We will put it to the ChangeLog and the release announce

ianboru commented 3 years ago

@komakai this looks like a legendary feature and like something I've been trying to find for over a year. I'm not sure if I missed it somewhere but are there docs on how to use this? I currently have a project using a bridging header between swift and objC but I am struggling to make expose data from objC to Swift (currently opencv vector data types). Will this by chance help with that?

komakai commented 3 years ago

@ianboru Since this is not in an official release yet you will need to make a build to get started like ./opencv/platforms/ios/build_framework.py my_build then take the opencv2.framework from the output directory and copy to your project. There is some preview documentation here: http://xtravision.stars.ne.jp/opencv-objc-doc-test/docs/index.html Also you can take a look at the samples in samples/swift to help you get started. If you find there is something you want to do that the current Swift integration doesn't allow you to do, please create an issue and I'll see about adding support for it

komakai commented 3 years ago

I published an article here to explain what this contribution is about: https://medium.com/@gilespayne/opencv-4-4-lands-with-out-of-the-box-support-for-swift-and-objective-c-cdf3904066f1 @alalek @vpisarev any chance you could link to the article from the 4.4 release page?

alalek commented 3 years ago

@komakai Added link to ObjC entry here: https://github.com/opencv/opencv/wiki/ChangeLog#version440 Also you may want to add that link into the description of initial PR (17165).

farfromrefug commented 3 years ago

@komakai @alalek who is responsible for the cocoapods ? https://cocoapods.org/pods/OpenCV https://cocoapods.org/pods/OpenCV2 Is there an official one? Would love to see 4.4.0 somewhere there. (and i dont want to create yet another opencv pod package).

mtfrctl commented 3 years ago

@komakai Thank you for your great work! Now I'm enjoying swifty OpenCV programming!! (which was my long-cherished wish!)

By the way, do you have any plans to add data() method which exposes cv::Mat::data (https://docs.opencv.org/master/d3/d63/classcv_1_1Mat.html#a4d33bed1c850265370d2af0ff02e1564) for ObjC/Swift APIs like below? (Example pseudo code for additional data method in opencv/modules/core/misc/objc/common/Mat.mm)

- (NSData*)data {
    return [NSData dataWithBytes: _nativePtr->data length: length-here];
}

Because of there is the convenience Mat<->UIImage conversion method like MatToUIImage (https://github.com/opencv/opencv/blob/master/modules/imgcodecs/src/ios_conversions.mm) only for iOS build but not for OSX(macOS) build (for NSImage/CGImage), I'm struggling to get a data pointer when trying to convert Mat to CGImage or NSImage. (Conventional iOS MatToUIImage uses cv::Mat::data in ObjectiveC++ context, but I want to get a data pointer also in swift API for creating CGImage from Mat.)

If you do not have plans to add the above feature in the near future, may I try to create a PR? (But I'm still wondering what is the right way to modify gen_objc.py for adding NSData*.) I would be grateful if you could consider it.

komakai commented 3 years ago

@mtfrctl Please try the following: https://gist.github.com/komakai/c48fcd738b87e28a482ee34a8f9bdcc9

komakai commented 3 years ago

@mtfrctl Or you can use this trick to create an NSImage:

Imgcodecs.imwrite(filename: "/Users/gilespayne/Temp/temp.jpg", img: mat)
imageView.image = NSImage(contentsOfFile: "/Users/gilespayne/Temp/temp.jpg")
mtfrctl commented 3 years ago

@kobetski Thank you for both ways you have suggested. Due to throughput issues, the former one is more suitable for my purposes than the latter one. I was able to confirm that the CGImage can be generated properly using the pointer obtained by your code! Thank you again.

komakai commented 3 years ago

@mtfrctl if you share your code for creating a CGImage from the raw Mat data then I will try to add Mat <-> NSImage extension functions

mtfrctl commented 3 years ago

@komakai Thank you for your kindness! This time I tried to submit a PR here https://github.com/opencv/opencv/pull/18547. I would be grateful if you could check it. 🙇‍♂️

AlvarHHM commented 3 years ago

Is it possible to generate Swift binding for opencv_contrib modules?

komakai commented 3 years ago

@AlvarHHM please check out: https://github.com/opencv/opencv/pull/17882 https://github.com/opencv/opencv_contrib/pull/2609 (these PRs just missed the 4.4 release - should be in the 4.5 release)

AlvarHHM commented 3 years ago

Is there any easier method to access or set the element inside Mat? using put and get are very cumbersome.

Thank you

komakai commented 3 years ago

@AlvarHHM

When a Mat is printed in the console, why does it only show meta-data instead of printing out the Mat's data like in C++?

Maybe print(mat.dump()) does what you need

Is there any easier method to access or set the element inside Mat? using put and get are very cumbersome.

It would be possible to implement Swift subscript operators so you could do something like mat[row, col] Not really sure what you are trying to do - but if you explore the OpenCV APIs a bit, very often you will find functions that allow you to do what you want, operating directly on a Mat rather than pulling data out with a get and putting back modified data with a set

AlvarHHM commented 3 years ago

Not really sure what you are trying to do - but if you explore the OpenCV APIs a bit, very often you will find functions that allow you to do what you want, operating directly on a Mat rather than pulling data out with a get and putting back modified data with a set

In my case, I am trying to parse the result of some neural network. Of course, I can do it in c++ with mat.at<double>(i,j) or with pointer, but it would be nice if the swift binding could match.

komakai commented 3 years ago

Of course, I can do it in c++ with mat.at(i,j) or with pointer, but it would be nice if the swift binding could match.

I see - would something like this https://github.com/komakai/opencv/commit/f251aaf3d0227e6544bc0f1987016334ca822bcb do what you want?