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

Migrate crop config creator node #191

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aljazkonec1
Copy link
Contributor

Purpose

This PR migrates the ProcessDetections node, renamed to CropConfigCreatorNode, from depthai-experiments to depthai-nodes. The node simplifies the pipeline by removing one extra script node and expands the usability as it unblocks RVC2 usage. The naming of the node and any other suggestions are kindly welcome!

Specification

Add a new node, called CropConfigCreatorNode. This node is used to create a dai.ImageManipConfigV2() object for every detection. The node works on both RVC2 and RVC4. Simultaneously, this node also addresses the issue with RVC2 with pool size that has been blocking depthai-experiments.

Dependencies & Potential Impact

No breaking changes outright, but an update to depthai-experiments will be needed. One risk is the way the node handles syncing of which crop config is for which frame. The node assumes the last frame is saved in the ImgManipV2 node and when recieving a detection_message it sends an empty crop config that skips the frame and loads the next frame in the queue. That frame should be the one from which the detection_message was gotten.

Deployment Plan

None / not applicable

Testing & Validation

Tested on age-gender experiment on this branch on both RVC2 and 4 for 0 to 6 faces present in the frame. No unsynced frames could be detected. PS: pay attention to how the node is used in main.py and how the following ImageManipNode (script_node) has to be set up in order for the correct sequence.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 31.25000% with 66 lines in your changes missing coverage. Please review.

Project coverage is 41.41%. Comparing base (69ffd94) to head (7e2e279).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
depthai_nodes/node/host_crop_config_creator.py 30.52% 66 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
- Coverage   41.63%   41.41%   -0.22%     
==========================================
  Files          75       76       +1     
  Lines        4285     4382      +97     
==========================================
+ Hits         1784     1815      +31     
- Misses       2501     2567      +66     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@kkeroo kkeroo left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. Please also add unit test for the node.

setReusePreviousImage flag set to False, which changes the previous frame for a
new one.
"""
assert isinstance(detections_input, ImgDetectionsExtended)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add here ... or isinstance(detections_input, dai.ImgDetection)? Might come handy if e.g. YOLO is the first model of the pipeline


for i in range(num_detections):
cfg = dai.ImageManipConfigV2()
detection: ImgDetectionExtended = detections[i]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the detections sorted by confidence? Because if not, we might not send out the desired ones (e.g. if a model makes 300 detections, and we limit them to 100, then it would make sense to output the top 100 most confident ones).

Copy link
Collaborator

@klemen1999 klemen1999 left a comment

Choose a reason for hiding this comment

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

Let's also add unittests

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.

6 participants