int to proper enum type

cv3d commented 5 years ago

TESTING... DO NOT MERGE! ~~Merge with opencv/opencv_contrib#1730 and opencv/opencv_extra#515~~

This pullrequest changes

There are hundreds of unnamed enums, and they are being passed as int in the C++ interface. In order to avoid passing a wrong value, and to maintain good practices, the hope is to name most of these enums, and change the int arguments into the proper enum types wherever possible.

Newly introduced enums

[x] ElemType for handling the depth and channels of matrix elements (Produced by DataType<T>::type and traits::Type<T>::value)
[x] ElemDepth for handling the depth of matrix elements (Produced by DataDepth<T>::type and traits::Depth<T>::value)
[x] MagicFlag for handling the mixed magic values of ElemType, AccessFlag and _InputArray::KindFlag

Flag enums

[x] AccessFlag for unnamed enum having ACCESS_READ as first member
[x] [Converted DrawMatchesFlags struct to enum struct] DrawMatchesFlags::unnamed enum having DEFAULT as first member
[x] MemoryFlag for UMatData::unnamed enum having COPY_ON_MAP as first member
[x] KindFlag for _InputArray::unnamed enum having KIND_SHIFT as first member

Mode enums

[x] DescriptorType for AKAZE::unnamed enum having DESCRIPTOR_KAZE_UPRIGHT as first member
[x] DetectorType for AgastFeatureDetector::unnamed enum having AGAST_5_8 as first member
[x] FeatureType for CvFeatureParams::unnamed enum having HAAR as first member
[x] DetectorType for FastFeatureDetector::unnamed enum having TYPE_5_8 as first member
[x] MatcherType for DescriptorMatcher::unnamed enum having FLANNBASED as first member
[x] FormatType for Formatter::unnamed enum having FMT_DEFAULT as first member
[x] HistogramNormType for HOGDescriptor::unnamed enum having L2Hys as the only member
[x] [Converted to static const NormTypes] for Hamming::unnamed enum having normType as the only member
[x] [Converted to static const NormTypes] for SL2::unnamed enum having normType as the only member
[x] [Converted to static const NormTypes] for L2::unnamed enum having normType as the only member
[x] [Converted to static const NormTypes] for L1::unnamed enum having normType as the only member
[x] [Converted to static const int] for Mat::unnamed enum having MAGIC_MASK as first member
[x] [Converted to static const int] for Mat::unnamed enum having MAGIC_VAL as first member
[x] [Converted to static const int] for SparseMat::unnamed enum having MAGIC_VAL as first member
[x] [Converted to static const int] for UMat::unnamed enum having MAGIC_MASK as first member
[x] [Converted to static const int] for UMat::unnamed enum having MAGIC_VAL as first member
[x] DiffusivityType for KAZE::unnamed enum having DIFF_PM_G1 as first member
[x] ScoreType for ORB::unnamed enum having kBytes as first member, and kBytes become a static const int
[x] [Converted Param struct to enum struct] Param::unnamed enum having INT as first member
[x] [Converted to static const int] for cuda::FastFeatureDetector::unnamed enum having LOCATION_ROW as first member
[x] [Removed the enum] for cuda::HOG::unnamed enum having DESCR_FORMAT_ROW_BY_ROW as first member
[x] [Converted to static const int] for cuda::ORB::unnamed enum having X_ROW as first member
[x] NormalizationType for xfeatures2d::DAISY::unnamed enum having NRM_NONE as first member
[x] [Converted to static const int] for xfeatures2d::FREAK::unnamed enum having NB_SCALES as first member
[x] DepthMask for _OutputArray::unnamed enum having DEPTH_MASK_8U as first member

Tasks list

[ ] :grey_question: for unnamed enum having CALIB_CB_ADAPTIVE_THRESH as first member
[ ] :grey_question: for unnamed enum having CALIB_CB_SYMMETRIC_GRID as first member
[ ] :grey_question: for unnamed enum having CALIB_USE_INTRINSIC_GUESS as first member
[ ] :grey_question: for unnamed enum having CAP_INTELPERC_DEPTH_GENERATOR as first member
[ ] :grey_question: for unnamed enum having CAP_INTELPERC_DEPTH_MAP as first member
[ ] :grey_question: for unnamed enum having CAP_OPENNI_DEPTH_GENERATOR as first member
[ ] :grey_question: for unnamed enum having CAP_OPENNI_DEPTH_MAP as first member
[ ] :grey_question: for unnamed enum having CAP_OPENNI_IMAGE_GENERATOR_PRESENT as first member
[ ] :grey_question: for unnamed enum having CAP_OPENNI_VGA_30HZ as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_DC1394_OFF as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_GIGA_FRAME_OFFSET_X as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_GPHOTO2_PREVIEW as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_GSTREAMER_QUEUE_LENGTH as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_IMAGES_BASE as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_INTELPERC_PROFILE_COUNT as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_IOS_DEVICE_FOCUS as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_OPENNI_OUTPUT_MODE as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_PVAPI_MULTICASTIP as first member
[ ] :grey_question: for unnamed enum having CAP_PROP_XI_DOWNSAMPLING as first member
[ ] :grey_question: for unnamed enum having CAP_PVAPI_DECIMATION_OFF as first member
[ ] :grey_question: for unnamed enum having CAP_PVAPI_FSTRIGMODE_FREERUN as first member
[ ] :grey_question: for unnamed enum having CAP_PVAPI_PIXELFORMAT_MONO8 as first member
[ ] :grey_question: for unnamed enum having CASCADE_DO_CANNY_PRUNING as first member
[ ] :grey_question: for unnamed enum having FM_7POINT as first member
[ ] :grey_question: for unnamed enum having INPAINT_NS as first member
[ ] :grey_question: for unnamed enum having LDR_SIZE as first member
[ ] :grey_question: for unnamed enum having LMEDS as first member
[ ] :grey_question: for unnamed enum having MOTION_TRANSLATION as first member
[ ] :grey_question: for unnamed enum having NORMAL_CLONE as first member
[ ] :grey_question: for unnamed enum having OPTFLOW_USE_INITIAL_FLOW as first member
[ ] :grey_question: for unnamed enum having RECURS_FILTER as first member
[ ] :grey_question: for unnamed enum having SOLVEPNP_ITERATIVE as first member
[ ] :grey_question: for RNG::unnamed enum having UNIFORM as first member
[ ] :grey_question: for StereoBM::unnamed enum having PREFILTER_NORMALIZED_RESPONSE as first member
[ ] :grey_question: for StereoMatcher::unnamed enum having DISP_SHIFT as first member
[ ] :grey_question: for StereoSGBM::unnamed enum having MODE_SGBM as first member
[ ] :grey_question: for Stitcher::unnamed enum having ORIG_RESOL as first member
[ ] :grey_question: for Subdiv2D::unnamed enum having NEXT_AROUND_ORG as first member
[ ] :grey_question: for Subdiv2D::unnamed enum having PTLOC_ERROR as first member
[ ] :grey_question: for TrackerSamplerCS::unnamed enum having MODE_POSITIVE as first member
[ ] :grey_question: for TrackerSamplerCSC::unnamed enum having MODE_INIT_POS as first member
[ ] :grey_question: for bioinspired::unnamed enum having RETINA_COLOR_RANDOM as first member
[ ] :grey_question: for detail::Blender::unnamed enum having NO as first member
[ ] :grey_question: for detail::ExposureCompensator::unnamed enum having NO as first member
[ ] :grey_question: for detail::Timelapser::unnamed enum having AS_IS as first member
[ ] :grey_question: for fisheye::unnamed enum having CALIB_USE_INTRINSIC_GUESS as first member
[ ] :grey_question: for ft::unnamed enum having LINEAR as first member
[ ] :grey_question: for ft::unnamed enum having ONE_STEP as first member
[ ] :grey_question: for line_descriptor::DrawLinesMatchesFlags::unnamed enum having DEFAULT as first member
[ ] :grey_question: for ml::EM::unnamed enum having DEFAULT_NCLUSTERS as first member
[ ] :grey_question: for ml::EM::unnamed enum having START_E_STEP as first member
[ ] :grey_question: for multicalib::MultiCameraCalibration::unnamed enum having PINHOLE as first member
[ ] :grey_question: for ocl::Device::unnamed enum having EXEC_KERNEL as first member
[ ] :grey_question: for ocl::Device::unnamed enum having FP_DENORM as first member
[ ] :grey_question: for ocl::Device::unnamed enum having NO_CACHE as first member
[ ] :grey_question: for ocl::Device::unnamed enum having NO_LOCAL_MEM as first member
[ ] :grey_question: for ocl::Device::unnamed enum having TYPE_DEFAULT as first member
[ ] :grey_question: for ocl::Device::unnamed enum having UNKNOWN_VENDOR as first member
[ ] :grey_question: for ocl::KernelArg::unnamed enum having LOCAL as first member
[ ] :grey_question: for omnidir::unnamed enum having CALIB_USE_GUESS as first member
[ ] :grey_question: for omnidir::unnamed enum having RECTIFY_PERSPECTIVE as first member
[ ] :grey_question: for omnidir::unnamed enum having XYZRGB as first member
[ ] :grey_question: for optflow::DISOpticalFlow::unnamed enum having PRESET_ULTRAFAST as first member
[ ] :grey_question: for rgbd::Odometry::unnamed enum having ROTATION as first member
[ ] :grey_question: for rgbd::OdometryFrame::unnamed enum having CACHE_SRC as first member
[ ] :grey_question: for structured_light::unnamed enum having DECODE_3D_UNDERWORLD as first member
[ ] :grey_question: for structured_light::unnamed enum having FTP as first member
[ ] :grey_question: for text::unnamed enum having ERFILTER_NM_RGBLGrad as first member
[ ] :grey_question: for text::unnamed enum having OCR_LEVEL_WORD as first member
[ ] :grey_question: for viz::KeyboardEvent::unnamed enum having NONE as first member
[ ] :grey_question: for viz::Mesh::unnamed enum having LOAD_AUTO as first member
[ ] :grey_question: for viz::WTrajectory::unnamed enum having FRAMES as first member

Current issues

[ ] [need remove from public API] FileStorage::unnamed enum having UNDEFINED as first member
[ ] A lot of static_cast<MagicFlag>() are utilized now. Proper operators needs to get enabled
[ ] Converteing HOGDescriptor::unnamed enum having DEFAULT_NLEVELS as the only member to to static const int causes some Java test to fail [javac] /build/precommit_android/build/android_test/src/org/opencv/test/objdetect/HOGDescriptorTest.java:204: error: cannot find symbol [javac] assertEquals(HOGDescriptor.DEFAULT_NLEVELS, hog.get_nlevels()); [javac] ^ [javac] symbol: variable DEFAULT_NLEVELS [javac] location: class HOGDescriptor [javac] Note: Some input files use unchecked or unsafe operations. [javac] Note: Recompile with -Xlint:unchecked for details. [javac] 1 error

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

alalek commented 5 years ago

enum ACCESS_FLAG

Use 'CamelCase' for types / enums. All capitals letters are reserved for macro / constants itself.

BTW, This is not regular (simple) enum. This is "bit-set" enum, for example: ACCESS_READ | ACCESS_FAST is valid parameter.

alalek commented 5 years ago

Some design work is required to resolve "enum" handling.

Enum categories

I see these classes of enums used in OpenCV:

regular enums with exclusive values (usually has "Mode" suffix, but avoid "Modes"): one value for parameter is allowed only.
bit-set enums ("Flag"/"Flags" suffix): OR-ed values are allowed. To avoid verbose "static_cast
(..)" we can try this:
- perhaps we should add enum entries for some actively used cased like :
  - ACCESS_RW = ACCESS_READ | ACCESS_WRITE
  - ACCESS_READ_FAST = ACCESS_READ | ACCESS_FAST
- alternatively we can try to overload operator | () (Will it work?). But it is C++ only. What is about Python / Java?
videoio's CAP_PROP_* enums. There are many enums, but they are passed to single function (.get() method). No idea about C++, but in Python we can group them into single enum type:
- VideoCaptureProperty.CAP_PROP_GSTREAMER_QUEUE_LENGTH
- VideoCaptureProperty.CAP_PROP_IMAGES_BASE
enums in classes. Some of them are used as a constants for native int parameters, like DEFAULT_NLEVELS. No need to name them. Perhaps some of these enums should be replaced to static const int DEFAULT_NLEVELS = 64; (there are no more issues with modern compilers anymore) and bindings generator should know them

Handling enums in Python.

Currently all enums are "int" contants in Python. This approach is not safe and error prone. It would be nice to introduce real enum types in Python.

Unfortunately, only Python 3.6+ have "native" enum types:

from enum import Enum
class AccessFlag(Enum):
    ACCESS_READ = 1 << 24
    ACCESS_WRITE = 2 << 24
    ACCESS_RW = 3 << 24
    ...

We should emulate this for other Python versions somehow.

Usage compatibility between C++ and Python

In C++ enum type is not used for accessing of enum value (added to namespace/class): cv::ACCESS_READ In Python it is required to use enum name: cv.AccessFlag.ACCESS_READ

So probably we should duplicate enum values into current module to save compatibility with existed Python code and keep similarity between C++/Python code:

class AccessFlag(Enum):
    ACCESS_READ = 1 << 24
    ...

ACCESS_READ = AccessFlag.ACCESS_READ
ACCESS_WRITE = AccessFlag.ACCESS_WRITE

cv3d commented 5 years ago

alternatively we can try to overload operator | () (Will it work?)

operator | () will not work, as it causes issues with Eigen library. Can we define it only for cv namespace by any chance? As for link you provided, it seems to refer to operator |= (), which I already defined.

hrnr commented 5 years ago

Thanks for revisiting enums for OpenCV 4 this was bothering me too.

enums in classes.

some of those enums in classes are there for encapsulation. However they seem to cause problems for Python wrapper (#10681). Could this be supported for wrapping? Should those enums be moved to be consistent with rest of the OpenCV?

Usage compatibility between C++ and Python

Python enums are basically enum class in C++11. Making all enums enum class would be, however, a big disaster (without any backward compatibility stuff).

operator | () will not work, as it causes issues with Eigen library.

I can't see why that should cause any issues for Eigen. What was the exact error?

If possible, I think introducing this operator for "flags" enums like this would be quite an easy and backward-compatible way. Of cause wrap this in macro, so we can easily add it for enums that need it. Like CV_FLAGS_ENUM(AccessFlag).

cv3d commented 5 years ago

I can't see why that should cause any issues for Eigen. What was the exact error?

If I define:

template<typename T, typename std::enable_if< std::is_enum<T>::value >::type* = nullptr>
static T operator|(const T& a, const T& b)
{
    typedef typename std::underlying_type<T>::type EnumType;
    return static_cast<T>(static_cast<EnumType>(a) | static_cast<EnumType>(b));
}

Then the following errors occurs, and while I can manage the first bullet, I do not know how to fix the second:

All enum definitions in the form ACCESS_READ_FAST = ACCESS_READ | ACCESS_FAST results in error C2057: expected constant expression
The type UpperBidiagonalization::DiagVectorType results in error C2975: '_Options' : invalid template argument for 'Eigen::Matrix', expected compile-time constant expression

alalek commented 5 years ago

Just don't use templates. Separate operator definition for each "flags" enum, something like this:

// in namespace cv
static inline AccessFlags operator | (const AccessFlags& a, const AccessFlags& b)
{
    return (AccessFlags)((int)a | (int)b);
}
// this should work now: AccessFlags flags = ACCESS_READ | ACCESS_WRITE;

CV_FLAGS_ENUM(AccessFlag)

I like this idea to declare multiple operators.

hrnr commented 5 years ago

that seems to be the same issue. constexpr should fix that.

However, I think it is not a good idea to declare templated operator |() for all enums. Even though the operator is declared in cv namespace it can be ADL-resolved for user code. That is what happened for Eigen.

@alalek exactly!

hrnr commented 5 years ago

To resolve ACCESS_READ_FAST = ACCESS_READ | ACCESS_FAST error I think you need to make the operator constexpr, so it can be used in constant expressions.

To be specific:

#define CV_FLAGS_ENUM(EnumType)                                                \
  static constexpr EnumType operator|(const EnumType &a, const EnumType &b) {  \
    typedef typename std::underlying_type<EnumType>::type u_type;              \
    return static_cast<EnumType>(static_cast<u_type>(a) |                      \
                                 static_cast<u_type>(b));                      \
  }

(you should add more operators for &, |=, &= etc.)

Then declare

enum AccessFlag { ACCESS_READ=1<<24, ACCESS_WRITE=1<<25,
 ACCESS_RW=3<<24, ACCESS_MASK=ACCESS_RW, ACCESS_FAST=1<<26,
 ACCESS_READ_FAST = ACCESS_READ | ACCESS_FAST };
CV_FLAGS_ENUM(AccessFlag);

constexpr allows to use the operator in constant expressions.

This approach allows us to declare the operator only for enums where it makes sense.

BTW: I'm not sure if we need ACCESS_READ_FAST really when we have working operator |(), but it possible to define it in this way with this approach.

cv3d commented 5 years ago

Up until now, it seems Java was not really involved, but we have problems with DescriptorMatcher::MatcherType enum and the overloaded DescriptorMatcher::create().

compile:
    [mkdir] Created dir: /build/precommit_linux64/build/java_test/build/classes
    [javac] Compiling 55 source files to /build/precommit_linux64/build/java_test/build/classes
    [javac] /build/precommit_linux64/build/java_test/src/org/opencv/test/features2d/BruteForceDescriptorMatcherTest.java:84: error: incompatible types: int cannot be converted to String
    [javac]         matcher = DescriptorMatcher.create(DescriptorMatcher.BRUTEFORCE);
    [javac]                                                             ^

Java assumes the type of DescriptorMatcher.BRUTEFORCE to be int (and all constants in general), thus tried the wrong overloaded variant instead of the enum-based one.

@alalek @hrnr It seems there is a need to update the code generator and pass the enum type to the consts before proceeding any further.

alalek commented 5 years ago

@cv3d You can temporary broke Java detection in CMake via separate "! test" commit, to pass compilation and run other C++/Python tests.

cv3d commented 5 years ago

@alalek Java issue has been fixed in #12303. Kindly be reminded to remove the future label.

BTW: Shall we merge chunks of this PR every now and then, or shall we make it a huge one?

cv3d commented 5 years ago

In Python it is required to use enum name: cv.AccessFlag.ACCESS_READ

The current wrapper generates: cv.ACCESS_READ for standard enums, and cv.AccessFlag_ACCESS_READ (note the underscore) for C++11 enums (enum class/struct).

Do you want me to map to the C++11 enums to cv.AccessFlag.ACCESS_READ instead?

alalek commented 5 years ago

cv.AccessFlag.ACCESS_READ

This is preferable way.

BTW, Perhaps in case of using C++11 enums entry names should be reviewed. There is no reason to duplicate ACCESS_ prefix: cv::AccessFlag::READ. So lets leave strict rules for C++11 enums out of scope for now.

hrnr commented 5 years ago

Hi, I have just got back from the holiday. Great progress on this. If you want some help with boring parts of this let me know.

cv3d commented 5 years ago

Could this be supported for wrapping? Should those enums be moved to be consistent with rest of the OpenCV?

@hrnr This is my main goal, actually. I might consider that soon enough after I get ride of the temporary operators in this PR. BTW: you can check #12269 and #12303 for my other Python-related PRs, in which binding is now much more easier, and it is getting C++11 ready.

cv3d commented 5 years ago

@alalek @hrnr I introduced the following:

ElemDepth type to represents CV_8U and sisters
ElemType type to represents CV_8UC1 and sisters

Which leads to this logical:

CV_8U and CV_8UC1 are not the same C++ type
When Mat.create expects a type, CV_8U will not work, but CV_8UC1 or CV_MAKETYPE(CV_8U, 1) will
convertTo and assignTo take only ElemDepth since the channels are kept intact. Thus CV_8UC1 will not work, but CV_MAT_DEPTH(CV_8UC1) or CV_8U will.

Currently, the implementation is not complete, and perhaps only the core module can compile. Therefore, defining the prerocessor __cv_strict_types is required to turn this experimental behavior on. I think there is a need to define some macros to simplify some task, such as comparing depth types to get the bigger one.

BTW: The work load of ElemDepth and ElemType enums is more than what I expected. So, how about we merge the current commits and collaborate on the remaining?

hrnr commented 5 years ago

You are really brave to touch this, I wouldn't dare. :-) I think the current strict version will literally break all the existing opencv code.

I see three alternatives:

add overload to all relevant functions to accept both types. Later the overloads could be deprecated. (I don't believe this will ever happen.) Those overloads could be also targeted using clang auto-refactoring tools.
Make them both the same type.
Keep all the functions accepting ints.

First two alternatives will probably break some opencv code, but the impact should be pretty minimal. They shouldn't break much of the user code. The last one seems to be a safe bet, although it is not type-safe.

cv3d commented 5 years ago

@hrnr I like the idea of overloads with deprecation notice, but I wonder how likely it can be implemented. Actually I am thinking of dropping the depth and type enums and just make the ints for the time being, but before that, I would give it another day or so.

hrnr commented 5 years ago

I think it should be pretty straightforward to implement. My comment was just about the deprecation notice, that would introduce a lot of warnings. That is IMHO unrealistic to do without tool support.

If you don't want to touch this after all, I think you can still leave those two enums there. Enums are implicitly convertible to ints, so it shouldn't break much of the code. However, I may be wrong here, as this is also a dark corner for me.

cv3d commented 5 years ago

@alalek Are there any files I should not touch? I feel like there are some that are meant to be C-only, and I wonder if it is still the case even in OpenCV v4

alalek commented 5 years ago

@cv3d Ignore legacy C API.

alalek commented 5 years ago

Great work!

But we need to ensure that "switch with deprecation notice" works really, because changes can be really massive in user code.

Notes:

modules/core/include/opencv2/core/hal/interface.h

Lets keep this file C compatible. Please add #ifdef __cplusplus and restore #define CV_8U 0 defines in "#else" branch.

Please don't force enum types in "hal" includes (keep "int"), like this:

CV_EXPORTS void cvtGraytoBGR(const uchar * src_data, size_t src_step,
                             uchar * dst_data, size_t dst_step,
                             int width, int height,
                             int depth, int dcn);

Lets update only general public OpenCV C++ API

If there is some issues with InputArray classes then you may leave them "as is" (they are not designed for using by OpenCV users directly, except may be algorithms developers).

cv3d commented 5 years ago

@alalek This build reports an error in test_superres without an actual error. Can you double check?

alalek commented 5 years ago

This is sporadic failure (not related to your patch).

cv3d commented 5 years ago

we need to ensure that "switch with deprecation notice" works really

@alalek How about cross validation? Previously compiled opencv_test_* and opencv_perf_* can be utilized to test the new interface, cannot they?

alalek commented 5 years ago

-    p = cv::Mat(3, N, CV_64F);
+    p = cv::Mat(3, N, CV_64FC1);
-    z = cv::Mat(3, N, CV_64F);
+    z = cv::Mat(3, N, CV_64FC1);
-    mn = cv::Mat::zeros(3, 1, CV_64F);
+    mn = cv::Mat::zeros(3, 1, CV_64FC1);

I agree that this proposed interface is stronger.

But there is a lot of user code that already using Depth instead of Type in Mat constructors. We can't fix their code. Also we should not make a lot of pain due OpenCV upgrade.

Lets make old existed code to work too. Don't break it. For example, add overloads for constructors which accepts Depth too (inline overload which just casts depth->type and then calls correct constructor with "type").

cv3d commented 5 years ago

Lets make old existed code to work too

This is on my schedule, but now I am still busy extending the new interface to fully cover the functionalities.

vpisarev commented 5 years ago

@cv3d, first of all, thank you for all the patches that you prepare for the library, it's truly great and useful work! And this patch will certainly makes the API more convenient, less error-prone and also will make debugging more convenient.

I will elaborate at bit what @alalek said - we've discussed your patch today in the office. It seems like a great idea to replace all macros with enumerations, and in most cases it works well. But when we try to do so with CV_8U ... CV_64F, CV_8UC1 ... CV_64FC4 etc. - we break too much stuff. I think, you've noticed it too when working on the patch. And the main problem is that it's not just OpenCV code itself that needs to be extensively patched. I foresee that with such a change many, many users will have to patch their code as well, since CV_8U becomes different from CV_8UC1 etc.

So, we need to undo this part of the patch. I don't know what would be the easiest way - either to undo all such changes or close this PR, open fresh one and copy the necessary changes without touching depth and type macros. In any case, I suggest to split this patch into several ones, and also start with the high-level algorithms that have little code dependent on them. E.g. refactoring everything in features2d and related samples - this can be one patch. Refactoring everything in calib3d - the second one etc. When you get to some enumerations that require dozens of files to be changed, probably it may sense to ask yourself and maybe us - whether it's worth to do. May be not. Compatibility is the priority number one, convenience is the priority number 2 - most of the time and especially when we talk about very basic stuff.

We are releasing OpenCV 4.0 beta next week and then OpenCV 4.0 final in about 1-1.5 months from now. Let's target OpenCV 4.0 final (I believe, there will be some 4.0rc or something like that).

What do you think?

cv3d commented 5 years ago

@vpisarev I know you are concerned about backwards compatibility, so do I. But I believe I can provide a fully backwards compatible version of this PR in two days, so how about you give it a check by then? Of course I will split the test commits into smaller chunks, and I hope you can accept it into the first beta release. In fact, I am working myself hard to get it merged before first release. So how about you consider that?

If you are concerned about the current strict version I am writing, then have no worries, as I am making it so only to enforce good practices inside the library itself, but once I finish I will relax that, and let it accept the legacy input as well, perhaps with a warning message that appears only once.

cv3d commented 5 years ago

@alalek I would really appreciate it if you can help me understanding why this build is getting bad accuracy on test_core.

If __cv_strict_types is not set, it just works great, and I beleive all the #if and #else parts around __cv_strict_types are equivalent.. Any idea?

cv3d commented 5 years ago

@alalek Since this PR changes a lot of files, surely you will have frequent conflicts when merging any future PRs from 3.4 branch into master. To reduce such conflicts, how about we first ship these commits to 3.4 branch with typedef int instead of the newly introduced enums, and then, in master, we utilize the enums..

alalek commented 5 years ago

There is really huge conflict with another huge patch: #11630 I will try to rebase this patch and resolve conflicts myself and provide link on updated commits.

Do you mean typedef int ElemType; and update all interfaces ?

It makes sense, but need to check/update bindings parser to understand this. But this will not avoid these conflicts.

cv3d commented 5 years ago

typedef int ElemType;

Yes. if we can merge a PR that looks like this into 3.4 with typedef int instead of the enums, then at least we can guarantee no more similar conflicts like the current one happens.

hrnr commented 5 years ago

Hi,

I have read though the current patch.

I was just wondering if you think it would make sense to introduce the compatibility overloads first, without touching much of tests and code.

The patch would be much smaller, with less conflicts. We would be also pretty sure that the patch does not break user code if it requires no substantial changes in opencv.

Later, we can fix the usage of the overloads in OpenCV and possibly also disable them for opencv builds.

Cheers, Jiri

cv3d commented 5 years ago

@hrnr It makes sense, but the library would remain type-unsafe from inside, and I doubt it would get ride of such weakness for a long time. Instead, the current approach makes OpenCV a type-safe library in no-time, then we can utilize utilize cross validation to make sure it is compatible with its int-based interface. Furthermore, with a small warning message, even users will gradually catch up to the type-safe API.

If OpenCV 4 is released without deprecating the int-based interface, I suspect it would get deprecated ever...

alalek commented 5 years ago

@cv3d Commits are on this branch in my fork: https://github.com/alalek/opencv/tree/pr12288_rebase

@hrnr Right, we can use non-core OpenCV code / tests as "user code", which we should not break.

vpisarev commented 5 years ago

@cv3d, well, one of the acceptance criteria for me would be - users do not have to change all CV_32F to CV_32FC1 etc. And in general CV_... and CV_...C1 should be identical entities, not different. In your patch this is quite big fraction of all changes. Otherwise, ok, I will definitely review the patch as soon as you finish it.

cv3d commented 5 years ago

@alalek @vpisarev @hrnr Here is a summary of the changes in the API. I suspect I changed some APIs that you do not want it to be changed, but please be reminded the overloaded interface should compensate for that. However, if you really insist on a change, then please let me know.

BTW: There are array and raw pointer-based APIs, should not that be replaced with vector and smart pointers?

cv3d commented 5 years ago

@alalek I am getting this error on Linux and Mac OS builds:

undefined reference to `cv::traits::Type<cv::Point_<int> >::value

Any idea?

alalek commented 5 years ago

It wants declaration outside of class. "constexpr" doesn't help here (until C++17). "inline static members" is C++17 feature too.

https://en.cppreference.com/w/cpp/language/static:

If a const non-inline (since C++17) static data member or a constexpr static data member (since C++11) is odr-used, a definition at namespace scope is still required, but it cannot have an initializer. This definition is deprecated for constexpr data members (since C++17).

"odr" includes "const ElemType&". For example, from here:

cv::operator | (cv::_InputArray::KindFlag const&, ElemType const&)

IMHO, "const reference" enums parameters should be avoided. Use just "const EnumType".

P.S. It is better to check "Linux Debug" build. Partial fix: https://github.com/alalek/opencv/commit/pr12288_fix_enum_odr

cv3d commented 5 years ago

@alalek @vpisarev @hrnr There is a chance I made some serious mistakes in the API.. Kindly be asked to review the new API from the summary table I wrote above as soon as you can, because it would be almost impossible to manage changing it a lot once I start splitting the commits into different PRs with several commits depending on each other.

As for type names, I still plan to change them to MatType and MatDepth after the library passes the compatibility checks, because such types are already utilized in the testing suite.

I guess I have finished the major parts of this PR, and I plan to start writing the overloaded legacy-compatible interface after a few hours break.

cv3d commented 5 years ago

@alalek I need your help, please!

I am writing the overloaded legacy API, but I got thousands of x function already defined in object y errors, which most probably means inline definition at the header file is not going to work. Shall I proceed and move the definition bodies to the corresponding cpp files?

More importantly, the overloaded API causes ambiguity such as in this build. Any idea for a workaround?

@alalek @hrnr @vpisarev At any rate, I am afraid I will not finish by today, which was the deadline I made for myself. Thus, if you think the current ambiguity problem is workable in time, then I will give it another day or so, otherwise I can rewrite all the enums as #typedef int EnumName, thus maintaining optimal compatibility (but not type-safety 😕), and to make the library in a slightly better shape. What do you think?

alalek commented 5 years ago

Preserving "legacy" API with "int" in my meaning is:

typedef int ElemType;
typedef int ElemDepth;

#define CV_8U 0
#define CV_8S 1
...

This is already done in interface.h.

No need to have both "overloads" with "enum" and "int" in OpenCV binaries. If users want to use "int" then they should recompile their OpenCV 4.0 copy with disabled __cv_strict_types definition.

Another "legacy" API compatibility is about using ElemDepth for ElemType like:

cv::Mat m(10, 10, CV_32F);
// do not require: cv::Mat m(10, 10, CV_32FC1);

It will be good if we handle the most part of cases from your commit "Refactored code for type-safety". BTW, Please leave "as is" interfaces for functions like cvCreateMat() (it is C-API, don't push enums here - there is no way to make helpers-operators)

So, save your commit "Refactored code for type-safety" in a separate branch (for further/separate PR) and remove it from the current patch. Use unchanged OpenCV code (with CV_32F instead of CV_32FC1) as a test ground. Then start adding overloads to cover massive build problems. Please make this overloads "inline" which just calls "original" method/function. Rare cases fix inplace (but they should be really rare). There is no goal to support both ElemType and ElemDepth in all cases.

alalek commented 5 years ago

Below are my proposals for this snippet (from mat.hpp):

#if defined(cv_deprecated_api) && defined(__cv_strict_types) && defined(__cplusplus)
    void create(Size sz, int type, int i = -1, bool allowTransposed = false, _OutputArray::DepthMask fixedDepthMask = static_cast<_OutputArray::DepthMask>(0)) const
    {
        CV_COUNTER_INIT()
        CV_DEPRECATED_TYPE_WARNING("type", "int", "ElemType", 1);
        CV_COUNTER_INC()
        return create(sz, static_cast<ElemType>(type), i, allowTransposed, fixedDepthMask);
    }
#endif

1.

&& defined(__cplusplus)

Remove this. Do not touch C-only files.

2.

        CV_COUNTER_INIT()
        CV_DEPRECATED_TYPE_WARNING("type", "int", "ElemType", 1);
        CV_COUNTER_INC()

Perhaps this should be done in other way (compiler warning, instead of runtime). Lets remove this and leave for further changes.

3.

void create(...)

Use "inline": inline void create(...)

4.

int type

Perhaps we don't want to see here "5" instead of CV_32F / CV_32FC1. Even in user code. @vpisarev , Right? Please use "ElemDepth type" instead to allow CV_32F only.

Completely legacy code which wants "int" should re-compile own OpenCV without __cv_strict_types and with typedef int ElemType.

cv3d commented 5 years ago

@alalek Even with that, there is still the cases where the user passes -1 for unspecified depth/type. I believe such case will not work unless int overload is there. Any decision about that?

alalek commented 5 years ago

-1

Usually such params has default "-1" value on the last place and it is not used.

There are some cases where such parameter is not on the last position:

-cv::boxFilter(src_roi, dst_roi, -1, ksize, anchor, normalize, borderType)
+cv::boxFilter(src_roi, dst_roi, CV_DEPTH_UNSPECIFIED, ksize, anchor, normalize, borderType)

Sobel, filter2D, Laplacian, etc

There is another type of problem - local type variables which uses 'int'

int type = src.type();
...
dst.create(..., type);

This may require larger changes in user code if there is no "int" overloads. Usually it is enough using auto type.

But both cases are rare than "cv::Mat m(10, 10, CV_32F);" problem.

vpisarev commented 5 years ago

1) I suggest to make 'depth' and 'type' enumerations the same thing, that will help us to avoid unnecessary CV_8U to CV_8UC1 conversions. 2) CV_DEPTH_UNSPECIFIED sounds too complex; let's use CV_DEPTH_AUTO or even better CV_TYPE_AUTO, as I suggest to combine depth and type into a single enumeration.

At the same time, the newly created mat's may be initialized with type=CV_TYPE_UNDEFINED, as we discussed with @alalek.

cv3d commented 5 years ago

@vpisarev @alalek @mshabunin I am all done with all the testing and verification.

Currently, this PR works as a proof that the mutually-exclusive "Refactor *" PRs not only builds successfully, but they are collectively resolving all warnings (please note the green lights on this PR) even if OPENCV_ENABLE_DEPRECATED_WARNING_ELEMDEPTH_ELEMTYPE_OVERLOAD is set (which I wish to keep it ON in the farm for code quality - enabled via the last commit)

opencv / opencv