Refactor

[vadixidav]

This branch attempts to refactor the library into a sys crate. Currently the sys crate is building, but the existing native folder is not as useful anymore now that there are actually native C++ types exposed to Rust. I might start by trying to get it all working with the previous bindings first before progressing to remove all the extra fake types and pointers.

[vadixidav]

CUDA is also completely untested for the time being.

[Pzixel]

I propose to not remove constructors since having builders only is not a good way.

[vadixidav]

@Pzixel There is a clippy lint for having too many args. I agree that the original had too many args. I can choose a few args that most people will want to set or I can just make a default constructor. I feel like just having a default may be sufficient here. Does that sound fine?

[vadixidav]

Also, I am going to take off work today to finish this sys crate refactoring and add additional functionality to the crate (in a follow-up PR).

[Pzixel]

There is a clippy lint for having too many args. I agree that the original had too many args. I can choose a few args that most people will want to set or I can just make a default constructor. I feel like just having a default may be sufficient here. Does that sound fine?

This makes usecase when you know all arguments but have to use build because there is no other API less pleasant.

You either leave all 100500 arguments as they are or create a struct that encapsulate all of them, but you shouldn't just remove it with "use builders, guys".

I can choose a few args that most people will want to set

And this one fits into builder fine. You can create SIFTBuilder::create_jpg_processor() with best params that handles JPG, for example.

[vadixidav]

Some of the things I am doing I will improve. For instance, currently there is some unsafe code that can be avoided (or isolated to a single instance). Before marking the PR ready for review I will resolve those issues.

[vadixidav]

I finally got cv-sys to build with C++ bindings now. There is no need to use extern "C" in the headers or C++ files. We can also expose methods to Rust provided certain conditions. Many of the opencv things we are interacting with can even have bindings generated for them, so we might be able to gradually erode away the wrapper entirely.

The last bit of work required is now just updating the cv crate to use the new bindings which have changed a little. I may need to expose a few more opaque types from OpenCV as well. Before marking ready for review I will turn all the enums I made into opaque types passed to bindgen so that they are generated automatically. I will also ensure that everything works with a prebuilt OpenCV as well using a feature on the sys crate.

I haven't tested the CUDA stuff at all, and I know it wont work. I hope that once I have everything sorted out that either I can get CUDA to work or somebody else can help with that.

[vadixidav]

I have found that, at least on Debug Windows static builds (I haven't gotten to others yet), canny_edge_detection seems to have a segfault. I don't know if I want to track down why that happens quite yet, but it probably needs to be tracked in this repository and also upstream in OpenCV if it turns out to be a bug in OpenCV.

[vadixidav]

Okay, now that all the functionality is working again (only on Debug Windows), I need to make sure it works on all builds (Windows, Linux, etc). I also need to make sure that CUDA works. I don't have a Mac, but I am not sure we ever supported that. Finally, I need to make it work for a prebuilt OpenCV with a feature flag.

• [x] Windows Debug
• [x] Windows Release
• [x] Linux Debug
• [x] Linux Release
• [ ] Windows CUDA
• [ ] Linux CUDA

[vadixidav]

There is a place in the bindings where bindgen uses the type u64 for MouseCallback. This is a function pointer, so there is a problem with the type changing from one platform to the next. We need to either make sure the binding generates a usize or a fn(...) type so that we dont have issues on non-64-bit platforms. Alternatively, we can use the pointer size in some cfg statements.

[vadixidav]

Linux Debug and Release is now good to go with the refactor. I am now going to perform some clean up and also generate bindings to a prebuilt OpenCV so that works. I still need to make CUDA work. Give me some time to ensure that is still working properly.

Also, I tried building on a machine with Visual Studio 2015 and encountered some issues. Ideally we would like to fix that (particularly for CUDA support). I will investigate if that is in the bindgen phase or the cmake phase. If it is the cmake part of the build script, then it's not a blocker since they can just use a prebuilt OpenCV. If it is a problem with the bindgen part (most likely) then since the new bindings system relies on bindgen there will be issues.

[vadixidav]

Confirmed today that there are no issues on Visual Studio 2015. The issue encountered was that Clang was not installed. After Clang was installed, bindgen worked as expected.

[vadixidav]

Seems that OpenCV fails to build with Clang (it works with g++). OpenCV master seems to have this issue fixed, but I wont make this a blocker.

[Pzixel]

Cool.

I only suggest to use bindgen CLI instead of package. I heard tons of stories about problems because of it. Storing CLI-generated source in repo is much better than having build.rs with bindgen description.

I can be in more detail if you wish, but the general suggestion is this.

There is a place in the bindings where bindgen uses the type u64 for MouseCallback. This is a function pointer, so there is a problem with the type changing from one platform to the next. We need to either make sure the binding generates a usize or a fn(...) type so that we dont have issues on non-64-bit platforms. Alternatively, we can use the pointer size in some cfg statements.

I believe I have a check that c_int is i32. But I think having proper type instead of u64 is more desirable.

---

Great work overall. I didn't see this woring until opencv C api issue is resolved, but you returned my faith in the project.

I'm going to push it anyway, because having stable C API is much better than homebrew silly C++ wrapping. It's a huge steap from the current crate state of course, but having all things work seamlessly is desirable, and is only possible with all C API generated automagically.

[vadixidav]

@Pzixel I was considering doing that, but hadn't yet made a decision. It might be best to do like you say and add the bindings to the repository. That would eliminate issues with bindgen giving different results in different scenarios (like different versions of libclang or different C++ headers). I will look into that after I make everything work with the system OpenCV install (behind a feature flag). It might actually be good to have a feature to generate the bindings optionally just so that we have the bindgen configuration tracked in revision control and tested in CI.

• [x] Add generated bindings into repository and make generating them optional.

[vadixidav]

The refactoring is complete and all issues brought up in this and the other PR I closed have been dealt with. Moving on from here, we can generate native bindings directly to C++ wrapper code, making the creation of bindings simpler. We can also potentially start whitelisting more of the bindings from the cv:: namespace and get rid of the wrappers entirely, but that is outside of the scope of this PR.

I would like you @Pzixel to try and enable the cuda feature (via --features cuda,system) at the command line with a CUDA-enabled OpenCV install. It almost certainly wont work, but I haven't been able to get OpenCV to build with CUDA, even on my CUDA enabled machine, because it refuses to install the Visual Studio integration. I need somebody to help fix up the cuda bindings.

As for the text and tesseract features, those may be untested as well.

I am going to boot into Debian and make any remaining fixes I need to so that it works on there. That should be the last thing that needs to be done.

Edit: I enabled edits on the branch to maintainers so cuda can be fixed. Just push directly to my branch.

[Pzixel]

@vadixidav did you use msvc_1_install_CUDA.ps1? It should configure everything for you. The only requirement is VS2015 since VS2017 doesn't work with CUDA. You can look at instruction, it should work fine (worked at my two machines at least).

[vadixidav]

@Pzixel Unfortunately it didn't work on my VS 2015 machine. The manual install also failed. Other people in my office have had similar issues where it fails VS integration. I don't see myself easily being able to access a cuda machine on 2015 in the near future. It also fails if you have multiple Visual Studio installations, so I couldn't do it on my laptop with VS 2017 without uninstalling VS 2017. Plus, that laptop uses an eGPU to access a GTX 1080, so I don't know how CUDA would interact with me plugging the GPU in while its running on Windows or Linux.

[vadixidav]

Working on making this work on Debian right now. It broke when I made changes.

[vadixidav]

Unfortunately I had to add generated bindings for Linux and Windows. It seems that names are mangled differently between gcc and MSVC, so we cant rely on common name-mangling. I wish we could generate bindings at build since it barely takes any time at all, but I wouldn't want downstream users to have to install Clang. Hopefully eventually rustup will package clang and libclang like it does for other llvm utilities.

[vadixidav]

Using a pre-installed OpenCV on Linux has issues. I am going to try and wrangle the dependencies with pkg-config in all instances.

[vadixidav]

I will see if I can get CUDA working on my Debian install on my laptop. If I can, I might be able to get everything through CI. I am going to have to generate another set of bindings for CUDA-enabled OpenCV as well.

[vadixidav]

At a Linux CUDA machine now to test.

[vadixidav]

@Pzixel I think we should consider depending on libclang to be installed and have bindings always generated. Unlike C, C++ makes no guarantee of name mangling stability even between compiler versions. We could have to support an inane amount of generated bindings. We could have had one set of bindings before with the C++ -> C -> Rust binding process, but now that we are going C++ -> Rust, it seems that generating bindings to C++ might have to be a requirement. What are your thoughts?

[Pzixel]

@vadixidav You could try removing vs2017 check. I introduced it becuse in a time CUDA didn't work well with VS2017. I think they might already fix it so compiling with 2017 may work.

Plus, that laptop uses an eGPU to access a GTX 1080, so I don't know how CUDA would interact with me plugging the GPU in while its running on Windows or Linux.

Worth trying anyway, imo

About your mangling question I think if nomangle may work. I don't see any solution working in all cases. We either have unstable bindings or have to pregenerate them and deal with mangling issues or something.

---

P.S. I'm planning sending https://github.com/opencv/opencv/issues/11666 to the https://github.com/opencv/opencv/wiki/GSoC_2019 and see if it will work. It won't benefit immediately, but we could switch to stable C ABI later if needed.

[vadixidav]

@Pzixel Oh, that would be awesome if somebody worked on that. Perhaps make a tracking issue in the repository once its up.

[Pzixel]

Btw I don't think replacing opaque enums with c_void is a good thing.

[vadixidav]

@Pzixel I agree. The reason it is like this currently is because if the types are not blacklisted then bindgen generates methods for them (from C++), but if they are blacklisted then it replaces pointers to them with *mut c_void. I would like to create wrapper types on the C++ side or attempt to generate bindings directly from the C++, but that would require yet further refactoring that I didn't want to do yet. Bindgen might let me use typedef Blah cv::Blah in the cvsys namespace, then I can use *mut cvsys_Blah in the code. Let me try doing that. Right now I am trying to fix the text feature.

[vadixidav]

Alright, now everything is typed. I created types on the C++ side to get around limitations with templating in bindgen. Now bindgen generates types on the Rust side that correspond to the C++ side and all pointers are now typed.

I would now like to revisit Windows one last time to make sure everything still builds on Windows in all configurations. I will see if I can get text and tesseract to work on Windows.

I would really appreciate it if someone else could try testing CUDA support on Windows. I haven't had any success with it so far personally, but I am sure the build script has some issues to work out in that scenario.

I will leave a comment on here after everything seems to be working fine on Windows. If it all works then I think this will be ready to merge.

[vadixidav]

Alright, aside from CUDA on Windows, I feel pretty confident about this build. I have tested it on multiple machines in every configuration.

We can always clean Windows CUDA support up if we need to later.

The last step now is to just make sure CI passes, which I will now await.

[vadixidav]

I got CUDA to work on my laptop (CUDA 10 with Visual Studio 2017). Apparently it does work with Visual Studio 2017 after all now with CUDA 10. I will do some tests on my laptop with my eGPU and make sure the opencv build works there.

I am still trying to get things working on CI. You will know when it turns green that things are good to go.

[vadixidav]

There is some weird issue with IlmImf on the Ubuntu 16.04 build server. I don't have a 16.04 box, so this might take me some time to investigate.

[vadixidav]

After this last build, I think it is time for me to take a break from this. I want to actually start adding APIs and not refactoring, even though there is probably more work to be done. I also have other open-source work to do rather than wrangle OpenCV builds.

So, I think it should be ready to merge after this. I just updated the README.md and the travis to reflect it. It builds on several versions of Ubuntu and Debian. It works on Windows. It works with and without CUDA. It works with tesseract. I did a lot of things to get it to work everywhere, so I hope it is good enough to merge now. Despite my best efforts, trying to wrangle a build of OpenCV was not easy, and I hope people will try to address the kinks in the future (like lack of OS X support). I really still don't feel super confident about the build, but it has worked on many machines, so hopefully that is sufficient for now.

Now after this I can move onto adding all the features I need to the package.

[vadixidav]

@Pzixel Feel free to review my changes. I am ready for this to be merged.

[Pzixel]

Okay.

It will also take a while so feel free to use git dependency with branch specification if you need it.

P.S. I wonder why there is no appveyor builds. Did you removed yml? P.P.S. Please, fix this minor PR conflict.

[vadixidav]

@Pzixel I am not sure why AppVeyor didn't build that last commit. The last two succeeded, but then it just ignored the last one. After I resolve the conflict, it should work.

[vadixidav]

@Pzixel Both CI ran this time and are passing.

[Pzixel]

@nebgnahz what's your opinion?

[vadixidav]

I have been busy with some other things (hwt crate and akaze crate), but I will address some of the issues brought up here now. Hopefully I can find some time to fix these soon before I diverge too much from master.

[vadixidav]

I think this branch has gotten too far out of sync with master, so I am just going to close it.