Refactor core module for type-safety

[cv3d]

Merge with opencv/opencv_contrib#1768 and opencv/opencv_extra#518

This pullrequest changes

• Provides a backward-compatible API based on enums for type-safty. Refer to int to proper enum type #12288 for further details

Utilized options/preprocessors:

• CV_TYPE_SAFE_API with CMAKE option ENABLE_TYPE_SAFE_API to enable enum-based type-safe API
• CV_TYPE_COMPATIBLE_API with CMAKE option ENABLE_COMPATIBLE_API to enable the overloaded int-based API. Although it is enabled by default and using it would raise deprecation warnings, it still recommended to disable it in the build farm to enforce good practices. CV_COMPATIBLE_API is only available when ENABLE_TYPE_SAFE_API is set.
• CV_TRANSNATIONAL_API utilized internally, and should be removed after migration completes.

force_builders=Custom,linux32,win32,windows_vc15,Android pack,ARMv7,ARMv8
docker_image:Custom=ubuntu-cuda:16.04
docker_image:Docs=docs-js

[alalek]

Looks like bug is catched here from the original code.

Why not _src.depth() == CV_8U ?

/cc @vpisarev

[cv3d]

You will see hundreds of similar cases! There are even functions that receive type, although they just need the depth... I have actually made several decisions in utilizing depth instead of type (e.g. in all arithmetic operations, convertTo, assign, assignTo, calcCovarMatrix), and I really hop you will catch what I missed.

[cv3d]

@alalek I am getting undefined reference to some functions that are already defined 😕 such as in this build.
Any help is highly appreciated.

[alalek]

undefined reference

This build step trying to build samples with "installed" OpenCV binaries + headers.

[alalek]

CV_EXPORTS

Remove CV_EXPORTS.
Use "static inline" for compatibility entry points.

[cv3d]

@alalek Never mind my previous request about some assertion failures, as it is already solved!
I was doomed with some virtual functions of int arguments, which cannot call the enum-type override!

[alalek]

virtual functions

Could you point on some examples? Perhaps we can skip compatibility stuff for them.

[cv3d]

Could you point on some examples?

It all originated from the class ArrayTest inside the test suite module.

    virtual void get_test_array_types_and_sizes(int test_case_idx, vector<vector<Size> >& sizes, vector<vector<int> >& types);
    virtual void get_minmax_bounds(int i, int j, int depth, Scalar& low, Scalar& high);

I handled part of the problem, but I still see some failures here and there, which most probably have relation to the above mentioned functions, and it is taking too much time of me already 😕

[alalek]

Compatibility stuff is not needed in tests.

[vpisarev]

@cv3d,

well, if you look at the statistics of the closed PRs, you will see that we usually merge over 100 patches per month, over a thousand of patches per year. Most of the patches are innocent, they fix a small bug, or bring some new optimization, or even add new functionality or new documentation. Such patches usually take little time to review and they get merged very fast.

Now the case of your patch is completely different. You started with very fundamental things in OpenCV that live there for 10 years already! You may not know, but I will tell you that many, many people still use OpenCV 2.4.x. I've heard about some big projects that only now consider migration to OpenCV 3.x, even though OpenCV 3.0 has been released 3 years ago. There is huge inertia in software.

If it ain't break, don't fix it. You said that you value compatibility, but I honestly do not see that. It's number 1 priority in our project. Between the radical patch that sort of improves API style and compatibility I would choose compatibility any time of day.

Say, if you come to Python community and suggest to change the language syntax "slightly", or come to Linux community and suggest to change a bit driver API that breaks backward compatibility - what would happen? Some PIPs in Python and some forks in Linux live separately for years.

OpenCV is 18 year-long project, we have hundreds of thousands of users, and even though we are quite liberal w.r.t patches and suggestions, we still have to carefully evaluate each patch that breaks compatibility. 1 month is nothing comparing to the years of support from our side and many man-years that OpenCV community needs to spend to repair all the code that is broken because of this tiny patch. If the argument is that users will build OpenCV in "compatibility mode", then it looks like the patch is not up to some claims it makes - it does not make user's OpenCV-based code safer, it just makes OpenCV itself more type-safe (but we have many unit tests, valgrind and such, so OpenCV code itself is rather well-tested).

If you are brave enough (as one of the reviewers said) to submit such radical patches, please get enough patience and be ready to find compromises.

I value that you've updated the PR, thank you very much.

I still see that it's quite a lot of changes, including many cases of CV_depth to CV_depthC1 changes, that I personally don't like, I think, users do not have to make such stupid changes in their code.

==

Here is the roadmap of this patch inclusion:

1. If you have time, please, prepare an alternative patch where there is no physically different type ElemDepth, just ElemType. Where there is zero changes from CV_8U to CV_8UC1 etc. In your patch I still do not see it. We need to have some numerical estimate (basically, the number of patched lines or the patch size), how much more effort does it take to make ElemDepth and ElemType phyisically different types.

2. If you do not have time, then give me some time, I will create such patch, based on your patch, by myself.

3. After we have both patches and can compare them, there will be next step. I'm enthusiastic to see such a patch not just because of type safety. It's a smaller goal, in my opinion. The bigger goal is to phyisically separate elemType and array (Mat, UMat, etc.) flags. In your patch you use MagicFlag data structure, probably we need to consider 2 separate members: flags (or arrayflags to catch all the cases of improper use) and elemType. As soon as we have it, we will be able to add new ElemType members, like CV_TYPE_RAW, CV_32U, CV_64S, CV_TYPE_UNDEFINED, CV_TYPE_AUTO etc. That will be a real motivation for us and for users to make those changes - the set of supported data types will be extended.

We try to do it all by OpenCV 4.0. If not, in principle we can do it in 4.1. Or maybe we'll split int flags into ElemType type and ArrayFlags arrayflags, introduce typedef int ElemType typedef int ArrayFlags synonyms and leave the enumeration anonymous by 4.0 and then do this radical type-safe change in 4.1. We need to see, but first I need this alternative patch to be prepared (by you, @alalek or myself) to clearly see the difference in the impact.

[cv3d]

@vpisarev I will prepare the ElemType-only patch. Hope that can be of any help!

[cv3d]

no physically different type ElemDepth, just ElemType

zero changes from CV_8U to CV_8UC1

@vpisarev PR #12609 does what you requested for. Please let me know if you still have further requirements.

After we have both patches and can compare them

I have grouped the differences in a single commit for your convenience: cv3d/opencv@e1c13c0
Anything else?

[cv3d]

As soon as we have it, we will be able to add new ElemType members, like CV_TYPE_RAW, CV_32U, CV_64S, CV_TYPE_UNDEFINED, CV_TYPE_AUTO etc.

I also have the same goal in mind, but my approach is different. I plan to increase the capacity of ElemDepth enum from 8 to 16, thus be able to introduce these new types.
How about taking a look at my proposal at #12584?
Perhaps I am biased here, but I think it is cleaner, easier, and does not confuse users, in compare to merging-depth-and-type thing.

If you are brave enough (as one of the reviewers said)

Darn! Is that why I am getting tortured now? It is not my fault.. I cannot control others 😢

please get enough patience and be ready to find compromises

please get enough faith/believe/trust and be ready to get amazed

[cv3d]

If it ain't break, don't fix it

We are not fixing it. We are introducing new feature: type-safety

You said that you value compatibility, but I honestly do not see that

Besides your own opinion, what do you see exactly? What does this PR break?
Kindly be more technical and concrete.

Where there is zero changes from CV_8U to CV_8UC1 etc. In your patch I still do not see it

I have reduced this PR to its minimal, in which there is zero changes from CV_8U to CV_8UC1 etc.
Please confirm, and kindly let me know if you still have any concrete concerns.

the roadmap of this patch inclusion

After I prepared the comparison patch, I believe we are in the final bullet (although I cannot get why this PR shall wait for the new types anyway), but you also said

there will be next step

It sounds like a recursive function call for me, but I do not see a termination criteria..

[cv3d]

It's a smaller goal, in my opinion. The bigger goal is to phyisically separate elemType and array (Mat, UMat, etc.) flags.

@vpisarev I said it a week ago, and I will say it again: I am not convinced by your logic, and I cannot understand why this PR should wait..
In fact, I do not find any relation between the new types PR and this one.. Each one have different goal, and affects different components, with very small overlap.

BTW: Why all this silence?

[cv3d]

I'm enthusiastic to see such a patch

As soon as we have it, we will be able to add new ElemType members

@vpisarev I believe you meant this patch. So, any chance to consider this PR as a separate and independent feature by now? I think I already demonstrated they are unrelated via my other design and new types PRs (i.e. #12729 & #12730).

Accordingly, I want to solve the conflicts in this PR in order to merge it, but first, I need your confirmation we will not have any further delays..

[vpisarev]

experiment with ElemType-only patch is not finished yet (#12609). I left you multiple comments, but they are not addressed; instead, you continue to create tons of new PRs

[cv3d]

@vpisarev My bad! I did not notice your reply 😕