-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Added overflow handling during conversion from float to int for Linea… #20977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@YashasSamaga Could you please take a look at it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to prepare regression test first.
Current fix attempt doesn't look valid.
@@ -95,6 +96,12 @@ namespace cv { namespace cuda { namespace device | |||
|
|||
const int x1 = __float2int_rd(x); | |||
const int y1 = __float2int_rd(y); | |||
if (x1 <= NPP_MIN_32S || x1 >= NPP_MAX_32S || y1 <= NPP_MIN_32S || y1 >= NPP_MAX_32S) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x < 0
y < 0
x >= src.width
y >= src.height
Need to take border value somehow (not sure if it is properly defined for remap).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src is an instance of the class BorderReader which decided how to handle values out of scope by itself depending on the type of border. Values greater than height and width are completely valid, we don't have to clamp them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note, that condition checks for x1,y1 which have "int" type, instead of the original x,y which can underflow/overflow/NaN/Inf "int" range (return value from __float2int_rd
is not something expected).
@@ -95,6 +96,12 @@ namespace cv { namespace cuda { namespace device | |||
|
|||
const int x1 = __float2int_rd(x); | |||
const int y1 = __float2int_rd(y); | |||
if (x1 <= NPP_MIN_32S || x1 >= NPP_MAX_32S || y1 <= NPP_MIN_32S || y1 >= NPP_MAX_32S) | |||
{ | |||
elem_type src_reg = src(y1, x1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src(y1, x1)
Doesn't look as valid access anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is valid because it would be handled by BorderReader class.
Existed code is totally correct for any values except that it's tried to add something to already MAX_INT value. Here:
const int x2 = x1 + 1;
const int y2 = y1 + 1;
That's the only thing which we have to fix. And that's what I did.
No problem, I'll add a test. The fix is valid, please take a look at the comments above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
…rFilter
Related issue #18224
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.