nebgnahz / cv-rs

Rust wrapper for OpenCV (manual at this point)
https://nebgnahz.github.io/cv-rs/cv/
MIT License
203 stars 41 forks source link

Separate OpenCV C bindings into sys crate and use bindgen on them #115

Open vadixidav opened 5 years ago

vadixidav commented 5 years ago

It is not necessary to write the C bindings again in Rust after they have been written in C. An example of this is in the crate flann-sys where the C bindings are wrappers around C++ bindings and bindgen is used on those C bindings. Separating these generated bindings into their own sys crate will also be helpful for allowing others to interact with them directly if desired.

Pzixel commented 5 years ago

Extract sys crate is a long needed feature, but project is in somewhat stalled state. Original author doesn't commit for a year, I guess, and I got dissapointed by manually writing wrappers. There should be some kind of codegen (bindgen won't help here), but there is none. I created an issue but it seems that nobody in opencv is interested in it. See https://github.com/opencv/opencv/issues/11666

vadixidav commented 5 years ago

Well, if you will continue maintaining the repository, I can continue to write more bindings for things I am interested in. It would be good to join with the photogrammetry community in Rust (see discord issue) and maybe we can get more done collectively. I would also recommend publishing to crates.io to get more attention.

If you would like, we can discuss better ways to potentially do binding generation.

vadixidav commented 5 years ago

Would you be fine if I manually separated the C bindings into a cv-sys crate? It shouldn't take too long and will ease the process of writing new bindings.

Pzixel commented 5 years ago

Ideally there should be 3 crates:

  1. sys-crate with wrappers
  2. rust crate with ideomatic wrappers
  3. crate with instruction how to properly build opencv (current .ci/.windows folders content).

But there still issues with original repo creator licensing/ownership...

Of course you're free to write whenever binding you want, I'l merge them while they conform the overall repo code, but I'm sure manually writing solves nothing. I wrote my considerations in linked issue. I just warn you to not waste your time at manually writing something that must be automatised. Of course having some bindings today is better than having them generated in some future, but only if your app is blocked with repo issues (as was mine when I wrote my image detecting bot).

Pzixel commented 5 years ago

P.S. I'm not the best seller as you can see cause I probably hardly demotivated you to write anything, but that's true: writing binding manually is a dead end.

vadixidav commented 5 years ago

We cannot generate Rust bindings from C++ in an automated fashion right now. The best we might get is some assisted C++ to C bindings. We can investigate that if you are interested. It would take me about a minute per method/function to create an appropriate C binding based on the docs if the Rust side was automatically generated from C. I get your concern, but I am convinced that I can easily add any API that I need from the OpenCV C++ docs to this crate every time I need it. It will only take me a minute or two to write the C binding, a few minutes to write the Rust binding and document/test it, and a minute to open a PR on here.

Pzixel commented 5 years ago

Well, it's up to you and your time. I think you're too optimistic about the ease of writing bindings.

vadixidav commented 5 years ago

In that case I'll separate everything out into a sys crate after work today.

vadixidav commented 5 years ago

I am actually finding some success in generating C++ bindings. See my PR at #119 for more details. I decided to go beyond just putting the bindings in their own crate and I am attempting to use wrapper C++ bindings instead. After that, it wont be too difficult to simply remove the wrapper entirely. That will require careful whitelisting of types for the build script, but it is entirely possible to expose the C++ bindings correctly directly from OpenCV. It even handles name mangling:

extern "C" {
    #[link_name = "\u{1}?hmm_drop@cvsys@@YAXPEAU?$Ptr@VOCRHMMDecoder@text@cv@@@cv@@@Z"]
    pub fn cvsys_hmm_drop(ocr: *mut u8);
}

The PR is still only limited to putting this in a sys crate. I will make a follow-up effort to remove the wrapper entirely soon thereafter.

vadixidav commented 5 years ago

This will be closed after #117 is merged.