CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
GAPI Fluid: Resize F32C1 scalar version. #21678
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
Conversation
162db56
to
5af6268
Compare
@@ -507,7 +507,7 @@ namespace imgproc { | |||
int outSz_w = saturate_cast<int>(in.size.width * fx); | |||
int outSz_h = saturate_cast<int>(in.size.height * fy); | |||
GAPI_Assert(outSz_w > 0 && outSz_h > 0); | |||
return in.withSize(Size(outSz_w, outSz_h)); | |||
return in.withType(in.depth, in.chan).withSize(Size(outSz_w, outSz_h)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to apply withType
?
also we get type from in
and apply it to in
back - how is it supposed to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It speak that type of output matrix must be the same as input type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as i see you returns copy of in
which have type of in
already without additional withType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In GAPI_TYPED_KERNEL we always sort of return a copy of the input. These are features of the implementation of the graph. However, we must make sure that the output type matches our requirements. And if this is not so for a particular case, then such graph will not pass the check, since the checking function will swear at our attempt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Applied.
@@ -1895,7 +1896,8 @@ static inline void initScratchLinear(const cv::GMatDesc& in, | |||
auto *clone = scr.clone; | |||
auto *index = scr.mapsx; | |||
|
|||
for (int x = 0; x < outSz.width; x++) { | |||
for (int x = 0; x < outSz.width; ++x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my perspective modern c++ compiler is smart enough in this automatization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not used to relying too much on compiler optimizations :-)
Unit u; | ||
|
||
u.index0 = std::max(s - start, 0); | ||
u.index1 = ((f == 0.0) || s + 1 >= max) ? s - start : s - start + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is correct to compare f with 0.0 without epsilon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you describe such calculations in /* comment */
please - what is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is correct to compare f with 0.0 without epsilon?
Ok. Applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you describe such calculations in
/* comment */
please - what is it?
This is calculation of the map of Resize F32C1. This can be understood from the name of the structure in which this method is located and from the name of the method itself. Additional comments are superfluous in my opinion.
5af6268
to
72e331b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I didn't get in what cases we use:
GAPI_DbgAssert
GAPI_Assert
GAPI_Error
const Size& outSz, | ||
cv::gapi::fluid::Buffer& scratch, | ||
int lpi) { | ||
CV_ALWAYS_INLINE void initScratchLinear(const cv::GMatDesc& in, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't usually use CV_*
macros because it might broke standalone
mode.
Consider of adding synonym to there: https://github.com/opencv/opencv/blob/4.x/modules/gapi/include/opencv2/gapi/own/exports.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the gfluidimgproc.cpp
it is permissible to use OCV error login system and other CV_*
macros, since this file is under #if !defined(GAPI_STANDALONE)
guard.
To implement own GAPI error logging system and then define GAPI_Error
for the Standalone mode, it's necessary to create a separate task and in bounds this task to add own error logging system for GAPI. This changes should be placed to a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the assert.hpp
one is irrelevant here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it as CV_ anyway.
} | ||
else | ||
{ | ||
CV_Error(cv::Error::StsBadArg, "unsupported combination of type and number of channel"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GAPI_Error
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. GAPI_Error
was added and applied.
{ | ||
calcRowLinearC<uint8_t, Mapper, 4>(in, out, scratch); | ||
CV_Error(cv::Error::StsBadArg, "unsupported combination of type and number of channel"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GAPI_Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then create it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. GAPI_Error
was added and applied.
72e331b
to
21899cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please fix @TolyaTalamanov comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔
@@ -18,6 +18,7 @@ | |||
#if !defined(GAPI_STANDALONE) | |||
#include <opencv2/core/base.hpp> | |||
#define GAPI_Assert CV_Assert | |||
#define GAPI_Error CV_Error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a STANDALONE version defined, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To implement own GAPI error logging system and then define GAPI_Error for the Standalone mode, it's necessary to create a separate task and in bounds this task to add own error logging system for GAPI. This changes should place to a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the gfluidimgproc.cpp
it is permissible to use OCV error login system, since this file is under #if !defined(GAPI_STANDALONE)
guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the gfluidimgproc.cpp it is permissible to use OCV error login system, since this file is under #if !defined(GAPI_STANDALONE) guard.
Then just drop GAPI_Error
, use CV_Error
in your code, and file a task to implement one.
Half-defining this macro here in assert only brings confusion (as it is generic and may be used elsewhere, not only at gfluidimgproc.hpp
)
const Size& outSz, | ||
cv::gapi::fluid::Buffer& scratch, | ||
int lpi) { | ||
CV_ALWAYS_INLINE void initScratchLinear(const cv::GMatDesc& in, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the assert.hpp
one is irrelevant here?
const Size& outSz, | ||
cv::gapi::fluid::Buffer& scratch, | ||
int lpi) { | ||
CV_ALWAYS_INLINE void initScratchLinear(const cv::GMatDesc& in, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it as CV_ anyway.
21899cc
to
9ae70cc
Compare
@dmatveev Ok. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@alalek Could you please take a look this PR? |
@@ -43,7 +43,6 @@ namespace detail | |||
#define GAPI_Assert(expr) \ | |||
{ if (!(expr)) ::detail::assert_abort(#expr, __LINE__, __FILE__, __func__); } | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid unnecessary changes in patches
…calar GAPI Fluid: Resize F32C1 scalar version. * GAPI Fluid: Resize F32C1 scalar. * Final version * Applied comments
Resize F32C1 scalar version.