GAPI: Kalman filter stateful kernel. by anna-khakimova · Pull Request #18869 · opencv/opencv

anna-khakimova · 2020-11-19T22:56:51Z

New stateful Kalman filter kernel for GAPI.

@OrestChura, @rgarnov, @AsyaPronina please review.

Published for review on 19th of November.

force_builders_only=linux,docs,Custom
Xforce_builders=Custom,Custom Win,Custom Mac,Linux32,Win32
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

Xbuild_image:Custom=ubuntu-openvino-2020.3.0:16.04
Xbuild_image:Custom Win=openvino-2020.3.0
build_image:Custom Mac=openvino-2020.3.0

test_modules:Custom=gapi
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

alalek · 2020-11-20T14:10:00Z

New BackgroundSubtractorMOG2 kernel

In makes sense to separate unrelated commits.

alalek · 2020-11-20T14:09:53Z

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

+#include "opencv2/core/mat.hpp"
+#include "opencv2/core.hpp"
+
+#undef countNonZero


Why is this in public GAPI headers?

@anna-khakimova countNonZero() is banned for a reason: this function can take ONLY single-channel Mat.
(See #16657)
You can use cv::norm() here instead

It is banned in tests only.
No idea how this appears in common public header.

This appear because both headers modules/gapi/include/opencv2/gapi/cpu/video.hpp and modules/gapi/test/test_precomp.hpp are included to one modules/gapi/test/cpu/gapi_video_tests_cpu.cpp. So,
#define countNonZero() countNonZero_is_forbidden_in_tests_use_norm_instead() in test_precomp.hpp affects content of cpu/video.hpp file.

Do you really need such details in the future?

Please move this hack to the tests if it is really necessary (or move code implementation into src/, or replace this call)

cv::countNonZero(...) > 0) is slow (and may cause unexpected result overflow which is worse). Replace to

norm(..., NORM_INF) != 0

Ok. Changed.

alalek · 2020-11-20T14:13:41Z

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

+struct KalmanParams
+{
+    KalmanParams()
+    {
+        statePre = Mat::zeros(dpDims, 1, type);
+        statePost = Mat::zeros(dpDims, 1, type);
+        transitionMatrix = Mat::eye(dpDims, dpDims, type);


A lot of code in public headers.

Public headers should define API for Users only.
Implementation must go into "src".

/cc @dmatveev

I think, implementation can be placed in modules/gapi/src/backends/cpu/gcpuvideo.cpp next to the kernel which uses this

alalek · 2020-11-20T14:14:37Z

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

+
+struct KalmanParams


Classes in public headers should be properly documented.

Ok. Applied.

modules/gapi/include/opencv2/gapi/video.hpp

OrestChura · 2020-11-20T14:27:37Z

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

+#include "opencv2/core/mat.hpp"
+#include "opencv2/core.hpp"
+
+#undef countNonZero


@anna-khakimova countNonZero() is banned for a reason: this function can take ONLY single-channel Mat.
(See #16657)
You can use cv::norm() here instead

OrestChura · 2020-11-20T14:30:05Z

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

+struct KalmanParams
+{
+    KalmanParams()
+    {
+        statePre = Mat::zeros(dpDims, 1, type);
+        statePost = Mat::zeros(dpDims, 1, type);
+        transitionMatrix = Mat::eye(dpDims, dpDims, type);


I think, implementation can be placed in modules/gapi/src/backends/cpu/gcpuvideo.cpp next to the kernel which uses this

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

modules/gapi/include/opencv2/gapi/video.hpp

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

modules/gapi/test/common/gapi_video_tests_inl.hpp

modules/gapi/test/cpu/gapi_video_tests_cpu.cpp

OrestChura

Such a complicated operation! Great!

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

modules/gapi/include/opencv2/gapi/video.hpp

modules/gapi/test/common/gapi_video_tests_inl.hpp

modules/gapi/include/opencv2/gapi/video.hpp

modules/gapi/test/common/gapi_video_tests_inl.hpp

dmatveev

Thanks @OrestChura for the initial review.

modules/gapi/include/opencv2/gapi/video.hpp

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

modules/gapi/include/opencv2/gapi/video.hpp

modules/gapi/src/api/kernels_video.cpp

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

modules/gapi/test/common/gapi_video_tests_inl.hpp

anna-khakimova · 2020-12-02T12:41:54Z

@OrestChura please resolve applied conversations.

modules/gapi/include/opencv2/gapi/video.hpp

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

modules/gapi/src/api/kernels_video.cpp

modules/gapi/include/opencv2/gapi/video.hpp

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

modules/gapi/test/common/gapi_video_tests_inl.hpp

modules/gapi/include/opencv2/gapi/video.hpp

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

OrestChura

Almost!!

modules/gapi/include/opencv2/gapi/video.hpp

modules/gapi/test/common/gapi_video_tests_inl.hpp

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

modules/gapi/include/opencv2/gapi/video.hpp

modules/gapi/test/common/gapi_video_tests_inl.hpp

anna-khakimova · 2020-12-10T17:17:18Z

@alalek please take a look.

modules/gapi/include/opencv2/gapi/video.hpp

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

modules/gapi/include/opencv2/gapi/video.hpp

modules/gapi/test/common/gapi_video_tests_inl.hpp

modules/gapi/include/opencv2/gapi/video.hpp

modules/gapi/test/common/gapi_video_tests_inl.hpp

modules/gapi/include/opencv2/gapi/video.hpp

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

modules/gapi/include/opencv2/gapi/video.hpp

anna-khakimova · 2020-12-11T12:54:32Z

@rgarnov , @OrestChura please check

rgarnov

LGTM, good job!

OrestChura

Great job! Thank you!
The remaining comments are minor, you may apply if there are further iterations

modules/gapi/src/api/kernels_video.cpp

anna-khakimova · 2020-12-11T15:58:58Z

@alalek please review.

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

modules/gapi/include/opencv2/gapi/video.hpp

alalek · 2020-12-11T16:28:18Z

Overall look good to me.
Please left feedback near comments if they are out of scope of this PR and will not be addressed here.

anna-khakimova · 2020-12-13T23:43:41Z

@alalek comments were addressed. Please check.

anna-khakimova · 2020-12-14T09:38:11Z

@alalek thank you.

* GAPI: Kalman filter stateful kernel * Applied comments * Applied comments. Second iteration * Add overload without control vector * Remove structure constructor and dimension fields. * Add sample as test * Remove visualization from test-sample + correct doxygen comments * Applied comments.

anna-khakimova force-pushed the ak/kalman branch 5 times, most recently from 10772df to 0b05f55 Compare November 20, 2020 13:35

anna-khakimova force-pushed the ak/kalman branch from 0b05f55 to cda8f46 Compare November 20, 2020 14:12

alalek reviewed Nov 20, 2020

View reviewed changes

OrestChura reviewed Nov 20, 2020

View reviewed changes

anna-khakimova force-pushed the ak/kalman branch 2 times, most recently from 5b02e05 to 256c4a6 Compare November 29, 2020 00:51

OrestChura reviewed Nov 30, 2020

View reviewed changes

GAPI: Kalman filter stateful kernel

ed88b13

anna-khakimova force-pushed the ak/kalman branch from 256c4a6 to ed88b13 Compare December 1, 2020 15:27

anna-khakimova changed the title ~~GAPI: New Kalman filter kernel.~~ GAPI: Kalman filter stateful kernel. Dec 1, 2020

dmatveev reviewed Dec 2, 2020

View reviewed changes

Applied comments

4c886a5

OrestChura reviewed Dec 2, 2020

View reviewed changes

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

modules/gapi/include/opencv2/gapi/cpu/video.hpp Outdated Show resolved Hide resolved

modules/gapi/src/api/kernels_video.cpp Show resolved Hide resolved

OrestChura reviewed Dec 2, 2020

View reviewed changes

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

OrestChura reviewed Dec 2, 2020

View reviewed changes

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

Applied comments. Second iteration

e91f4f3

OrestChura reviewed Dec 3, 2020

View reviewed changes

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

Add overload without control vector

e51ae4d

OrestChura reviewed Dec 4, 2020

View reviewed changes

modules/gapi/test/common/gapi_video_tests_inl.hpp Outdated Show resolved Hide resolved

rgarnov requested changes Dec 4, 2020

View reviewed changes

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

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

modules/gapi/test/common/gapi_video_tests_inl.hpp Show resolved Hide resolved

Remove structure constructor and dimension fields.

1475a57

anna-khakimova force-pushed the ak/kalman branch from eb37623 to 1475a57 Compare December 8, 2020 09:55

Add sample as test

8969531

anna-khakimova force-pushed the ak/kalman branch from 88fad40 to abbbc9d Compare December 10, 2020 16:36

rgarnov requested changes Dec 10, 2020

View reviewed changes

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

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

OrestChura reviewed Dec 10, 2020

View reviewed changes

anna-khakimova requested review from OrestChura, alalek and rgarnov December 11, 2020 08:59

anna-khakimova force-pushed the ak/kalman branch from abbbc9d to 08e90bc Compare December 11, 2020 09:01

OrestChura reviewed Dec 11, 2020

View reviewed changes

modules/gapi/test/common/gapi_video_tests_inl.hpp Outdated Show resolved Hide resolved

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

anna-khakimova force-pushed the ak/kalman branch from 08e90bc to c5e9c0d Compare December 11, 2020 10:13

rgarnov requested changes Dec 11, 2020

View reviewed changes

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

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

anna-khakimova force-pushed the ak/kalman branch from c5e9c0d to d9f8269 Compare December 11, 2020 12:31

anna-khakimova requested review from OrestChura and rgarnov December 11, 2020 12:38

rgarnov approved these changes Dec 11, 2020

View reviewed changes

OrestChura approved these changes Dec 11, 2020

View reviewed changes

modules/gapi/src/api/kernels_video.cpp Show resolved Hide resolved

modules/gapi/src/api/kernels_video.cpp Outdated Show resolved Hide resolved

OrestChura reviewed Dec 11, 2020

View reviewed changes

modules/gapi/src/api/kernels_video.cpp Outdated Show resolved Hide resolved

alalek reviewed Dec 11, 2020

View reviewed changes

Applied comments.

96741f5

anna-khakimova force-pushed the ak/kalman branch from d9f8269 to 96741f5 Compare December 13, 2020 23:40

alalek approved these changes Dec 14, 2020

View reviewed changes

alalek assigned rgarnov Dec 14, 2020

alalek merged commit 46e275d into opencv:master Dec 14, 2020

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged


		struct KalmanParams

Uh oh!

Conversation

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

Uh oh!

Uh oh!

alalek commented Nov 20, 2020

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 Nov 20, 2020 • 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!

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!

OrestChura left a comment

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!

Uh oh!

Uh oh!

dmatveev left a comment

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!

Uh oh!

Uh oh!

anna-khakimova commented Dec 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anna-khakimova commented Nov 19, 2020 •

edited by alalek

Loading

anna-khakimova Nov 20, 2020 •

edited

Loading