[G-API]: Introduce RMat serialization API by smirnov-alexey · Pull Request #18584 · opencv/opencv

smirnov-alexey · 2020-10-13T16:57:55Z

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

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

Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2020.3.0:16.04
build_image:Custom Win=openvino-2020.3.0
build_image:Custom Mac=openvino-2020.3.0

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

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

modules/gapi/include/opencv2/gapi/s11n.hpp

modules/gapi/test/s11n/gapi_s11n_tests.cpp

modules/gapi/include/opencv2/gapi/s11n.hpp

dmatveev

Looks good generally!

dmatveev · 2020-10-13T17:44:20Z

modules/gapi/include/opencv2/gapi/s11n.hpp

+
+template<typename... Ts>
+IOStream& operator<< (IOStream& os, const cv::util::variant<Ts...> &v) {
+    os << (uint32_t)v.index();


Hmm I believe it should be just os<< v.index(), isn't it?

It returns size_t. We don't support operator<< for that type so the cast here is required

Got it, but I'd use a static_cast instead

modules/gapi/include/opencv2/gapi/s11n.hpp

dmatveev · 2020-10-13T17:48:40Z

modules/gapi/test/s11n/gapi_s11n_tests.cpp

 } // namespace cv

+namespace {
+    class MyRMatAdapter : public cv::RMat::Adapter {


No alignment between class (in OpenCV, namespaces don't increase the indent level)

modules/gapi/test/s11n/gapi_s11n_tests.cpp

smirnov-alexey · 2020-10-14T14:58:07Z

modules/gapi/include/opencv2/gapi/s11n.hpp

+    if (i == gi) {
+        X x{};
+        is >> x;
+        v = std::move(x);


On windows build this code for some reason marked as unreachable which is not true I believe

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)

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.

Pushed an alternative solution

dmatveev

Great, thanks!

alalek · 2020-10-15T14:00:00Z

Build warnings must be fixed before merge.

AsyaPronina · 2020-10-15T14:16:01Z

modules/gapi/include/opencv2/gapi/s11n.hpp

 }
 };

+template<typename T> struct deserialize_runarg;


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

carg and rarg are harder to understand

dmatveev · 2020-10-16T08:11:05Z

Build warnings must be fixed before merge.

@alalek we have a little idea how to fix this particular warning, do you have any suggestions?

alalek · 2020-10-16T19:39:30Z

modules/gapi/CMakeLists.txt

  # 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


Fixed, thanks

dmatveev · 2020-10-19T13:18:16Z

@alalek required builds have passed. Can you please merge this? Our internal integration this WW depends on this feature.

Thanks!
D

dmatveev · 2020-10-20T09:41:53Z

@alalek @mshabunin is there anything else to do for merge?

dmatveev · 2020-10-20T17:43:31Z

...

alalek · 2020-10-20T19:21:12Z

@dmatveev See https://github.com/opencv/opencv/pull/18584/files#r508380384
If G-API team doesn't care please react properly and let us know.

dmatveev · 2020-10-20T19:35:27Z

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

dmatveev · 2020-10-21T08:42:30Z

@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

Introduce RMat serialization API

d08a9b1

smirnov-alexey requested a review from dmatveev October 13, 2020 16:58

smirnov-alexey commented Oct 13, 2020

View reviewed changes

modules/gapi/include/opencv2/gapi/s11n.hpp Outdated Show resolved Hide resolved

smirnov-alexey commented Oct 13, 2020

View reviewed changes

modules/gapi/test/s11n/gapi_s11n_tests.cpp Show resolved Hide resolved

dmatveev added the category: g-api / gapi label Oct 13, 2020

smirnov-alexey commented Oct 13, 2020

View reviewed changes

modules/gapi/include/opencv2/gapi/s11n.hpp Show resolved Hide resolved

smirnov-alexey commented Oct 13, 2020

View reviewed changes

modules/gapi/include/opencv2/gapi/s11n.hpp Outdated Show resolved Hide resolved

Fix RunArgs deserialization

3ede725

dmatveev reviewed Oct 13, 2020

View reviewed changes

Address review comments

e4fb330

smirnov-alexey commented Oct 14, 2020

View reviewed changes

Export operators for GRunArg serialization

9b60c27

dmatveev self-assigned this Oct 15, 2020

dmatveev approved these changes Oct 15, 2020

View reviewed changes

AsyaPronina reviewed Oct 15, 2020

View reviewed changes

Fix warning and add handling for RMat in bind()

25d03f9

alalek reviewed Oct 16, 2020

View reviewed changes

Update CMakeLists.txt

c51466d

dmatveev approved these changes Oct 19, 2020

View reviewed changes

G-API: RMat S11N -- probably fix the Windows warning

8637b7a

alalek merged commit 2669d8c into opencv:master Oct 20, 2020

alalek mentioned this pull request Nov 27, 2020

Merge 4.x #18951

Merged

Uh oh!

Conversation

smirnov-alexey commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

alalek commented Oct 15, 2020

Uh oh!

AsyaPronina Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatveev commented Oct 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatveev commented Oct 19, 2020

Uh oh!

dmatveev commented Oct 20, 2020

Uh oh!

dmatveev commented Oct 20, 2020

Uh oh!

alalek commented Oct 20, 2020

Uh oh!

dmatveev commented Oct 20, 2020

Uh oh!

dmatveev commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smirnov-alexey commented Oct 13, 2020 •

edited

Loading

AsyaPronina Oct 15, 2020 •

edited

Loading