Adds Edge-Preserving Filter #1690

simonreich · 2018-07-12T12:17:27Z

This pullrequest changes

This request adds the Edge-Preserving Filter to the OpenCV libreary as described in
Reich, S. and Wörgötter, F. and Dellen, B. (2018). A Real-Time Edge-Preserving Denoising Filter. Proceedings of the 13th International Joint Conference on Computer Vision, Imaging and Computer Graphics Theory and Applications (VISIGRAPP): Visapp, 85-94, 4. DOI: 10.5220/0006509000850094.

http://fortknox.physik3.gwdg.de/cns/modules/BibtexModule/uploads/PDF/reichwoergoetterdellen2018.pdf

simonreich · 2018-07-12T12:19:34Z

Please advise on further steps. Thank you!

alalek · 2018-07-12T12:29:18Z

Thank you for contribution!

Perhaps this functionality should go to ximgproc module.
Please use this license header (for headers and module sources):

// This file is part of OpenCV project.
// It is subject to the license terms in the LICENSE file found in the top-level directory
// of this distribution and at http://opencv.org/license.html.

Remove license header from the sample files (most part of OpenCV samples are inserted into documentation).
Remove extra indentation from "namespace" blocks
#ifdef __cplusplus is no necessary too in .hpp files.
Please add some simple enough test. You can reuse images from opencv_extra repository.

alalek · 2018-07-12T12:31:56Z

modules/epf/samples/epf_demo.cpp

+
+#include <iostream>
+#include <string>
+#include <opencv2/opencv.hpp>


opencv2/imgcodecs.hpp
opencv2/highgui.hpp
opencv2/ximgproc.hpp

opencv2/imgcodecs.hpp opencv2/opencv.hpp ''' do not seem to be necessary and are not included any more.

alalek · 2018-07-12T12:33:14Z

modules/epf/samples/epf_demo.cpp

+
+int showWindow(cv::Mat image, std::string title);
+
+int showWindow(cv::Mat image, std::string title)


Use "static" to eliminate forward declaration:
static int showWindow(...) { }

Use const reference for input parameters:

const Mat&

const std::string&

showWindow function removed.

alalek · 2018-07-12T12:37:07Z

modules/epf/samples/epf_demo.cpp

+int main(int argc, char **argv)
+{
+	// Help text
+	if (argc != 2)


It is better to use OpenCV's CommandLineParser: similar example

alalek · 2018-07-12T12:40:48Z

modules/epf/samples/epf_demo.cpp

+
+	// Initialize filter. Kernel size 5x5, threshold 20
+	Ptr<epf::edgepreservingFilter> filter =
+	    epf::edgepreservingFilter::create(&image, &res, 5, 20);


Looks like using "filter" object is over-complicated for filtering case. Ptr is not necessary.
It is better to use just a function, like OpenCV's cv::blur.

Please use OpenCV's InputArray / OutputArray types for parameters (they are used for Python/Java bindings generation at least)

I did not know how to implement this properly. It is now a part of ximgproc.

InputArray is used.

No pointers are used.

alalek · 2018-07-12T12:41:33Z

modules/epf/samples/epf_demo.cpp

+
+int showWindow(cv::Mat image, std::string title)
+{
+	namedWindow(title);


namedWindow() is not necessary here.

imshow() implies namedWindow() call

showWindow function removed.

simonreich · 2018-07-12T12:46:34Z

Thanks for the quick reply.

I will close this request and resubmit after I added the above changes.

alalek · 2018-07-12T12:57:07Z

No need to open a new pull request. Just push changes / new commits into your branch "epf" (probably with "--force" option to override patch).

Lets keep discussion in a single place.

…arser.

berak · 2018-07-12T14:07:43Z

modules/epf/include/opencv2/epf.hpp

+             * @param d Diameter of each pixel neighborhood that is used during filtering. Must be greater or equal 3..
+             * @param threshold Threshold, which distinguishes between noise, outliers, and data.
+             */
+            CV_WRAP static Ptr<edgepreservingFilter> create(const cv::Mat *src, cv::Mat *dst, int d, int threshold);


if i may ask: why is this a class ? (maybe having a simple function, at least in the public interface might be better)

it's not doing anything outside of the constructor, and we'd need to create a new object for different images here, right ?

also, please rather use InputArray and OutputArray instead of raw Mat * (which cannot be properly wrapped into python / java)

I did not know how to implement this properly. It is now a part of ximgproc.

InputArray is used.

No pointers are used.

berak · 2018-07-12T14:11:05Z

modules/ximgproc/include/opencv2/ximgproc/edgepreserving_filter.hpp

+    * @param d Diameter of each pixel neighborhood that is used during filtering. Must be greater or equal 3..
+    * @param threshold Threshold, which distinguishes between noise, outliers, and data.
+    */
+    CV_EXPORTS_W void edgepreservingFilter( const InputArray *src, OutputArray *dst, int d, int threshold );


can you try without the pointers ?

CV_EXPORTS_W void edgepreservingFilter( const InputArray src, OutputArray dst, int d, int threshold );

should be enough. (passing cv::Mat by address is usually a bad idea, since it defeats the internal refcounting)

(the buildbot does not like it, either )

berak · 2018-07-12T14:21:45Z

modules/ximgproc/samples/edgepreserving_filter_demo.cpp

+#include <opencv2/highgui.hpp>
+#include <opencv2/ximgproc.hpp>
+#include <opencv2/opencv.hpp>
+#include <opencv2/epf.hpp>


this does not exist, right ? ( #include <opencv2/ximgproc.hpp> should be enough, here)

berak · 2018-07-13T07:35:36Z

modules/ximgproc/src/edgepreserving_filter.cpp

+					     subPosY < subwindow.rows;
+					     subPosY++)
+					{
+						if (meanPixelwiseDist <=


i can't see any difference between the if and the else block.

Historic reasons of this implementation. Is removed in latest commit.

berak · 2018-07-13T07:36:46Z

modules/ximgproc/src/edgepreserving_filter.cpp

+{
+using namespace std;
+
+	edgepreservingFilter(const InputArray *src, OutputArray *dst, int d,


void edgepreservingFilter(...

berak · 2018-07-13T09:41:05Z

modules/ximgproc/src/edgepreserving_filter.cpp

+				cv::GaussianBlur(subwindow1, subwindow,
+				                 cv::Size(5, 5), 0.3, 0.3);
+
+				// compute arithmetic mean of subwindow


Implemented cv::mean instead of manual computation.

…m/opencv/opencv_contrib/master/modules/cvv/.clang-format

simonreich · 2018-07-31T11:30:21Z

Thanks for all the helpful comments and tips! I think I included all of them in the last commits.

I am waiting for the build bot to finish and will check for further issues.

Please let me know what I should be doing next.

alalek · 2018-07-31T11:52:27Z

modules/ximgproc/perf/perf_edgepreserving_filter.cpp

+PERF_TEST_P(EdgepreservingFilterTest, perf,
+            Combine(Values(-20, 0, 10), Values(-100, 0, 20)))
+{
+	/* 3. Get actual test parameters */


Please use 4 spaces

I used https://raw.githubusercontent.com/opencv/opencv_contrib/master/modules/cvv/.clang-format for beautification.

Is there a working .clang-format file (or any other code beautifier)?

The above code should read

PERF_TEST_P(EdgepreservingFilterTest, perf, Combine(Values(-20, 0, 10), Values(-100, 0, 20))) { /* 3. Get actual test parameters */

Instead of

PERF_TEST_P(EdgepreservingFilterTest, perf, Combine(Values(-20, 0, 10), Values(-100, 0, 20))) { /* 3. Get actual test parameters */

Is this correct?

Sorry, I did not notice that the above .clang-format file uses tabs instead of 4 spaces.

Please ignore my last comment.

alalek · 2018-07-31T11:57:28Z

modules/ximgproc/perf/perf_edgepreserving_filter.cpp

+	double threshold = get<1>(params);
+
+	/* 4. Allocate and initialize arguments for tested function */
+	std::string filename = getDataPath("samples/data/corridor.jpg");


Currently test framework doesn't search test images in samples/data directory.

Tests using testdata from opencv_extra repository:
https://github.com/opencv/opencv_extra/tree/master/testdata

Please create patch with image here too (the the same name of branch: "epf")

Is it considered good practice to use https://github.com/opencv/opencv_extra/blob/master/testdata/perf/320x260.png instead of creating my own image?

Thus, no patch would be needed.

Reusing existed test images is a good practice.

alalek · 2018-07-31T11:59:56Z

modules/ximgproc/perf/perf_edgepreserving_filter.cpp

+
+/* 2. Declare the testsuite */
+PERF_TEST_P(EdgepreservingFilterTest, perf,
+            Combine(Values(-20, 0, 10), Values(-100, 0, 20)))


May be we should start from accuracy test and do some checks?
Performance tests have not any checks usually, they just measure performance.

Are there any resources on OpenCV on how to write an accuracy test available? I just found lengthy example code at https://github.com/opencv/opencv/wiki/QA_in_OpenCV.

I added an accuracy test.

However, some more resources would be helpful.

alalek · 2018-07-31T12:00:52Z

modules/ximgproc/perf/perf_edgepreserving_filter.cpp

+	Mat dst(src.size(), src.type());
+
+	/* 5. Manifest your expectations about this test */
+	declare.in(src, WARMUP_RNG).out(dst);


in(src, WARMUP_RNG)

This code overrides your input image. Do not do this.

Removed , WARMUP_RNG.

alalek · 2018-07-31T12:10:19Z

modules/ximgproc/perf/perf_edgepreserving_filter.cpp

+	/* 6. Collect the samples! */
+	TEST_CYCLE_N(1)
+	{
+		ximgproc::edgepreservingFilter(src, dst, kernelSize, threshold);


Please use this pattern:

PERF_SAMPLE_BEGIN(); Canny(img, edges, thresh_low, thresh_high, aperture, useL2); PERF_SAMPLE_END();

alalek · 2018-07-31T12:10:29Z

modules/ximgproc/samples/edgepreserving_filter_demo.cpp

+
+int main(int argc, char **argv)
+{
+	cv::CommandLineParser parser(


Changed '\t ' to " ".

alalek · 2018-07-31T12:10:42Z

modules/ximgproc/src/edgepreserving_filter.cpp

+void edgepreservingFilter(const InputArray _src, OutputArray _dst, int d,
+                          double threshold)
+{
+	Mat src = _src.getMat();


Changed '\t ' to " ".

alalek · 2018-07-31T12:12:40Z

modules/ximgproc/src/edgepreserving_filter.cpp

+			    cv::Rect(posX, posY, subwindowX, subwindowY);
+			subwindow = src(roi);
+			cv::Mat subwindow1 = src(roi);
+			cv::GaussianBlur(subwindow1, subwindow, cv::Size(5, 5),


subwindow = src(roi);

This line seems useless. Due cv::GaussianBlur() call behavior.

Removed subwindow = src(roi);.

alalek · 2018-07-31T12:14:11Z

modules/ximgproc/src/edgepreserving_filter.cpp

+	Mat dst = _dst.getMat();
+	src.copyTo(dst);
+
+	CV_Assert(src.type() == CV_8UC3);


Move this check in the beginning of function. Perhaps via _src.type()

alalek · 2018-07-31T12:15:08Z

modules/ximgproc/include/opencv2/ximgproc/edgepreserving_filter.hpp

+
+namespace cv { namespace ximgproc {
+
+    //! @addtogroup ximgproc


No indentation in namespaces please.

…ation

…ka 'const cv::_InputArray &') has no effect

simonreich · 2018-08-01T10:30:35Z

I worked on all of the above issues. One question remains: Currently, there is one performance test and one accuracy test. Which one should be deleted?

alalek

Which one should be deleted?

No one. It is OK now.
Accuracy and performance tests have different purposes.

Patch looks good to me. Please check the last one comment below.

alalek · 2018-08-01T13:13:42Z

modules/ximgproc/include/opencv2/ximgproc/edgepreserving_filter.hpp

+    * @param d Diameter of each pixel neighborhood that is used during filtering. Must be greater or equal 3.
+    * @param threshold Threshold, which distinguishes between noise, outliers, and data.
+    */
+    CV_EXPORTS_W void edgepreservingFilter( InputArray src, OutputArray dst, int d, double threshold );


edgepreservingFilter

Rename to "edgePreservingFilter". See naming guide for details.

BTW, OpenCV already has some edgePreservingFilter() function (from photo module). But there is no huge problem due different namespaces.

simonreich · 2018-08-02T10:56:25Z

Thanks for all the help! If I can be of further assistance, please do not hesitate to contact me.

alalek

Well done! Looks good to me 👍

Simon Reich added 3 commits July 12, 2018 14:02

Module EPF - Edge-Preserving Filter added

e23e838

Changed name from template to epf

3891dd5

Removed clang-format file

538049f

alalek reviewed Jul 12, 2018

View reviewed changes

simonreich closed this Jul 12, 2018

Simon Reich added 2 commits July 12, 2018 15:13

Added header Files. Eliminated showWindow function. Used CommandLineP…

0d96f6d

…arser.

Moved filter from epf module to ximgproc

c272800

berak reviewed Jul 12, 2018

View reviewed changes

Removed header files from sample

23d45a6

berak reviewed Jul 13, 2018

View reviewed changes

Simon Reich added 8 commits July 13, 2018 12:47

Minor bug fix in demo. Pointers in demo removed.

82dfa23

Pointers removed. InputArray/OutputArray added

88ce7a7

License header added

1f115b6

License header from sample file removed

19686de

Unit test for performance added

9cc391b

Replaced manual mean computation with cv::mean

c411b39

Beautified code via clang-format and https://raw.githubusercontent.co…

55766d9

…m/opencv/opencv_contrib/master/modules/cvv/.clang-format

Merged historic if... else if statement into one if statement

7e89c3d

alalek reviewed Jul 31, 2018

View reviewed changes

Simon Reich added 4 commits August 1, 2018 08:17

Trailing whitespace removed and .. changed into .

1b079bd

Tabs replaced with 4 spaces.

16ee637

Removed subwindow = src(roi);

3216c78

Moved type test to beginning of code

5fa0aba

Simon Reich added 7 commits August 1, 2018 09:05

Removed indentation from namespace and added //! @}

845d807

Added name to header

c6ea43f

git cleanup introduced some errors fixed here

a91e629

Changed path testdata/perf/320x260.png to perf/320x260.png

7e7d108

Fixed warning declaration of 'subwindow1' hides previous local declar…

acb5b27

…ation

Fixed warning 'const' qualifier on reference type 'cv::InputArray' (a…

c7e4c3a

…ka 'const cv::_InputArray &') has no effect

Accuracy test added/

fee6573

alalek reviewed Aug 1, 2018

View reviewed changes

Renamed void edgepreservingFilter to void edgePreservingFilter

5795c0a

alalek approved these changes Aug 2, 2018

View reviewed changes

alalek merged commit 9695384 into opencv:master Aug 2, 2018

simonreich deleted the epf branch August 6, 2018 06:14


		int showWindow(cv::Mat image, std::string title);

		int showWindow(cv::Mat image, std::string title)

Adds Edge-Preserving Filter #1690

Adds Edge-Preserving Filter #1690

Conversation

simonreich commented Jul 12, 2018

This pullrequest changes

simonreich commented Jul 12, 2018

alalek commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonreich commented Jul 12, 2018

alalek commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berak Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berak Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonreich commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonreich commented Aug 1, 2018

alalek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonreich commented Aug 2, 2018

alalek left a comment

Choose a reason for hiding this comment

berak Jul 12, 2018 •

edited

Loading

berak Jul 12, 2018 •

edited

Loading