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

Add face landmark task #11

Closed
wants to merge 7 commits into from
Closed

Conversation

vinlemon
Copy link
Contributor

@vinlemon vinlemon commented Jul 17, 2024

This PR adds the Face Landmark task #7 .
The handling of output_face_blendshapes and output_facial_transformation_matrixes is still missing.

Copy link
Contributor

alabulei1 commented Jul 17, 2024

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Potential Issues and Errors:

  1. Missing Model URLs: Ensure all necessary URLs for the face landmark model are included and handled appropriately.
  2. Error Handling: Include mechanisms to handle download failures effectively in the script.
  3. Testing: Verify the functionality of downloading the model and its usage through thorough testing.
  4. Code Readability: Verify coding standards, variable naming conventions, and documentation for maintainability.
  5. Dependencies: Confirm correct setup or documentation of dependencies for the face landmark model.
  6. Input Validation: Validate input parameters such as landmark_radius_percent for the new method in DrawLandmarksOptions.
  7. Unit Tests: Add comprehensive unit tests for new methods and functionalities introduced.
  8. Documentation: Enhance code documentation to explain new structures, macros, and functionalities added clearly.

Important Findings:

  1. The pull request introduces various changes related to adding a face landmark task, including new functionalities, error checks, and test cases to the existing codebase.
  2. Considerations for code maintainability, readability, and reliability have been highlighted, emphasizing the need for thorough testing, adequate error handling, and adherence to coding standards.
  3. Attention is required to validate URLs, check model integrity, and ensure compatibility with existing components for a seamless integration of the face landmark task.
  4. Additionally, the introduction of new modules and structs necessitates consistency in naming conventions, documentation, and the avoidance of potential code duplication or maintenance issues.
  5. Confirming the accuracy of the model URL, verifying version adjustments, and managing dependencies are crucial before merging to maintain the stability and performance of the project.
  6. The overall changes appear significant, necessitating a holistic review to ensure they align with project requirements, do not introduce regressions, and enhance the functionality of the application effectively.

Details

Commit 032f416cbce47d2053332741719c5b8188dd7f57

Key Changes:

  1. Added a new function face_landmark_init() to the download-models.sh script to download a face landmark model.
  2. The function creates a directory for the face landmark model and downloads the model file from a specific URL.
  3. The function is called in the script after the face_detection_init() function, indicating the addition of a new model download task in the script.

Potential Problems:

  1. Missing Model URLs: The model_urls array in the face_landmark_init() function currently has only one element. Ensure that all necessary URLs for the face landmark model are included in this array or handled appropriately.

  2. Error Handling: The script currently uses curl -sLO "${url}" to download the model file. It may be beneficial to include error handling mechanisms in case the download fails or encounters issues.

  3. Testing: The patch adds a new feature to download the face landmark model. Ensure that this functionality has been tested to verify that the model is downloaded correctly and can be used as intended.

  4. Code Readability: Check if the code follows consistent coding standards, variable naming conventions, and is properly documented for maintainability.

  5. Potential Dependencies: Ensure that any dependencies required for the face landmark model are correctly set up or documented.

Overall, the addition of the face landmark model download task seems straightforward, but attention to error handling, testing, and completeness of the model URLs is essential to ensure the reliability and stability of the script.

Commit 35c7231c339c99501da132f3bed22843d5231b7d

Key Changes:

  1. Added a new method landmark_radius_percent to DrawLandmarksOptions struct in the file draw_landmarks.rs.
  2. The method sets the landmark_radius_percent field of the struct.

Potential Problems:

  1. The method landmark_radius_percent doesn't have any input validation for the landmark_radius_percent parameter. It should validate that the input is within a specific range or meets certain criteria.
  2. It's important to ensure that the method is correctly updating the landmark_radius_percent field without causing any side effects or breaking existing functionality.
  3. The commit message could be more descriptive to explain the rationale behind adding this new method and how it fits into the overall goal of the project.
  4. It's recommended to include unit tests for the new method to validate its functionality and ensure future changes don't break it.

Commit 5b473080e3e86d45846d2ff7a10533ee11f92dd8

Key Changes:

  1. Added a new struct FaceLandmarkOptions with fields related to face landmark tasks.
  2. Implemented default values for the fields in the FaceLandmarkOptions struct.
  3. Added macros for setting and getting values for the FaceLandmarkOptions struct.
  4. Added error checking in a macro for validating the input values.
  5. Modified the module to include the FaceLandmarkOptions struct.

Potential Problems:

  1. The implemented error checking logic in the macro might need more comprehensive testing to ensure it covers all possible edge cases.
  2. The use of macros for setting and getting values could lead to code duplication or maintenance issues if not handled carefully.
  3. It would be beneficial to have unit tests for the newly added functionality to ensure it works as expected and handles different scenarios correctly.
  4. Further documentation could be added to the code to explain the purpose and usage of the FaceLandmarkOptions struct and related macros for better understanding by other developers.

Commit bbe13e676ea1af8a44a8fe59240a9484d93f7765

Key changes in the pull request titled "Add face landmark task":

  1. Added a new module for face landmark under src/tasks/vision/face_landmark/ with several new files such as builder.rs, face_landmark_blendshapes.rs, face_landmark_connections.rs, mod.rs, and result.rs.
  2. The FaceLandmarkerBuilder struct was included to configure build options for a new face landmark task instance.
  3. Defined enums and constants for face landmark blendshapes and connections.
  4. Implemented methods to build face landmark tasks from buffers and handle model resources.
  5. Added functionality to parse model resources, check model integrity, and build graphs for the face landmarker.

Potential problems or considerations:

  1. The patch introduces a significant number of insertions, which could impact the codebase's size and complexity. Ensure that the added functionality aligns with project requirements and maintainability.
  2. The new files and modules should follow existing project conventions to maintain consistency in the codebase.
  3. Ensure that the enums, constants, and methods added for face landmark functionality are named appropriately and documented well for clarity.
  4. Consider adding unit tests for the new face landmark features to ensure they function correctly and prevent regressions.
  5. Review the error handling mechanisms, such as the Result usage, to handle potential failures during tasks like model parsing and graph building effectively.
  6. Check for any hardcoding or magic values that could be abstracted or made configurable for better flexibility.
  7. Verify if any dependencies or version compatibility requirements are introduced by these changes.
  8. Review the patch with a focus on readability, maintainability, and performance implications in the context of the existing codebase.

Commit 06464f42409855aa0777c2a738d90932120640df

Key changes:

  • Addition of a new test file tests/face_landmark.rs that includes tests for face detection and face landmark detection.
  • Implementation of test functions test_face_detection and test_face_landmark to perform face detection and face landmark detection using specific models and images.
  • Addition of a function draw_face_landmarks to draw landmarks on an image based on the detected face landmarks.

Potential problems:

  1. The patch introduces new test functionality, including face detection and face landmark detection. These tests rely on specific model paths and image paths. Ensure that these paths are correctly set up and accessible in the current project structure.
  2. The patch adds a function draw_face_landmarks to draw landmarks on an image. Make sure that the drawing functionality works correctly and does not introduce any performance issues.
  3. The display of debug information using eprintln! statements may be useful for testing and debugging but should be removed or replaced with appropriate logging mechanisms before merging the changes.
  4. Ensure that the new code adheres to the existing coding standards and style guidelines of the project. Consider adding relevant documentation and comments to improve code readability and maintainability.
  5. Verify that all new dependencies are correctly managed and included in the project setup to prevent build and runtime issues.

Commit fd371777a496cd971fb35750f47f778fd50613f5

Key Changes:

  1. The patch fixes the face landmark model URL in the download-models.sh script from "https://storage.googleapis.com/mediapipe-assets/face_landmark.tflite" to "https://storage.googleapis.com/mediapipe-tasks/face_landmarker/face_landmarker.task".
  2. In the FaceLandmarkerBuilder in builder.rs, the model_base_check_impl now checks for version 1, 2 instead of version 1, 3.

Potential Problems:

  1. The change in the model URL might lead to issues if the new URL is incorrect or if the model file is not available at that location.
  2. The adjustment in the model_base_check_impl version may need to be validated to ensure it aligns with the model being used and the expectations of the application.

Overall, the changes seem straightforward, but you should confirm that the new model URL is accurate, and the version adjustment in model_base_check_impl is in line with the requirements of the face landmark task.

vinlemon added 5 commits July 17, 2024 14:19
Signed-off-by: vinlemon <vinlemon0@gmail.com>
Signed-off-by: vinlemon <vinlemon0@gmail.com>
Signed-off-by: vinlemon <vinlemon0@gmail.com>
Signed-off-by: vinlemon <vinlemon0@gmail.com>
Signed-off-by: vinlemon <vinlemon0@gmail.com>
@vinlemon vinlemon force-pushed the feat-face-landmark branch from 278a3b0 to 06464f4 Compare July 17, 2024 12:22
@vinlemon vinlemon changed the title Add face landmark task #7 Add face landmark task Jul 17, 2024
@hydai
Copy link
Member

hydai commented Jul 17, 2024

Hi @yanghaku
Could you please help review this PR?

Signed-off-by: vinlemon <vinlemon0@gmail.com>
@vinlemon vinlemon force-pushed the feat-face-landmark branch from c60b835 to fd37177 Compare July 17, 2024 20:23
@yanghaku
Copy link
Contributor

@hydai Ok!

@vinlemon
Copy link
Contributor Author

@yanghaku , I'm waiting for your review :) The tests aren't failing because of my changes. Check here.

@vinlemon
Copy link
Contributor Author

Hi @hydai I fixed the issue with the tests. Can someone review this pull request?

Signed-off-by: vinlemon <vinlemon0@gmail.com>
@vinlemon vinlemon force-pushed the feat-face-landmark branch from 0343548 to 2a40453 Compare August 10, 2024 20:19
@vinlemon vinlemon closed this Aug 30, 2024
@yiyinglai
Copy link

@vinlemon just wondering, why is this PR closed?

@vinlemon
Copy link
Contributor Author

@yiyinglai No one reviewed or responded to my pull request, so I decided to close it. If I need to make any further changes, I’ll just use my own repo.

@hydai
Copy link
Member

hydai commented Sep 25, 2024

Hi @vinlemon

I am sorry that I missed this PR, previously I expected there is already an reviewer for it. However, it dosn't.
I would be the new reviewer for this; if you are still interested in contributing, please feel free to reopen it.

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.

5 participants