Skip to content

G-API: Fix G_TYPED_KERNEL_M macro#16221

Merged
alalek merged 5 commits intoopencv:masterfrom
TolyaTalamanov:at/fix-g_typed_kernel_m
Jan 13, 2020
Merged

G-API: Fix G_TYPED_KERNEL_M macro#16221
alalek merged 5 commits intoopencv:masterfrom
TolyaTalamanov:at/fix-g_typed_kernel_m

Conversation

@TolyaTalamanov
Copy link
Copy Markdown
Contributor

@TolyaTalamanov TolyaTalamanov commented Dec 22, 2019 •
edited by alalek
Loading

Problem

The following code does not compile

G_TYPED_KERNEL_M(GSomeKernel, <std::tuple<GMat, GMat>(GMat)>, "some_kernel") {
    static std::tuple<GMatDesc, GMatDesc> outMeta(GMatdesc in) {
        std::make_tuple(in, in)
    }
};

Workaround:

using GMat2 = std::tuple<GMat, GMat>;
G_TYPED_KERNEL_M(GSomeKernel, <GMat2(GMat)>, "some_kernel") {
    static std::tuple<GMatDesc, GMatDesc> outMeta(GMatdesc in) {
        std::make_tuple(in, in)
    }
};

So, I think this isn't clear to the user

The problem is that preprocessor split std::tuple<GMat, GMat> by 2 parameters: std::tuple<GMat and GMat> We pass 4 parameters to the macro instead of 3 in this case

Solution

I just added overload for G_TYPED_KERNEL_M for 5 and 6 parameters and added assert if the number parameters of tuple greater than 4 (But now it's work only for 5 parameters)

Of course it works on a limited number of parameters and it's not a perfect solution but I think it's better than it was

force_builders=Custom
buildworker:Custom=linux-1
build_image:Custom=plaidml2
test_modules:Custom=gapi
test_filter:Custom=*ML*

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

@asmorkalov asmorkalov requested a review from dmatveev December 23, 2019 08:05
@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

TolyaTalamanov commented Dec 23, 2019

@rgarnov

@TolyaTalamanov TolyaTalamanov force-pushed the at/fix-g_typed_kernel_m branch from 79c4e11 to fa81030 Compare December 24, 2019 10:38
@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

TolyaTalamanov commented Dec 25, 2019

@smirnov-alexey

@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

TolyaTalamanov commented Dec 25, 2019

@dmatveev

@TolyaTalamanov
Fixes
e142a34 
* Fix windows build
* Fix doxygen
@TolyaTalamanov TolyaTalamanov force-pushed the at/fix-g_typed_kernel_m branch from fa81030 to e142a34 Compare December 30, 2019 10:02
dmatveev
dmatveev reviewed Jan 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about tests? If the code is compiled, assume the test is just passed :) (I believe we have such tests already with static_assert inside.

Would it work for cases when a kernel has multiple input parameters (not the output ones)? Worth adding to the test suite

dmatveev
dmatveev reviewed Jan 9, 2020
View reviewed changes
modules/gapi/include/opencv2/gapi/gkernel.hpp Outdated
// {body} is to be defined by user

#define GET_HELPER(_1, _2, _3, _4, _5, _6, NAME, ...) NAME
#define API(...) __VA_ARGS__
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these names are too generic (especially API). I'd rename it to something more specific to avoild hardly-debuggable collisions

Copy link
Copy Markdown
Contributor Author

@TolyaTalamanov TolyaTalamanov Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@dmatveev dmatveev mentioned this pull request Jan 9, 2020
Merged
@TolyaTalamanov TolyaTalamanov force-pushed the at/fix-g_typed_kernel_m branch from f7ed5c4 to 8e18a13 Compare January 10, 2020 07:58
@TolyaTalamanov TolyaTalamanov force-pushed the at/fix-g_typed_kernel_m branch from 8e18a13 to c426b9b Compare January 10, 2020 09:07
@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

TolyaTalamanov commented Jan 10, 2020

@smirnov-alexey

@smirnov-alexey
Copy link
Copy Markdown
Contributor

smirnov-alexey commented Jan 10, 2020

Looks good to me

@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

TolyaTalamanov commented Jan 13, 2020

@dmatveev

@dmatveev
Copy link
Copy Markdown
Contributor

dmatveev commented Jan 13, 2020

👍 thanks!

@alalek alalek merged commit 55f2370 into opencv:master Jan 13, 2020
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
@TolyaTalamanov
Merge pull request opencv#16221 from TolyaTalamanov:at/fix-g_typed_ke…
4ddde2b 
…rnel_m

G-API: Fix G_TYPED_KERNEL_M macro

* Fix G_TYPED_KERNEL_M macro

* Fixes

* Fix windows build
* Fix doxygen

* Added several macros

* Add overloads for G_TYPED_KERNEL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmatveev dmatveev dmatveev approved these changes

Assignees

@dmatveev dmatveev

Labels

category: g-api / gapi

Projects

None yet

Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

5 participants

@TolyaTalamanov @smirnov-alexey @dmatveev @asmorkalov @alalek