G-API: Implement variant visit() by sivanov-work · Pull Request #20039 · opencv/opencv

sivanov-work · 2021-05-06T11:03:07Z

Implement visitor for variant and few helpers:

type_list_element for getting type based on index (similarly with std::get<>)
variant_size for getting holden types count (similarly with std::variant_size<>)
overload_set for Lambdas to enable lambda operator() overloading
visit for applying function on variant type (similarly with std::visit) use two approaches:
a) Specific visitor type must inherits either adapter static_visitor mix-in or static_indexed_visitor and implement visit method which receives: variant param and extra data passed to visit or additional index of variant param passed too for descendant of static_indexed_visitor.
b) uses overload_set of lambdas as in-place visitor

Added UTs:

for variant get functionality
for visit with using static_visitor interface with additional params and no params
for visit with using static_indexed_visitor interface with additional params and no params
for visit with overloaded Lambdas set

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

force_builders=Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

Xbuild_image:Custom=ubuntu-openvino-2021.3.0:20.04
build_image:Custom Win=openvino-2021.2.0
build_image:Custom Mac=openvino-2021.2.0

test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

dmatveev

Good start!

modules/gapi/include/opencv2/gapi/util/variant.hpp

dmatveev · 2021-05-20T19:55:37Z

modules/gapi/include/opencv2/gapi/util/variant.hpp

+    };
+
+    template<typename Visitor, typename Variant, typename... VisitorArg>
+    typename Visitor::result_type visit(Visitor &visitor, //FIXME: Visitor && -> forbiddeby by Microsoft Visual Studio 2019


forbidden by?

it doesn't compile in that way: Visitor && - cannot deduct following arguments. I spent much time to workaround it and just put in as is. Maybe it MVS specific bug, because GCC version compiled fine

But, i' ve popped about some fix since i read your comment: it is possible to eliminate VisitorArgs such Lambda can capture it and Class can capture it as member and it doesn't require to pass it as additional args. Moreover std::visit doesn't receive any additional args

What do you think about it? After elimination VisitorArgs it will became possible to pass Visitor by universal reference and unblock passing instant lambda here

dmatveev · 2021-05-20T20:00:13Z

modules/gapi/include/opencv2/gapi/util/variant.hpp

+        suppress_unused_warning(v);
+        suppress_unused_warning(processed);
+        return {};
+    }


What's the purpose of this one?

Oh I seem to get it - it just does nothing if a particular variant's type has been visited already?
If it is, then why should we have such an overload? We may possibly terminate the traversal on the match (haven't looked into the below code yet).

May be this implementation is not pretty accurate - we can terminate recursion in another way... But in this PR I mostly focused on usability & applicability of visit instead of it's inner implementation

I hope we can use std variant soon or later :) and i doesn't concentrate on pretty templates implementation here...
But maybe i wrong and turning on c++14 & c++17 is very very far enough

modules/gapi/include/opencv2/gapi/util/variant.hpp

modules/gapi/src/compiler/gcompiler.cpp

dmatveev · 2021-05-20T20:31:11Z

modules/gapi/src/compiler/gcompiler.cpp

+            if (!ret)
+            {
+                out << "Expected: " << cv::detail::meta_to_string<Descr>() << ", got: ";
+                meta_print_visitor v{out};


hmm I see, its sort of prints type not the value -- actually, visitor is not required here at all? It is just out << cv::detail::meta_to_string<MetaT>() (except the case for monostate but that's workarountable)

cv::detail::meta_to_string()

Do you mean the second visitor? it's required cause out is variant too and we need expand it to print-out actual type we have (which actually in controversy with expected type), just for troubleshooting info in exception (user just read and change it)

dmatveev · 2021-05-20T20:32:15Z

modules/gapi/src/compiler/gcompiler.cpp

-    const auto meta_matches = [](const GMetaArg &meta, const GProtoArg &proto) {
-        switch (proto.index())
-        {
-        // FIXME: Auto-generate methods like this from traits:
-        case GProtoArg::index_of<cv::GMat>():
-        case GProtoArg::index_of<cv::GMatP>():
-            return util::holds_alternative<cv::GMatDesc>(meta);
-
-        case GProtoArg::index_of<cv::GFrame>():
-            return util::holds_alternative<cv::GFrameDesc>(meta);
-
-        case GProtoArg::index_of<cv::GScalar>():
-            return util::holds_alternative<cv::GScalarDesc>(meta);
-
-        case GProtoArg::index_of<cv::detail::GArrayU>():
-            return util::holds_alternative<cv::GArrayDesc>(meta);
-
-        case GProtoArg::index_of<cv::detail::GOpaqueU>():
-            return util::holds_alternative<cv::GOpaqueDesc>(meta);
-
-        default:
-            GAPI_Assert(false);
-        }
-        return false; // should never happen
-    };


Amount of code removed here and added atop of that gives some hint... :)

That's the controversial point of having a visitor as some class/etc. -> while previously we could use plain switch (well, not very automated but integrated perfectly into its environment), with a distinct class the handling logic is torn away from the context - we don't have scope closures for such objects like we could have with lambdas, for example.

Would it be possible to construct an ad-hoc visitor with typed lambdas right in place? It would be a game changer then.

I believe the limitation I've got into was that the visit return type was determined by the visitor lambda called - and it could vary; but I believe we could relax such requirement here and now.

That's the controversial point of having a visitor as some class/etc. -> while previously we could use plain switch

in current piece of code we need two stacked switchs similar with

switch(expected) { case A: ... switch (got) { case AA: PRINT "A and AA mismatch" case BB: PRINT "A and BB mismatch" } case B: switch (got) case AA: PRINT "B and AA mismatch" }

In current implementation it solved by using two simple classes

I believe the limitation I've got into was that the visit return type was determined

exactly! it is the problem i faced ( without writting a lot of code for cast Lambda to std::fuction<> and extract return type
a) static_visitor as base is 'boost'-like way
b) lamba is std

But in case if we fix visitor for lambda usage, we cannot use lambda here, because lambda with auto params is beginning with C++14, so code like that

auto l = []( const auto &v) {} //<---auto ?

It' won't work in c++11

So looks like class-style is only way for C++11

P.S.
it is possible to use overloading in lambda like that

auto overloaded_lamda = make_overloaded_lambda([] (const GMatDescr& m) {},[] (const GOpaqDesc& o) {}); visit(overloaded_lambda, variant);

but it looks more ugly

modules/gapi/include/opencv2/gapi/util/variant.hpp

sivanov-work · 2021-05-21T06:56:14Z

modules/gapi/include/opencv2/gapi/util/variant.hpp

+    };
+
+    template<typename Visitor, typename Variant, typename... VisitorArg>
+    typename Visitor::result_type visit(Visitor &visitor, //FIXME: Visitor && -> forbiddeby by Microsoft Visual Studio 2019


it doesn't compile in that way: Visitor && - cannot deduct following arguments. I spent much time to workaround it and just put in as is. Maybe it MVS specific bug, because GCC version compiled fine

But, i' ve popped about some fix since i read your comment: it is possible to eliminate VisitorArgs such Lambda can capture it and Class can capture it as member and it doesn't require to pass it as additional args. Moreover std::visit doesn't receive any additional args

What do you think about it? After elimination VisitorArgs it will became possible to pass Visitor by universal reference and unblock passing instant lambda here

modules/gapi/include/opencv2/gapi/util/variant.hpp

sivanov-work · 2021-05-21T07:01:49Z

modules/gapi/include/opencv2/gapi/util/variant.hpp

+        suppress_unused_warning(v);
+        suppress_unused_warning(processed);
+        return {};
+    }


May be this implementation is not pretty accurate - we can terminate recursion in another way... But in this PR I mostly focused on usability & applicability of visit instead of it's inner implementation

I hope we can use std variant soon or later :) and i doesn't concentrate on pretty templates implementation here...
But maybe i wrong and turning on c++14 & c++17 is very very far enough

modules/gapi/src/compiler/gcompiler.cpp

sivanov-work · 2021-05-21T07:31:13Z

modules/gapi/src/compiler/gcompiler.cpp

+            if (!ret)
+            {
+                out << "Expected: " << cv::detail::meta_to_string<Descr>() << ", got: ";
+                meta_print_visitor v{out};


cv::detail::meta_to_string()

Do you mean the second visitor? it's required cause out is variant too and we need expand it to print-out actual type we have (which actually in controversy with expected type), just for troubleshooting info in exception (user just read and change it)

sivanov-work · 2021-05-21T07:40:57Z

modules/gapi/src/compiler/gcompiler.cpp

-    const auto meta_matches = [](const GMetaArg &meta, const GProtoArg &proto) {
-        switch (proto.index())
-        {
-        // FIXME: Auto-generate methods like this from traits:
-        case GProtoArg::index_of<cv::GMat>():
-        case GProtoArg::index_of<cv::GMatP>():
-            return util::holds_alternative<cv::GMatDesc>(meta);
-
-        case GProtoArg::index_of<cv::GFrame>():
-            return util::holds_alternative<cv::GFrameDesc>(meta);
-
-        case GProtoArg::index_of<cv::GScalar>():
-            return util::holds_alternative<cv::GScalarDesc>(meta);
-
-        case GProtoArg::index_of<cv::detail::GArrayU>():
-            return util::holds_alternative<cv::GArrayDesc>(meta);
-
-        case GProtoArg::index_of<cv::detail::GOpaqueU>():
-            return util::holds_alternative<cv::GOpaqueDesc>(meta);
-
-        default:
-            GAPI_Assert(false);
-        }
-        return false; // should never happen
-    };


That's the controversial point of having a visitor as some class/etc. -> while previously we could use plain switch

in current piece of code we need two stacked switchs similar with

switch(expected) { case A: ... switch (got) { case AA: PRINT "A and AA mismatch" case BB: PRINT "A and BB mismatch" } case B: switch (got) case AA: PRINT "B and AA mismatch" }

In current implementation it solved by using two simple classes

I believe the limitation I've got into was that the visit return type was determined

exactly! it is the problem i faced ( without writting a lot of code for cast Lambda to std::fuction<> and extract return type
a) static_visitor as base is 'boost'-like way
b) lamba is std

But in case if we fix visitor for lambda usage, we cannot use lambda here, because lambda with auto params is beginning with C++14, so code like that

auto l = []( const auto &v) {} //<---auto ?

It' won't work in c++11

So looks like class-style is only way for C++11

sivanov-work · 2021-05-21T07:43:22Z

modules/gapi/src/compiler/gcompiler.cpp

-    const auto meta_matches = [](const GMetaArg &meta, const GProtoArg &proto) {
-        switch (proto.index())
-        {
-        // FIXME: Auto-generate methods like this from traits:
-        case GProtoArg::index_of<cv::GMat>():
-        case GProtoArg::index_of<cv::GMatP>():
-            return util::holds_alternative<cv::GMatDesc>(meta);
-
-        case GProtoArg::index_of<cv::GFrame>():
-            return util::holds_alternative<cv::GFrameDesc>(meta);
-
-        case GProtoArg::index_of<cv::GScalar>():
-            return util::holds_alternative<cv::GScalarDesc>(meta);
-
-        case GProtoArg::index_of<cv::detail::GArrayU>():
-            return util::holds_alternative<cv::GArrayDesc>(meta);
-
-        case GProtoArg::index_of<cv::detail::GOpaqueU>():
-            return util::holds_alternative<cv::GOpaqueDesc>(meta);
-
-        default:
-            GAPI_Assert(false);
-        }
-        return false; // should never happen
-    };


P.S.
it is possible to use overloading in lambda like that

auto overloaded_lamda = make_overloaded_lambda([] (const GMatDescr& m) {},[] (const GOpaqDesc& o) {}); visit(overloaded_lambda, variant);

but it looks more ugly

sivanov-work · 2021-05-21T07:44:51Z

modules/gapi/test/util/variant_tests.cpp

+        return true;
+    }
+    std::ostream &out;
+};


is outside of test scope because c++ told me local classes with templates are forbidden
for special (non-templated) methods it should compile fine

sivanov-work · 2021-05-24T15:17:19Z

@alalek
after review of variant::visit implementation we have question: Is it expected for OpenCV 5.0 will increase C++ standard version from 11 up to 14? I asked because variant::visit implementation are faced with some limitations( lambda return value detection, and, more valuable, auto types arguments deductions in lambda).

TolyaTalamanov

LGTM 👍

modules/gapi/include/opencv2/gapi/util/variant.hpp

dmatveev · 2021-06-21T12:19:23Z

modules/gapi/test/util/variant_tests.cpp

+    }
+};
+
+struct MyParamDynamicVisitor : cv::util::static_indexed_visitor<bool, MyParamDynamicVisitor>


Is this bool mandatory? can't it be inferred via CRTP?

since the static_indexed_visitor is already using CRTP

From my perspective it breaks erroneous-prone criteria:

It is possible writing by mistake

class MyClass : public static_visitor<MyClass> { bool operator()(int) {/*1000 lines of code*/} void operator()(float) {/*1000 lines of code*/} int operator() (char) }

but with explicit type it less possible

AND the more strongest reason

to deduct return_type in CRTP you must use the same construction as in visit() decltype(visit_impl(get<0>(var))) where visit_impl is upper-level hierarchy wrapper class. But

decltype(visit(get<0>(var))) <---- where can i get var variable here?

Look

somewhere in CRTP... template<class T> auto operator() (size_t index, const T& val) -> decltype(Impl::visit(declval(T))) <---!

declval(T) - is useless here, because it's declaration and doesn't define nothing because nothing to instantiate

modules/gapi/test/util/variant_tests.cpp

dmatveev · 2021-06-21T12:26:07Z

modules/gapi/test/util/variant_tests.cpp

+                    EXPECT_EQ(value, 42);
+                },
+                [](double) {
+                    EXPECT_TRUE(false);


I believe ADD_FAILURE() or FAIL is the right thing in this case.

Also, should this list cover all Types.. range? What if we want to handle only this or that case? (e.g. T1 and T3 only)? Can a "default" lambda (with no value passed in, or with variant itself passed in) be introduced here?

NO, it's limitation for C++11 lambda. In C++14 generic lambda can helps to reduce amount of code
That's because I insist to keep ClassVariant implementation too

Can a "default" lambda

It is possible to implement Bypass struct here

namespace opencv { namespace utils { template<class T> class Bypass { operator(T val) {} <---nothing to do }; } } temlplate<class T...> overload_lambdas_set<Bypass<T>...> make_bypass() {...}

Then

cv::util::visit(cv::overload_lambdas([] (int) {}, make_bypass<double,... ???,...>(), ...);

But I'm not sure about usefulness of this case. maybe as theoretic exploration pros-cons

My comment was not about the template lambdas but about not specifying some overloads

It won't compile, i believe

Also i believe that it is not extendable, error proneous and dangerous: Because after planned extension of variant types list ( adding new type for example) you will not receive any error and newly added type would not be processed silently

dmatveev · 2021-06-21T12:27:19Z

modules/gapi/test/util/variant_tests.cpp

+};
+
+
+struct MyNoParamStaticVisitor : cv::util::static_visitor<void, MyNoParamStaticVisitor>


Also, what's the difference between Static and Dynamic visitors here?

This class implements visit(Type val), but Dynamic implements visit(std::size_t index, Type val

I agree that class name is confused, i will rename it

What's the difference between those and should user care about that?

the second version visit(std::size_t index, Type val) for indexed_visitor returns not only value from variant but also its position index and provide additional flexibility in processing the value ( for example logging, printing, or more compilex use case in serialization maybe)

modules/gapi/include/opencv2/gapi/util/variant.hpp

dmatveev · 2021-06-21T12:42:22Z

modules/gapi/include/opencv2/gapi/util/variant.hpp

+                            public detail::visitor_return_type_deduction_helper<R> {
+
+        using return_type = typename detail::visitor_return_type_deduction_helper<R>::return_type;
+        using detail::visitor_return_type_deduction_helper<R>::operator();


What's the idea behind that?

for getting return type in common case: Lambda + Class Visitor - and keep one interface invocation point: visit() only - it requires to unify return_type deduction mechanism. For Lambda it is possible to take type of decltype(visitor(get<0>(var))) but for Class Visitor there is no operator() in base case, because it provides operator() (std::size_t index, ...)

So operator() uses only for Class Visitor only for deduction return type in visit()

Got it, but a comment in the code would help here a lot.

sure, i'll add it

dmatveev · 2021-06-21T12:56:48Z

modules/gapi/include/opencv2/gapi/util/variant.hpp

+        std::array<bool, non_variadic_args_num + sizeof...(VisitorArgs)> dummy{
+                                    (suppress_unused_warning(visitor), true),
+                                    (suppress_unused_warning(v), true),
+                                    (suppress_unused_warning(processed), true),
+                                    (suppress_unused_warning(no_return), true),
+                                    (suppress_unused_warning(args),true)...};


I don't quite get what it does - why do we need a construct like this? Does it have any side effects? I believe no?

just stack allocation of several booleans in the end of recursion.

Used cause several compilers notifies it as unused variable

So this whole construct is solely about the unused warning?

I believe we can drop names from the method signature then and that's it :)

good idea, thanks. i'll try it

modules/gapi/include/opencv2/gapi/util/variant.hpp

dmatveev

Well done!

dmatveev · 2021-07-05T15:55:16Z

modules/gapi/include/opencv2/gapi/util/variant.hpp

+                            public detail::visitor_return_type_deduction_helper<R> {
+
+        using return_type = typename detail::visitor_return_type_deduction_helper<R>::return_type;
+        using detail::visitor_return_type_deduction_helper<R>::operator();


Got it, but a comment in the code would help here a lot.

sivanov-work · 2021-07-07T09:03:22Z

@alalek
Seems it ready to merge. Could you merge it please?

G-API: Implement variant visit() * Add variant visitor, use visitor for check compile args * Fix GAPI UT: variant *compiler * Aling apply_visior with std, fix indentations * Fix compilation (included compiler_hints.hpp) * Fix compilation (due gapi standalone) * Fix compilation2 (Docs) * Add Lambdas overload, Refactor visit() * Add ReturnType auto deduction * Fix comilation * Fix compilation * Fix warnings * Try to fix MSVC14 * Fix docs * Try fix Win compile * Fix Docs again * Revert GAPI empty input fix * Apply comment for `tuple_element` * Add std::decay for std::base_of to work arounf armv7 problem * Apply review comments * Apply review comments: added comment & removed unused args * Fix docs compilation

sivanov-work requested review from AsyaPronina, TolyaTalamanov, dmatveev and rgarnov May 6, 2021 11:03

dmatveev reviewed May 20, 2021

View reviewed changes

dmatveev self-assigned this May 20, 2021

dmatveev added this to the 4.5.3 milestone May 20, 2021

sivanov-work commented May 21, 2021

View reviewed changes

sivanov-work requested a review from dmatveev May 27, 2021 14:57

sivanov-work force-pushed the gapi_empty_input branch from 7b375b9 to 101f068 Compare May 28, 2021 09:58

sivanov-work changed the title ~~G-API: Graph with empty input shouldn't be compiled~~ G-API: Implement variant visit() May 31, 2021

TolyaTalamanov approved these changes Jun 18, 2021

View reviewed changes

dmatveev reviewed Jun 21, 2021

View reviewed changes

dmatveev approved these changes Jul 5, 2021

View reviewed changes

sivanov-work added 15 commits July 7, 2021 10:14

Add variant visitor, use visitor for check compile args

84aee49

Fix GAPI UT: variant *compiler

f404ec7

Aling apply_visior with std, fix indentations

0cc92dd

Fix compilation (included compiler_hints.hpp)

61a9e87

Fix compilation (due gapi standalone)

55f67f1

Fix compilation2 (Docs)

cae7d3b

Add Lambdas overload, Refactor visit()

0cffb2b

Add ReturnType auto deduction

7dac163

Fix comilation

a79fdfc

Fix compilation

036501d

Fix warnings

cbefd51

Try to fix MSVC14

f0f1c74

Fix docs

7cfe9d1

Try fix Win compile

08defe4

Fix Docs again

f1cc84a

sivanov-work added 6 commits July 7, 2021 10:21

Revert GAPI empty input fix

377334b

Apply comment for tuple_element

b4c8021

Add std::decay for std::base_of to work arounf armv7 problem

d20158e

Apply review comments

e4848fc

Apply review comments: added comment & removed unused args

549850b

Fix docs compilation

3160171

sivanov-work force-pushed the gapi_empty_input branch from fee25ef to 3160171 Compare July 7, 2021 07:40

alalek merged commit c0f63eb into opencv:master Jul 7, 2021

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged

sivanov-work deleted the gapi_empty_input branch March 5, 2022 05:50

		};


		struct MyNoParamStaticVisitor : cv::util::static_visitor<void, MyNoParamStaticVisitor>

Uh oh!

Conversation

sivanov-work commented May 6, 2021 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivanov-work May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivanov-work May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivanov-work May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivanov-work May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivanov-work May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivanov-work commented May 24, 2021

Uh oh!

TolyaTalamanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivanov-work commented May 6, 2021 •

edited by alalek

Loading

sivanov-work May 21, 2021 •

edited

Loading

sivanov-work May 21, 2021 •

edited

Loading

sivanov-work May 21, 2021 •

edited

Loading

sivanov-work May 21, 2021 •

edited

Loading

sivanov-work May 21, 2021 •

edited

Loading