ITEP-93534: Multi-camera object metadata fusion in time-chunking#1636
ITEP-93534: Multi-camera object metadata fusion in time-chunking#1636MikolajKasprzak wants to merge 16 commits into
Conversation
| const std::string field = key.substr(std::char_traits<char>::length(MetadataPrefix)); | ||
| const auto confidence = metadataConfidence(object, field); | ||
| const auto winner = winners.find(field); | ||
| const bool replace = winner == winners.end() || (confidence && !winner->second.confidence) |
There was a problem hiding this comment.
confidence && !winner->second.confidence
This scenario is also not covered in the controller/src/robot_vision/test/MetadataFusionTests.cpp
| const bool replace = winner == winners.end() || (confidence && !winner->second.confidence) | ||
| || (confidence && winner->second.confidence | ||
| && (*confidence > *winner->second.confidence | ||
| || (*confidence == *winner->second.confidence && cameraIndex > winner->second.cameraIndex))) |
There was a problem hiding this comment.
*confidence == *winner->second.confidence && cameraIndex > winner->second.cameraIndex
This scenario is also not covered in the controller/src/robot_vision/test/MetadataFusionTests.cpp
There was a problem hiding this comment.
I agree that all possible cases in this expression must be covered with unit tests.
|
|
||
| const auto InitialTimestamp = std::chrono::system_clock::time_point(std::chrono::milliseconds(10)); | ||
|
|
||
| TEST(MetadataFusionTest, HighestConfidenceWinsPerField) |
There was a problem hiding this comment.
Two tests missing -
- current object has confidence && winner does not have confidence
- both objects have the same confidence
| assert metadata["gender"]["label"] == "female" | ||
| assert metadata["age"]["label"] == "senior" | ||
|
|
||
| validate_scene_output({ |
There was a problem hiding this comment.
This validate_scene_output call wraps the candidate_track with a fabricated envelope, so the only meaningful thing we validate is the candidate_track's payload.
My suggestion is to pass the actual received message that contained candidate_track so that the envelope schema drift is also caught here.
tdorauintc
left a comment
tdorauintc
left a comment
There was a problem hiding this comment.
Reviewed partially, I still need to look into fusion logic more carefully.
| const bool replace = winner == winners.end() || (confidence && !winner->second.confidence) | ||
| || (confidence && winner->second.confidence | ||
| && (*confidence > *winner->second.confidence | ||
| || (*confidence == *winner->second.confidence && cameraIndex > winner->second.cameraIndex))) |
There was a problem hiding this comment.
I agree that all possible cases in this expression must be covered with unit tests.
| client.disconnect() | ||
|
|
||
|
|
||
| TEST_NAME = "NEX-T25896" |
There was a problem hiding this comment.
nit: The variable is never referenced
| res2.wait_for_publish() | ||
|
|
||
| if i < 19: | ||
| import time |
There was a problem hiding this comment.
nit: I suggest to do the import once at the top
| candidate_track = None | ||
| for msg in received_messages: | ||
| for obj in msg.get("objects", []): | ||
| metadata = obj.get("metadata") |
There was a problem hiding this comment.
Please make this null-safe the same way as above: use obj.get("metadata", {}) here too. Otherwise metadata can be None and metadata.get(...) can raise AttributeError, causing flaky E2E failures unrelated to fusion logic.
4 participants
📝 Description
Adds deterministic multi-camera metadata fusion to RobotVision. Metadata is merged per field, preferring the highest confidence or the latest camera when confidence is unavailable. Includes legacy and tracker-service support with unit and end-to-end coverage.
✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: