Skip to content

Add checks for empty operands in Matrix expressions that#15966

Merged
alalek merged 5 commits intoopencv:3.4from
saskatchewancatch:issue-15760
Dec 12, 2019
Merged

Add checks for empty operands in Matrix expressions that#15966
alalek merged 5 commits intoopencv:3.4from
saskatchewancatch:issue-15760

Conversation

@saskatchewancatch
Copy link
Copy Markdown
Contributor

@saskatchewancatch saskatchewancatch commented Nov 21, 2019 •
edited by alalek
Loading

don't check properly.

More checks and tests coming soon.

resolves #15760

@saskatchewancatch saskatchewancatch changed the title Starting to add checks for empty operands in Matrix expressions that Add checks for empty operands in Matrix expressions that Nov 21, 2019
@saskatchewancatch saskatchewancatch mentioned this pull request Nov 21, 2019
Closed
@saskatchewancatch
Copy link
Copy Markdown
Contributor Author

saskatchewancatch commented Nov 22, 2019

I'll take care of the missing declarations.
Mat.hpp:L3592 would be the place to declare checkOperandsExist if I am not mistaken.

@saskatchewancatch
Copy link
Copy Markdown
Contributor Author

saskatchewancatch commented Nov 24, 2019

Hmmm now why would those fail .... did we just find some places where MatExpr operations are being done on empty operands? Will take a look.

@saskatchewancatch
Copy link
Copy Markdown
Contributor Author

saskatchewancatch commented Nov 30, 2019 •
edited
Loading

Test Calib3d_UndistortPoints.badarg is a Bad Args style test, and it is failing because of one of the sub-tests inside of it.

This subtest in test_undistort_badarg.cpp essentially checks that the input src matrix to cv::undistortPoints is not empty.

I see one of two things can be done:

  1. That one sub-test may no longer be needed, in which case it can be removed
    OR
  2. In cv::undistort undistort_dispatch.cpp, adjust behaviour when src matrix is empty:
    2.1 Return immediately because there is nothing to undistort??
    OR
    2.2 CV_Error from here.

@sovrasov @alalek Any thoughts on this?

@mshabunin
Copy link
Copy Markdown
Contributor

mshabunin commented Dec 5, 2019

Looks like the problem with the test is that it expects CV_StsAssert, but you throw CV_StsBadArg.

@saskatchewancatch
Copy link
Copy Markdown
Contributor Author

saskatchewancatch commented Dec 6, 2019

Indeed, it's expecting the wrong one exception. I was debugging the tests in that class to see if it could be improved. Seems fine as it is. Thank you.

@saskatchewancatch
Copy link
Copy Markdown
Contributor Author

saskatchewancatch commented Dec 8, 2019

@alalek The issue that this PR resolves (#15760) is for the 3.4.9 milestone. Maybe this PR could also be targeted for 3.4.9?

alalek
alalek reviewed Dec 9, 2019
View reviewed changes
modules/core/include/opencv2/core/mat.hpp Outdated

void checkOperandsExist(const Mat& a);
void checkOperandsExist(const Mat& a, const Mat& b);

Copy link
Copy Markdown
Member

@alalek alalek Dec 9, 2019

Choose a reason for hiding this comment

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

These functions should not be a part of public OpenCV API.

Moreover, they are "local", so you should use "static" before definition instead.

Copy link
Copy Markdown
Contributor Author

@saskatchewancatch saskatchewancatch Dec 11, 2019

Choose a reason for hiding this comment

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

Thanks @alalek. I forgot about that.

I suppose then this is best: Remove declarations from mat.hpp and add static qualifier to checkOperandsExist in matrix_expressions.cpp. So checkOperandsExist will not be exposed outside matrix_expressions.cpp.

Copy link
Copy Markdown
Member

@alalek alalek Dec 11, 2019

Choose a reason for hiding this comment

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

Right!

@saskatchewancatch
Copy link
Copy Markdown
Contributor Author

saskatchewancatch commented Dec 12, 2019 •
edited
Loading

imgproc_perf test failing on Mac build, but the build farm's imgproc_perf stdio for that platform doesn't show which test failed or why. I test on Ubuntu-64/Win10-64.

@alalek
Copy link
Copy Markdown
Member

alalek commented Dec 12, 2019 •
edited
Loading

These sporadic failures are not related to this patch. See #16137

alalek
alalek approved these changes Dec 12, 2019
View reviewed changes
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 for contribution 👍

@alalek alalek merged commit e6ce752 into opencv:3.4 Dec 12, 2019
@alalek alalek mentioned this pull request Dec 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek approved these changes

Assignees

@alalek alalek

Labels

category: core test

Projects

None yet

Milestone

3.4.9

Development

Successfully merging this pull request may close these issues.

3 participants

@saskatchewancatch @mshabunin @alalek