| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
[G-API]: Introduce RMat serialization API #18584
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
Conversation
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.
Looks good generally!
|
|
||
| template<typename... Ts> | ||
| IOStream& operator<< (IOStream& os, const cv::util::variant<Ts...> &v) { | ||
| os << (uint32_t)v.index(); |
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.
Hmm I believe it should be just os<< v.index(), isn't it?
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 returns size_t. We don't support operator<< for that type so the cast here is 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.
Got it, but I'd use a static_cast instead
| @@ -45,6 +46,49 @@ template<> struct CompileArgTag<MyCustomType> { | |||
| } // namespace detail | |||
| } // namespace cv | |||
|
|
|||
| namespace { | |||
| class MyRMatAdapter : public cv::RMat::Adapter { | |||
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.
No alignment between class (in OpenCV, namespaces don't increase the indent level)
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.
Fixed
| if (i == gi) { | ||
| X x{}; | ||
| is >> x; | ||
| v = std::move(x); |
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.
On windows build this code for some reason marked as unreachable which is not true I believe
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.
https://pullrequest.opencv.org/buildbot/builders/precommit_windows64/builds/26706 @alalek can you take a look, please? Can we suppress this or fix somehow? This code is surely reachable (not in .so but in tests)
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.
As it is a public header, so disabling of this warning in CMake scripts doesn't help with User applications (these CMake scripts are internal and not available for Users).
Try pragma push/disable/pop approach.
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.
Pushed an alternative solution
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.
Great, thanks!
|
Build warnings must be fixed before merge. |
| @@ -209,6 +286,23 @@ static GCompileArg exec(cv::gapi::s11n::IIStream& is, const std::string& tag) { | |||
| } | |||
| }; | |||
|
|
|||
| template<typename T> struct deserialize_runarg; | |||
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 to name structure above : deserialize_carg and this one deserialize_rarg?
It is only a suggestion, no actions required if you don't think that it is reasonable to do now
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.
carg and rarg are harder to understand
@alalek we have a little idea how to fix this particular warning, do you have any suggestions? |
modules/gapi/CMakeLists.txt
Outdated
| @@ -31,7 +31,8 @@ ocv_add_module(gapi | |||
| if(MSVC) | |||
| # Disable obsollete warning C4503 popping up on MSVC <<2017 | |||
| # https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4503?view=vs-2019 | |||
| ocv_warnings_disable(CMAKE_CXX_FLAGS /wd4503) | |||
| # FXIME: enable 4702 warning (unreachable code) when the issue is fixed | |||
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.
typo
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.
Fixed, thanks
|
@alalek required builds have passed. Can you please merge this? Our internal integration this WW depends on this feature. Thanks! |
|
@alalek @mshabunin is there anything else to do for merge? |
|
... |
|
@dmatveev See https://github.com/opencv/opencv/pull/18584/files#r508380384 |
Just noticed this, thanks. Alexey is on vacation currently. Let's see what's with push/pop |
|
@alalek thanks! |
[G-API]: Introduce RMat serialization API * Introduce RMat serialization API * Fix RunArgs deserialization * Address review comments * Export operators for GRunArg serialization * Fix warning and add handling for RMat in bind() * Update CMakeLists.txt * G-API: RMat S11N -- probably fix the Windows warning
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.