Not serialize compile arguments which have no S11N<T> template specialization by AsyaPronina · Pull Request #18651 · opencv/opencv

AsyaPronina · 2020-10-23T20:12:09Z

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

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

Xbuild_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

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=*

AsyaPronina · 2020-10-31T18:00:46Z

@smirnov-alexey , @dmatveev.
Hi Alexey, Dmitry!
Cases where user passes more than 1 variable of certain type as compile arguments to cv::gapi::serialize(GCompileArgs) are not handled.

dmatveev · 2020-11-02T08:24:57Z

modules/gapi/src/backends/common/serialization.cpp

-    os << arg.tag;
-    arg.serialize(os);
+    ByteMemoryOutStream tmpS;
+    tmpS << arg.tag;


Tag could be written to a primary stream.

Thanks! Reworked this part!

dmatveev · 2020-11-02T08:26:43Z

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

-            cv::gapi::s11n::detail::S11N<T>::deserialize(is)
-        };
+        static_assert(cv::gapi::s11n::detail::has_S11N_spec<T>::value,
+            "cv::gapi::deserialize<GCompileArgs, Types...> waits Types to have S11N specializations"


waits -> expects

dmatveev · 2020-11-02T08:30:06Z

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

    cv::gapi::s11n::IIStream& is = *pIs;
+
+    std::string tag;
+    is >> tag;


Here you read tag directly from the stream, but in the serialization code, you write tag to a temporary stream and then serialize its vector.

In theory, this code should work incorrectly. Please double-check.

https://github.com/opencv/opencv/pull/18651/files#diff-7b0234dbbb6f5f5bae56c90408ad90fe57c7d3070a1ed19af9ee4c909289685fR342

Here I have a GCompileArg as serialized vector of char, which were deserialized from the main stream and this vector of char should contain that that were deserialized in temporary stream.

https://github.com/opencv/opencv/pull/18651/files/adb2cb4891cafabe2b70b679a0a6a11ed1459505#diff-464a0d9684f92e3f84f3ce12af7c7a8917fcc121480caa99d46884534002cb65R329

Please simplify, the code shouldn't cause the confusion like mine

Simplified, thanks!

dmatveev · 2020-11-02T08:31:11Z

modules/gapi/test/s11n/gapi_s11n_tests.cpp

+    GCompileArgs dArgsEmptyVar4 = cv::gapi::deserialize<GCompileArgs, EmptyCustomType>(sArgs4);
+    EXPECT_EQ(1u, dArgsEmptyVar4.size());
+    EXPECT_TRUE(cv::gapi::getCompileArg<EmptyCustomType>(dArgsEmptyVar4).has_value());
+}


There must be a test failing due to inconsistency in S11N for custom compile arg tags.

dmatveev · 2020-11-02T13:38:40Z

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

    for (uint32_t i = 0; i < sz; ++i) {
        std::string tag;
        is >> tag;
-        args.push_back(cv::gapi::detail::deserialize_arg<std::tuple<Types...>>::exec(is, tag));
+
+        std::vector<char> sArg;
+        is >> sArg;
+
+        cv::util::optional<GCompileArg> dArg =


This whole thing can be further simplified, but let's leave it for now.
Just put a task in our tracker to implement GCompileArgs serialization in a more generic two-stage way (based on vectors).

smirnov-alexey

Overall looks good

…CompileArgs - cv::gapi::serialize bypasses compile arguments which have no S11N specialization with serialize/deserialize callbacks for underlying types - cv::gapi::deserialize can accept arbitraty number of serialized compile args in a stream but will return only those which are requested by user via template parameter pack if they are presented in the stream. If some or all of them are not presented cv::gapi::deserialize will ignore and return only those which are presented - cv::gapi::deserialize can accept only types which can be deserialized (have S11N<T> specialization with the user callbacks) - Added cv::gapi::s11n::detail::has_S11N_spec<T> trait to separate compile arguments which have S11N<T> specialization with the user callbacks

AsyaPronina force-pushed the asyadev/add_possibility_to_check_that_gcompilearg_has_serialize branch 6 times, most recently from cc1eca1 to adb2cb4 Compare October 31, 2020 17:51

AsyaPronina requested review from dmatveev and smirnov-alexey October 31, 2020 18:00

dmatveev reviewed Nov 2, 2020

View reviewed changes

AsyaPronina force-pushed the asyadev/add_possibility_to_check_that_gcompilearg_has_serialize branch from 6145aea to 60ff600 Compare November 2, 2020 13:32

dmatveev approved these changes Nov 2, 2020

View reviewed changes

smirnov-alexey approved these changes Nov 2, 2020

View reviewed changes

AsyaPronina force-pushed the asyadev/add_possibility_to_check_that_gcompilearg_has_serialize branch from 60ff600 to 48ccbe3 Compare November 2, 2020 15:57

alalek assigned dmatveev Nov 2, 2020

opencv-pushbot merged commit 6df92b3 into opencv:master Nov 2, 2020

alalek mentioned this pull request Nov 27, 2020

Merge 4.x #18951

Merged

Uh oh!

Conversation

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

Uh oh!

Pull Request Readiness Checklist

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

AsyaPronina Nov 2, 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!

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!

smirnov-alexey left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AsyaPronina commented Oct 23, 2020 •

edited by alalek

Loading

AsyaPronina commented Oct 31, 2020 •

edited

Loading

AsyaPronina Nov 2, 2020 •

edited

Loading