Deriche Filter #701

LaurentBerger · 2016-06-05T18:01:42Z

Add deriche Filter implementation

stereomatchingkiss · 2016-06-15T18:54:48Z

Hi, may I ask you what are the advantages/disadvantages of deriche Filter?When should I use it?What is it for?Blurring?Sharpening?

I use Gaussian filter to replace deriche Filter in my pull request #688(Marr-Hildreth hash). Do you think it is applicable?Thank you very much

LaurentBerger · 2016-06-15T19:08:47Z

About deriche filter you can find paper here You can try it here Difference with other filters is implemantation using IIR.

ilya-lavrenov · 2016-07-14T10:26:40Z

modules/ximgproc/include/opencv2/ximgproc/deriche_filter.hpp

+*
+* @sa GradientDericheX, GradientDericheY
+*/
+CV_EXPORTS UMat GradientDericheY(UMat op, double alphaDerive,double alphaMean);


I expect the signature of the function as following:

void GradientDericheY(InputArray src, OutputArray dst, double alphaDerive,double alphaMean);

Why do you use UMat directly? If your implementation does not imply OpenCL optimizations, I suggest not to use UMats inside the library.

mshabunin

@LaurentBerger , please fix last comments by @ilya-lavrenov

I've found some uninitialized memory access, could you please check?

use following example:

#include "opencv2/core.hpp"
#include "opencv2/imgcodecs.hpp"
#include "opencv2/ximgproc/deriche_filter.hpp"

using namespace cv;
using namespace cv::ximgproc;

int main(int, char* argv[])
{
    Mat img = imread(argv[1]);
    Mat rx;
    GradientDericheX(img, rx, 4, 4);
    double a = 0, b = 0;
    minMaxLoc(rx, &a, &b);
    return 0;
}

build OpenCV (-DWITH_OPENCL=OFF -DWITH_IPP=OFF) and app in Debug mode
run valgrind --track-origins=yes app img.png
get several warnings:

==22260== Conditional jump or move depends on uninitialised value(s)
==22260==    at 0x591C391: void cv::minMaxIdx_<float, float>(float const*, unsigned char const*, float*, float*, unsigned long*, unsigned long*, int, unsigned long) (stat.cpp:1893)
==22260==    by 0x590A8E9: cv::minMaxIdx_32f(float const*, unsigned char const*, float*, float*, unsigned long*, unsigned long*, int, unsigned long) (stat.cpp:1946)
==22260==    by 0x590AE02: cv::minMaxIdx(cv::_InputArray const&, double*, double*, int*, int*, cv::_InputArray const&) (stat.cpp:2375)
==22260==    by 0x590B109: cv::minMaxLoc(cv::_InputArray const&, double*, double*, cv::Point_<int>*, cv::Point_<int>*, cv::_InputArray const&) (stat.cpp:2410)
==22260==    by 0x4016C1: main (deriche_demo.cpp:14)
==22260==  Uninitialised value was created by a heap allocation
==22260==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22260==    by 0x57DEAD1: cv::fastMalloc(unsigned long) (alloc.cpp:64)
==22260==    by 0x59273F8: cv::StdMatAllocator::allocate(int, int const*, int, void*, unsigned long*, int, cv::UMatUsageFlags) const (matrix.cpp:192)
==22260==    by 0x59284B5: cv::Mat::create(int, int const*, int) (matrix.cpp:426)
==22260==    by 0x593611E: cv::_OutputArray::create(int, int const*, int, int, bool, int) const (matrix.cpp:2510)
==22260==    by 0x58CD14E: cv::split(cv::_InputArray const&, cv::_OutputArray const&) (convert.cpp:199)
==22260==    by 0x4E7C000: cv::ximgproc::GradientDericheX(cv::_InputArray const&, cv::_OutputArray const&, double, double) (deriche_filter.cpp:561)
==22260==    by 0x40164E: main (deriche_demo.cpp:12)

mshabunin · 2016-11-17T15:35:03Z

modules/ximgproc/src/deriche_filter.cpp

+            break;
+            case CV_16S:
+            {
+                short *c1;


Looks like all processing blocks can be implemented with single C++ template function.

LaurentBerger · 2016-11-18T19:31:38Z

I use now template function. My platform is windows so I haven't got valgrind. I have found https://github.com/DynamoRIO/drmemory/wiki/Downloads. I have just try and I cannot found memory leaks.

alalek · 2016-11-30T12:09:37Z

modules/ximgproc/include/opencv2/ximgproc/deriche_filter.hpp

+ *  Redistribution and use in source and binary forms, with or without modification,
+ *  are permitted provided that the following conditions are met :
+ *
+ *  *Redistributions of source code must retain the above copyright notice,


There is missing "space" before "Redistributions".

BTW, You can use short version of license header:

// 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

alalek · 2016-11-30T12:19:43Z

modules/ximgproc/src/deriche_filter.cpp

+
+namespace cv {
+namespace ximgproc {
+    template<typename T>


Please avoid extra indentation in namespaces.

alalek · 2016-11-30T12:21:14Z

modules/ximgproc/src/deriche_filter.cpp

@@ -0,0 +1,410 @@
+#include "precomp.hpp"


Please add license header into this source file (license header is not required for samples, and small utility files like precomp.hpp).

Please resolve this item too.
Also please check other license headers in your patch for missing space after * symbol:

*Redistributions of source code

alalek · 2016-11-30T12:25:50Z

modules/ximgproc/src/deriche_filter.cpp

+        std::vector<Mat> planTmp;
+        split(tmp, planTmp);
+        std::vector<Mat> planDst;
+        split(_dst, planDst);


split calls for tmp (not used at all) and dst are useless.
Just create Mat elements for std::vector<Mat>.

I don't understand I use it at line https://github.com/LaurentBerger/opencv_contrib/blob/DericheFilter/modules/ximgproc/src/deriche_filter.cpp#L401

I mean, you don't need split here (it copies pixels). You just need to create Mat, something like this:

std::vector<Mat> planTmp; std::vector<Mat> planDst; for (size_t i = 0; i < planSrc.size(); i++) { planTmp.push_back(Mat(_op.size(), CV_32FC1)); planDst.push_back(Mat(_op.size(), CV_32FC1)); }

alalek · 2016-11-30T12:26:59Z

modules/ximgproc/src/deriche_filter.cpp

+        _dst.create( _op.size(),CV_32FC(tmp.channels()) );
+        Mat opSrc = _op.getMat();
+        std::vector<Mat> planSrc;
+        split(opSrc, planSrc);


You can pass InputArray directly into split function:
opSrc -> _op

alalek · 2016-11-30T12:29:22Z

modules/ximgproc/samples/deriche_demo.cpp

+
+int alDerive=100;
+int alMean=100;
+Mat img;


Global Mat objects may not work properly in some cases. To resolve such problems use cv::Ptr<Mat> and allocate Mat in the main.

Ok I have changed code but I don't understand why.

alalek · 2016-11-30T12:30:48Z

modules/ximgproc/src/deriche_filter.cpp

+namespace cv {
+namespace ximgproc {
+    template<typename T>
+    static void VerticalIIRFilter(Mat img,Mat dst,const Range r,T *c1,double alphaDerive)


const Mat& img, Mat& dst, const Range& r

alalek · 2016-11-30T12:37:45Z

modules/ximgproc/src/deriche_filter.cpp

+            int i = 0;
+            g1[i] = (a1 + a2)* *c1;
+            i++;
+            c1 += cols;


c1 += cols;
This works for contiguous matrix only.

Probably you need this: T value = img.at<T>(row, col);

I have tested it here https://github.com/LaurentBerger/opencv_contrib/blob/DericheFilter/modules/ximgproc/src/deriche_filter.cpp#L399?

alalek · 2016-11-30T12:38:57Z

modules/ximgproc/src/deriche_filter.cpp

+namespace cv {
+namespace ximgproc {
+    template<typename T>
+    static void VerticalIIRFilter(Mat img,Mat dst,const Range r,T *c1,double alphaDerive)


Looks like passed argument c1 is not used anywhere.

I agree, it will be better to make this variable local and call the function with explicit template specification: VerticalIIRFilter<uchar>(....

LaurentBerger · 2016-12-07T19:28:01Z

@alalek do you want that i add space after * in all files ?

    modified:   modules/ximgproc/include/opencv2/ximgproc.hpp
    modified:   modules/ximgproc/include/opencv2/ximgproc/disparity_filter.h                                                                   pp
    modified:   modules/ximgproc/include/opencv2/ximgproc/edge_filter.hpp
    modified:   modules/ximgproc/include/opencv2/ximgproc/paillou_filter.hpp
    modified:   modules/ximgproc/include/opencv2/ximgproc/sparse_match_inter                                                                   polator.hpp
    modified:   modules/ximgproc/src/deriche_filter.cpp
    modified:   modules/ximgproc/src/disparity_filters.cpp
    modified:   modules/ximgproc/src/precomp.hpp

alalek · 2016-12-07T22:20:03Z

No, please use fixed license header for file in your patch only. It is to preserve conflicts, I will fix others soon.

LaurentBerger · 2016-12-20T17:58:16Z

@alalek I am writting a book about opencv (in french of course). I am starting a chapter about segmentation. I need to know if this PR is accepted (not pushed now I can work with my own branch) or not before process Canny example.

alalek · 2016-12-20T20:40:46Z

@LaurentBerger I don't see strict blockers on this PR if there is no patent/license conflicts.
But before merge you need to rebase your commits (to resolve pending conflicts with current master branch):

Conflicting files:
modules/ximgproc/README.md
modules/ximgproc/include/opencv2/ximgproc.hpp

(If you have some problems with git I have rebased your changes from this PR here(with minor readme updates))

P.S. It will be great to have some tests for this code (like a sample, but without user interaction), but this can be done later in the next PRs.

LaurentBerger · 2016-12-21T10:26:54Z

this PR if there is no patent/license conflicts.

I don't think so because it is too old but I sent a message to author. I'm waiting his reply.

I have rebased your changes from this PR here
How can I use your rebase ?

alalek · 2016-12-21T13:24:46Z

You can use something like these:

# commit/stash your pending changes somewhere

# make backup
git branch DericheFilter_20161221 DericheFilter

git fetch https://github.com/alalek/opencv_contrib pr701
git checkout -B DericheFilter FETCH_HEAD

alalek · 2016-12-23T09:56:06Z

@LaurentBerger Typo is my bad. Do you plan to add something or we can merge this PR as is?

LaurentBerger · 2016-12-23T11:04:43Z

Not now. i will make another PR about a test (without data needed) for this code as you proposed.
This PR 701 is ready.

About opencv 3.2 I have another PR ready to pull about Paillou filter which included all remarks in deriche PR (license, missing space about * and template function and global variable Ptr ) https://github.com/LaurentBerger/opencv_contrib/blob/ImprovePaillou/modules/ximgproc/src/paillou_filter.cpp

alalek · 2016-12-23T11:09:15Z

Thank you! 👍

ilya-lavrenov self-assigned this Jul 14, 2016

ilya-lavrenov reviewed Jul 14, 2016
View reviewed changes

LaurentBerger force-pushed the DericheFilter branch from a8cd301 to 76e6ab7 Compare August 11, 2016 08:22

mshabunin reviewed Nov 17, 2016

View reviewed changes

alalek reviewed Nov 30, 2016

View reviewed changes

alalek unassigned ilya-lavrenov Dec 16, 2016

Diriche Filter

e96e3bc

LaurentBerger force-pushed the DericheFilter branch from c7f5437 to e96e3bc Compare December 21, 2016 14:00

Typo

4c164dd

alalek merged commit 8634252 into opencv:master Dec 23, 2016

LaurentBerger deleted the DericheFilter branch December 23, 2016 17:26

Deriche Filter #701

Deriche Filter #701

Uh oh!

Conversation

LaurentBerger commented Jun 5, 2016

Uh oh!

stereomatchingkiss commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaurentBerger commented Jun 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshabunin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurentBerger commented Nov 18, 2016

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!

LaurentBerger Nov 30, 2016 • 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!

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!

LaurentBerger commented Dec 7, 2016

Uh oh!

alalek commented Dec 7, 2016

Uh oh!

LaurentBerger commented Dec 20, 2016

Uh oh!

alalek commented Dec 20, 2016

Uh oh!

LaurentBerger commented Dec 21, 2016

Uh oh!

alalek commented Dec 21, 2016

Uh oh!

stereomatchingkiss commented Jun 15, 2016 •

edited

Loading

LaurentBerger Nov 30, 2016 •

edited

Loading

LaurentBerger commented Dec 23, 2016 •

edited

Loading