Skip to content

Bit-exact Nearest Neighbor Resizing#18053

Merged
alalek merged 17 commits intoopencv:3.4from
Yosshi999:bit-exact-resizeNN
Aug 28, 2020
Merged

Bit-exact Nearest Neighbor Resizing#18053
alalek merged 17 commits intoopencv:3.4from
Yosshi999:bit-exact-resizeNN

Conversation

@Yosshi999
Copy link
Copy Markdown
Contributor

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 OpenCV (BSD) 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 accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@terfendail
Copy link
Copy Markdown
Contributor

@Yosshi999 Could you please check the Nearest8U_vsNonExact failure reason on Win64? That could be near-edge issue we discussed

@Yosshi999
Copy link
Copy Markdown
Contributor Author

I thought x2 or x0.5 resizing is bit-exact even if the method is non-exact version because multiplication by 0.5 or 2 will not modify fraction fields of floating point data. But it seems I have a misunderstanding.

std::cout << "ifx: " << std::hex << ifx.raw() << std::endl;
for( int x = 0; x < dsize.width; x++ )
{
int sx = int32_t( ((ifx * static_cast<uint32_t>(x)) >> 32) << 32 ); // equivalent to cvFloor
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.

It seems (x >> 32) << 32 is optimized to x in the windows build.

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!

testing::Combine(
testing::Values(CV_8UC1, CV_8UC2, CV_8UC4),
testing::Values(szVGA, szqHD, sz720p, sz1080p, sz2160p),
testing::Values(2.4, 3.4, 1.3)
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.

Need to fix scope of perf tests.

NN resize is usually used for downscaling (instead of upscaling), include 0.5: 0.25, 0.5, 2 should be enough.

Please also reduce tested sizes (there are no significant changes in implementation) - sz720p, sz1080p is enough.

Add 8UC3 type as frequently used case. Remove 8UC2.

@vpisarev
Copy link
Copy Markdown
Contributor

@Yosshi999, thank you for the contribution!
Can you, please, implement the following algorithm instead #17068 (comment)?

@alalek
Copy link
Copy Markdown
Member

alalek commented Aug 14, 2020

Can you, please, implement the following algorithm instead

This is completely different algorithm.

Bit-exact implementation should follow this rule and must provide minimal possible difference between:

  • bit-exact result of 8U input
  • convert to double/floats, run original version on floating-point data, convert result back to 8U.

Without changing of the base NN implementation this should not be called bit-exact.

@Yosshi999
Copy link
Copy Markdown
Contributor Author

According to the discussion in #17068, it seems that there are some options:

  • Define it as a new method like INTER_NEAREST_PILLOW or INTER_NEAREST_CENTER_PIX_CONVENTION or something. (@catree said we should avoid using the name of a library so I prefer INTER_NEAREST_CENTER_... if this option is chosen)
  • Put it into the current implementation of INTER_NEAREST (@vpisarev wants to do this, but @alalek disagrees?)

In these cases above, I will remove the suffix _EXACT and related tests even though this is intended to be bit-exact. I think we don't have to create non-exact version because this doesn't use softfloat so the overhead is not large.

Or:

  • Revert to ee74e10 and define it as INTER_NEAREST_EXACT

I will vote for the second one.

@terfendail
Copy link
Copy Markdown
Contributor

I prefer the first option.
On the one hand there is a request for new kind of NN resize in OpenCV library. On the other hand original implementation of NN resize is used internally by several other algorithms already implemented and tuned for existing specific implementation of NN. So changing of NN resize implementation could cause accuracy issues in other pieces of the library. Tracing all such places is an extra effort that is outside of this GSoC project scope, moreover there could be external projects that depend on the existing implementation specific, so such a change could be introduced not early than OpenCV5(or probably even later)

From my point of view BITEXACT part of the algorithm name mean that it produce the same result regardless of the platform it executed on even if the result differs from non-exact implementation, but naming conventions is another thing to discuss. For me it will be enough if there will be a zero tolerance accuracy test for this method with a statement that zero tolerance is important here and that this requirement must not be relaxed.

Copy link
Copy Markdown
Contributor

@terfendail terfendail left a comment

Choose a reason for hiding this comment

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

Implementation looks fine for me. The only thing to discuss is the name of the method enum. However it is outside of this PR intention scope.

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