GAPI: Move Resize kernel from core to imgproc by alexgiving · Pull Request #21157 · opencv/opencv

alexgiving · 2021-11-30T10:55:02Z

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

force_builders=Custom,Custom Win,Custom Mac
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

Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2021.4.1:20.04
build_image:Custom Win=openvino-2021.4.1
build_image:Custom Mac=openvino-2021.4.1

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
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

mpashchenkov · 2021-12-03T08:53:13Z

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

+            if (sz.width != 0 && sz.height != 0)
+            {


Suggested change

if (sz.width != 0 && sz.height != 0)

{

if (sz.width != 0 && sz.height != 0) {

Existed coding style is fine.
There is no Java.

Please use "Google" or "WebKit" style from clang-format

I thought it would be good to align the code style in the file.
There are both variants ) { and ) enter { in file.

I will follow the ) enter { style through the files. Thanks @mpashchenkov

mpashchenkov · 2021-12-03T08:53:42Z

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

+            }
+            else
+            {


Suggested change

}

else

{

} else {

mpashchenkov · 2021-12-03T08:58:53Z

modules/gapi/test/cpu/gapi_imgproc_tests_cpu.cpp

 {

+INSTANTIATE_TEST_CASE_P(ResizeTestCPU, ResizeTest,
+                        Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),


Suggested change

Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),

Combine(Values(CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),

Applied, thanks

I didn't notice the space at the end. It can be reverted to its previous condition or:
6SC1, CV_32FC1 ) --> 6SC1, CV_32FC1)

Thanks, I removed both of them

mpashchenkov · 2021-12-03T08:59:21Z

modules/gapi/test/cpu/gapi_imgproc_tests_cpu.cpp

+                                       cv::Size(30,30))));
+
+INSTANTIATE_TEST_CASE_P(ResizePTestCPU, ResizePTest,
+                        Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),


Suggested change

Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),

Combine(Values(CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),

Applied, thanks

mpashchenkov · 2021-12-03T08:59:37Z

modules/gapi/test/cpu/gapi_imgproc_tests_cpu.cpp

+                                       cv::Size(30,30))));
+
+INSTANTIATE_TEST_CASE_P(ResizeTestCPU, ResizeTestFxFy,
+                        Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),


Applied, thanks

@mpashchenkov this and other "debug" code were added and left for some reason. Why it should be removed during resize move from core to imgproc?

Because this code was commented. And I think that reason is removing extra test cases for reduce OCV tests duration.
Commented code is unused and doesn't make sense from my point of view.

mpashchenkov · 2021-12-03T09:03:32Z

modules/gapi/test/cpu/gapi_imgproc_tests_fluid.cpp

+                                Values(-1),
+                                Values(IMGPROC_FLUID),
+                                Values(Tolerance_FloatRel_IntAbs(1e-5, 1).to_compare_obj()),
+                                Values(/*cv::INTER_NEAREST,*/ cv::INTER_LINEAR/*, cv::INTER_AREA*/),


And here. Debug code.

modules/gapi/test/gapi_fluid_test.cpp

mpashchenkov · 2021-12-03T09:06:43Z

modules/gapi/test/gpu/gapi_imgproc_tests_gpu.cpp

 {

+INSTANTIATE_TEST_CASE_P(ResizeTestGPU, ResizeTest,
+                        Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),


Extra space

Applied, thanks

mpashchenkov · 2021-12-03T09:06:59Z

modules/gapi/test/gpu/gapi_imgproc_tests_gpu.cpp

+                                       cv::Size(30,30))));
+
+INSTANTIATE_TEST_CASE_P(ResizeTestGPU, ResizeTestFxFy,
+                        Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),


Extra space

Applied, thanks

mpashchenkov · 2021-12-03T09:11:00Z

samples/cpp/tutorial_code/gapi/doc_snippets/dynamic_graph_snippets.cpp

 #include <opencv2/gapi.hpp>
 #include <opencv2/gapi/core.hpp>
 #include <opencv2/gapi/cpu/core.hpp>
+#include <opencv2/gapi/imgproc.hpp>


Does the sample work correctly?

auto stream = cc.compileStreaming(cv::compile_args(cv::gapi::core::cpu::kernels()));

Is it necessary to keep

#include <opencv2/gapi/core.hpp> #include <opencv2/gapi/cpu/core.hpp>

here?

Applied, thanks

dmatveev

Great, thanks.

Since you've got acquainted with Resize now, can you please also have a look at #19379 ?

OrestChura

Great!

alalek · 2021-12-28T12:07:38Z

Conflicting files
modules/gapi/src/backends/fluid/gfluidcore.cpp

Need to rebase commits (relates #21339)

mpashchenkov

One more question.

mpashchenkov · 2021-12-29T11:24:16Z

modules/gapi/include/opencv2/gapi/core.hpp

 #include <utility> // std::tuple

 #include <opencv2/imgproc.hpp>
+#include <opencv2/gapi/imgproc.hpp>


Do this headers make sense for core?

Yes, <opencv2/gapi/imgproc.hpp> header is necessary for backward compatibility

mpashchenkov · 2021-12-29T11:26:18Z

modules/gapi/include/opencv2/gapi/core.hpp

 */
 namespace core {
+    using GResize = cv::gapi::imgproc::GResize;
+    using GResizeP = cv::gapi::imgproc::GResizeP;


Why are GResize and GResizeP located here (in core)?

That aliases for backward compatibility too

GAPI: Move Resize kernel from core to imgproc * Move Resize kernel from core to imgproc * Applied style comments * Adding backward compatibility * Applied Asya PR

alexgiving force-pushed the atrutnev/move_resize branch from 25ca3e4 to 2fd2b0b Compare December 1, 2021 12:45

anna-khakimova self-requested a review December 2, 2021 13:08

mpashchenkov reviewed Dec 3, 2021

View reviewed changes

alexgiving added the category: g-api / gapi label Dec 6, 2021

alexgiving force-pushed the atrutnev/move_resize branch 2 times, most recently from c2a6890 to e8da3be Compare December 7, 2021 10:34

alexgiving changed the title ~~GAPI: Move Resize kernel from core to imgproc~~ [HOLD] GAPI: Move Resize kernel from core to imgproc Dec 13, 2021

alexgiving changed the title ~~[HOLD] GAPI: Move Resize kernel from core to imgproc~~ GAPI: Move Resize kernel from core to imgproc Dec 20, 2021

alexgiving force-pushed the atrutnev/move_resize branch from 131d0bd to e879858 Compare December 21, 2021 10:00

dmatveev self-assigned this Dec 21, 2021

dmatveev added this to the 4.6.0 milestone Dec 21, 2021

dmatveev approved these changes Dec 21, 2021

View reviewed changes

alexgiving requested review from OrestChura and mpashchenkov December 22, 2021 10:51

OrestChura approved these changes Dec 22, 2021

View reviewed changes

alexgiving force-pushed the atrutnev/move_resize branch from e879858 to 5b37f17 Compare December 29, 2021 09:52

mpashchenkov approved these changes Dec 29, 2021

View reviewed changes

atrutnev added 4 commits December 29, 2021 15:53

Move Resize kernel from core to imgproc

8da3b51

Applied style comments

10b48b0

Adding backward compatibility

bded580

Applied Asya PR

5b37f17

alalek merged commit 43c04c2 into opencv:4.x Dec 29, 2021

alalek mentioned this pull request Dec 30, 2021

(5.x) Merge 4.x #21371

Merged

alexgiving mentioned this pull request Jan 10, 2022

Fix Fluid Resize Valgrind issue #21358

Closed

6 tasks

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #21651

Merged

	if (sz.width != 0 && sz.height != 0)
	{
	if (sz.width != 0 && sz.height != 0) {

-            }
-            else
-            {
+            } else {

	Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),
	Combine(Values(CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ),

Uh oh!

Conversation

alexgiving commented Nov 30, 2021 • edited by anna-khakimova Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek Dec 3, 2021 • 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

OrestChura left a comment

Choose a reason for hiding this comment

Uh oh!

alalek commented Dec 28, 2021

Uh oh!

mpashchenkov left a comment

Choose a reason for hiding this comment

Uh oh!

mpashchenkov Dec 29, 2021 • 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!

Reviewers

Assignees

Labels

Projects

alexgiving commented Nov 30, 2021 •

edited by anna-khakimova

Loading

alalek Dec 3, 2021 •

edited

Loading

mpashchenkov Dec 29, 2021 •

edited

Loading