GAPI Fluid: Resize F32C1 scalar version. by anna-khakimova · Pull Request #21678 · opencv/opencv

anna-khakimova · 2022-03-03T14:41:05Z

Resize F32C1 scalar version.

force_builders=Linux AVX2,Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
Xbuild_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-2021.3.0:20.04
build_image:Custom Win=openvino-2021.4.1
build_image:Custom Mac=openvino-2021.2.0

buildworker:Custom Win=windows-3

test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
Xtest_opencl:Custom=OFF
Xtest_bigdata:Custom=1
Xtest_filter:Custom=*

CPU_BASELINE:Custom Win=AVX512_SKX
CPU_BASELINE:Custom=SSE4_2

sivanov-work · 2022-03-04T06:27:52Z

modules/gapi/include/opencv2/gapi/imgproc.hpp

                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));


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?

It speak that type of output matrix must be the same as input type.

as i see you returns copy of in which have type of in already without additional withType

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.

Ok. Applied.

sivanov-work · 2022-03-04T06:29:02Z

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

    auto *index = scr.mapsx;

-    for (int x = 0; x < outSz.width; x++) {
+    for (int x = 0; x < outSz.width; ++x)


from my perspective modern c++ compiler is smart enough in this automatization

I'm not used to relying too much on compiler optimizations :-)

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

sivanov-work · 2022-03-04T06:41:06Z

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

+        Unit u;
+
+        u.index0 = std::max(s - start, 0);
+        u.index1 = ((f == 0.0) || s + 1 >= max) ? s - start : s - start + 1;


how is correct to compare f with 0.0 without epsilon?

could you describe such calculations in /* comment */ please - what is it?

how is correct to compare f with 0.0 without epsilon?

Ok. Applied.

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.

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

TolyaTalamanov

Honestly I didn't get in what cases we use:

GAPI_DbgAssert
GAPI_Assert
GAPI_Error

TolyaTalamanov · 2022-03-04T09:13:48Z

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

-                                     const         Size& outSz,
-                                     cv::gapi::fluid::Buffer& scratch,
-                                     int  lpi) {
+CV_ALWAYS_INLINE void initScratchLinear(const cv::GMatDesc& in,


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

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.

I believe the assert.hpp one is irrelevant here?

Let's keep it as CV_ anyway.

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

TolyaTalamanov · 2022-03-04T09:18:59Z

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

-    }
+       else
+       {
+           CV_Error(cv::Error::StsBadArg, "unsupported combination of type and number of channel");


GAPI_Error?

Ok. GAPI_Error was added and applied.

TolyaTalamanov · 2022-03-04T09:19:10Z

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

+        else
        {
-            calcRowLinearC<uint8_t, Mapper, 4>(in, out, scratch);
+            CV_Error(cv::Error::StsBadArg, "unsupported combination of type and number of channel");


GAPI_Error

Then create it :)

Ok. GAPI_Error was added and applied.

sivanov-work

LGTM, but please fix @TolyaTalamanov comments

modules/gapi/include/opencv2/gapi/own/assert.hpp

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

dmatveev

❔

dmatveev · 2022-03-16T11:04:15Z

modules/gapi/include/opencv2/gapi/own/assert.hpp

 #if !defined(GAPI_STANDALONE)
 #include <opencv2/core/base.hpp>
 #define GAPI_Assert CV_Assert
+#define GAPI_Error CV_Error


There should be a STANDALONE version defined, too.

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.

For the gfluidimgproc.cpp it is permissible to use OCV error login system, since this file is under #if !defined(GAPI_STANDALONE) guard.

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)

dmatveev · 2022-03-16T11:05:07Z

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

-                                     const         Size& outSz,
-                                     cv::gapi::fluid::Buffer& scratch,
-                                     int  lpi) {
+CV_ALWAYS_INLINE void initScratchLinear(const cv::GMatDesc& in,


I believe the assert.hpp one is irrelevant here?

dmatveev · 2022-03-16T11:05:43Z

modules/gapi/src/backends/fluid/gfluidimgproc.cpp

-                                     const         Size& outSz,
-                                     cv::gapi::fluid::Buffer& scratch,
-                                     int  lpi) {
+CV_ALWAYS_INLINE void initScratchLinear(const cv::GMatDesc& in,


Let's keep it as CV_ anyway.

anna-khakimova · 2022-03-16T12:55:16Z

@dmatveev Ok. The GAPI_Error define was reverted.

dmatveev

👍

anna-khakimova · 2022-03-17T16:28:45Z

@alalek Could you please take a look this PR?

alalek · 2022-03-17T15:00:18Z

modules/gapi/include/opencv2/gapi/own/assert.hpp

 #define GAPI_Assert(expr) \
 { if (!(expr)) ::detail::assert_abort(#expr, __LINE__, __FILE__, __func__); }

-


please avoid unnecessary changes in patches

…calar GAPI Fluid: Resize F32C1 scalar version. * GAPI Fluid: Resize F32C1 scalar. * Final version * Applied comments

anna-khakimova requested review from TolyaTalamanov, alexgiving and sivanov-work March 3, 2022 14:41

anna-khakimova assigned dmatveev Mar 3, 2022

anna-khakimova added the category: g-api / gapi label Mar 3, 2022

anna-khakimova added this to the 4.6.0 milestone Mar 3, 2022

anna-khakimova force-pushed the ak/resize_f32c1_scalar branch 3 times, most recently from 162db56 to 5af6268 Compare March 3, 2022 23:49

sivanov-work reviewed Mar 4, 2022

View reviewed changes

anna-khakimova force-pushed the ak/resize_f32c1_scalar branch from 5af6268 to 72e331b Compare March 4, 2022 08:54

TolyaTalamanov reviewed Mar 4, 2022

View reviewed changes

anna-khakimova force-pushed the ak/resize_f32c1_scalar branch from 72e331b to 21899cc Compare March 15, 2022 10:20

anna-khakimova requested a review from sivanov-work March 16, 2022 08:03

anna-khakimova mentioned this pull request Mar 16, 2022

GAPI Fluid: SIMD SSE41 for Resize F32C1. #21685

Closed

sivanov-work approved these changes Mar 16, 2022

View reviewed changes

modules/gapi/include/opencv2/gapi/own/assert.hpp Outdated Show resolved Hide resolved

modules/gapi/src/backends/fluid/gfluidimgproc.cpp Show resolved Hide resolved

dmatveev reviewed Mar 16, 2022

View reviewed changes

Anna Khakimova added 3 commits March 16, 2022 15:46

GAPI Fluid: Resize F32C1 scalar.

e792d75

Final version

87b11f5

Applied comments

9ae70cc

anna-khakimova force-pushed the ak/resize_f32c1_scalar branch from 21899cc to 9ae70cc Compare March 16, 2022 12:52

anna-khakimova requested a review from dmatveev March 16, 2022 12:57

dmatveev approved these changes Mar 17, 2022

View reviewed changes

alalek reviewed Mar 17, 2022

View reviewed changes

alalek merged commit 48cd2d1 into opencv:4.x Mar 17, 2022

opencv-pushbot mentioned this pull request Apr 23, 2022

(5.x) Merge 4.x #21903

Merged

		#define GAPI_Assert(expr) \
		{ if (!(expr)) ::detail::assert_abort(#expr, __LINE__, __FILE__, __func__); }

Uh oh!

Conversation

anna-khakimova commented Mar 3, 2022

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!

anna-khakimova Mar 14, 2022 • 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!

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!

Uh oh!

Uh oh!

TolyaTalamanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-khakimova Mar 15, 2022 • 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!

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!

sivanov-work left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-khakimova Mar 16, 2022 • 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!

anna-khakimova Mar 16, 2022 • 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

anna-khakimova Mar 14, 2022 •

edited

Loading

anna-khakimova Mar 15, 2022 •

edited

Loading

anna-khakimova Mar 16, 2022 •

edited

Loading

anna-khakimova Mar 16, 2022 •

edited

Loading