-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 minimum_consecutive_frames to bytetrack #1076
add minimum_consecutive_frames to bytetrack #1076
Conversation
@@ -290,6 +299,7 @@ def callback(frame: np.ndarray, index: int) -> np.ndarray: | |||
return detections[detections.tracker_id != -1] | |||
|
|||
else: | |||
detections = Detections.empty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rolson24 👋
These changes look good, but I wanted to ask about this line.
Can you bring me up-to-speed for why resetting the detections to empty
is needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @LinasKo,
I made a PR last week that introduced a small bug where if the update_with_tensors function doesn't return any valid tracks and we set the tracker_id array to an empty array, then we run into issues with the annotators because they expect the tracker_id array to be the same length as the other arrays in the Detections object. Also we wouldn't want to return the detections that aren't tracked anyways, so the easiest solution I found is to just return an empty Detections object with an empty tracker_id array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense. I bumped into the unequally-sized array issue myself a few weeks back.
I need to sleep on this a bit, but I believe we do need the untracked detections to come out the other side unharmed.
The tracker tutorials look as follows:
model = YOLO("yolov8n.pt")
tracker = sv.ByteTrack()
box_annotator = sv.BoundingBoxAnnotator()
def callback(frame: np.ndarray, _: int) -> np.ndarray:
results = model(frame)[0]
detections = sv.Detections.from_ultralytics(results)
detections = tracker.update_with_detections(detections)
return box_annotator.annotate(frame.copy(), detections=detections)
Suppose there are many similar objects in the scene, lots of movement, or things tend to appear for brief moments only - detectable, but hard to track. Would be a shame if that produced no detections (and no annotations - user sees nothing happening).
I think it's a matter of initializing the tracker_id array. Do you think it's possible? Or would we need a bunch of None values that we can't cram in there?
(Note to self: possible setup of #943 error, possible trigger in update_with_tensors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I guess we could change the tracker_id array to also store None values, and put None values in the tracker_id array. Alternatively, we could put -1 for the tracker_id (can only do this after update_with_tensors) and then change the put()
method in the Trace class to only add a detection to the existing traces if the tracker_id is valid (non-negative).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one to handle. But you're right - it is better to let the tracker eat the detections instead of crashing the annotator. Well done for spotting that!
I'll open an issue about tracker_id
. When it's used consistently throughout the codebase, then we might revisit this part.
Happy to merge.
@@ -290,6 +299,7 @@ def callback(frame: np.ndarray, index: int) -> np.ndarray: | |||
return detections[detections.tracker_id != -1] | |||
|
|||
else: | |||
detections = Detections.empty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one to handle. But you're right - it is better to let the tracker eat the detections instead of crashing the annotator. Well done for spotting that!
I'll open an issue about tracker_id
. When it's used consistently throughout the codebase, then we might revisit this part.
Happy to merge.
@SkalskiP, handing it over to you - happy to merge. With This is related to #943, and would be solved when |
Hi, @rolson24 and @LinasKo 👋🏻 I tested the code included in this PR and have some comments.
people-walking-result-0.mp4
people-walking-result-10.mp4I focused primarily on two aspects: tracker ID stability and the overall number of generated tracker IDs.
I think we could solve this problem by initializing the track tracker ID as None and only assigning it when the track is activated. |
@@ -60,7 +62,7 @@ def activate(self, kalman_filter, frame_id): | |||
|
|||
self.tracklet_len = 0 | |||
self.state = TrackState.Tracked | |||
if frame_id == 1: | |||
if frame_id == 1 and self.minimum_consecutive_frames == 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could solve the problem of cycling through tracker IDs too quickly if we actually assigned a tracker_id only when that condition is met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This condition only handles the first frame of the video. I considered only assigning a tracker_id
once the condition on line 98 was met, (track has existed for longer than minimum_consecutive_frames
), but that would break the rest of the tracker because it relies on tracker_id to identify different tracks in joint_stracks and sub_stracks, so tracker_id must have a unique value as soon as it gets activated.
One possible solution is to create an additional variable in the STrack object called something like internal_tracker_id
that is used for all of the ByteTrack internal logic, and then leave the original tracker_id
variable to only get assigned when a track has existed for long enough. This would allow the tracker to function as usual, and would make the output detections have reasonable tracker_ids.
I also considered just using the tracker_id
variable and when a track is first initialized assigning the tracker_id
to a temporary value with a new function just like next_id
called next_temporary_id()
. Then once a track has existed for minimum_consecutive_frames
, the tracker_id would be reassigned to a valid track_id with next_id()
. But I believe this solution would be problematic because there is a high likelihood that the next_temporary_id()
and next_id()
values would overlap, causing two tracks to share the same tracker_id. This could be solved by making the temporary_id's negative, but I don't know if we want to do that.
The final solution I have thought of is redesigning sub_stracks and joint_stracks to use iou overlap like remove_duplicate_tracks does instead of relying on tracker_id to identify tracks, which would allow more freedom to use tracker_id in other ways. (this would slow down the tracker a bit because of the linear assignment problem)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rolson24 👋🏻
I'm very sorry for the lack of response. I somehow missed the email notification that you had replied.
In that case, I am leaning towards a solution that introduces an internal and external tracker ID. The internal one can have arbitrary values, while the external one should have consecutive tracker IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I have implemented this and it seems to be working.
Here you can see the end of the video with minimum_consecutive_frames = 1
. The tracker ids have gotten to quite a high number for a 13 second video (463!)
And here is the video for minimum_consecutive_frames = 1
:
test_box_min_consecutive_frames_1.2.-.Trim.mp4
And here you can see the same area of the video with minimum_consecutive_frames = 10
. The tracker ids have now only reached 110, and the person in the bottom middle is not tracked because the track hasn't existed for long enough.
Here is the video for minimum_consecutive_frames = 10
:
test_box_min_consecutive_frames_15.1.-.Trim.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rolson24 👋🏻 #463
seems high. It is pretty much the same result we got here: #1076 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm i just checked with the release version and you seem to be correct. I will take a look.
64efb72
to
2c9a1f6
Compare
…ttps://github.com/rolson24/supervision into add-minimum_consecutive_frames-to-bytetrack-better
Just wanted to bump this. I think I fixed the issues we were running into. |
Hi @rolson24 👋🏻 I just tested the code, and it looks AWESOME! Thanks a lot for your contribution. 🙏🏻 Unfortunately, we already closed |
Good to hear! |
Description
This PR fixes #1044. It replaces this PR #1050 which caused only one track to be initialized per frame (see the comment) I decided to re-implement this by instead of setting the tracker_id to -1, I used the existing is_activated property of STrack to mark whether the detection is valid or not. This does not affect the functionality of ByteTrack because every unconfirmed track will get added to the self.tracked_stracks list every time it is matched with a new detection.
Type of change
How has this change been tested, please provide a testcase or example of how you tested the change?
I tested this by running writing pytests for ByteTrack and using them in a colab notebook. I will submit another PR with these tests added to supervision. They cover several cases, including all of the edge cases that I have run into.
Any specific deployment considerations
This might require updating the documentation for ByteTrack, which I am willing to do if this is approved.
Docs