Skip to content

[G-API] Introduce GOpaque and GArray for python#19319

Merged
alalek merged 5 commits intoopencv:masterfrom
TolyaTalamanov:at/introduce-gopaque-garray-for-python
Feb 9, 2021
Merged

[G-API] Introduce GOpaque and GArray for python#19319
alalek merged 5 commits intoopencv:masterfrom
TolyaTalamanov:at/introduce-gopaque-garray-for-python

Conversation

@TolyaTalamanov
Copy link
Copy Markdown
Contributor

@TolyaTalamanov TolyaTalamanov commented Jan 13, 2021

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

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)

  • Added gapi/include/python_bridge.hpp and gapi/src/gapi/python_bridge.cpp. They must be in public api to allow python module work with entities from there.
  • Implemented GOpaqueT/GArrayT wrapper, thus all types collected in a single class. This makes it easier to handle them.
  • ArgType is implemented as enum not enum class to use cv.gapi.CV_BOOL from python instead of cv.gapi.ArgType_CV_BOOL.
  • ArgType contains CV_GMAT value to work with GArray<cv::GMat>

Build configuration

Xforce_builders_only=linux,docs
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.1.0:20.04
build_image:Custom Win=openvino-2021.1.0
build_image:Custom Mac=openvino-2021.1.0

Xtest_modules:Custom=gapi
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

Xbuildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
Xtest_opencl:Custom=OFF
Xtest_bigdata:Custom=1
Xtest_filter:Custom=*

@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

@dmatveev @smirnov-alexey

@TolyaTalamanov TolyaTalamanov force-pushed the at/introduce-gopaque-garray-for-python branch from 9ac459b to 5eccf38 Compare January 13, 2021 13:23
@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

@dmatveev Could you take a look ?

{

#define HANDLE_CASE(T, K) case gapi::ArgType::CV_##T: \
m_arg = cv::GArray<K>(); \
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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 ?

Copy link
Copy Markdown
Contributor

@smirnov-alexey smirnov-alexey Jan 19, 2021

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

Copy link
Copy Markdown
Contributor

@smirnov-alexey smirnov-alexey left a 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

Comment on lines +205 to +216
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);
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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,
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

@TolyaTalamanov TolyaTalamanov Jan 20, 2021

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 this enum you mentioned

Copy link
Copy Markdown
Member

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)

Copy link
Copy Markdown
Contributor Author

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!

@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

@dmatveev Could you check it ?

@TolyaTalamanov TolyaTalamanov force-pushed the at/introduce-gopaque-garray-for-python branch 4 times, most recently from bb1a3e9 to 8c1520e Compare January 27, 2021 11:48
@TolyaTalamanov TolyaTalamanov force-pushed the at/introduce-gopaque-garray-for-python branch from 8c1520e to afc381c Compare January 27, 2021 12:52
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.

LGTM

// Copyright (C) 2021 Intel Corporation

#ifndef OPENCV_GAPI_PYTHON_BRIDGE_HPP
#define OPENCV_GAPI_PYTHON_BRIDGE_HPP
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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,
Copy link
Copy Markdown
Member

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)

@alalek
Copy link
Copy Markdown
Member

alalek commented Feb 5, 2021

Please take a look on Windows compilation errors (they were here before my commit too)

@TolyaTalamanov TolyaTalamanov force-pushed the at/introduce-gopaque-garray-for-python branch from c41ece6 to 158824e Compare February 9, 2021 13:00
@alalek alalek merged commit c527b3c into opencv:master Feb 9, 2021
@alalek alalek mentioned this pull request Apr 9, 2021
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants