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 #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

[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.

[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.

[cv3d]

I managed to make Windows compile and test all modules properly, but since I do not have access to Linux or Mac, I find it difficult to debug them and find the reason for failing tests.

@alalek Can you please help?

[alalek]

@cv3d We need to process OpenCV headers from "standalone samples" with the same flags. Workaround is here: https://github.com/alalek/opencv/commit/pr12487 (but proper solution is to update generated OpenCVConfig.cmake - I will do it later)

[cv3d]

@alalek Can you please extend your workaround for Android pack? It has the same CMake issue as for samples..

[alalek]

Could we separate warnings into two types:

• compatibility for ElemDepth->ElemType: warning is disabled by default, can be enabled by extra C++ flag only (no need to extend CMake)
• compatibility via 'int': warnings are enabled by default, but can be disabled C++ flag (no need to change CMake)

#ifdef OPENCV_WARNING_ENABLE_DEPRECATED_ELEMDEPTH_ELEMTYPE_OVERLOAD
#define CV_DEPRECATED_ELEMDEPTH_PARAM(type) CV_DEPRECATED_MSG(CV_DEPRECATED_PARAM(ElemDepth, type, ElemType, type))
#else
#define CV_DEPRECATED_ELEMDEPTH_PARAM(type) /* nothing, default */
#endif

#ifndef OPENCV_WARNING_DISABLE_DEPRECATED_INT_ELEMTYPE_OVERLOAD
#define CV_DEPRECATED_ELEMDEPTH_INT_PARAM(type) CV_DEPRECATED_MSG(CV_DEPRECATED_PARAM(ElemDepth, type, ElemDepth, type))
#define CV_DEPRECATED_ELEMTYPE_INT_PARAM(type) CV_DEPRECATED_MSG(CV_DEPRECATED_PARAM(int, type, ElemType, type))
#else
#define CV_DEPRECATED_ELEMDEPTH_INT_PARAM(type) /* nothing */
#define CV_DEPRECATED_ELEMTYPE_INT_PARAM(type) /* nothing */
#endif

workaround for Android pack

Working on it.

[cv3d]

Sure! Will do that.

Do you plan to enable all warnings in the build farm? If not, I am afraid all the efforts spent in this whole work will eventually go in vain..

BTW: If you are concerned about the currently reported warnings, I guarantee you all of them are going to disappear after I push the patches for the rest of modules.

[alalek]

Android pack

Please replace my commit by another one: https://github.com/alalek/opencv/commits/pr12487

[cv3d]

@alalek Thanks for your efforts! However, with this workaround, CUDA cannot link the libraries 😕 Can you please investigate?

[cv3d]

Hooray! All requirements are now met, thanks goes to @alalek for his efforts and continuous support 😄 As you might already know, the remaining warnings are originating from the not-factored-yet modules and tests, so how about a thorough review?

@alalek @vpisarev @hrnr Now, I am about pushing new PRs for the rest modules based on this one. Hopefully, you can finish reviewing them all before the initial beta release. They would not be as big as this one anyway.

[cv3d]

I am sure you are aware of this, but let me stress on the fact that all functions/methods that are now receiving ElemDepth should be just fine.

However, if the function has an ElemType parameter then the question is: does it really need the channel information? Is not ElemDepth enough? If you find any such cases, please let me know, and I will be happy to change it.

[vpisarev]

@cv3d, @alalek, this is great effort and I fully support it.

However, ElemType and ElemDepth should be made the same type.

[cv3d]

users do not have to change all CV_32F to CV32FC1 etc. And in general CV... and CV_...C1 should be identical entities, not different

@vpisarev Currently, users do not have to change CV_* to CV_*C1 and they are exchangable, and the comparison CV_* == CV_*C1 holds true, thus I cannot understand why

ElemType and ElemDepth should be made the same type

Are you implying that CV_32FC3 should be considered ElemDepth as well?

[alalek]

ElemType and ElemDepth should be made the same type

There is a lot of code like this:

• ddepth <= CV_32F ? CV_32F : CV_64F
• max(ddepth, CV_32F)

OpenCV has well defined ordering of elements depth (actually it had, until CV_16F patch).

There is no simple way to define similar ordering for "complete" type with channels (CV_8UC2 vs CV_16SC1 - ... lets don't allow users to shoot foots).

C++ forbids sharing "values" between two non-class enums.

Another sample with type/depth mess:

auto type = src.type();
CV_Assert(type == CV_8U);

Do we want to avoid supporting CV_8UC3 here or does it a coding mistake and depth should be used here?

Additionally:

switch (depth)
{
    case CV_8U: ...
    case CV_8S: ...
    ...
    // compiler emit warnings if we missing some type depth (currently there are warnings for CV_16F in tests)
}

---

This patch avoids the most part of required code refactoring (8k changed lines => less that 100 changes outside of core module for both opencv + contrib codebases - check my "test branches": #10318 ) and functions accepts depth instead of type and vise versa (however functions documentation forbid this). So impact on user code should be tolerable if code doesn't look like this: UMat dst(1, 1, 0);.

Warnings are configurable (will be disabled for user code and enabled for OpenCV itself for better code quality).

[cv3d]

@alalek Do we need your last three commits?

[alalek]

Merge changes from "cmake: change type safe flags defaults" into your commits, other are just tests for green builds (plus one commit in contrib): #10318 .

[cv3d]

@alalek All done

[cv3d]

UMat dst(1, 1, 0);

@alalek When CV_TYPE_COMPATIBLE_API is set (default), even this ugly code shall work 😉

@vpisarev @mshabunin I have refactored the whole library including apps and samples. With "Refactor *" PRs, OpenCV gets more robust, convenient, while maintaining high compatibility with previous versions with configurable deprecation level.

[cv3d]

The following test must be added to the core tests, however, as you might already know, it is currently failing 😕

    vector<ElemDepth> order { CV_8U, CV_8S, CV_16U, CV_16S, CV_16F, CV_32S, CV_32F, CV_64F };

    for (int i = 0; i < order.size(); ++i)
    {
        for (int j = 0; j < order.size(); ++j)
        {
            ASSERT_EQ(order[i] <  order[j], i <  j);
            ASSERT_EQ(order[i] <= order[j], i <= j);
            ASSERT_EQ(order[i] >  order[j], i >  j);
            ASSERT_EQ(order[i] >= order[j], i >= j);
        }
    }

Since CV_MAX_DEPTH and CV_MIN_DEPTH introduced in this PR are doing their job properly, my idea is to introduce the following operators:

static inline bool operator>=(const ElemDepth left, const ElemDepth right) 
{
    return left == CV_MAX_DEPTH(left, right);
}
static inline bool operator<=(const ElemDepth left, const ElemDepth right) 
{
    return left == CV_MIN_DEPTH(left, right);
}
static inline bool operator>(const ElemDepth left, const ElemDepth right) 
{
    return left != right && left >= right;
}
static inline bool operator<(const ElemDepth left, const ElemDepth right) 
{
    return left != right && left <= right;
}

but these operators causes ambiguity! Can you find any workaround?

Another idea is to make ElemDepth a class instead of an enum. What do you think?

@alalek P.S. The same ambiguity prevents us from deprecating the (ElemType, ElemType) comparison

[vpisarev]

@cv3d, @alalek, well, first of all, in order to prove that users do not have to change CV_32F to CV_32FC1 I'd like to ask you to remove all such changes in the patch.

Then, I understand that you provided some implicit conversions between ElemDepth and ElemType, but then why not just make them the same type?

@alalek, regarding the switch statement - good robust practice is to put "default" case regardless of whether all the cases are covered or not. It's future-proof decision that stops all the compiler complains.

max(), min() etc. - not a problem either. The enumeration values can be converted to int's and the result can be converted to the enumeration - easy. The same is about comparison operations. Often it's useful to have ordering operations just in order to be able to create std::map with ElemType keys or just get some consistent ordering. The actual ordering is often not that important (as soon as it's fixed and known to user).

Actually, instead of using min(), max() etc. I would rewrite the corresponding code so that it extracts bit-depth, sign presence and integer/floating-point flag from the type value and makes the decision based on these properties of the type.

In other words, I cannot find any reasons not to make those types identical. In my opinion, it will break much less code than the solution with 2 different types and implicit conversions between them.

[cv3d]

to remove all such changes in the patch

In this patch, the base type is already changed from int to either ElemType or ElemDepth. Once such change is made, it is not possible to assign and int to ElemType or ElemDepth, or to assign a CV_\d+[UFS] to a ElemType, or CV_\d+[UFS]C\d+ to ElemDepth, and that is exactly the point of type-safety. On the other hand, user code is still using int as the base type, therefore there is no immediate need for them to change anything, and it will just work the same way the rest non-core OpenCV library did. If you really insist, then I can reduce this particular PR to a minimal one, in which the base types remain int, but then the question is: what is the point of providing overloads that are not tested at all? Please note that this PR in its current situation provides a strong evidence that both the new API (ElemType & ElemDepth) and the legacy API (int-based) can work interchangeably without issues.

why not just make them the same type

There are two types of implicit conversions: int to ElemType/ElemDepth, and ElemDepth to ElemType. The first type is meant to avoid breaking user code, while encouraging them to migrate from the type-unsafe int usage via a gentle compile-time warning. The second type (which I believe you are referring to) is again to mitigate breaking user code, and to improve the code quality. Here are some examples:

• convertTo() changes the depth of a matrix. However, there is inconsistency in calling it. Cases such as mat.coverTo(dst, CV_32FC3) or mat.coverTo(dst, CV_MAKETYPE(depth, 3)) are very common even inside OpenCV itself, and inside the function you would notice things like type = CV_MAT_DEPTH(type) to strip the channel information. On the other hand, via ElemDepth and ElemType seperation, we would like to maintain clean code, in which beautiful and meaningful statements such as mat.coverTo(dst, depth) are utilized.
• int type = ...; if (type == CV_32F) { ... } These cases creates huge ambiguity and are hard-to-maintain. It is not clear if the intetion is to allow a specific depth (variable name is badly chosen?) or if single channel is the only allowed case.

Your question boils down to the basic nature of depth and type of a matrix. If they are made the same, then why does .depth() and .type() exist? Is not .type() enough? I bet convenience, clarity, and maintainability were the original motivation behind them, and so do our motivation behind this PR.

max(), min() etc. - not a problem either

With all my respect, it is a problem. For instance, max(CV_16F, CV_32F) would result in CV_16F, and that is mostly not the user intent. We have deprecated such functions while correcting their behaviour for the time-being, and proposed to use CV_MAX_DEPTH or CV_MIN_DEPTH instead.

The actual ordering is often not that important

I am sure I misunderstood, because your statement sounds to me like changing CV_8U value to 6 and CV_32F to 0 is not that important either.

it will break much less code

It seems there is a conflict between our definitions of depth and type. Personally, I cannot say depth is a type, or type is a depth. But I can say type consists of a depth and number of channels. If depth is a type, and type is a depth, then channel is a depth, and channel is a type as well by induction. More importantly, to make such argument more concrete, could you please provide a scenario in which this PR would fail, while a single-type proposal would not?

[alalek]

@vpisarev

order to prove that users do not have to change CV_32F to CV_32FC1

Do you see in this patch changes outside of core module? in samples? This is a prove that legacy code can be compiled without required massive changes. Yes, users would get a set of warnings about their code. Warnings are still configurable to remove most part of them, see #10318 (green builds with this patch). Anyway they can turn off type-safety completely (and continue to use 'int').

Prove has been here since 10:00 UTC (Friday, 14 Sep).

them the same type

They are not the same entity.

1. Try to #define CV_MAT_DEPTH(type) (type) for making them the same. Nothing will work.

2. We can't declare them to the same, because the current OpenCV implementation and related documentation doesn't expect "type" instead of "depth". Explicit documentation statements about expected depth values only:

$ grep -R '@param' ./modules | grep depth | wc -l
103

But it is documentation only. Idea of the current patch is enabling of these existed "documentation-only" declaration via C++ code too.

---

switch statement - good robust practice is to put "default" case regardless of whether all the cases are covered or not.

"Robust practice" to compile code only. But problem will be observed in runtime only (hopefully test coverage is enough) or via bug tracker report from a customer. These warning are useful then you are working with enums and trying to extend them. These warnings are added into compilers (static code analyzers) for are reason.

P.S. CV_16F patch brokes "ordering" of depths, so a lot of existed code would not work as expected:

• CV_32F < CV_64F is true
• CV_16F < CV_64F is false (?!)

[cv3d]

CV_16F patch brokes "ordering"

I verified locally (to some extent) that ElemDepth can handle this issue if converted to enum class. But I prefer to have some security that my future efforts will not go in vain, so I will wait until most of the open PRs are merged first.

Edit: I think enum class is not a good idea after all, as it will definitely break all user code. I will consider other approaches ...

[vpisarev]

@alalec

Do you see in this patch changes outside of core module? in samples? This is a prove that legacy code can be compiled without required massive changes.

first of all, I see over a thousand of warnings produced by PR itself, we did not even touch the user code yet. Can you or @cv3d get rid of them in order to continue the discussion?

They are not the same entity.

ok, we planned to introduce CV_TYPE_RAW, CV_TYPE_UNDEFINED types or something like that. What's the corresponding depth of those types? How do I specify such type if the corresponding function now takes ElemDepth parameter?

Try to #define CV_MAT_DEPTH(type) (type) for making them the same. Nothing will work.

I do not have to; we can have CV_MAT_DEPTH(type) cv::typeDepth(type) where typeDepth is declared as

namespace cv { ElemType typeDepth(ElemType type); }

We can't declare them to the same, because the current OpenCV implementation and related documentation doesn't expect "type" instead of "depth".

$ grep -R '@param' ./modules | grep depth | wc -l
103

Look at imgproc and other modules that take the desired output type argument. We use both depth and type interchangeably, without any strict rules.

grep -R '@param' ./modules | grep type | wc -l
532

P.S. CV_16F patch brokes "ordering" of depths, so a lot of existed code would not work as expected:

• CV_32F < CV_64F is true
• CV_16F < CV_64F is false (?!)

in your opinion, what should be the result of CV_16F < CV_32S?

[alalek]

Green builds based on this patch are here: #10318. 18 (eighteen) changes to suppress all warnings.

to introduce CV_TYPE_RAW, CV_TYPE_UNDEFINED types or something like that. What's the corresponding depth of those types?

CV_DEPTH_RAW, CV_DEPTH_INVALID. In my understanding, CV_TYPE_RAW is not a single value, because you should store somewhere size of the structure.

ordering

I believe current <,<= operators, max/min should gone and deprecated. Instead of this we need some calls like this:

• getWDepth32F(src1_type, src2_type = CV_TYPE_UNDEFINED, ...) => {CV_32F, CV_64F}
• getWDepth32S(src1_type, ...) => {CV_32S, CV_32F, CV_64F}

It is much easy to implement them for depth only instead of for complete types with channels.

We use both depth and type interchangeably, without any strict rules.

Using CV_8U instead of CV_8UC1 is one thing, but using CV_8UC3 instead CV_8U is a mess. This mess doesn't help improve code quality or help with code reviews. So, are all implementations already support depth/type interchangeability? I'm not sure about that.

[cv3d]

@vpisarev I think you got confused by the warnings and thought low of this PR. The main point of splitting the proposal into several PRs is to proof that each and every part can operate with user code. If you want to see the green lights, then how about giving #12288 a visit, which merges all the pieces together? BTW: It is no longer up-to-date.

Again, the proposal have several modes:

• You can switch all types to int (ElemDepth & ElemType are typedefed to int - they are exactly the same thing here)
• You can enable enums (ElemDepth & ElemType are traditional enums, thus they still have int as their base type)
• You can enable/disable int overloads and implicit conversions to maintain compatibility with user code (enabled by default, hopefully will get disabled by OpenCV 5)
• You can control if none, some, or all deprecation warnings are raised.

[vpisarev]

@cv3d, providing a million of options is not a solution of the problem, it's a workaround.

The headers should contain as few conditional compilation clauses as possible.

Please, create "all-green" PR. I want to see the possible damage to OpenCV itself and to the user code that the change brings. If it's too many changes, we'll just keep the status quo.

[vpisarev]

@cv3d, I discussed this one more time with @alalek. The common agreement is that it makes sense to start with higher-level modules and replace anonymous enumeration with named ones there.

I mean, we should start with videoio, introduce named enumeration for CAPPROP, continue with features2d, refactor enumerations there etc.

And in the end, we can do the final step and try to introduce ElemType/ElemDepth.

The attempt to do everything in one huge PR is not very good, because it's almost impossible to review

[vpisarev]

I mean, PRs #12550, #12551, #12553, #12555 should be closed. Because they attempt to change the same thing, just in different places. Instead, let's try to refactor those modules first w/o touching ElemType/ElemDepth at the moment

[cv3d]

I can rename ElemType and ElemDepth to int in all open PRs, thus you can consider them all as just some kind of cleanup. This is to avoid wasting the effort (due to conflicts that I fix everyday), and to simplify the future steps once we arrive to ElemType and ElemDepth again. @vpisarev Is that fair enough?

P.S. If you would allow ElemType and ElemDepth as just typedef to int, that would be great, and would save most of my efforts.

[alalek]

@vpisarev Changes of "High" level enums are in another PR (before this one): #12310 (and corresponding opencv_contrib)

[cv3d]

My plan is to tackle all the unnamed enums (only 32 out of 107 are complete), but it seems frustration is getting me now due to the tedious amendments ...

It is now up to @vpisarev, who seems to be the boss here, to let me to continue with my tasks, or to kick me away!

[cv3d]

create "all-green" PR

@vpisarev All lights are green now

[cv3d]

the possible damage to OpenCV itself and to the user code

@vpisarev Basically, the effects user might experience would be warnings from:

• The operator?: when comparing depth and type. This is believed to be useful in removing ambiguities
• The switch case of depth or type without a default case. Similarly, this is useful to avoid any possible bugs.
• The operator<< with a depth or type value. It can be overloaded, but we want the user to get notified. Perhaps user prefers to utilize depthToString() or typeToString() functions instead of an int value.

it's almost impossible to review they attempt to change the same thing, just in different places

Half of the PR changes are just a word in each line (e.g. CV_8U -> CV_8UC1, int -> ElemType), and the other half is the overloading part, in which once you see it two or three times, you would always recognize it, and can analyze it rapidly. Moreover, splitting a change into different PRs simplify the review process, enable parallel reviews, and more importantly, proves robustness in a mutually-exclusive and collectively-exhaustive way.

[cv3d]

it's almost impossible to review

I have reduced the size of PRs to something like half, and if you still think it is still huge, including this one, then I will split them further to make each one around 500 lines or so.

users do not have to change all CV_32F to CV32FC1 etc. And in general CV... and CV_...C1 should be identical entities, not different. ... Otherwise, ok, I will definitely review the patch as soon as you finish it.

create "all-green" PR

ElemType and ElemDepth should be made the same type

in the end, we can do the final step and try to introduce ElemType/ElemDepth

should be closed

@vpisarev Let me remind you you initially asked that single-channel type should not be different from a depth, and I did that. Later, you asked for green lights, and I did that. Even by now, I do not see an upper limit to your decisions, but to throw away a month-worth of effort, is not that what you are asking for now? Now, if you really want them so badly to be the same even in case of multi-channels, then I will do that, given that this is your final change of mind.

it makes sense to start with higher-level modules and replace anonymous enumeration with named ones there

I would understand your point if I am part of your team, but this is an external contribution, and I do not find it enough of a reason to reject a PR, just because someone makes it out of your expected order... Anyway, have you seen my "higher-level" changes in #12310 from a month ago? It is not merged yet either 😕

[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@e1c13c0c991af987e0f2b8d2e14516db3307aca3 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 (https://github.com/opencv/opencv/pull/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 😕