CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
[G-API Wrap streaming #18493
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 Wrap streaming #18493
Conversation
11fd3c3
to
4d51027
Compare
Not the best reviewer here too. Please ping me when it is ready. |
d22d8e0
to
fdfa6f8
Compare
@dkurt Could you take a look, please ? |
fdfa6f8
to
86f031d
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.
👍 Well done!
@@ -135,7 +135,7 @@ GRunArg value_of(const GOrigin &origin); | |||
// Transform run-time computation arguments into a collection of metadata | |||
// extracted from that arguments | |||
GMetaArg GAPI_EXPORTS descr_of(const GRunArg &arg ); | |||
GMetaArgs GAPI_EXPORTS descr_of(const GRunArgs &args); | |||
GMetaArgs GAPI_EXPORTS_W descr_of(const GRunArgs &args); |
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 is about the function above?
Bindings doesn't support overloads properly. Sometimes new names for overloads wrapping are required.
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.
I think only one overload should be in python in nearest future
@@ -49,11 +49,11 @@ namespace cv { | |||
* | |||
* @sa GCompiled | |||
*/ | |||
class GAPI_EXPORTS GStreamingCompiled | |||
class GAPI_EXPORTS_W_SIMPLE GStreamingCompiled |
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.
GAPI_EXPORTS_W_SIMPLE
is for simple key/value maps only (CV_PROP
/CV_PROP_RW
only).
It passes/handles arguments "by value".
Normal EXPORTS_W
uses smart pointers for that and handles arguments "by reference". It should be used by default for classes.
IMHO, It is misused almost everywhere in gapi module.
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.
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.
I see, but if this object is returned from function which also wrapped, it should be SIMPLE
Net
- https://github.com/opencv/opencv/blob/master/modules/dnn/include/opencv2/dnn/dnn.hpp#L390
function
https://github.com/opencv/opencv/blob/master/modules/dnn/include/opencv2/dnn/dnn.hpp#L759
Because for CV_EXPORTS_W
wrapper generates code for Ptr<Object>
but function return Object
by value, so it means in case Object
is returned from function need to use CV_EXPORTS_W_SIMPLE
, doesn't it ?
It's a reason why in G-API
uses GAPI_EXPORTS_W_SIMPLE
almost for all classes
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.
So you are right, need to avoid it for objects which is not pimpl
, but in case GCompiledStreaming
it works fine. Opened a ticket
@alalek Can it be merged ? |
[G-API Wrap streaming * Wrap streaming * Fix build * Add comments * Remove comment * Fix comments to review * Add test for python pull overload
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.
Buildbot configuration