Bindings Aren't Very Rusty

[ElykDeer]

Hey there. Love this project. Sorry if this is a duplicate, but I couldn't find anything similar.

The pattern of using out parameters and neglecting a proper return value I've noticed:

let mut result = Mat::default();
warp_perspective(
    &src,
    &mut result,
    ...
)?; // <- no actionable return value beyond errors

Violates Rust conventions. In particular, this one. In that example it's showing the case of multiple return values (a common case in C where you'd use out parameters), but I believe it applies here as well because of its general description and specific exception for buffer reuse.

That said, I believe that that these function should use their actual return values and look something more like:

let result = warp_perspective(
    &src,
    ...
)?;

Which would result in cleaner code, and I suspect (because of compiler optimizations on not-mut variables) be faster as well...though that really depends on how you implement this.

I might go a step further and suggest that a large swath of these functions couple be members of the core::ToInputArray or some other trait, since most of the time these functions take a buffer as their first parameter (much like "self"). So code could look something like:

let result = src.warp_perspective(
    ...
)?;

And then we'd really be cooking with fire! This would also allow for us to chain multiple operations, such as (please ignore the somewhat nonsensical behavior of this example, it's just what I can come up with off the top of my head):

let edges = img.cvt_color(
    ...
)?
.cvt_color(
    ...
)?
.canny(...);

---

Finally, and maybe this should be another issue, passing enums as i32s instead of their actual enums is another documented antipattern that should be avoided. This would also allow for cleaner code:

warp_perspective(
    ...
    InterpolationFlags::INTER_LINEAR as i32,
    BorderTypes::BORDER_CONSTANT as i32,
)?;

vs

warp_perspective(
    ...
    InterpolationFlags::InterLinear,
    BorderTypes::BorderConstant,
)?;

(I fixed the enum names to match Rust naming conventions there as well)

[twistedfall]

Hi!

What you say definitely makes sense and I agree with you. But the problem here is that the API surface of the OpenCV library is just huge and manually handling even a fraction of the functions is quite a considerable maintenance burden (and then functions sometimes change the amount and types of the arguments between OpenCV versions). That's why the bindings are generated by automatically translating the OpenCV headers into the Rust bindings with the minimal manual intervention. There are some tweaks in settings.rs for those cases where automatic detection is not possible or difficult, but I try to keep those from growing.

With that said I can probably make at least the mutable argument to return transformation quite easily, I'll take a look into it. Turning a free-standing function into a method on InputArray is trickier. The enum problem stems from the fact that OpenCV function takes int and the fact that it's actually takes an enum value is explained in the documentation. There is no way this can be handled automatically. But it gets better in newer OpenCV versions, some arguments get changed from int to the proper enum type.

[ElykDeer]

Ah, okay. That makes sense.

The biggest change that would be nice to have is swapping those out parameters to return values, and the rest is more icing on the cake for me.

I was hoping since the docs know that the parameters are enums, that information could be propagated to the type itself, but alas.

[twistedfall]

I did some experiments and moving of mutable args to return somewhat works, but I decided not to go forward with the solution. Besides increasing code complexity it also removes the possibility to pre-allocate a Mat to be used in several calls by passing it as &mut argument. Additionally it's not always a Mat that you want to use as OutputArray, a Vector is another possibility.