Added reinterpret() method to Mat to convert meta-data without actual data conversion#25394
Added reinterpret() method to Mat to convert meta-data without actual data conversion#25394asmorkalov merged 1 commit intoopencv:4.xfrom HaoYuan-Gao:in_place_convertTo
Conversation
|
|
The condition-based modification changed the way dst is created, and under the specified conditions, it does not allocate new memory. And I don't understand why the reference counter affects memory reuse. |
|
@Gao-HaoYuan, ok, please add a test anyway. |
|
@dkurt you are right, I need to add a test to verify the result. Thanks for your review. If there are still issues, please help me identify them. |
| dst.create(dims, size, dtype); | ||
| Mat dstMat = dst.getMat(); | ||
| Mat dstMat; | ||
| if (this->data == dst.getMat().data && this->elemSize() == CV_ELEM_SIZE(dtype)) { |
There was a problem hiding this comment.
There is no check for size/dims match. For example, ROI will have the same data ptr:
Mat m (10, 10, CV_8U);
Mat roi = m.rowRange(0, 2);
m.convertTo(roi, CV_8S);There was a problem hiding this comment.
I had through !dst.getMat().isSubmatrix() to determine this case.
There was a problem hiding this comment.
What about the following case? .data matched but not the shapes.
Mat m (10, 10, CV_8U);
Mat m2(5, 10, CV_8U, m.ptr<uint8_t>());
m.convertTo(m2, CV_8S);There was a problem hiding this comment.
Firstly, if roi is a submatrix, the refcount will add 1.
if (this->data == dst.getMat().data && this->elemSize() == CV_ELEM_SIZE(dtype) && refcount == 2)
This condition can recognize the first case.
Mat m (10, 10, CV_8U); Mat roi = m.rowRange(0, 2); m.convertTo(roi, CV_8S);
BUt, I have come up with a new question. Like,
Mat m (10, 10, CV_8U); Mat m2(10, 10, CV_8U, m.ptr<uint8_t>()); m.convertTo(m2, CV_8S);
In this case, the member 'u' is NULL. By checking whether 'u' is null, we can solve this problem.
There was a problem hiding this comment.
Mat m (10, 10, CV_8U); Mat m2(10, 10, CV_8U, m.ptr<uint8_t>()); m.convertTo(m, CV_8S);
And, in this case, the original data of 'm' will be released, and 'm2' may encounter unpredictable errors. I believe such risky behavior should be noted by OpenCV users.
modules/core/test/test_mat.cpp
Outdated
| int dtype = CV_MAKE_TYPE(depth, cn); | ||
| A.convertTo(A, depth); | ||
| EXPECT_EQ(p, A.data); | ||
| EXPECT_EQ(A.type(), dtype); |
There was a problem hiding this comment.
Add a check for resulting values:
EXPECT_EQ(0, countNonZero(A != cv::Scalar(1, 2, 3)));There was a problem hiding this comment.
I had add a function to replace countNonZero, because countNonZero only support cn == 1.
|
@opencv-alalek could you take a look? |
| Mat dstMat = dst.getMat(); | ||
| Mat dstMat; | ||
| int refcount = this->u->refcount; | ||
| if (this->data == dst.getMat().data && dst.getMat().u != NULL && this->elemSize() == CV_ELEM_SIZE(dtype) && refcount == 2) { |
There was a problem hiding this comment.
it's quite bad idea to do it at such a high-level with such unsafe low-level details: 1) refcount==2, 2) 3 calls to getMat(), which can be rather expensive. 3) dst.assign() call, which is not that safe.
There was a problem hiding this comment.
@vpisarev
I have added a reinterpretType API to reinterpret the data type of Mat. However, it seems that these checks are unavoidable. Instead, I have changed it to refcount == 1 and reduced the number of dst.getMat calls.
|
@Gao-HaoYuan, thanks for the patch. This could be a useful feature, but I don't like implementation, it has many drawbacks. I would rather suggest to add Mat::reinterpret_cast() method to return a new header pointing to the same data. |
|
|
The patch breaks a lot of existing tests, e.g.: Details |
|
@vpisarev Thank you for your suggestion. Indeed, this implementation is not suitable for a high-level framework, and I need to consider more cases. I will think about a better implementation. Your ideas are very good. I need to learn coding strategies for high-level frameworks. |
|
@asmorkalov I will check and fix these failing tests. Thank you. |
|
Your PR contains severan not relevant commits now. Please rebase your branch on top of 4.x. |
|
@asmorkalov Sorry, I've been busy the past few days. Today, I'm trying to investigate the cause of the error. This isn't a formal submission. I'm wondering if there is a platform for test code. |
|
@Gao-HaoYuan friendly reminder |
|
Sorry for the delay in addressing this issue. I've just updated the code. |
@asmorkalov Hello, I have solved the crash with the tests. Please review and let me know if there are any areas that need improvement. |
modules/core/src/matrix.cpp
Outdated
| type = CV_MAT_TYPE(type); | ||
| CV_Assert(CV_ELEM_SIZE(this->type()) == CV_ELEM_SIZE(type)); | ||
| Mat m = *this; | ||
| m.flags = (type & CV_MAT_TYPE_MASK) | MAGIC_VAL; |
There was a problem hiding this comment.
it's safer to use m.flags = (m.flags & ~CV_MAT_TYPE_MASK) | type; here.
modules/core/src/matrix_wrap.cpp
Outdated
| Mat _OutputArray::reinterpretType(int mtype) const | ||
| { | ||
| mtype = CV_MAT_TYPE(mtype); | ||
| Mat& m = *(Mat*)obj; |
There was a problem hiding this comment.
don't silently assume that obj is Mat*. Do
return getMat().reinterpretType(mtype); instead
| void release() const; | ||
| void clear() const; | ||
| void setTo(const _InputArray& value, const _InputArray & mask = _InputArray()) const; | ||
| Mat reinterpretType( int type ) const; |
There was a problem hiding this comment.
can we skip Type suffix in the name? (Mat reinterpret(int type) const;?)
| Mat src; | ||
| Mat dstMat; | ||
| if (u != NULL && u->refcount == 1 && data == dst.getMat().data | ||
| && CV_ELEM_SIZE(stype) == CV_ELEM_SIZE(dtype) && isContinuous()) |
There was a problem hiding this comment.
this is wrong. When 8u is converted to 8s, for example, 255 should map to 127. In the case of opposite conversion (8s to 8u) all negative numbers should be converted to 0's. You also don't check noScale flag. I suggest to just retain reinterpret() and don't try to patch convertTo(). If reinterpret() works for you, just make the proper test that demonstrates the use of reinterpret().
There was a problem hiding this comment.
@vpisarev The function aims to reuse memory while performing data type conversion within the conversion function. such as BinaryFunc func = noScale ? getConvertFunc(sdepth, ddepth) : getConvertScaleFunc(sdepth, ddepth);
However, I believe your suggestion is correct: if memory reuse is needed, it's more reasonable to first call the reinterpret function. This is a more correct approach compared to modifying the convertTo function.
There was a problem hiding this comment.
@vpisarev My understanding and use of OpenCV, as well as my coding skills, indeed have some shortcomings. I am very grateful for your guidance, which has been very beneficial to me.
|
|
||
| Mat Mat::reinterpret(int type) const | ||
| { | ||
| type = CV_MAT_TYPE(type); |
There was a problem hiding this comment.
Why do we need CV_MAT_TYPE operation?
There was a problem hiding this comment.
I want to retrieve the actual type of the cv::Mat to prevent users from passing incorrect parameters, and then perform an assertion check.
There was a problem hiding this comment.
Need to start with "bad param" test then.
Silent ignoring of invalid input doesn't look good.
There was a problem hiding this comment.
I had added assert checks CV_Assert(CV_ELEM_SIZE(this->type()) == CV_ELEM_SIZE(type));
Surely, I will add test cases, and should I create a new PR to update this issue?
There was a problem hiding this comment.
It is still not clear why do we need type = CV_MAT_TYPE(type) operation:
- it is useless for valid MatType (works as no-op).
- it silently ignores invalid out-of-range input converting/truncating it to some value (which user may not expect).
There was a problem hiding this comment.
My initial thought was that types with the same element size could perform a reinterpret operation, and for any other type, an assert operation would be executed. Your point seems to be:
When newtype < this->type, a reinterpret operation (truncate) can also be performed.
When newtype > this->type, a new mat (convert) should be created.
The CV_MAT_TYPE operation is indeed redundant, as these operations are actually the responsibility of the convertTo function. CV_MAT_TYPE is indeed unnecessary.
I have an idea to optimize the convertTo operation for a Mat object. When src and dst are the same variable and their elemSize are equal, it could perform the operation in-place. Based on the suggestion of vpisarev, I added an
reinterpretAPI for reinterpreting the Mat type. Now, We can perform the following operations to reuse memory.origin : (This will allocate new memory)
optimization: （dst and src point to the same address, and the convertTo operation will not allocate new memory.）
In the convertTo scenario, leaving the decision of whether to reuse memory to the user is a more reasonable approach.
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.