CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
GAPI: Move Resize kernel from core to imgproc #21157
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
25ca3e4
to
2fd2b0b
Compare
if (sz.width != 0 && sz.height != 0) | ||
{ |
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.
if (sz.width != 0 && sz.height != 0) | |
{ | |
if (sz.width != 0 && sz.height != 0) { |
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.
Existed coding style is fine.
There is no Java.
Please use "Google" or "WebKit" style from clang-format
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 thought it would be good to align the code style in the file.
There are both variants ) {
and ) enter {
in file.
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 will follow the ) enter {
style through the files. Thanks @mpashchenkov
} | ||
else | ||
{ |
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.
} | |
else | |
{ | |
} else { |
@@ -18,6 +18,42 @@ namespace | |||
namespace opencv_test | |||
{ | |||
|
|||
INSTANTIATE_TEST_CASE_P(ResizeTestCPU, ResizeTest, | |||
Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), |
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.
Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), | |
Combine(Values(CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), |
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.
Applied, thanks
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 didn't notice the space at the end. It can be reverted to its previous condition or:
6SC1, CV_32FC1 )
--> 6SC1, CV_32FC1)
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.
Thanks, I removed both of them
cv::Size(30,30)))); | ||
|
||
INSTANTIATE_TEST_CASE_P(ResizePTestCPU, ResizePTest, | ||
Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), |
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.
Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), | |
Combine(Values(CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), |
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.
Applied, thanks
cv::Size(30,30)))); | ||
|
||
INSTANTIATE_TEST_CASE_P(ResizeTestCPU, ResizeTestFxFy, | ||
Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), |
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.
And 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.
Applied, thanks
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.
@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?
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.
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.
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*/), |
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.
And here. Debug code.
@@ -17,6 +17,30 @@ namespace | |||
namespace opencv_test | |||
{ | |||
|
|||
INSTANTIATE_TEST_CASE_P(ResizeTestGPU, ResizeTest, | |||
Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), |
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.
Extra space
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.
Applied, thanks
cv::Size(30,30)))); | ||
|
||
INSTANTIATE_TEST_CASE_P(ResizeTestGPU, ResizeTestFxFy, | ||
Combine(Values( CV_8UC1, CV_8UC3, CV_16UC1, CV_16SC1, CV_32FC1 ), |
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.
Extra space
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.
Applied, thanks
@@ -1,6 +1,7 @@ | |||
#include <opencv2/gapi.hpp> | |||
#include <opencv2/gapi/core.hpp> | |||
#include <opencv2/gapi/cpu/core.hpp> | |||
#include <opencv2/gapi/imgproc.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.
Does the sample work correctly?
auto stream = cc.compileStreaming(cv::compile_args(cv::gapi::core::cpu::kernels()));
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.
Is it necessary to keep
#include <opencv2/gapi/core.hpp>
#include <opencv2/gapi/cpu/core.hpp>
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.
Applied, thanks
c2a6890
to
e8da3be
Compare
131d0bd
to
e879858
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.
Great, thanks.
Since you've got acquainted with Resize now, can you please also have a look at #19379 ?
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.
Great!
Need to rebase commits (relates #21339) |
e879858
to
5b37f17
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.
One more question.
#include <utility> // std::tuple | ||
|
||
#include <opencv2/imgproc.hpp> | ||
#include <opencv2/gapi/imgproc.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.
Do this headers make sense for core?
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.
Yes, <opencv2/gapi/imgproc.hpp> header is necessary for backward compatibility
@@ -34,6 +34,9 @@ namespace cv { namespace gapi { | |||
* Core module functionality. | |||
*/ | |||
namespace core { | |||
using GResize = cv::gapi::imgproc::GResize; | |||
using GResizeP = cv::gapi::imgproc::GResizeP; |
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 are GResize and GResizeP located here (in core)?
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.
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
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.