G-API Stereo operation and tests by aDanPin · Pull Request #19732 · opencv/opencv

aDanPin · 2021-03-16T09:12:59Z

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

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom Win=openvino-2021.2.0
build_image:Custom Mac=openvino-2021.2.0

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

aDanPin · 2021-03-19T10:06:17Z

@AsyaPronina @TolyaTalamanov Please review

TolyaTalamanov · 2021-03-19T10:23:56Z

modules/gapi/include/opencv2/gapi/cpu/stereo.hpp

+namespace stereo {
+namespace cpu {
+
+GAPI_EXPORTS_W GKernelPackage kernels();


I believe it shouldn't be in python now, GAPI_EXPORTS would be enough

TolyaTalamanov · 2021-03-19T10:25:04Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+namespace stereo {
+
+enum OutputFormat {
+    disparity,


Capital letters are usually used + enum class

Capital letters are usually used for enum values and mixed-case style for enum name, @TolyaTalamanov , do you agree?

OUTPUT_FORMAT -> OutputFormat
disparity -> DISPARITY

TolyaTalamanov · 2021-03-19T10:26:34Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+    depth
+};
+
+G_TYPED_KERNEL(GStereoBM, <GMat(GMat, GMat,const int , const int, const OutputFormat, const int, const double, const double)>, "org.opencv.stereoBM") {


GMat, const int , -> GMat, const int, const ...

Didn`t get
Could you explain?

Have a look on alignment

TolyaTalamanov · 2021-03-19T10:27:02Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+};
+
+G_TYPED_KERNEL(GStereoBM, <GMat(GMat, GMat,const int , const int, const OutputFormat, const int, const double, const double)>, "org.opencv.stereoBM") {
+    static GMatDesc outMeta(GMatDesc ly, GMatDesc ry, const int, const int, const OutputFormat, const int ddtype,


Just wondering what is the ddtype ?

Its a type of output cv::Mat
ddtype -> outputType

I meant it just dtype

outputType -> dtype

TolyaTalamanov · 2021-03-19T10:28:46Z

modules/gapi/src/backends/cpu/gcpustereo.cpp

+        cv::Mat disp;
+        cv::StereoBM::create(numDisparities, blockSize) -> compute(left, right, disp);
+
+        // See https://answers.opencv.org/question/199915/how-to-get-depth-from-disparity-map/?sort=votes#:~:text=Z%20%3D%20B*f%20%2F%20disparity%20is%20the%20correct%20formula%20to,get%20the%20depth%20in%20mm.


I believe such kind of reference shouldn't be there

modules/gapi/include/opencv2/gapi/cpu/stereo.hpp

TolyaTalamanov · 2021-03-19T10:29:11Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+        GAPI_Assert(ry.chan == 1);
+        GAPI_Assert(ry.depth == CV_8U);
+
+        return ddtype == CV_16U ? ly.withDepth(CV_16U) : ly.withDepth(CV_16F);


dpype -> outputType

TolyaTalamanov · 2021-03-19T10:31:05Z

modules/gapi/src/backends/cpu/gcpustereo.cpp

+
+cv::gapi::GKernelPackage cv::gapi::stereo::cpu::kernels()
+{
+    static auto pkg = cv::gapi::kernels


static auto pkg = cv::gapi::kernels<GCPUStereoBM>();

Fixed I hope...

TolyaTalamanov · 2021-03-19T10:34:22Z

modules/gapi/src/backends/cpu/gcpustereo.cpp

+#include <opencv2/gapi/cpu/stereo.hpp>
+#include <opencv2/gapi/cpu/gcpukernel.hpp>
+
+#include <iostream>


Is it necessary ?

TolyaTalamanov · 2021-03-19T10:34:54Z

modules/gapi/test/stereo/gapi_stereo_tests.cpp

+
+namespace
+{
+#define STR_CPU [] () { return cv::compile_args(cv::gapi::use_only{cv::gapi::str::cpu::kernels()}); }


How is it called in another tests ?

I did a parameterized tests
Now it is used there

modules/gapi/include/opencv2/gapi/cpu/stereo.hpp

modules/gapi/include/opencv2/gapi/stereo.hpp

modules/gapi/src/api/kernels_stereo.cpp

modules/gapi/src/backends/cpu/gcpustereo.cpp

modules/gapi/include/opencv2/gapi/stereo.hpp

modules/gapi/src/backends/cpu/gcpustereo.cpp

modules/gapi/test/common/gapi_stereo_tests_inl.hpp

AsyaPronina · 2021-03-23T12:06:55Z

modules/gapi/include/opencv2/gapi/cpu/stereo.hpp

+
+namespace cv {
+namespace gapi {
+namespace ster {


Why ster?
Where did the stereo go?

In this case there is name collision of namespace stereo and operation stereo

Please discuss namespace name with the team -- to comply with OpenCV modules names we should create calib3d namespace, but we are focusing on addition of stereoscopic functionality, so may be namespace should be named as stereo or stereoscopic.

cv::gapi::ster -> cv::gapi::calib3d

AsyaPronina · 2021-03-23T12:09:54Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+namespace cv {
+
+namespace gapi {
+namespace ster {


AsyaPronina · 2021-03-23T12:10:16Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+namespace stereo {
+
+enum OutputFormat {
+    disparity,


Capital letters are usually used for enum values and mixed-case style for enum name, @TolyaTalamanov , do you agree?

AsyaPronina · 2021-03-23T12:22:05Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+
+G_TYPED_KERNEL(GStereo, <GMat(GMat, GMat, const cv::gapi::ster::OUTPUT_FORMAT, const int)>, "org.opencv.stereo") {
+    static GMatDesc outMeta(GMatDesc ly, GMatDesc ry,
+                            const cv::gapi::ster::OUTPUT_FORMAT,


Should we also check that OUTPUT_FORMAT is one of existent?

AsyaPronina · 2021-03-23T12:24:17Z

modules/gapi/src/backends/cpu/gcpustereo.cpp

+                    )
+    {
+        cv::Mat disp;
+        cv::StereoBM::create(


You shouldn't create new instance each time for cv::StereoBM if you have opportunity to use state...

AsyaPronina · 2021-03-23T14:13:07Z

modules/gapi/src/backends/cpu/gcpustereo.cpp

+
+#include <opencv2/calib3d.hpp>
+
+GAPI_OCV_KERNEL_ST(GCPUStereo, cv::gapi::ster::GStereo, cv::gapi::ster::StereoSetup)


StereoSetup may be just a structure to initialize state and state may be just StereoBM class, don't you think?

We also have to keep focus and baseLine to calculate depths from disparity
Easier way to keep it all in StereoSetup structure, I think...
Or what do you mean?

Please split the StereoSetup structure from cv::StereoBM. Lets cv::StereoBM be implementation details and be hidden from user. Lets cv::StereoBM be the state and StereoSetup be the initialization structure.

If you can't retrieve focus and baseline from the cv::StereoBM, then create a structure to hold cv::StereoBM and focus + baseline. Lets this structure be the state and StereoSetup be the structure to just initialize the state.

AsyaPronina · 2021-03-25T13:14:19Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+
+enum class OutputFormat {
+    DISPARITY,
+    DEPTHS


AsyaPronina · 2021-03-25T14:08:25Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+
+namespace calib3d {
+
+struct StereoSetup {


Please move this out of calib3d namespace.

Please also see how the initialization parameters are declared and documented here: https://github.com/opencv/opencv/blob/master/modules/gapi/include/opencv2/gapi/video.hpp#L22
Suggest to declare and document them the same way.

Done I hope...

AsyaPronina · 2021-03-25T14:09:07Z

modules/gapi/src/backends/cpu/gcpustereo.cpp

+
+#include <opencv2/calib3d.hpp>
+
+GAPI_OCV_KERNEL_ST(GCPUStereo, cv::gapi::calib3d::GStereo, cv::gapi::calib3d::StereoSetup)


Please see my answers in discussion about cv::gapi::calib3d::StereoSetup as a state.

Done I hope...

Can you please explain why it is stateful?

Its stateful for not to create cv::gapi::calib3d::StereoSetup every time kernel is called

Also its needed for keep approved kernel API and also have opportunity to configure blockSize, numDisparity and etc.

modules/gapi/include/opencv2/gapi/stereo.hpp

AsyaPronina · 2021-03-26T15:43:50Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+
+@param ly Y plane from left camera: 8-bit unsigned 1-channel matrix of @ref CV_8UC1 type
+@param ry Y plane from right camera: 8-bit unsigned 1-channel matrix of @ref CV_8UC1 type
+@param oF the enum class specifies do disparity to depth conversion or not


enum class to specify to make disparity to depth conversion or not

AsyaPronina · 2021-03-26T15:45:10Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+
+} // namespace calib3d
+
+/** @brief Extracts disparity or, optionally, depth information from stereo-pair.


Optionally is extra here.
We provide return type exclusively: user can retrieve either disparity or depth

Extract disparity/depth information depending on passed StereoOutputFormat argument

Please fix detailed description the same way

AsyaPronina · 2021-03-26T15:46:45Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+
+template<> struct CompileArgTag<cv::gapi::StereoInitParam> {
+    static const char* tag() {
+        return "org.opencv.stereoSetup";


dmatveev

Good overall! Some clarification is required though.

modules/gapi/CMakeLists.txt

modules/gapi/include/opencv2/gapi/stereo.hpp

dmatveev · 2021-03-29T13:43:09Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+/** @brief Structure for the Stereo operation setup parameters.*/
+struct GAPI_EXPORTS StereoSetup {
+    double baseline;
+    double focus;
+    cv::Ptr<cv::StereoBM> stereoBM;
+};


Where it is used?

That is state in stateful kernel

It should be hidden if it is not yet

dmatveev · 2021-03-29T13:44:08Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+@param ly Y plane from left camera: 8-bit unsigned 1-channel matrix of @ref CV_8UC1 type
+@param ry Y plane from right camera: 8-bit unsigned 1-channel matrix of @ref CV_8UC1 type
+@param oF the enum class to specify to make disparity to depth conversion or not
+@param dtype is a type of output @ref cv::Mat. Can be @ref CV_16U or @ref CV_16F


Does this apply to both output types? (Disparity vs. depth)

dmatveev · 2021-03-29T13:45:00Z

modules/gapi/src/backends/cpu/gcpustereo.cpp

+
+#include <opencv2/calib3d.hpp>
+
+GAPI_OCV_KERNEL_ST(GCPUStereo, cv::gapi::calib3d::GStereo, cv::gapi::calib3d::StereoSetup)


Can you please explain why it is stateful?

modules/gapi/src/backends/cpu/gcpustereo.cpp

dmatveev · 2021-03-29T13:46:32Z

modules/gapi/src/backends/cpu/gcpustereo.cpp

+            disp = (stereoSetup.focus * stereoSetup.baseline) / disp;
+        }
+
+        disp.convertTo(out_disp, dtype);


so is disp ok with such dtype? does it conform to the ocv behavior?

modules/gapi/test/common/gapi_stereo_tests_inl.hpp

modules/gapi/include/opencv2/gapi/stereo.hpp

modules/gapi/src/backends/cpu/gcpustereo.cpp

modules/gapi/test/common/gapi_stereo_tests_inl.hpp

modules/gapi/src/backends/cpu/gcpustereo.cpp

modules/gapi/include/opencv2/gapi/cpu/stereo.hpp

modules/gapi/include/opencv2/gapi/stereo.hpp

modules/gapi/src/backends/cpu/gcpustereo.cpp

modules/gapi/test/common/gapi_stereo_tests_inl.hpp

modules/gapi/src/backends/cpu/gcpustereo.cpp

dmatveev

👍 approved, please merge it as-is

dmatveev · 2021-03-31T19:16:10Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+    DEPTH_FLOAT16,
+    DEPTH_FLOAT32,
+    DISPARITY_FIXED16_11_5,
+    DISPARITY_FIXED16_12_4


This is questionable, at least I'd give some better names to this enum in the future iteration (after merge).

FLOAT16 -> F16, FLOAT32 -> F32, FIXED16_ -> Q (as for quantized).

When you'll do this, please don't break the existing enumerations, simply provides aliases with the same name (cc: @AsyaPronina @rgarnov )

dmatveev · 2021-03-31T21:31:11Z

Fantastic! Thanks a lot @aDanPin, @AsyaPronina, @rgarnov for your work! And kudos to @alalek for merging this!

alalek · 2021-04-01T02:34:22Z

modules/gapi/include/opencv2/gapi/stereo.hpp

+    DISPARITY_FIXED16_12_4
+};
+
+namespace calib3d {


calib3d

In OpenCV 5.0 calib3d is gone (see the "next" branch and PR #18940).
Used functionality is moved to "stereo" namespace.

Perhaps it makes sense to use new names for namespaces and functions instead.

Thanks Alexander,

This namespace here is used only to hold an Operation's type -- the user code is supposed to use cv::gapi::stereo instead so we can move these types around easily with no visible impact

-D

aDanPin force-pushed the danpin/gapi/stereo_operation_and_test branch from 1878cdc to d19f4c4 Compare March 19, 2021 04:48

TolyaTalamanov reviewed Mar 19, 2021

View reviewed changes

aDanPin force-pushed the danpin/gapi/stereo_operation_and_test branch from 7622eea to fadfe89 Compare March 19, 2021 15:50

AsyaPronina requested changes Mar 22, 2021

View reviewed changes

AsyaPronina requested changes Mar 23, 2021

View reviewed changes

aDanPin force-pushed the danpin/gapi/stereo_operation_and_test branch from 9ed4aa5 to a1a2e73 Compare March 23, 2021 13:30

AsyaPronina reviewed Mar 23, 2021

View reviewed changes

AsyaPronina reviewed Mar 25, 2021

View reviewed changes

AsyaPronina reviewed Mar 26, 2021

View reviewed changes

dmatveev reviewed Mar 29, 2021

View reviewed changes

aDanPin force-pushed the danpin/gapi/stereo_operation_and_test branch from e0cf9ca to d8ef570 Compare March 30, 2021 06:43

rgarnov requested changes Mar 31, 2021

View reviewed changes

modules/gapi/include/opencv2/gapi/stereo.hpp Outdated Show resolved Hide resolved

aDanPin force-pushed the danpin/gapi/stereo_operation_and_test branch from 370e9ea to 53d6089 Compare March 31, 2021 14:18

AsyaPronina reviewed Mar 31, 2021

View reviewed changes

modules/gapi/src/backends/cpu/gcpustereo.cpp Outdated Show resolved Hide resolved

AsyaPronina reviewed Mar 31, 2021

View reviewed changes

modules/gapi/src/backends/cpu/gcpustereo.cpp Outdated Show resolved Hide resolved

aDanPin force-pushed the danpin/gapi/stereo_operation_and_test branch from 214eb27 to fee2816 Compare March 31, 2021 14:48

AsyaPronina reviewed Mar 31, 2021

View reviewed changes

modules/gapi/src/backends/cpu/gcpustereo.cpp Outdated Show resolved Hide resolved

AsyaPronina approved these changes Mar 31, 2021

View reviewed changes

Adding stereo operation and tests

e6f52b0

aDanPin force-pushed the danpin/gapi/stereo_operation_and_test branch from 4b2b505 to e6f52b0 Compare March 31, 2021 17:10

rgarnov approved these changes Mar 31, 2021

View reviewed changes

dmatveev self-assigned this Mar 31, 2021

dmatveev added the category: g-api / gapi label Mar 31, 2021

dmatveev added this to the 4.5.2 milestone Mar 31, 2021

dmatveev approved these changes Mar 31, 2021

View reviewed changes

alalek merged commit 9f13fdb into opencv:master Mar 31, 2021

alalek reviewed Apr 1, 2021

View reviewed changes

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged


		#include <opencv2/calib3d.hpp>

		GAPI_OCV_KERNEL_ST(GCPUStereo, cv::gapi::ster::GStereo, cv::gapi::ster::StereoSetup)


		} // namespace calib3d

		/** @brief Extracts disparity or, optionally, depth information from stereo-pair.

Uh oh!

Conversation

aDanPin commented Mar 16, 2021 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

aDanPin commented Mar 19, 2021

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

aDanPin commented Mar 16, 2021 •

edited by alalek

Loading