It is now a bit faster and finally seems to be correct.

without fixes:
0m14.239s

with fixes:
0m13.314s

Found one more issue. My own tests pass but some imgproc accuracy tests, reproducers and regression tests still fail. going through it case by case now.

imgproc performance tests pass

I'm still investigating test differences but i think it's ready for a look.

getPointVec is not complete yet.

Calib3d_ChessboardDetector.accuracy (among others) throws:

Unsupported format or combination of formats (InputArray is not formatted correctly) in getPointVec, file /build/precommit_linux64/4.x/opencv/modules/imgproc/src/convhull.cpp, line 80

@kallaballa Thanks a lot for the issue investigation and fix. Couple of thoughts:
If I understood correctly, the major issue is related to buffers casting: the same data is casted as Point* and Point2f* and used as integer of floating point.
You do not need to invent your own getPointVec, just use the following code snippet:

std::vector<Point> points;
std::vector<Point2f> points2f;
if(..)
  _points.copyTo(points2f);
else
  Mat tmp = _points.getMat();
  tmp.convertTo(points);

The similar case for output array. Just use copyTo method of Mat/InputArray/OutputArray. It handles the shapes and std::vectors automatically.

Warnings:

/Users/opencv-cn/GHA-OCV-3/_work/opencv/opencv/opencv/modules/imgproc/src/convhull.cpp:201:20: warning: variable 'i' is uninitialized when used here [-Wuninitialized]

Warnings:

/Users/opencv-cn/GHA-OCV-3/_work/opencv/opencv/opencv/modules/imgproc/src/convhull.cpp:201:20: warning: variable 'i' is uninitialized when used here [-Wuninitialized]

thx!

@kallaballa Thanks a lot for the issue investigation and fix. Couple of thoughts: If I understood correctly, the major issue is related to buffers casting: the same data is casted as Point* and Point2f* and used as integer of floating point. You do not need to invent your own getPointVec, just use the following code snippet:

std::vector<Point> points;
std::vector<Point2f> points2f;
if(..)
  _points.copyTo(points2f);
else
  Mat tmp = _points.getMat();
  tmp.convertTo(points);

The similar case for output array. Just use copyTo method of Mat/InputArray/OutputArray. It handles the shapes and std::vectors automatically.

good to know! but those this also cover cases where numerical values are converted to Points? Also i found copyTo to be quite slow in comparision.

oh my. i think i misunderstood something. convexHull isn't supposed to be callable with numerical values instead of points which make getPointVec rubbish.

Simplified the implementation. Less tests fail.

If I am not wrong some regression tests e.g.:

and I am not sure what an implicit conversion from bool to Point2i should do:

am i right about some tests being invalid? because then this means i have to fix them one by one.

oh my. i think i misunderstood something. convexHull isn't supposed to be callable with numerical values instead of points which make getPointVec rubbish.

The tests assume that int[][2] should work as input - the docs say otherwise. What should i do?

So i fixed Imgproc_ConvexityDefects.ordering_4539 (
82474ac) by using vector and doing manual conversions. It doesn't crash anymore but there are accuracy problems:

[ RUN      ] Imgproc_ConvexityDefects.ordering_4539
/home/elchaschab/devel/opencv/modules/imgproc/test/test_convhull.cpp:302: Failure
Expected equality of these values:
  pt
    Which is: [801, 179]
  hull[i]
    Which is: [921, 261]

i think those changes in behavior are legitimate but I'm gonna test some more.

How should i proceed with broken tests?

I understand that those changes effect a lot of users, but since it is about memory corruption i can't see a chance to guarantee behavior doesn't change. so i think it's best to enforce the documentation by error checking and fix up tests and depending algorithms.
Do you agree?

maybe convexityDefects suffers from the same problem. still have to check many tests.

I understand that those changes effect a lot of users, but since it is about memory corruption i can't see a chance to guarantee behavior doesn't change. so i think it's best to enforce the documentation by error checking and fix up tests and depending algorithms. Do you agree?

I also think by adding more error checks the change won't go unnoticed because user-code will break with a meaningful message. which i would like :)

Afaics there is no legal way to find the element type (except for numerical opencv types) of a container which makes rather complete error handling impossible.

Maybe deprecation (with a big warning) in favor of a new function would be an option.

I still got spurious memory errors, so i changed getVec to use OutputArray::copyTo as suggested - didn't help. There is more in the bush i suspect. Investigating...

is this correct?

std::vector<Point2f> vec = {{2.0f,3.0f}};
std::vector<Point2f> vec2;
OutputArray arr = vec;
arr.copyTo(vec2)
CV_Assert(vec.size() == vec2.size())

I pushed the current version that uses OutputArray::copy. It spuriously fails but always at (with asan enabled):

Note that I removed undocumented behavior (returning points automatically depending on the OutputArray type)

1. InputArray / OutputArray are not designed to be used as local variables, wrappers for function para,meters only.
2. OutputArray should be pre-allocated before .copy() call. You need to calll OutputArray::create() first.

1. InputArray / OutputArray are not designed to be used as local variables, wrappers for function para,meters only.

I understand now.

2. OutputArray should be pre-allocated before .copy() call. You need to call OutputArray::create() first.

I saw that in the documentation and tried that but i can only create an OutputArray with numerical types. How to create an OutputArray that contains a std::vector<Point2f>?

is this correct?

std::vector<Point2f> vec = {{2.0f,3.0f}};
std::vector<Point2f> vec2;
OutputArray arr = vec;
arr.copyTo(vec2)
CV_Assert(vec.size() == vec2.size())

Should be InputArray...

void copyVec(InputArray _input, OutputArray _output) {
//  _output.create ?
 _input.copyTo(_output);
}

std::vector<Point2f> vec = {{2.0f,3.0f}};
std::vector<Point2f> vec2;
copyVec(vec, vec2)
CV_Assert(vec.size() == vec2.size())

So this should be correct?

neither ASAN nor valgrind complain about both version (copy/no-copy. running UBSAN and TSAN next.

With lessons learned I am going back to the proverbial drawing board...

I duplicated the original branch to https://github.com/kallaballa/opencv/tree/convexhullpointer_old and forcibly reset this one to 8e65075

how about this: to stick to the documentation as close as possible and catch all that did stick to the documentation, while making it obvious to others that they need to change something.

Preferred signature: easy to be implemented fast and correct.

template<typename Thull>
void convexHull(InputArray array, std::vector<Thull>& hull, clockwise>

Deprecated signature 1: does the same as the preferred signature, only deprecated because the last parameter should be removed

template<typename Thull>
void convexHull(InputArray array, std::vector<Thull>& hull, clockwise, bool ignored>

Deprecated signature 2: the original function fixed to the point where it retains behavior as closely as possible while preventing memory errors. probably considerably slower.

void convexHull( InputArray _points, OutputArray _hull, bool clockwise, bool returnPoints )