twistedfall / opencv-rust

Rust bindings for OpenCV 3 & 4
MIT License
1.93k stars 151 forks source link

Plans for allowing access to pixels? #17

Closed phdoerfler closed 8 years ago

phdoerfler commented 8 years ago

Is it planned to have Mat::at or Mat::data be part of the generated rust bindings? This would allow direct access to the individual elements of a matrix.

Right now (if I did not miss anything) there seems to be no way of accessing pixel values of a Mat.

kali commented 8 years ago

I'll have a look to see if/how I can make it possible. I am also trying to get 3.1 to work.

FuegoFro commented 8 years ago

I looked into this a bit. It seems like the existing ptr(...) methods would do the trick here. They're currently ignored during generation because they return pointer types, specifically uchar*, but this is also a primitive type (based on the primitives dict in gen_rust.py). This is because the parse_type function checks for raw pointers before it checks for primitive types. Changing the ordering there such that it looks for primitives first allows it to generate the ptr(...) functions.

This leads to a second problem. There are two versions of every ptr(...) overload, one that's const (and returns a const uchar*) and one that's normal. However, the generator doesn't take constness into account when generating the C++ methods (or at least it doesn't seem to; I haven't read through enough to fully understand the const support). In any case, it generates two C++ methods with the exact same signature, which then fails to compile (complaining about redefining a symbol).

There's a few solutions I've thought of (and probably many I that haven't :P):

  1. Take constness into account when generating the C++ methods (specifically in the method signatures).
  2. Determine the set of things that go into a method and then coalesce methods so that we only generate each unique method once.
  3. Extend the functionality for ignoring methods to be able to take the attributes of the method into account (eg constness).
  4. Some entirely different approach for accessing the pixel data :)

What do you think a reasonable approach would be?

kali commented 8 years ago

Well, these methods are specifically enough that we may not want to generate them but just write them: The generator is already very messy. This is because a huge lot of "c++ features" are used across opencv with or without consistency. This situation is actually a bit better in 3.x . Also, as soon as the pkg-config mess will improve in opencv size, we should be able to switch to the cv3 branch, where the generator has already been extensively modified.

Bottom line: it's not worth digging a trench in the generator just to support features used by one or two methods.

But instead, we can add these methods manually. I used this at the beginning, but it should still work: just add a Trait for these methods and impl it in the rust code in src, and put the matching C wrappers in native/something.cpp. The builder will compile and link code in native/*.cpp in the library along with the generated code.

Tell me what you think.

phdoerfler commented 8 years ago

I guess hand writing them is the best option. I'd like to help with that but unfortunately it exceeds my current rust knowledge by far (I still don't quite get how rust can even access C++. Even the rust manual says that only C is supported by the FFI). However if someone was willing to remote pair (e.g. via floobits) I'd love to give it a shot.

kali commented 8 years ago

Yeah. Rust does not call c++. The generator creates a C interface on top of the c++ class for rust FFI to talk to. The generated c interface is then built into a library by build.rs.

phdoerfler commented 8 years ago

Any news on this?

FuegoFro commented 8 years ago

I took a stab at one of the approaches for fixing this in https://github.com/kali/opencv-rust/issues/19. The solution worked for me, was a bit more general than handwriting, and didn't actually end up complicating the generator too much (at least, in my opinion). Even if it doesn't get merged in, you could probably patch it in locally @phdoerfler.

kali commented 8 years ago

Merged, built, and just released as 0.2.3 . Thanks again for the PR.

phdoerfler commented 8 years ago

Awesome! Thanks for doing this @FuegoFro

FuegoFro commented 8 years ago

Woo! No problem :)