Skip to content

G-API: GAPI introduce compile guard for some types for gin/gout params passing #21041

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

Merged
merged 18 commits into from
Dec 3, 2021

Conversation

sivanov-work
Copy link
Contributor

@sivanov-work sivanov-work commented Nov 11, 2021

Problem is cv::gin()/gout() accept types like cv::GMat and cv::gapi::own::Mat
But when passed, objects of this type become {{OpaqueRef}}s and our underlying image-based operations never expect that.

Need to restrict the generic type T when wrapping the "host" data to types NOT known to G-API, and fail if there is an own::Mat(???) or some of the G*-types passed.

Add static check for field GTypeTraits::shaped or placement type into GAPI_OWN_TYPES_LIST by itself. Either condition will fail compilation if enabled.

And produces compilation error like

!!! WHAT ---> ...modules\gapi\include\opencv2/gapi/gtype_traits.hpp(188,90): error C2338: **gin/gout must not be used with G* structures with tag::Meta or gapi::own**


...modules\gapi\include\opencv2/gapi/garg.hpp(275): message : see reference to function template instantiation 'cv::detail::GTypeTraits<C>::strip_type cv::detail::WrapValue<cv::
GOpaque<U>,void>::wrap_in<M>(const U &)' being compiled [...modules\gapi\opencv_test_gapi.vcxproj]
          with
          [
              C=cv::GOpaque<M>,
              U=M
          ]
...modules\gapi\include\opencv2/gapi/garg.hpp(275): message : see reference to function template instantiation 'cv::detail::GTypeTraits<C>::strip_type cv::detail::WrapValue<cv::
GOpaque<U>,void>::wrap_in<M>(const U &)' being compiled [...\modules\gapi\opencv_test_gapi.vcxproj]
          with
          [
              C=cv::GOpaque<M>,
              U=M
          ]



!!! WHERE ---> ...modules\gapi\test\gapi_util_tests.cpp(115): message : see reference to function template instantiation 'cv::GRunArgs cv::gin<M>(const M &)' being compiled ...\modules\gapi\opencv_test_gapi.vcxproj]
  gapi_int_gmetaarg_test.cpp

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

@sivanov-work sivanov-work marked this pull request as ready for review November 18, 2021 12:03
Copy link
Contributor

@rgarnov rgarnov left a comment

Choose a reason for hiding this comment

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

Not sure if we need to develop a new tagging mechanism here, it will oblige us to add these tags for every new g-type. We can check GTypeTraits::shape to define a g-type, this will automate this gin/gout prohibition for newly added types. For special types (own::Mat) we can simply have a list of prohibited types and check it too

@sivanov-work
Copy link
Contributor Author

@rgarnov

Not sure if we need to develop a new tagging mechanism here, it will oblige us to add these tags for every new g-type.

Now tags are part of GTypeTraits and adding new g-type will oblige us to add new GtypeTraits with shape for this type anyway.

(own::Mat) we can simply have a list of prohibited types and check it too

Additional list of prohibites type will increase complexity in several times, because we will need to implement SFINAE for GtypeTraits & ProhibitesList both and moreover for all it's combinations. Looks like it is possible to add gtypetraits for own::mat

Personally I would prefer tagging mechanism here because it do-not-broke-anything and simple-in-use... BUT current implementation is quite far from my initial intention due to failing Python compilation and I need to recycle GtypeTraits here anyway.. So maybe you right and reusing shape from traits solve initial problem here and should be solution

Let's reuse shape here

@sivanov-work sivanov-work requested a review from rgarnov November 24, 2021 09:23
Copy link
Contributor

@rgarnov rgarnov left a comment

Choose a reason for hiding this comment

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

Looks good, just minor cosmetics

Copy link
Contributor

@rgarnov rgarnov left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@dmatveev dmatveev self-assigned this Dec 2, 2021
@dmatveev dmatveev added this to the 4.5.5 milestone Dec 2, 2021
Comment on lines +17 to +22
#define GAPI_OWN_TYPES_LIST cv::gapi::own::Rect, \
cv::gapi::own::Size, \
cv::gapi::own::Point, \
cv::gapi::own::Point2f, \
cv::gapi::own::Scalar, \
cv::gapi::own::Mat
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use macro here and not an enumeration via tuple?

Macros seem old-fashioned (and sometimes dangerous) now when we have variadic templates

@sivanov-work
Copy link
Contributor Author

@dmatveev about MACRO, it's quite usable sometimes, for example std::tuple<own::Mat, own::Size, ...> will trigger context templates instantiation again and again in every compilation unit (compile guard doesn't work here). sometimes it gives compilation overhead.. It could be solved with external template but then we need single instantiation point and quite complicated.

In current code this MACRO does declare list for variadic params in std::tuple in deepest

@sivanov-work
Copy link
Contributor Author

@alalek Could you merge it please ?

@sivanov-work sivanov-work changed the title G-API: GAPI object tagged G-API: GAPI introduce compile guard for some types for gin/gout params passing Dec 3, 2021
@alalek alalek merged commit c5b8b56 into opencv:4.x Dec 3, 2021
@alalek alalek mentioned this pull request Dec 30, 2021
@alalek alalek mentioned this pull request Feb 22, 2022
@sivanov-work sivanov-work deleted the gin_gout_concept branch March 5, 2022 05:48
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
G-API: GAPI introduce compile guard for some types for gin/gout params passing

* Initial for taged solution

* Move out tags to gtags.hpp & add protection for own::Mat

* Add compile guard to proper place

* Fix MACRO concat

* Add unit tests

* Remove class MACRO injection due to Python3

* Revert back unproper changes

* Apply comments: reuse shape from traits

* Throw away unused gtags

* Apply comments

* Handle own::*

* Fix test

* Fix test(1)

* Fix unix build

* Try on type list

* Apply comments

* Apply comments

* Fix warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants