CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
[G-API] Introduce GOpaque and GArray for python #19319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[G-API] Introduce GOpaque and GArray for python #19319
Conversation
9ac459b
to
5eccf38
Compare
@dmatveev Could you take a look ? |
{ | ||
|
||
#define HANDLE_CASE(T, K) case gapi::ArgType::CV_##T: \ | ||
m_arg = cv::GArray<K>(); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can some of this code become template to support both array and opaque? All those duplicates of HANDLE_CASE
with the same types looks way too massive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. move some switches to template functions which return desired type and then use those functions in constructors/etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should return this template function ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can try to utilize something like getFromStream
(from serialization.cpp
), non-blocking though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for code duplication looks good
HANDLE_CASE(bool); | ||
HANDLE_CASE(int); | ||
HANDLE_CASE(double); | ||
HANDLE_CASE(float); | ||
HANDLE_CASE(std::string); | ||
HANDLE_CASE(cv::Point); | ||
HANDLE_CASE(cv::Point2f); | ||
HANDLE_CASE(cv::Size); | ||
HANDLE_CASE(cv::Rect); | ||
HANDLE_CASE(cv::Scalar); | ||
HANDLE_CASE(cv::Mat); | ||
HANDLE_CASE(cv::GMat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please declare this type list once, and then reuse in your functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving you some idea.
#include <iostream>
#include <tuple>
#include <vector>
#include <string>
#include <type_traits>
#define ID(X) X
#define ID_(X) ID(X),
#define NOTHING(X)
#define SPACE
#define COMMA ,
#define TYPE_LIST_G(G, X, A1, L1, A2, L2) \
G(A1(int) X A2("int")) \
G(A1(double) X A2("double")) \
G(L1(char) X L2("char"))
#define TYPE_LIST(A1, L1, A2, L2) \
TYPE_LIST_G(ID, SPACE, A1, L1, A2, L2)
#define TYPE_LIST2_G(G, A1, A2) \
TYPE_LIST_G(G, COMMA, A1, A1, A2, A2)
int main() {
// Example 1: generate a tuple from our type list
// A1 is identity plus ','
// L1 is identity (no comma)
// A2 is nothing
// L2 is nothing
using T = std::tuple< TYPE_LIST(ID_, ID, NOTHING, NOTHING) >;
static_assert(std::is_same<int, std::tuple_element<0, T>::type>::value, "First type is int");
static_assert(std::is_same<double, std::tuple_element<1, T>::type>::value, "Second type is double");
// Example 2: generate a string list from our type list
// A1 is nothing
// L1 is nothing
// A2 is identity plus ','
// L2 is identity (no comma)
std::vector<std::string> names = {
TYPE_LIST(NOTHING, NOTHING, ID_, ID)
};
for (const auto &s : names) std::cout << s << std::endl;
// Example 3: generate code around elements
// Functors for all and terminating elements are the same
#define HANDLE_CASE(T, S) std::cout << "sizeof(" << S << ") is " << sizeof(T) << std::endl;
TYPE_LIST2_G(HANDLE_CASE, ID, ID)
return 0;
}
int
double
char
sizeof(int) is 4
sizeof(double) is 8
sizeof(char) is 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks! Fixed
CV_RECT, | ||
CV_SCALAR, | ||
CV_MAT, | ||
CV_GMAT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have enum for Opaque types. Is this one different from that? Can you reuse that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It is placed in
cv::detail
-cv.detail_CV_MAT
from python looks ugly CV_GMAT
isn't in thisenum
you mentioned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, It make sense to update bindings generators to support CV_WRAP_AS
for enums (perhaps with full name, to escape `detail namespace)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will file a ticket, thanks!
@dmatveev Could you check it ? |
bb1a3e9
to
8c1520e
Compare
8c1520e
to
afc381c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// Copyright (C) 2021 Intel Corporation | ||
|
||
#ifndef OPENCV_GAPI_PYTHON_BRIDGE_HPP | ||
#define OPENCV_GAPI_PYTHON_BRIDGE_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modules/gapi/include
This location should be used for C++ public API.
Python-specific files should go into modules/gapi/misc/python
.cpp
No need to store this content in gapi C++ binary file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but didn't know how to do that. Thank for you commit.
CV_RECT, | ||
CV_SCALAR, | ||
CV_MAT, | ||
CV_GMAT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, It make sense to update bindings generators to support CV_WRAP_AS
for enums (perhaps with full name, to escape `detail namespace)
Please take a look on Windows compilation errors (they were here before my commit too) |
c41ece6
to
158824e
Compare
…que-garray-for-python [G-API] Introduce GOpaque and GArray for python * Introduce GOpaque and GArray for python * Fix ctor * Avoid code duplication by using macros * gapi: move Python-specific files to misc/python * Fix windows build Co-authored-by: Alexander Alekhin <alexander.a.alekhin@gmail.com>
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
Overview
This PR is a part of #18900. To make review process easier decided to split this on several PR's.
Templates classes that GOpaque/GArray are can't be wrapped into python by using macros. (Parser ignores templates objects)
gapi/include/python_bridge.hpp
andgapi/src/gapi/python_bridge.cpp
. They must be in public api to allowpython
module work with entities from there.GOpaqueT
/GArrayT
wrapper, thus all types collected in a single class. This makes it easier to handle them.ArgType
is implemented asenum
notenum class
to usecv.gapi.CV_BOOL
from python instead ofcv.gapi.ArgType_CV_BOOL
.ArgType
containsCV_GMAT
value to work withGArray<cv::GMat>
Build configuration