Skip to content

Fix bug in put/get of non-continuous Mat#19503

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
komakai:fix-android-putget
Feb 24, 2021
Merged

Fix bug in put/get of non-continuous Mat#19503
opencv-pushbot merged 1 commit intoopencv:3.4from
komakai:fix-android-putget

Conversation

@komakai
Copy link
Copy Markdown
Contributor

@komakai komakai commented Feb 11, 2021

Pull Request Readiness Checklist

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is a test

Fix for #19502

Comment on lines +318 to +319
if (ar1.length != ar2.length) {
fail("Arrays have different sizes.");
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertEqual?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmorkalov this was a copy/paste from another assertArrayEquals function - I'll change all of them to be like this

Comment on lines +2175 to +2177
for (int index = 0; index < inc; index++) {
if (incIdx(mat, indices)) {
return true;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increasing by one multiple times is really slow (for HD images inc value is up to 1280).
We need "fast path" to handle "always contiguous" last dim (cols).

{
    int cols_dim = mat->dims - 1;
    size_t cols = mat->size[cols_dim];
    CV_CheckLE(cols, (size_t)INT_MAX, "");  // 'int' only, size_t type is not supported for indexes
    size_t new_cols_idx = (size_t)indices[cols_dim] + inc;
    // only "cols" idx is allowed to be increased till the end
    // do not allow to jump forward because we should support non-contiguous layouts too (submats)
    CV_CheckLE(new_cols_idx, cols, "");
    if (new_cols_idx == cols)
    {
        indices[cols_dim] = 0;
        for (int dim = cols_dim - 1; dim >= 0; --dim)
        {
            int new_idx = indices[dim] + 1;
            if (new_idx != mat->size[dim])
            {
                indices[dim] = new_idx;
                return false;
            }
            else
            {
                indices[dim] = 0;
                continue;
            }
        }
        return true;
    }
    else
    {
        indices[cols_dim] = (int)new_cols_idx;
        return false;
    }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually updating the indices doesn't require any iteration - we can just calculate where the new indices will end up.
But I get your point - we can optimize the copying of data if the trailing dimensions are contiguous. Will push a new optimized implementation.

assertEquals(4, bytesNum);
assertTrue(Arrays.equals(new short[] {340, 341, 0, 0}, buff11));

Mat m2 = new Mat(new int[]{ 5, 5, 5 }, CvType.CV_16S);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5, 5, 5

different numbers are preferable (to ensure that we don't mess with "size[]" array order in implementation):

5, 6, 8

@komakai
Copy link
Copy Markdown
Contributor Author

komakai commented Feb 14, 2021

alalek added the pr: needs rebase label 2 days ago

Rebasing on the 3.4 branch

@komakai komakai changed the base branch from master to 3.4 February 14, 2021 06:12
@komakai komakai force-pushed the fix-android-putget branch 6 times, most recently from 724aafd to 775eda1 Compare February 15, 2021 10:48
@komakai
Copy link
Copy Markdown
Contributor Author

komakai commented Feb 15, 2021

I spotted another bug in the calculation of elements remaining in the Mat:

    int rest = (int)m->elemSize();
    for (int i = 0; i < m->dims; i++) {
        rest *= (m->size[i] - idx[i]);
    }

I will convert to draft while I fix this

@komakai komakai marked this pull request as draft February 15, 2021 12:33
@komakai
Copy link
Copy Markdown
Contributor Author

komakai commented Feb 19, 2021

OK - I have reworked this a bit.
Basically the mat_put_idx and the mat_get_idx implementations were just the same except for the direction in which the data was copied - so I have merged the 2 implementations.
Also the mat_put(row,col) and the mat_get(row,col) were just special cases of mat_put_idx and mat_get_idx so I modified them to just call to the generic case. So 4 functions are now essentially condensed down to one.

@asmorkalov
Copy link
Copy Markdown
Contributor

@komakai Is it ready for review?

@komakai
Copy link
Copy Markdown
Contributor Author

komakai commented Feb 19, 2021

@komakai Is it ready for review?

@asmorkalov Yeah (once the builds pass!)

@asmorkalov asmorkalov marked this pull request as ready for review February 20, 2021 09:13
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

@opencv-pushbot opencv-pushbot merged commit 67b6ef4 into opencv:3.4 Feb 24, 2021
@alalek alalek mentioned this pull request Feb 27, 2021
@alalek alalek mentioned this pull request Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants