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.

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

perf: skip frames, roi, fixed aspect ratio #225

Open
wants to merge 86 commits into
base: tier4/universe
Choose a base branch
from

Conversation

SergioReyesSan
Copy link
Contributor

Description

This PR includes:

-ROI to keep track of the calibration board, making future detections faster (performance improvement).
-Will skip board detection on some future frames, if the board was not detected on the current frame (avoid long queues).
-Increase the decimal digits when saving intrinsic parameters, to avoid the bubble effect when rectifying the image.
-Fix the issue that suddenly was pausing the operation of the calibration tool.
-Implements a new method to fix the aspect ratio on the rectified image.
-Changes the distortion model according to the number of distortion coefficients.

Related links

Tests performed

Tested mainly on an x86 platform and using rosbags, but also non-extensive tests on anvil were performed.

Notes for reviewers

It might have some spelling errors, please let me know.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

yabuta and others added 30 commits July 6, 2022 16:18
copy calibration tools from private repository
merge main for galactic release
…and calibration parameters into a file, change ui elements in eval mode.
…rinsics_evaluation_mode

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Copy link
Collaborator

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

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

Left some comments, but we still need to discuss the correctness of the rectification method

@SergioReyesSan
Copy link
Contributor Author

Left some comments, but we still need to discuss the correctness of the rectification method

I was checking a book that has a good explanation of the rectification process, and the equations used on the undistortion map seem to be good,

https://amroamroamro.github.io/mexopencv/matlab/cv.initUndistortRectifyMap.html

https://books.google.ch/books?id=seAgiOfu2EIC&lpg=PA373&pg=PA373#v=onepage&q&f=false

# if chessboard was found, keep track of the region to try to detect easily in the next frame
if detected:
self.roi = get_roi(corners, img.shape[:2])
self.lost_frames = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.lost_frames is reset in a protected way now, but it is not here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm wrong, but we are protecting this value because it might change the max value on the UI side. So, is it necessary to add a lock here?

@@ -992,6 +1016,9 @@ def process_detection_results(self, img: np.array, detection: BoardDetection, im
alpha_indicators=self.indicators_alpha_spinbox.value(),
value=False,
)
self.skip_next_img = (
self.frames_to_skip.value
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are accessed an unprotected value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SergioReyesSan SergioReyesSan requested a review from knzo25 January 21, 2025 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants