Serialization && deserialization for compile arguments by AsyaPronina · Pull Request #18496 · opencv/opencv

AsyaPronina · 2020-10-02T15:08:58Z

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

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

force_builders=ARMv7,Custom
Xbuild_image:Custom=centos:7
build_image:Custom=ubuntu-clang:18.04
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

…ort_serialization_api

…nto comp_args_serialization

…rgs_serialization

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

dmatveev

Looks ok in general, but can be simplified.

Please simplify

modules/gapi/include/opencv2/gapi/gcommon.hpp

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

modules/gapi/src/api/s11n.cpp

modules/gapi/test/s11n/gapi_s11n_tests.cpp

…rgs_serialization

dmatveev · 2020-10-07T07:55:04Z

modules/gapi/include/opencv2/gapi/fluid/gfluidkernel.hpp

+} // namespace detail
+} // namespace s11n
+} // namespace gapi
+} // namespace cv


Should be removed

With the test for it?

You can put your S11N with your test.

I can't because another test cpp-s which use GFluidOutputRois will use instantiations of primary S11N template and not my specialization, and compiler will choose instantiations from my and theirs (in other tests) on its own

Should I add my specialization in any test cpp which uses GFluidOutputRois ?

No, why? Only where you want to test it for the S11N.

Created a JIRA ticket as discussed: https://jira.devtools.intel.com/browse/ADE-2179

modules/gapi/include/opencv2/gapi/gcommon.hpp

dmatveev · 2020-10-07T07:56:55Z

modules/gapi/include/opencv2/gapi/gcommon.hpp

+    static void serialize(gapi::s11n::IOStream& os, const util::any& arg)
+    {
+        using decayed_type = typename std::decay<T>::type;
+        S11N<decayed_type>::serialize(os, util::any_cast<decayed_type>(arg));


so then you call arg.get<T>() (the existing one) instead of calling any_cast directly... (2/3)

I've left util::any here because have added 'serialize()' method into GCompileArg which passes its private arg member into this callback

Put a cv::GCompileArg here as an argument.

Fixed, thanks!

modules/gapi/include/opencv2/gapi/gcommon.hpp

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

dmatveev

Commented outside the review

dmatveev

👍 thanks

alalek · 2020-10-07T13:54:34Z

Please take a look on build warnings

~~#include "backends/common/serialization.hpp" in tests is not valid (due to redefined EXPORTS semantic)~~

alalek · 2020-10-07T20:31:26Z

modules/gapi/include/opencv2/gapi/gcommon.hpp

+    }
+
 private:
+    std::function<void(cv::gapi::s11n::IOStream&, const GCompileArg&)> serializeF;


problem is here

and in GAPI_EXPORTS_W_SIMPLE in GCompileArg

Can you please explain what the problem is here? (CC: @TolyaTalamanov )

dmatveev · 2020-10-08T10:00:42Z

Thanks for merge! Great

Serialization && deserialization for compile arguments * Initial stub * Add test on serialization of a custom type * Namespaces rework * Fix isSupported in test struct * Fix clang lookup issue * Initial implementation * Drop the isSupported flag * Initial implementation * Removed internal header inclusion * Switched to public API * Implemented serialization * Adding desirialize: WIP * Fixed merge errors * Implemented * Final polishing * Addressed review comments and added debug throw * Added FluidROI test * Polishing * Polishing * Polishing * Polishing * Polishing * Updated CMakeLists.txt * Fixed comments * Addressed review comments * Removed decay from deserialize_arg * Addressed review comments * Removed extra inclusion * Fixed Win64 warning * Update gcommon.hpp * Update serialization.cpp * Update gcommon.hpp * gapi: drop GAPI_EXPORTS_W_SIMPLE from GCompileArg Co-authored-by: Smirnov Alexey <alexey.smirnov@intel.com> Co-authored-by: AsyaPronina <155jj@mail.ru>

smirnov-alexey and others added 19 commits September 28, 2020 21:28

Initial stub

d42f914

Merge branch 'master' of https://github.com/opencv/opencv into as/exp…

f9c00ae

…ort_serialization_api

Add test on serialization of a custom type

9ce83a4

Namespaces rework

c21d7c8

Fix isSupported in test struct

2e742fc

Fix clang lookup issue

7d45ced

Initial implementation

3a85dfe

Merge branch 'master' of https://github.com/opencv/opencv into as/exp…

2bdc90a

…ort_serialization_api

Drop the isSupported flag

7b19cba

Initial implementation

daf8724

Removed internal header inclusion

3a67827

Merge remote-tracking branch 'asmirnov/as/export_serialization_api' i…

8d484c4

…nto comp_args_serialization

Switched to public API

ad1d24d

Implemented serialization

00fae57

Adding desirialize: WIP

2376801

Merge branch 'master' of https://github.com/opencv/opencv into comp_a…

38f508e

…rgs_serialization

Fixed merge errors

8921e17

Implemented

9c0ef10

Final polishing

2225bf9

AsyaPronina requested review from dbudniko, dmatveev and smirnov-alexey October 2, 2020 15:08

AsyaPronina commented Oct 2, 2020

View reviewed changes

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

dmatveev reviewed Oct 2, 2020

View reviewed changes

AsyaPronina added 6 commits October 6, 2020 18:54

Addressed review comments and added debug throw

346a1b8

Added FluidROI test

b5facd5

Polishing

4b51192

Polishing

5c69ea4

Polishing

95cc0fb

Polishing

bea55a3

AsyaPronina added 3 commits October 7, 2020 01:07

Polishing

4ddb1a8

Merge branch 'master' of https://github.com/opencv/opencv into comp_a…

7a7569c

…rgs_serialization

Updated CMakeLists.txt

61a68c6

dmatveev reviewed Oct 7, 2020

View reviewed changes

AsyaPronina added 3 commits October 7, 2020 12:53

Fixed comments

108a0f1

Addressed review comments

49cadf9

Removed decay from deserialize_arg

6d05e34

dmatveev reviewed Oct 7, 2020

View reviewed changes

AsyaPronina added 2 commits October 7, 2020 14:43

Addressed review comments

e947e67

Removed extra inclusion

d8be911

dmatveev approved these changes Oct 7, 2020

View reviewed changes

dmatveev self-assigned this Oct 7, 2020

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

dmatveev added this to the 4.5.0 milestone Oct 7, 2020

AsyaPronina and others added 5 commits October 7, 2020 18:40

Fixed Win64 warning

8540b95

Update gcommon.hpp

3bb9be2

Update serialization.cpp

18794d8

Update gcommon.hpp

574e8ed

gapi: drop GAPI_EXPORTS_W_SIMPLE from GCompileArg

06bc5eb

alalek reviewed Oct 7, 2020

View reviewed changes

alalek merged commit af2f8c6 into opencv:master Oct 7, 2020

alalek mentioned this pull request Nov 27, 2020

Merge 4.x #18951

Merged

Uh oh!

Conversation

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

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

AsyaPronina Oct 7, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmatveev left a comment

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 7, 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!

dmatveev commented Oct 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AsyaPronina commented Oct 2, 2020 •

edited by alalek

Loading

AsyaPronina Oct 7, 2020 •

edited

Loading

alalek commented Oct 7, 2020 •

edited

Loading