int to proper enum type #12288

cv3d · 2018-08-23T09:59:31Z

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

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

Flag enums

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

Mode enums

Tasks list

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 · 2018-08-23T10:05:49Z

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 · 2018-08-23T12:14:45Z

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 · 2018-08-23T12:23:27Z

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 · 2018-08-23T17:01:20Z

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

hrnr · 2018-08-23T17:09:07Z

modules/core/include/opencv2/core/types.hpp

@@ -63,6 +63,16 @@ namespace cv
 //! @addtogroup core_basic
 //! @{

+//////////////////////////////// Enum operators //////////////////////////////
+
+template<typename T, typename std::enable_if< std::is_enum<T>::value >::type* = nullptr>


Maybe consider adding this only for "flags" enums on per-enum basis. For "mode" enums this does not make sense. It could confuse users to use this to pass OR-ed values where it should not be used.

Also operator | () is needed too.

cv3d · 2018-08-23T17:25:00Z

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 · 2018-08-23T19:22:17Z

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 · 2018-08-23T19:24:45Z

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 · 2018-08-23T19:44:26Z

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

How about this solution? I guess it is mature enough, is not it?

cv3d · 2018-08-23T19:46:56Z

modules/core/include/opencv2/core/types.hpp

+template<typename T, typename T2, typename std::enable_if< std::is_enum<T>::value
+    && T::CV_FLAGS_ENUM == T::CV_FLAGS_ENUM >::type* = nullptr>
+static T operator|(const T2& a, const T& b)
+{


How about utilizing SFINAE and introducing CV_FLAGS_ENUM in the flag-based enums? I guess this solution is elegant enough.

cv3d · 2018-08-23T19:48:48Z

modules/core/include/opencv2/core/mat.hpp

-    ACCESS_RW=3<<24, ACCESS_MASK=ACCESS_RW, ACCESS_FAST=1<<26 };
+enum AccessFlag { ACCESS_READ=1<<24, ACCESS_WRITE=1<<25,
+    ACCESS_RW=3<<24, ACCESS_MASK=ACCESS_RW, ACCESS_FAST=1<<26,
+    CV_FLAGS_ENUM = ACCESS_READ };



As you can see, CV_FLAGS_ENUM is introduced to enable bitwise operators, and it can take any value, and in this case, it is assigned ACCESS_READ for safety against ignorant users.

Cool. I think that would work.

The obvious disadvantage is that we have extra CV_FLAGS_ENUM in every flags enum and in documentation etc. Also in my experience widely used SFINAE can affect compilation times pretty badly.

Therefore, I would prefer the solution above, but this is certainly workable too. If you want to go with this, I suggest to add constexpr to allow using it in constant expressions.

constexpr is available from Visual Studio 2015.
Do we really want to drop support of Visual Studio 2013? It is still widely used...

I think OpenCV 4 will be C++11 as per OE-4

cv3d · 2018-08-23T20:22:45Z

modules/core/src/matrix_wrap.cpp

@@ -15,7 +15,7 @@ namespace cv {
 Mat _InputArray::getMat_(int i) const
 {
    int k = kind();
-    int accessFlags = flags & ACCESS_MASK;
+    AccessFlag accessFlags = flags & ACCESS_MASK;



@hrnr The problem with the macro-based solution is that it cannot handle two types T1, T2, thus the above expression would not compile since flags is int. Of couse we can introduce cast in such cases, but this might break user code as well. The only solution that seems to work with minimal changes is a template-based one. Accordingly, I would like to go with my proposal, at least for the time-being.

What is about this?

AccessFlag operator & (const int a, const AccessFlag& b)

This also would work, but I am not sure if int is the only type utilized with bitwise operations (how about long?), and I do not think we want to provide all the variants. On the other hand, the template-based solution referenced above is just doing that with minimal effort, I guess.

int, long and other helper operators can be placed under CV_FLAGS_ENUM(enumType) macro:

enum AccessFlag { ... }; CV_FLAGS_ENUM(AccessFlag); // define helper operators here

It can be added for each bit-set/flags enum types.

cv3d · 2018-08-23T22:12:57Z

modules/core/include/opencv2/core/types.hpp

+CV_FLAGS_ENUM_XOR_EQ(EnumType, EnumType);                                                          \
+CV_FLAGS_ENUM_OR_EQ (EnumType, int);                                                               \
+CV_FLAGS_ENUM_AND_EQ(EnumType, int);                                                               \
+CV_FLAGS_ENUM_XOR_EQ(EnumType, int);                                                               \



@alalek @hrnr I am glad there is no limit for macros length ;)

Although it is hard to maintain, I need to admit that it is looks beautiful and have optimal performance.

Nice work. Keep going!

I think that int is enough for the overloads as enums convert implicitly to ints (we have only int enums). Moreover, I think we should get rid of those overloads one day to disallow AccessFlags flags = cv::RANSAC | ACCESS_WRITE;. Casts would need to be added to some places though.

The good news is, if cv::RANSAC is not an int, AccessFlags flags = cv::RANSAC | ACCESS_WRITE would not work.
That is another reason to avoid using int, and use enum types instead.

cv3d · 2018-08-24T07:50:49Z

modules/core/src/ocl.cpp

+        if (!(arg.flags & KernelArg::READ_ONLY))
+            accessFlags &= ~ACCESS_READ;
+        if (!(arg.flags & KernelArg::WRITE_ONLY))
+            accessFlags &= ~ACCESS_WRITE;
        bool ptronly = (arg.flags & KernelArg::PTR_ONLY) != 0;


@alalek @hrnr We do not have ACCESS_NONE to write something like ((arg.flags & KernelArg::READ_ONLY) ? ACCESS_READ : ACCESS_NONE) | ((arg.flags & KernelArg::WRITE_ONLY) ? ACCESS_WRITE : ACCESS_NONE) and even the ACCESS_MASK is not covering the FAST_ACCESS bit...
In this case, all I can think of is the negative logic. Is there any better idea to do this more efficiently?

For any "flags" enum, "0" is default "none" value.

I would like to avoid introducing separate meaningless enum value like ACCESS_NONE. Perhaps we can write here (AccessFlag)0 instead.

It is better to drop "negative logic" in code, like your suggestion.

Alternatively we can try to overload operator - () for "flag" enums (but we should make it safe).

cv3d · 2018-08-24T13:29:21Z

modules/core/include/opencv2/core/types.hpp

+
+    // Exposes the listed 5 members of the enum class AccessFlag to the current namespace
+    CV_ENUM_CLASS_EXPOSE(AccessFlag, 5, ACCESS_READ, ACCESS_WRITE, ACCESS_RW, ACCESS_MASK, ACCESS_FAST);
+@endcode


Making all enums enum class would be, however, a big disaster (without any backward compatibility stuff).

I managed to get C++11 enum classes working with backwards compatibility and minimal changes.

Basically, instead of:

enum xyz { m1, m2, m3};

one would define:

enum class xyz { m1, m2, m3}; CV_ENUM_CLASS_EXPOSE(xyz, m1, m2, m3);

and in case it is a flag enum, then CV_ENUM_FLAGS_CLASS(AccessFlag); instead of CV_ENUM_FLAGS(AccessFlag); would do the trick.

Only Python module still have issues with that, but I believe I can work around that as well.
@alalek @hrnr Shall I consider changing the enums into enum classes?

cv3d mentioned this pull request Aug 23, 2018

int to proper enum type opencv/opencv_contrib#1730

Closed

cv3d force-pushed the improvements/enum_interface branch from adafff3 to 2e2892d Compare August 23, 2018 10:25

cv3d force-pushed the improvements/enum_interface branch 6 times, most recently from 8fd21a8 to 1cf6d48 Compare August 23, 2018 16:48

hrnr reviewed Aug 23, 2018

View reviewed changes

cv3d force-pushed the improvements/enum_interface branch from 1cf6d48 to a69f060 Compare August 23, 2018 18:33

cv3d force-pushed the improvements/enum_interface branch from a69f060 to 80d8839 Compare August 23, 2018 19:44

cv3d commented Aug 23, 2018

View reviewed changes

cv3d force-pushed the improvements/enum_interface branch from 80d8839 to 25c0bd6 Compare August 23, 2018 22:10

cv3d commented Aug 23, 2018

View reviewed changes

cv3d force-pushed the improvements/enum_interface branch from 25c0bd6 to cff9c46 Compare August 24, 2018 07:46

cv3d commented Aug 24, 2018

View reviewed changes

cv3d force-pushed the improvements/enum_interface branch 3 times, most recently from e6f1bbc to 20517ce Compare August 24, 2018 13:19

cv3d commented Aug 24, 2018

View reviewed changes

cv3d force-pushed the improvements/enum_interface branch from 20517ce to a01d44a Compare August 24, 2018 13:32

cv3d added 20 commits September 20, 2018 06:27

Macros for enum-arguments backwards compatibility

47ee431

Convert struct Param to enum struct

07ba24d

Enabled ParamType.type for enum types

6080fd6

Enabled cv.read and cv.write for enum types

55b1c9a

Rename unnamed enum to AAKAZE.DescriptorType

dcac3ca

Rename unnamed enum to AccessFlag

e19a916

Rename unnamed enum to AgastFeatureDetector.DetectorType

dc32213

Convert struct DrawMatchesFlags to enum struct

e832e09

Rename unnamed enum to FastFeatureDetector.DetectorType

1994093

Rename unnamed enum to Formatter.FormatType

b42c732

Rename unnamed enum to HOGDescriptor.HistogramNormType

8b14a7f

Rename unnamed enum to DescriptorMatcher.MatcherType

c5e80e0

Rename unnamed enum to KAZE.DiffusivityType

07bb784

Rename unnamed enum to ORB.ScoreType

8ca0e3d

Rename unnamed enum to UMatData.MemoryFlag

42d915b

Rename unnamed enum to _InputArray.KindFlag

94f6825

Rename unnamed enum to _OutputArray.DepthMask

dd523e5

Convert normType enums to static const NormTypes

b9fd60b

Avoid conflicts with ElemType

f448ce8

Rename unnamed enum to DescriptorStorageFormat

cd8f009

cv3d force-pushed the improvements/enum_interface branch 7 times, most recently from 5bc7784 to 794e88d Compare September 20, 2018 14:38

Refactor

65b4ad7

cv3d force-pushed the improvements/enum_interface branch from 794e88d to 65b4ad7 Compare September 20, 2018 15:09

vpisarev closed this Oct 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

int to proper enum type #12288

int to proper enum type #12288

cv3d commented Aug 23, 2018 •

edited

alalek commented Aug 23, 2018

alalek commented Aug 23, 2018 •

edited

cv3d commented Aug 23, 2018

hrnr commented Aug 23, 2018

Usage compatibility between C++ and Python

hrnr Aug 23, 2018

hrnr Aug 23, 2018

cv3d commented Aug 23, 2018 •

edited

alalek commented Aug 23, 2018 •

edited

hrnr commented Aug 23, 2018

hrnr commented Aug 23, 2018 •

edited

cv3d left a comment

cv3d Aug 23, 2018

cv3d Aug 23, 2018

hrnr Aug 23, 2018

cv3d Aug 23, 2018

hrnr Aug 23, 2018

cv3d Aug 23, 2018

alalek Aug 23, 2018

cv3d Aug 23, 2018

alalek Aug 23, 2018

cv3d Aug 23, 2018

cv3d Aug 23, 2018

hrnr Aug 23, 2018

cv3d Aug 24, 2018 •

edited

cv3d Aug 24, 2018

alalek Aug 24, 2018

cv3d Aug 24, 2018 •

edited

int to proper enum type #12288

int to proper enum type #12288

Conversation

cv3d commented Aug 23, 2018 • edited

This pullrequest changes

Newly introduced enums

Flag enums

Mode enums

Tasks list

Current issues

alalek commented Aug 23, 2018

alalek commented Aug 23, 2018 • edited

Enum categories

Handling enums in Python.

Usage compatibility between C++ and Python

cv3d commented Aug 23, 2018

hrnr commented Aug 23, 2018

Usage compatibility between C++ and Python

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cv3d commented Aug 23, 2018 • edited

alalek commented Aug 23, 2018 • edited

hrnr commented Aug 23, 2018

hrnr commented Aug 23, 2018 • edited

cv3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cv3d Aug 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cv3d Aug 24, 2018 • edited

Choose a reason for hiding this comment

cv3d commented Aug 23, 2018 •

edited

alalek commented Aug 23, 2018 •

edited

cv3d commented Aug 23, 2018 •

edited

alalek commented Aug 23, 2018 •

edited

hrnr commented Aug 23, 2018 •

edited

cv3d Aug 24, 2018 •

edited

cv3d Aug 24, 2018 •

edited