Skip to content

G-API: GOpaque implementation#16118

Merged
alalek merged 8 commits intoopencv:masterfrom
smirnov-alexey:as/gopaque
Jan 30, 2020
Merged

G-API: GOpaque implementation#16118
alalek merged 8 commits intoopencv:masterfrom
smirnov-alexey:as/gopaque

Conversation

@smirnov-alexey
Copy link
Copy Markdown
Contributor

@smirnov-alexey smirnov-alexey commented Dec 10, 2019 •
edited by alalek
Loading

This pullrequest adds a new type in G-API - GOpaque which is designed to store a single custom user type.

Currently it's almost copypasted GArray except other internal types are not related to vector.

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=Custom,Custom Win,ARMv7
buildworker:Custom=linux-1
#build_image:Custom=ubuntu-clang:18.04
build_image:Custom=centos:7
build_contrib:Custom=OFF

@smirnov-alexey
Copy link
Copy Markdown
Contributor Author

smirnov-alexey commented Dec 12, 2019

@TolyaTalamanov, @AsyaPronina, @anton-potapov. Any feedback is appreciated

@smirnov-alexey
Copy link
Copy Markdown
Contributor Author

smirnov-alexey commented Dec 12, 2019

Adding GOpaque to variant broke the ABI

@alalek
Copy link
Copy Markdown
Member

alalek commented Dec 16, 2019

Removed Symbols 121

Unfortunately there is no reliable way to maintain ABI checker for G-API anymore.
We need to define long term solution.

@smirnov-alexey smirnov-alexey changed the title WIP: G-API: GOpaque implementation G-API: GOpaque implementation Dec 26, 2019
@smirnov-alexey
Copy link
Copy Markdown
Contributor Author

smirnov-alexey commented Dec 26, 2019

@rgarnov, @AsyaPronina, @TolyaTalamanov, please, review. As discussed with @dmatveev, 'copypaste' of GArray is OK for now, so it can be reviewed and merged.

smirnov-alexey
smirnov-alexey commented Dec 26, 2019
View reviewed changes
smirnov-alexey
smirnov-alexey commented Dec 26, 2019
View reviewed changes
smirnov-alexey
smirnov-alexey commented Dec 26, 2019
View reviewed changes
smirnov-alexey
smirnov-alexey commented Dec 26, 2019
View reviewed changes
dmatveev
dmatveev reviewed Jan 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well done!

dmatveev
dmatveev approved these changes Jan 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, thanks!

@dmatveev
Copy link
Copy Markdown
Contributor

dmatveev commented Jan 13, 2020

Unfortunately there is no reliable way to maintain ABI checker for G-API anymore.
We need to define long term solution.

@alalek I thought ABI compatibility requirements were relaxed already since 4.0?

@alalek
Copy link
Copy Markdown
Member

alalek commented Jan 13, 2020

We use "ABI checker" tool in "source" compatibility mode for OpenCV 4.x. OpenCV 3.4 uses full binary/source checks.

@dmatveev
Copy link
Copy Markdown
Contributor

dmatveev commented Jan 13, 2020

https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/24319/steps/Compare%20ABI%20dumps/logs/report-html

This thing just can't into template changes :) I believe source compatibility is actually preserved here.

@alalek alalek mentioned this pull request Jan 13, 2020
Merged
@alalek
Copy link
Copy Markdown
Member

alalek commented Jan 27, 2020 •
edited
Loading

There is some issue with ARMv7 builder (GCC 4.8). My guess, it is related to unique_ptr.

alalek
alalek reviewed Jan 27, 2020
View reviewed changes
modules/gapi/include/opencv2/gapi/gopaque.hpp Outdated
OpaqueRef() = default;
template<typename T> explicit OpaqueRef(const T& obj) : m_ref(new OpaqueRefT<T>(obj)) {}
template<typename T> explicit OpaqueRef( T& obj) : m_ref(new OpaqueRefT<T>(obj)) {}
template<typename T> explicit OpaqueRef( T&& obj) : m_ref(new OpaqueRefT<T>(obj)) {}
Copy link
Copy Markdown
Member

@alalek alalek Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removal of T&& case + adding specialization for WrapValue<cv::detail::OpaqueRef> helps with compilation on GCC 4.8

Copy link
Copy Markdown
Member

@alalek alalek Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removal of T&&

You may try std::remove_reference instead:

        template<typename T> explicit OpaqueRef(const typename std::remove_reference<T>::type&  obj) : m_ref(new OpaqueRefT<T>(obj)) {}
        template<typename T> explicit OpaqueRef(      typename std::remove_reference<T>::type&  obj) : m_ref(new OpaqueRefT<T>(obj)) {}
        template<typename T> explicit OpaqueRef(      typename std::remove_reference<T>::type&& obj) : m_ref(new OpaqueRefT<T>(obj)) {}
Error message 
build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/util/any.hpp:69:59: error: call of overloaded 'OpaqueRef(cv::detail::OpaqueRef&)' is ambiguous
          holder_impl(arg_t&& a) : v(std::forward<arg_t>(a)) {}
                                                           ^
/build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/util/any.hpp:69:59: note: candidates are:
In file included from /build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/garg.hpp:23:0,
                 from /build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/gcall.hpp:11,
                 from /build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/gkernel.hpp:20,
                 from /build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/core.hpp:19,
                 from /build/precommit_armv7/opencv/modules/gapi/src/precomp.hpp:14,
                 from /build/precommit_armv7/opencv/modules/gapi/src/backends/cpu/gcpubackend.cpp:8:
/build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/gopaque.hpp:222:39: note: cv::detail::OpaqueRef::OpaqueRef(T&&) [with T = cv::detail::OpaqueRef&]
         template<typename T> explicit OpaqueRef(      T&& obj) : m_ref(new OpaqueRefT<T>(obj)) {}
                                       ^
/build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/gopaque.hpp:221:39: note: cv::detail::OpaqueRef::OpaqueRef(T&) [with T = cv::detail::OpaqueRef]
         template<typename T> explicit OpaqueRef(      T&  obj) : m_ref(new OpaqueRefT<T>(obj)) {}
                                       ^
/build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/gopaque.hpp:220:39: note: cv::detail::OpaqueRef::OpaqueRef(const T&) [with T = cv::detail::OpaqueRef]
         template<typename T> explicit OpaqueRef(const T&  obj) : m_ref(new OpaqueRefT<T>(obj)) {}
                                       ^
/build/precommit_armv7/opencv/modules/gapi/include/opencv2/gapi/gopaque.hpp:209:11: note: cv::detail::OpaqueRef::OpaqueRef(const cv::detail::OpaqueRef&)
     class OpaqueRef
           ^

@alalek alalek merged commit 0d456f9 into opencv:master Jan 30, 2020
@andrey-golubev andrey-golubev mentioned this pull request Mar 6, 2020
Closed
4 tasks
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
@smirnov-alexey
Merge pull request opencv#16118 from smirnov-alexey:as/gopaque
83533eb 
G-API: GOpaque implementation

* Stub initial copypasted solution

* Fix mov test and add a couple of others

* Fix warnings

* More code coverage and tests

* fix macos warning

* address review comments

* Address review comments and fix indentation

* Fix build on armv7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek left review comments

@dmatveev dmatveev dmatveev approved these changes

@rgarnov rgarnov Awaiting requested review from rgarnov

Assignees

@dmatveev dmatveev

Labels

category: g-api / gapi

Projects

None yet

Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@smirnov-alexey @alalek @dmatveev @asmorkalov