Redirects opencv internal errors using cvRedirectError

[joelgallant]

This is probably not good/idiomatic rust, and I'd appreciate help making it follow the project's style.

Is there a way to unit test this?

[Pzixel]

Hi there!

Thank you for your PR.

However, I don't think that mutable global handlers should be exposed to the Rust since it's completely against Rust philosophy.

I'd like to ask why you implemented it on the first place? You miss this funcationallity or something? Because all OpenCV errors are supposed to be wrapped into corresponding Result's. It doesn't fit your needs?

[joelgallant]

I agree re: one global handler - it feels dirty but I wasn't sure what the appropriate way to handle it is.

I required this because a fair amount of opencv functions will terminate on assertions, and not output debug messages or anything when the program crashes.

It seems that it's doing this, which panics underneath rust which doesn't output any useful messages.

How it's done inside of opencv: https://github.com/opencv/opencv/blob/2da96be217ab437de854aca7f7670c2048fb554b/modules/core/src/system.cpp#L897

[joelgallant]

It would be extremely onerous for a single handler to dispatch errors anywhere inside of this library, somehow (I have no idea how that might even work).

[Pzixel]

I required this because a fair amount of opencv functions will terminate on assertions, and not output debug messages or anything when the program crashes.

As far as I see it's disabled by default and never gets set to true:

static bool breakOnError = false;

So you shouldn't be affraid of unexpected panic - the switch is never enabled.

I actually implemented a wrapper that handles an exception if it occurs and transforms into rust Result type, e.g. see https://github.com/nebgnahz/cv-rs/blob/master/native/common.h#L68 and https://github.com/nebgnahz/cv-rs/blob/master/native/text.cc#L30 . Then it gets propagated and here we have a test that checks that error is actually here: https://github.com/nebgnahz/cv-rs/blob/master/tests/test_text.rs#L78 . It's unwrapped so you get the same panic as before, but here is a big difference: you're able to deal with the error, e.g. log it somewhere or try to recover or show it to the user or whatever you want to.

It's true that in most places we are just panicing, but it happens because cpp code doesn't handle exceptions properly and they raise panic on Rust side.

So I think the better approach is just find the places where exception may be raised and wrap them into FromFunction so they would be correctly propagated in Rust code.

It would be extremely onerous for a single handler to dispatch errors anywhere inside of this library, somehow (I have no idea how that might even work)

I personally thing the opposite: all kinds of something global should not exist until absolutely necessary. I think that local Result's are much better, because you have the entire context required to handle the error. With global handler you could just log it somewhere, which is not really helpful. And if we're talking about some production-ready code, you have to have some kind of configurable DI and so on so it's become pain to work with such kind of global objects.

[joelgallant]

all kinds of something global should not exist until absolutely necessary

Totally agree, sorry if it sounds like I want this, I'd much rather deal with result types. As far as I could tell, opencv exceptions/errors that are thrown in CV_Assert aren't catcheable when called via the C API (like they could be with c++ exceptions). If I'm wrong about that, that's awesome.

I guess the idea is to wrap every native function like that in a c++ closure, and utilize the result type?

[Pzixel]

As far as I could tell, opencv exceptions/errors that are thrown in CV_Assert aren't catcheable when called via the C API (like they could be with c++ exceptions).

That's probably true, but the main idea here that we don't use C API to call OpenCV. It's our own wrappers (see native folder) that could do everything, including transforming exceptions as I have shown. But I generaly don't believe in uncatchable panics instead of exceptions - that could be a disaster for the whole ecosystem! :) I believe OpenCV devs are working with such a big project and just couldn't do that.

I guess the idea is to wrap every native function like that in a c++ closure, and utilize the result type?

Yes, it's the main idea, except for places where exception could never occur. That's what I have written above:

So I think the better approach is just find the places where exception may be raised and wrap them into FromFunction so they would be correctly propagated in Rust code.

[joelgallant]

My apologies for not taking more time to understand this repo before confusing you with this PR, I now understand this much better.

With OpenCV, exceptions could crop up anywhere. Is there a guarantee that all ffi functions are wrapped in a catch-all? Otherwise, is it of any use to use some variation of this PR? (without the global catcher maybe) I just spent some time with this library and ended up running into some, what look like on the surface, segfaults, which were actually assertions internal to OpenCV that were not caught. I'm sure the idea is to catch all of these and create appropriate results, but in lieu of that, maybe utilizing this redirection would help users of the library?

Just my 2 cents - if you prefer the fix-them-as-they-come approach that's cool too, this just helps in my debugging (as I need to implement more functions than are currently supported, so I am running into this more).

[Pzixel]

My apologies for not taking more time to understand this repo before confusing you with this PR, I now understand this much better.

That's perfectly fine, I'm glad you found time to dive deeper into it.

With OpenCV, exceptions could crop up anywhere. Is there a guarantee that all ffi functions are wrapped in a catch-all?

If you wrap them all - yes, you can 😄 However, it's not done yet. I just wrapped functions I personally worked with, and where I personally was getting panic instead of errors. I just didn't get enough time to handle all places. You can easily wrap all calls, but it's not perfect since some of them just couldn't rise any exception. In this case it's just complicates API and usage experience without any benefit. Another point is that we were thinking about code generation instead of current manual wrapping approach. Because of that, I personally maintain the project fixing bugs people find, but I don't add much new value.

There is multiple discussion about it, e.g. https://github.com/nebgnahz/cv-rs/pull/67#issuecomment-370133765 and #78. So I'm not sure about how we should proceed. The last time I was working on it I was stuck with getting generated headers by the python. Adopting python script to generate some kind of rustish types instead of python ones could be a solution. But it didn't work well on windows (because of multiple problems with pip on the platform), so I didn't implement it. I only created an issue which I do believe will be stalled forever.

Just my 2 cents - if you prefer the fix-them-as-they-come approach that's cool too, this just helps in my debugging (as I need to implement more functions than are currently supported, so I am running into this more).

I'd appreciate any work you want to share.