Skip to content
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.

Sign up for GitHub

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

Jump to bottom

freetype: fix for huge CtoL param. #3585

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from
Open

freetype: fix for huge CtoL param. #3585

wants to merge 2 commits into from

Conversation

Kumataro
Copy link
Contributor

The freetype module uses CtoL param to split bezier curve segment to polylines.
This patch makes local upper limitation for each segments. It is determined with total length of polylines at anchors.
And skip to push_back() same points, likes imgproc::ellipse2Poly().

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 another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the 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

asmorkalov
asmorkalov reviewed Nov 8, 2023
View reviewed changes
modules/freetype/src/freetype.cpp Outdated
Comment on lines 735 to 736
std::pow( dx1 * dx1 + dy1 * dy1, 0.5 ) +
std::pow( dx2 * dx2 + dy2 * dy2, 0.5 )
Copy link
Contributor

Choose a reason for hiding this comment

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

pow 0.5 is sqrt. I propose to use sqrt as it's faster and more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review! Yes, it is better, I fixed it.

modules/freetype/src/freetype.cpp Outdated
// Split Bezier to lines ( in FreeType coordinates ).
double u = (double)i * 1.0 / (p->mCtoL) ;
double u = (double)i * 1.0 / (iCtoL) ;
Copy link
Contributor

Choose a reason for hiding this comment

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

*1.0 is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I fixed it. And I replaced from (double)i that is C-style-cast, to static_cast<double>(i).

modules/freetype/src/freetype.cpp Outdated
Comment on lines 791 to 793
std::pow( dx1 * dx1 + dy1 * dy1, 0.5 ) +
std::pow( dx2 * dx2 + dy2 * dy2, 0.5 ) +
std::pow( dx3 * dx3 + dy3 * dy3, 0.5 )
Copy link
Contributor

Choose a reason for hiding this comment

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

the same for pow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it, thank you!

modules/freetype/src/freetype.cpp Outdated
// Split Bezier to lines ( in FreeType coordinates ).
double u = (double)i * 1.0 / (p->mCtoL) ;
double u = (double)i * 1.0 / (iCtoL) ;
Copy link
Contributor

Choose a reason for hiding this comment

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

*1.0 is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it, thank you!

@Kumataro
Copy link
Contributor Author

Kumataro commented Nov 9, 2023

I believe that iCtoL is determined with font glyph path data, so it should be care about the range of it more than before.

  • if iCtoL == 0, dounle u is calicurated with i / 0.
  • if iCtoL == INT_MAX, int i will be overflow.

I think that such a case is basically impossible. However, it's not absolute, so I'd like to put in some protection just in case.
So in additional patch, iCtoL is fitted to range from 1 to INT_MAX-1.

@Kumataro
Copy link
Contributor Author

Kumataro commented Nov 9, 2023

Test is failed, but it is about unexpected module/function. Maybe it is missjudgement.

I believe this file doesn't reference to freetype module.

Test result is here.

======================================================================
FAIL: test_ric_interpolator (test_sparse_match_interpolator.Interpolator_test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ci/opencv_contrib/modules/ximgproc/misc/python/test/test_sparse_match_interpolator.py", line 67, in test_ric_interpolator
    self.assertTrue(cv.norm(dense_flow, ref_flow, cv.NORM_INF) <= MAX_DIF)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 297 tests in 84.373s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmorkalov asmorkalov asmorkalov left review comments

Assignees
No one assigned
Labels
bug category: freetype
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Kumataro @asmorkalov @opencv-alalek