Modified key point tracking #151
Conversation
|
Automatic code review is enabled for this repository. Reviewing this PR now. |
![devloai[bot]](https://avatars.githubusercontent.com/in/886113?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've reviewed the changes to the key point tracking system. While the modifications improve functionality by supporting additional landmarks and adding Python bindings, there are a few important items to address: a security concern regarding landmark size validation, needed documentation improvements for the landmark array layout, and some code maintainability enhancements for the visualization code. Please review the specific comments and let me know if you need any clarification.
| } | ||
| void SetLandmark(const std::vector<inspirecv::Point2f> &lmk, bool update_rect = true, bool update_matrix = true, float h = 0.06f, int n = 5, | ||
| int num_of_lmk = 106 * 2) { | ||
| // if (lmk.size() != landmark_.size()) { |
There was a problem hiding this comment.
The size check for landmarks should not be commented out. This is an important validation that prevents buffer overflows and ensures data consistency. If the landmark size needs to be flexible, consider adding a resize operation instead of removing the check.
| // Update face key points | ||
| face.SetLandmark(landmark_back, true, true, m_track_mode_smooth_ratio_, m_track_mode_num_smooth_cache_frame_); | ||
| face.SetLandmark(landmark_back, true, true, m_track_mode_smooth_ratio_, m_track_mode_num_smooth_cache_frame_, | ||
| (FaceLandmarkAdapt::NUM_OF_LANDMARK + 10) * 2); | ||
| // Get the smoothed landmark | ||
| auto &landmark_smooth = face.landmark_smooth_aux_.back(); |
There was a problem hiding this comment.
The landmark handling has been changed from using explicit enum indices to using NUM_OF_LANDMARK offset. While this makes the code more maintainable, consider adding a comment explaining the layout of the landmark array and why the offset is needed. This will help future maintainers understand the data structure.
| @@ -119,6 +123,10 @@ def case_face_tracker_from_video(source, show, out): | |||
| for x, y in lmk.astype(int): | |||
| cv2.circle(frame, (x, y), 0, color, 4) | |||
|
|
|||
| five_key_points = session.get_face_five_key_points(face) | |||
There was a problem hiding this comment.
The visualization code for five key points uses inverted colors which may not be visually optimal in all cases. Consider using a more distinct color scheme or making the color choice configurable. Also, the magic number 6 for circle radius should be defined as a constant.
| @@ -315,7 +315,7 @@ | |||
| std::vector<STrack> output_stracks = m_TbD_tracker_->update(objects); | |||
| for (const auto &st_track : output_stracks) { | |||
| inspirecv::Rect<int> rect = inspirecv::Rect<int>(st_track.tlwh[0], st_track.tlwh[1], st_track.tlwh[2], st_track.tlwh[3]); | |||
| FaceObjectInternal faceinfo(st_track.track_id, rect, FaceLandmarkAdapt::NUM_OF_LANDMARK); | |||
| FaceObjectInternal faceinfo(st_track.track_id, rect, FaceLandmarkAdapt::NUM_OF_LANDMARK + 10); | |||
There was a problem hiding this comment.
The magic number 10 in NUM_OF_LANDMARK + 10 appears multiple times in the code. This should be defined as a named constant to improve maintainability and make the purpose of this offset clear.
No description provided.