Skip to content

Fix H clamping for very small negative values.#21063

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
vrabaud:3.4_h_clamping
Nov 19, 2021
Merged

Fix H clamping for very small negative values.#21063
opencv-pushbot merged 1 commit intoopencv:3.4from
vrabaud:3.4_h_clamping

Conversation

@vrabaud
Copy link
Copy Markdown
Contributor

@vrabaud vrabaud commented Nov 16, 2021

In case of very small negative h (e.g. -1e-40), with the current implementation,
you will go through the first condition and end up with h = 6.f, and will miss
the second condition.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • 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

h *= hscale;
if( h < 0 )
do h += 6; while( h < 0 );
else if( h >= 6 )
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.

Why we can't replace else if => if + add comment to avoid replacing this back.

Proposed expression work for SIMD code (with comparison through v_select) but expression still may lead to crash of this code (with array indexes, without strong CV_Assert()).

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.

We can indeed remove the else, I just wanted the code to be like SIMD for consistency and also thought that the while loop might be an overkill if h is too big.

@vrabaud vrabaud changed the title Compute H clamping in the same was as in the SIMD implementation Compute H clamping in the same way as in the SIMD implementation Nov 18, 2021
@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Nov 18, 2021

Sorry I actually did not get: in the end, do you want the simple solution where I just remove the else, or the one that copies the SIMD code and that does not have two while loops ?

@alalek
Copy link
Copy Markdown
Member

alalek commented Nov 18, 2021

I prefer minimal changes (with removal of the else).

@vrabaud vrabaud force-pushed the 3.4_h_clamping branch 2 times, most recently from 0465253 to b0fe829 Compare November 18, 2021 16:04
@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Nov 18, 2021

ok, done.

@vrabaud vrabaud changed the title Compute H clamping in the same way as in the SIMD implementation Fix H clamping for very small negative values. Nov 19, 2021
In case of very small negative h (e.g. -1e-40), with the current implementation,
you will go through the first condition and end up with h = 6.f, and will miss
the second condition.
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 93b6e80 into opencv:3.4 Nov 19, 2021
@alalek alalek mentioned this pull request Nov 20, 2021
@vrabaud vrabaud deleted the 3.4_h_clamping branch November 22, 2021 14:29
@alalek alalek mentioned this pull request Dec 30, 2021
@alalek alalek mentioned this pull request Feb 22, 2022
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.

3 participants