OpenCV: avoiding memory corruption when using Mat constructors taking primitive arrays

[HGuillemet]

org.bytedeco.opencv.presets.opencv_core.java defines Mat constructors taking either a java array (var args) or a Pointer.

Concerning arrays, one can write :

Mat m = new Mat(-1, -1, -1, -1, 8, -1, -1, -1, -1);

and, since 1D matrices are not what is needed most of the time, one is tempted to do:

m = m.reshape(1, 3);

This looks practical and quite armless for anyone used to OpenCV patterns but leads to memory corruption since this Mat constructor actually allocates a buffer, copies the Java array to the buffer, and creates an OpenCV Mat header pointing to this (opencv-external) data. A pointer to the buffer is kept in the Mat object, but when the Mat is garbage collected, the buffer is collected to and the buffer is freed. The reshaped Mat then points to a deallocated buffer.

This PR replaces the Java-side allocation by the allocation of a normal OpenCV Mat, that would benefit from OpenCV reference counting feature. There is no performance loss since we copy the data in both cases.

Concerning Mat constructors taking a Pointer as argument, they suffer from the same dangerous limitation. This PR simply adds a warning comment since creating a Mat from a Pointer enables the option to apply opencv processing to any kind of data without memory copy.

[saudet]

Instead of only adding warnings like you did, could you instead add a boolean copyData parameter flag and add default overloads that sets it to false? That would mostly mimic the original constructors from OpenCV, while keeping backward compatibility for us:
https://docs.opencv.org/master/d3/d63/classcv_1_1Mat.html#a23b182c4ffd46abe38f460c1480ae887

[HGuillemet]

Good idea.
But maybe we shouldn't add overloads for the 18 concerned constructors.
What about passing true for constructors taking Scalar and Points ?

Another possible addition : constructors taking 2D Java arrays ?

[HGuillemet]

Hmm, forget my remark, Points and Scalars can be arbitrarily long. So 18 overloads ?

[saudet]

Good idea.
But maybe we shouldn't add overloads for the 18 concerned constructors.

Why not? :) It's just like public Mat(Point points) { this(points, false); } etc, could even be on the same line...

What about passing true for constructors taking Scalar and Points ?

No, that would break backward compatibility and it doesn't work well in our case where everything is a Pointer and which constructor might get used isn't always clear.

Another possible addition : constructors taking 2D Java arrays ?

Yes, of course, there's a lot we can do :)

[saudet]

BTW, since this is just Java code, we can use for loops and what not to generate code more easily, for example, like this:
https://github.com/bytedeco/javacpp-presets/blob/master/caffe/src/main/java/org/bytedeco/caffe/presets/caffe.java#L148

[HGuillemet]

In the constructors taking pointers, if the pointers'limit equals the position, the length is set to 1.
What is the rationale behind that ? Doesn't that causes obscure bugs ?

[saudet]

In the constructors taking pointers, if the pointers'limit equals the position, the length is set to 1.
What is the rationale behind that ? Doesn't that causes obscure bugs ?

I suppose we could leave that to 0? That's probably going to throw some exception, which would be acceptable.

[HGuillemet]

In the constructors taking pointers, if the pointers'limit equals the position, the length is set to 1.
What is the rationale behind that ? Doesn't that causes obscure bugs ?

I suppose we could leave that to 0? That's probably going to throw some exception, which would be acceptable.

Pointers may have a limit of 0 when it's unknown (that seems to be the case of Mat.data()).
But throwing an exception should be better than silently supposing the length is 1.

See new proposal with copyData in last commit.
Similarly to the case position == limit, I also added a check when casting a long to int, instead of clamping.

[saudet]

Except for a couple of small mistakes in the comments, looks good! To be consistent with the rest of JavaCPP though, instead of throwing an exception, let's saturate sizes this way: size < Integer.MAX_VALUE ? (int)size : Integer.MAX_VALUE.

[HGuillemet]

To be consistent with the rest of JavaCPP though, instead of throwing an exception, let's saturate sizes this way: size < Integer.MAX_VALUE ? (int)size : Integer.MAX_VALUE.

I'm not aware of the consistency problem, but saturation would yield something like:

FloatPointer p = new FloatPointer(0x90000000L);
Mat m = new Mat(p);

silently creating a mat of length 0x7FFFFFFF instead of throwing an exception. Is this really why is preferred ?
Fortunately, any reshape afterwards would yield to an exception because of non-divisibility of 0x7FFFFFFF but other situations could lead to non obvious disaster.

[saudet]

I like it better that way anyway, less exceptions to worry about :) It's handled like that everywhere else in JavaCPP and I never heard anyone having any problems with it. Containers like that don't support large enough indices, so the user will get into trouble trying to index those with long anyway, and that throws compiler errors. Much better than an exception at runtime.

[saudet]

@HGuillemet Please let me know if that looks good enough and all correct! Thanks