Extendible parameters in OpenCV functions

[vpisarev]

The discussion thread for https://github.com/opencv/opencv/wiki/OE-34.-Named-Parameters

[vpisarev]

This is a toy example, which was tested on macOS 10.15.6 with Apple's Clang 11 and GCC 9.2.0 from hpc.sourceforge.net. It should compile and run with any C++ 11 compiler; with C++14 or later it uses aggregate initialization feature

Update: in Visual Studio "/std:c++latest" option should be set in order to use the proposed method of passing parameters. However, the parameter declaration and the function itself compile fine when older standards are used.

Update 2: added 'call chain' method to set properties in the case of older standards. It's less crisp than C++ 20 version, but shorter than the manual parameter initialization one by one. Hopefully, compilers can optimize such a chain of calls and will not copy the structure each time. But in any case it's very little overhead

Update 3: added diagnostic messages, which method we use, and also defined a helper macro to illustrate how it could have been used in OpenCV

#include <assert.h>
#include <stdio.h>
#include <inttypes.h>
#include <math.h>
#include <limits>
#include <string>

template<typename T> struct matrix_t
{
    matrix_t() = default;
    ~matrix_t()
    {
        release();
    }
    matrix_t(const matrix_t& m)
        : rows(m.rows), cols(m.cols),
        data(m.data), refcount(m.refcount)
    {
        if(refcount)
            ++*refcount;
    }
    matrix_t(int rows_, int cols_)
    {
        init();
        create(rows_, cols_);
    }
    matrix_t(int rows_, int cols_, std::initializer_list<T> elems)
    {
        init();
        create(rows_, cols_);
        assert((size_t)rows_*cols_ == elems.size());
        
        T* ptr = data;
        for( auto x: elems )
            *ptr++ = x;
    }
    matrix_t& operator = (const matrix_t& m)
    {
        if(this != &m)
        {
            release();
            rows = m.rows; cols = m.cols;
            data = m.data; refcount = m.refcount;
            if(refcount)
                ++*refcount;
        }
        return *this;
    }
    void init()
    {
        data = 0;
        refcount = 0;
        rows = 0; cols = 0;
    }
    void release()
    {
        if(refcount && --*refcount == 0)
        {
            if(data)
                delete[] data;
            delete refcount;
        }
        init();
    }
    void create(int rows_, int cols_)
    {
        assert(rows_ >= 0 && cols_ >= 0);
        if(rows == rows_ && cols == cols_)
            return;
        release();
        int total = rows_*cols_;
        rows = rows_; cols = cols_;
        if(total > 0)
            data = new T[rows_*cols_];
        else
            data = 0;
        refcount = new int(1);
    }
    int rows = 0;
    int cols = 0;
    T* data = 0;
    int* refcount = 0;
};

enum BorderMode
{
    BORDER_REPLICATE = 0,
    BORDER_REFECT_101 = 1,
    BORDER_WRAP = 2,
    BORDER_CONSTANT = 3
};

struct filter2DParams
{
    int anchor_x = -1;
    int anchor_y = -1;
    BorderMode border_mode = BORDER_REPLICATE;
    double border_value = 0.;
    double scale = 1.;
    double shift = 0.;

    filter2DParams& _anchor_x(int ax) { anchor_x = ax; return *this; }
    filter2DParams& _anchor_y(int ay) { anchor_y = ay; return *this; }
    filter2DParams& _border_mode(BorderMode bm) { border_mode = bm; return *this; }
    filter2DParams& _border_value(double bv) { border_value = bv; return *this; }
    filter2DParams& _scale(double sc) { scale = sc; return *this; }
    filter2DParams& _shift(double sh) { shift = sh; return *this; }
};

template<typename T> static inline T saturate_cast(double v)
{
    int iv = (int)round(v);
    const T minval = std::numeric_limits<T>::min();
    const T maxval = std::numeric_limits<T>::max();
    return iv < minval ? minval : iv > maxval ? maxval : (T)iv;
}

template<typename T1, typename T2>
    void filter2D(const matrix_t<T1>& src, matrix_t<T2>& dst,
                  const matrix_t<float>& kernel,
                  const filter2DParams& params=filter2DParams())
{
    int rows = src.rows, cols = src.cols, krows = kernel.rows, kcols = kernel.cols;
    int anchor_x = params.anchor_x, anchor_y = params.anchor_y;
    int border_mode = params.border_mode;
    double border_value = params.border_value;
    double scale = params.scale, shift = params.shift;
    
    if (anchor_x < 0)
        anchor_x = kcols/2;
    if (anchor_y < 0)
        anchor_y = krows/2;
    
    dst.create(rows, cols);
    
    const T1* srcdata = src.data;
    T2* dstdata = dst.data;
    const float* kdata = kernel.data;
    
    for( int i = 0; i < rows; i++ )
        for( int j = 0; j < cols; j++ )
    {
        double s = 0;
        for(int ki = 0; ki < krows; ki++)
            for(int kj = 0; kj < kcols; kj++)
        {
            int sy = i + ki - anchor_y;
            int sx = j + kj - anchor_x;
            double v;

            if( (unsigned)sx < (unsigned)cols &&
                (unsigned)sy < (unsigned)rows )
                v = srcdata[sy*cols + sx];
            else
            {
                if(border_mode == BORDER_REPLICATE)
                {
                    sx = sx < 0 ? 0 : sx >= cols ? cols-1 : sx;
                    sy = sy < 0 ? 0 : sy >= rows ? rows-1 : sy;
                    v = srcdata[sy*cols + sx];
                }
                else
                {
                    assert(border_mode == BORDER_CONSTANT);
                    v = border_value;
                }
            }
                
            s += kdata[ki*kcols + kj]*v;
        }
        dstdata[i*cols + j] = saturate_cast<T2>(s*scale + shift);
    }
}

template<typename T>
static void randu(matrix_t<T>& m, int a, int b)
{
    for( int i = 0; i < m.rows; i++ )
        for( int j = 0; j < m.cols; j++ )
            m.data[i*m.cols + j] = (T)(rand()%(b - a + 1) + a);
}

template<typename T>
static void print(const char* mname, const matrix_t<T>& m)
{
    printf("\n%s:\n\t", mname);
    for( int i = 0; i < m.rows; i++ )
        for( int j = 0; j < m.cols; j++ )
    {
        // assume matrix is integer
        printf("%3d%s", (int)m.data[m.cols*i + j],
            j < m.cols-1 ? ", " : i < m.rows - 1 ?"\n\t" : "\n");
    }
}

#if __cplusplus > 201703L || \
    ((defined __GNUC__ || defined __clang__) && __cplusplus >= 201402L) || \
    _MSVC_LANG > 201703L
    #define CV_USE_NAMED_ARGS 1
#else
    #undef CV_USE_NAMED_ARGS
#endif

int main(int, char**)
{
    matrix_t<uint8_t> a(5, 5);
    matrix_t<uint8_t> b, b0;
    matrix_t<float> k(3, 3, {1.f, 2.f, 1.f, 2.f, 4.f, 2.f, 1.f, 2.f, 1.f});
    
    randu(a, 0, 9);
    filter2D(a, b0, k); // call with all default parameters

#if CV_USE_NAMED_ARGS
    printf("using 'named args'...\n");
    filter2D(a, b, k, {.border_mode=BORDER_CONSTANT, .border_value=5, .scale=1./16});
#elif 0
    printf("using direct parameter assignment...\n");
    filter2DParams p;
    p.border_mode = BORDER_CONSTANT;
    p.border_value = 5;
    p.scale = 1./16;
    filter2D(a, b, k, p);
#else
    printf("using 'call chain' method...\n");
    filter2D(a, b, k, filter2DParams().
        _border_mode(BORDER_CONSTANT).
        _border_value(5).
        _scale(1./16));
#endif
    
    print("a", a);
    print("b0", b0);
    print("b", b);
    
    return 0;
}

[tompollok]

That looks very promising to me. Especcially because it allows C++20 users to have a more compact function calls.

What about optional output arrays that have default initializations? Seems like outputarrays will not work for this. However they can simply be added as default parameters to the function arguments as its currently done i guess.

I think that this solution will help with reducing the number of different function overloads to some degree, however it is not possible to completely get rid of multiple overloads. For example in findEssentialMat, most of the times it is handled as if it its the essential matrix between two frames of the same camera, but sometimes users want to specify that it is from two different cameras. So adding the intrinsic calibration parameters for two differnt cameras to the struct could be possible, but i feel like it should be only used for a large overlap. So in the future, should these functions be named slightly different or keeping multiple overloads? I guess some guidelines would then be useful for contributors.

I personally currently make use of a implementation of a heterogeneous container (to enable creating plugins that have just a single function with single argument), into which i can store any amount of parameters with type information, value and a key at runtime. So you can dynamically pass any combinations of arguments to the function. This can be really handy for creating pipelines but seems not the right choice for opencv.

[vpisarev]

@tompollok, thank you!

I think, optional OutputArray and such can be specified using this method:

// option 1
struct EssentialMatrixParams
{
      OutputArray inliers = OutputArray();
      int method = RANSAC;
      double tolerance = 1.0;
};
/*
   usage:
    vector<int> inliers;
    Mat E = findEssentialMatrix(points1, points2, camera1, dist1, camera2, dist2, {.inliers = inliers, .tolerance = 3.0 });
*/

// option 2
struct EssentialMatrixParams
{
      vector<int>* inliers = 0;
      int method = RANSAC;
      double tolerance = 1.0;
};
/*
   usage:
    vector<int> inliers;
    Mat E = findEssentialMatrix(points1, points2, camera1, dist1, camera2, dist2, {.inliers = &inliers, .tolerance = 3.0 });
*/

This approach will not completely eliminate the need for overloaded functions, of course, but will greatly reduce it. Most of the time it can be just overloads, in rare cases it may require different function names. Heterogeneous container solution is one of the option, of course, and we tried it (still use for layer parameters in OpenCV DNN), but it's convenient when you have a whole hierarchy of classes with mostly similar parameters. If you have a flat library of different algorithms, such approach may be too heavy. Besides, there is little support from compiler and IDE. Different usage models need different approaches.

[tompollok]

Ok the provided essential mat example looks good to me. I think your proposal looks sound and the way to go. I also like that there is no strict ordering of the parameters anymore.

[vpisarev]

unfortunately, C++ 20 standard and the actual implementations require that the declaration order of the 'params' structure fields and their order in the aggregate initialiser do match. One of the solutions, as suggested above, is to require alphabetically sorted order. But it's better than optional parameters, of course, because 1) you see which parameters get which values, i.e. it's self-documenting and 2) you can indeed skip some parameters that you want to keep default.

[catree]

No idea what would be the best solution in C++. I am not a C++ expert.

---

From an user point of view, a Python like syntax, way to call the functions, looks like a good improvement. What about bindings with other languages (Python, Java, ...)?

Wondering:

• how this kind of issues are solved for other big libraries?
• does it mean that sometimes new code are merged too quickly in OpenCV while an emphasis on API quality should be privileged?

Few examples:

• how an user could know which one to use between fitEllipse(), fitEllipseAMS() and fitEllipseDirect()? Can you guess that it will do n == 5 ? fitEllipseDirect(points) : fitEllipseNoDirect(points);? There are some fallbacks in fitEllipseAMS?
• which one to use between calibrateCamera() and calibrateCameraRO()? Can you guess that internally calibrateCamera() is equivalent to calibrateCameraRO()?
• some mess (I have contributed to introduce) between: solveP3P(), solvePnP(), solvePnPGeneric()
• what is the difference between findHomography() and getPerspectiveTransform()? Maybe getPerspectiveTransform() is better when using only 4 points, but I doubt a newcomer can see the difference. Maybe getPerspectiveTransform() naming is more explicit, I don't know?
• I think it is time to deprecate getPerspectiveTransform() with const Point2f | src[] interface? At least it is explicit that Point2d is expected. Not so much on the number of points.
• there are other examples if you look carefully

Probably some of these "issues" come from the natural life cycle of a library, like introducing new methods. But I think also a better emphasis on the API quality could avoid some of these issues. At least clear rules and guidelines are needed.

Because I am sure there are lots of functions that are not really used, and potentially have some bugs. It is worse with OpenCV contrib where I believe a large part of the code are not really used.

[diablodale]

The Named Parameter idiom (NPI) and method chaining is my preferred approach for opencv. ISOCPP discusses this feature as the Named Parameter Idiom https://isocpp.org/wiki/faq/ctors#named-parameter-idiom

From OE34, the author writes this as their view of drawbacks

conversion of an algorithm to a class is probably the most poweful solution in w.r.t obtained extendibility. But such classes take more lines of code to use. Instead of a single call, users have to write several: create class, tune parameters one by one and then call the processing method. 'Call chain' approach may somewhat help, but it's still quite a bit of typing. Besides, it's more difficult for developers to convert functions to classes, make a proper future-proof design. And the classes may also be a bit challenging for the wrapper generators.

I agree with that author that NPI is the most powerful solution. However, I disagree with the author in their drawback list when compared to the "make a struct for everything" (MASFE) approach.

1. NPI is "more lines of code to use" and "quite a bit of typing". No, the opposite is true. NPI is less lines and less typing than MASFE. All can be on a single line. There is no need to do work in separate discrete calls or construction.

   filter2D(a, b, k, {.border_mode=BORDER_CONSTANT, .border_value=5, .scale=1./16}); // MASFE
   filter2D(a, b, k).border_mode(BORDER_CONSTANT).border_value(5).scale(1./16);      // NPI

2. It is more difficult to convert functions to NPI classes. This is rumor. It is equally difficult for any dev to
   1. MASFE: create struct with member fields in exact alphabetical order, change signatures to accept struct, in function parse/validate all struct member values
   2. NPI: create wrapper class for function, add member field "set" functions, in each those functions validate values
3. Both approaches have challenges of backward/forwards/future -compatibility and -proofing. However, NPI is more flexible because it is a full class with member functions and all C++ allows those to do. Therefore, NPI can better adjust in the future.
4. NPI easily supports accepting any type as a value because it is just c++ member function and those can have signature overloading

   filter2D(a, b, k).border_value(5);   // accepts int (because floats are not guaranteed to be exact)
   filter2D(a, b, k).border_value(123.4567);  //accepts double
   filter2D(a, b, k).border_value(my3dvector);  // accepts 3d vector

5. MASFE must think of all possible value types now...or...later when you have a type you want to support it has to be inserted in alphabetical order and likely with an odd name. See the following example.
6. MASFE quickly fails when attempting to accept any value. Examples:
   1. very value has to be a std::variant and all the supporting code for it. Unions are outdated and error-prone.
   2. Every struct has to have multiple fields for every possible type for a setting/field. Causing the struct to have clutter

   struct filter2DParams {
     int border_value = 0;
     double border_value_double = 0.; // oops, now want to support double
     vector<float> border_value_vector = {}; // oops, now want to support a vector
     ...

7. NPI easily supports deprecation because the member functions can ignore values or throw with no stack overhead. In contrast, MASFE leaves deprecated fields in the struct which uses extra memory and causes clutter.
8. NPI does not have the alphabetic/order initializer limitations of MASFE.
9. NPI does not have the C++ 20 limitation of MASFE.
10. NPI is well known and easy to use by users in languages like java, javascript, C++... and python. All those languages and more have method chaining.
11. Wrapper generator comment is rumor until the analysis is done. It is a valid point to consider and should be treated equally until the analysis is done.

[diablodale]

Hi. I have pushed an example of a new cv::remap() that uses the NPI (named parameter idiom) approach using a functor (c++ function object). It is a single commit on a branch from official 4.5.2. It includes the c++ code plus test cases for c++ and python.
https://github.com/diablodale/opencv/tree/functor-remap2

Result

Good news. Works today in both C++ and python. No code change needed to OpenCV 4 for the NPI approach.

My example is a simple functor wrapping the existing free-function remap() to show NPI. Using a functor has additional benefits and features (search google). The functor will gain benefits and efficiencies when the existing remap code is integrated directly into the functor instead of the quick wrapper. This allows things like:

• only once convert maps and params from C++ or python/numpy. All the conversions, parameter checks, OpenCL down/uploads, ... all of that can happen once when the param is set and not when the remap is actually run/applied. This is likely a measurable improvement in performance and memory use.
• reuse. Create the functor once, set the parameters once, and use it multiple times on different source/dest. This is a feature I would use today in my realtime image processing.
• works great with the C++ STL. Whole sections of STL are built to use functors.

Comments

• Macros or templates can simplify the repetitive/boilerplate code. Ideas like https://stackoverflow.com/questions/22553181/c-template-to-convert-function-into-function-object-type
• C++ programs that call free-function cv::remap() or the very old cvRemap() work. No migration of code needed.
• If python doesn't have something similar to operator(), then we must use the CV_WRAP_AS(name). I prefer the name run(), exec(), or go(). I used run() in my example. I see both approaches in OpenCV, e.g. ocl::kernel::run() and FeaturesMatcher::apply().

  dst = remapper.interpolation(cv.INTER_LINEAR).apply(src)  # might be misunderstood to be "applying the interpolation setting"
  dst = remapper.interpolation(cv.INTER_LINEAR).run(src)    # clear I am running the remap code

• It is possible to make python backwards compatible with the free-function cv.remap(). I'm not an expert at the wrappers, so it might already work. Otherwise, I think the wrapper changes are...
  1. identify when a C++ constructor has an out parameter (like OutputArray). I know how to do this in gen2.py.
  2. remove an assert allowing (1) and populate a python attribute with these out parameters. I know how to do this also.
  3. change logic in gen2.py to not put these constructors in the generated code for constructors...because they are hardcoded to return (int)0. Instead, put these in the section of generated code that is similar to object methods so that the correct return signature is used. I can see some of the gen2.py code that does this, but it was taking too much time to reverse-engineer it to do for this example.