GAPI: New BackgroundSubtractor stateful kernel by anna-khakimova · Pull Request #18674 · opencv/opencv

anna-khakimova · 2020-10-27T21:39:41Z

New BackgroundSubtractor kernel represents 2 types of operation such as:

Background Subtractor MOG2 (default);
Background Subtractor KNN

Published for review on 27th of October.

@dmatveev , @OrestChura , please take a look.


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
Xbuild_image:Custom Mac=openvino-2020.3.0

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

alalek

/cc @dmatveev

alalek · 2020-10-30T18:15:59Z

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

+@param src input image: 8-bit unsigned 3-channel image @ref CV_8UC3.
+@sa RGB2BGR
+*/
+GAPI_EXPORTS GMat BGR2RGB(const GMat& src);


Consider adding RGB2BGR too (as inlines alias for example).

/cc @dmatveev

I don't understand your proposal. Please explain.

alalek · 2020-10-30T18:18:25Z

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

-
 //! @} gapi_filters

 //! @addtogroup gapi_colorconvert


Perhaps it is better to move these doxygen headers instead of the function.
Keep patches small.

How is this change related to this PR? Each PR should have single clear purpose.

I noticed that this description accidentally was put into the wrong documentation group and I corrected that. Nothing criminal. This is my code from the previous PR. It seems to me that we shouldn't approach this issue so formally.

alalek · 2020-10-30T18:23:27Z

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

+
+@sa BackSubMOG2
+*/
+GAPI_EXPORTS GMat BackSubMOG2(const GMat& src);


OpenCV implementation of this algorithm has several parameters. Why do we lose them?

createBackgroundSubtractorMOG2(int history=500, double varThreshold=16, bool detectShadows=true);

BackSubMOG2

Do we really want to obfuscate function name?

@sa BackSubMOG2

Points on that? on this function again?

alalek · 2020-10-30T18:25:13Z

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

+
+    static void run(const cv::Mat& in, cv::Mat &out, cv::BackgroundSubtractorMOG2& state)
+    {
+        state.apply(in, out, -1);


double learningRate

Lost this parameter too.

asmorkalov · 2020-11-12T10:56:59Z

@anna-khakimova Friendly reminder.

OrestChura

Great!

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

modules/gapi/include/opencv2/gapi/imgproc.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/test/common/gapi_video_tests_common.hpp

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

OrestChura

Almost nothing left, some minor suggestions

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

modules/gapi/test/common/gapi_video_tests_inl.hpp

modules/gapi/test/common/gapi_video_tests_common.hpp

anna-khakimova · 2020-11-26T09:27:56Z

@alalek please take a look one more.

alalek · 2020-11-26T10:50:39Z

modules/gapi/test/cpu/gapi_video_tests_cpu.cpp

+                                             Values(500, 400, 300, 550),
+                                             Values(true, false),
+                                             Values(-1, 0, 0.5, 1),
+                                             Values("cv/video/768x576.avi")));


[----------] 256 tests from BackgroundSubtractorTestCPU/BackgroundSubtractorTest (413881 ms total)

This one test case consumes the 2x times more than other 30+k G-API tests.

Keep tests fast.
Do not enable tests if they don't increase code coverage.

TODO:

please reduce amount of enabled test cases (you may want to create "full" case for development disabled by default).

reduce amount of processed frames (no need to process all 100 frames of video, 3-5 frames should be enough)

/cc @dmatveev

modules/gapi/test/common/gapi_video_tests_common.hpp

alalek · 2020-11-26T10:52:46Z

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

+
+namespace cv { namespace detail {
+        template<> struct CompileArgTag<cv::gapi::video::BackgroundSubtractorParams>
+        {


avoid indentation in namespaces

alalek · 2020-11-26T10:54:52Z

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

+    double threshold = 16; //!< For MOG2: Threshold on the squared Mahalanobis distance between
+                           //!< the pixel and the model to decide whether a pixel is well described by the background model.
+                           //!< For KNN: Threshold on the squared distance between the pixel and the sample to decide
+                           //!< whether a pixel is close to that sample.


It is better to keep multi-line comments before declaration to avoid excessive indentations.

I did as you said on the previous step, but doxygen misinterpreted the comments in this arrangement. @OrestChura can confirm my words. He also observed this behavior. I can prove it by rolling back, but I don't want to waste time on it. So I would rather leave it as it is now.

misinterpreted the comments
//!<

Please learn how to use doxygen comments: https://www.doxygen.nl/manual/docblocks.html

Ok. Thanks for prompt. Changed

alalek · 2020-11-26T10:55:30Z

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

 } // namespace gapi
 } // namespace cv

-


unnecessary change

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

anna-khakimova · 2020-11-26T18:26:25Z

@OrestChura please take a look one more time. And if it's ok please remove change request and approve.

OrestChura

I don't see any other obstacles to merge it! Thank you!

anna-khakimova · 2020-11-26T19:43:11Z

@alalek please check.

alalek · 2020-11-30T00:47:43Z

modules/gapi/test/common/gapi_video_tests_inl.hpp

+    try
+    {
+        gapiBackSub.setSource(gapi::wip::make_src<cv::gapi::wip::GCaptureSource>
+                                                      (findDataFile(filePath)));
+    }
+    catch (...)
+    { throw SkipTestException("Video file can't be opened."); }


Please move out findDataFile call from try-catch block. We don't want to suppress its exceptions.

catch (...)

it make sense to capture const cv::Exception& only.
/cc @mpashchenkov

alalek

LGTM

…ractor GAPI: New BackgroundSubtractor stateful kernel * New BackgroundSubtractorMOG2 kernel * Add BS parameters

anna-khakimova force-pushed the ak/backgroundSubtractor branch 4 times, most recently from 706297c to 63181be Compare October 28, 2020 01:30

anna-khakimova changed the title ~~New BackgroundSubtractorMOG2 kernel~~ GAPI: Add new BackgroundSubtractorMOG2 stateful kernel Oct 28, 2020

alalek reviewed Oct 30, 2020

View reviewed changes

anna-khakimova force-pushed the ak/backgroundSubtractor branch 12 times, most recently from e551028 to f83166d Compare November 17, 2020 23:26

OrestChura reviewed Nov 18, 2020

View reviewed changes

OrestChura added category: g-api / gapi feature test labels Nov 18, 2020

OrestChura reviewed Nov 18, 2020

View reviewed changes

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

anna-khakimova force-pushed the ak/backgroundSubtractor branch 6 times, most recently from 07966fe to cd914d1 Compare November 18, 2020 08:53

anna-khakimova force-pushed the ak/backgroundSubtractor branch 2 times, most recently from 0e65bae to 4b6636e Compare November 25, 2020 23:09

anna-khakimova requested a review from OrestChura November 25, 2020 23:11

OrestChura reviewed Nov 25, 2020

View reviewed changes

anna-khakimova force-pushed the ak/backgroundSubtractor branch from 4b6636e to 9bc1f67 Compare November 26, 2020 00:32

anna-khakimova requested a review from alalek November 26, 2020 08:46

alalek reviewed Nov 26, 2020

View reviewed changes

anna-khakimova force-pushed the ak/backgroundSubtractor branch 3 times, most recently from c1fb3a5 to e5ed281 Compare November 26, 2020 17:35

OrestChura reviewed Nov 26, 2020

View reviewed changes

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

anna-khakimova force-pushed the ak/backgroundSubtractor branch 3 times, most recently from c0b2f0c to 0bcc830 Compare November 26, 2020 18:21

anna-khakimova requested a review from OrestChura November 26, 2020 18:23

OrestChura approved these changes Nov 26, 2020

View reviewed changes

anna-khakimova requested a review from alalek November 26, 2020 19:41

alalek reviewed Nov 30, 2020

View reviewed changes

anna-khakimova force-pushed the ak/backgroundSubtractor branch 2 times, most recently from 88eca7f to 5d66c72 Compare November 30, 2020 10:52

Add BS parameters

8ca1f57

anna-khakimova force-pushed the ak/backgroundSubtractor branch from 5d66c72 to 8ca1f57 Compare November 30, 2020 15:36

alalek approved these changes Nov 30, 2020

View reviewed changes

alalek assigned OrestChura Nov 30, 2020

alalek merged commit 56568da into opencv:master Nov 30, 2020

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged

a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023

Merge pull request opencv#18674 from anna-khakimova:ak/backgroundSubt…

bab6906

…ractor GAPI: New BackgroundSubtractor stateful kernel * New BackgroundSubtractorMOG2 kernel * Add BS parameters

Uh oh!

Conversation

anna-khakimova commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek left a comment

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!

alalek Oct 30, 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!

asmorkalov commented Nov 12, 2020

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!

Uh oh!

OrestChura left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anna-khakimova commented Nov 26, 2020

Uh oh!

alalek Nov 26, 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!

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!

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

Uh oh!

anna-khakimova commented Oct 27, 2020 •

edited

Loading

alalek Oct 30, 2020 •

edited

Loading

OrestChura left a comment •

edited

Loading

alalek Nov 26, 2020 •

edited

Loading

anna-khakimova commented Nov 26, 2020 •

edited

Loading

OrestChura left a comment •

edited

Loading