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 minimum_consecutive_frames to bytetrack #1076

Conversation

rolson24
Copy link
Contributor

@rolson24 rolson24 commented Mar 30, 2024

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

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

  • Docs updated?

@rolson24 rolson24 changed the title ByteTrack unit tests add minimum_consecutive_frames to bytetrack Mar 30, 2024
@@ -290,6 +299,7 @@ def callback(frame: np.ndarray, index: int) -> np.ndarray:
return detections[detections.tracker_id != -1]

else:
detections = Detections.empty()
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@LinasKo LinasKo Apr 3, 2024

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)

Copy link
Contributor Author

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).

Copy link
Contributor

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()
Copy link
Contributor

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.

@LinasKo
Copy link
Contributor

LinasKo commented Apr 4, 2024

@SkalskiP, handing it over to you - happy to merge.

With self.minimum_consecutive_frames=1 this causes no adverse effects, but otherwise increases the chance that neither tracking nor detections will be shown to the user.

This is related to #943, and would be solved when tracker_id is used consistently throughout the codebase - I'll open an issue.

@SkalskiP
Copy link
Collaborator

SkalskiP commented Apr 5, 2024

Hi, @rolson24 and @LinasKo 👋🏻 I tested the code included in this PR and have some comments.

  • test with minimum_consecutive_frames = 0
people-walking-result-0.mp4
  • test with minimum_consecutive_frames = 10
people-walking-result-10.mp4

I focused primarily on two aspects: tracker ID stability and the overall number of generated tracker IDs.

  • If we focus our attention on the group of people standing on the left side, we see that with minimum_consecutive_frames = 0, the following IDs appear: #20, #21, #39, #79, #119, #186, and #292. When we change minimum_consecutive_frames = 10, the following IDs appear: #20, #21, #185, and #316. It works!

  • On the other hand, I see that we are generating tons of tracker IDs. In the configuration with minimum_consecutive_frames = 10, we even reached #452. I think the problem lies in the fact that even tracks that are only candidates and ultimately not used are assigned an ID.

    Screenshot 2024-04-05 at 16 04 04

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:
Copy link
Collaborator

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.

Copy link
Contributor Author

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)

Copy link
Collaborator

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.

Copy link
Contributor Author

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!)
add_min_frames_test_video_1_frame

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.
add_min_frames_test_video_15_frames

Here is the video for minimum_consecutive_frames = 10:

test_box_min_consecutive_frames_15.1.-.Trim.mp4

Copy link
Collaborator

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)

Copy link
Contributor Author

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.

@rolson24 rolson24 force-pushed the add-minimum_consecutive_frames-to-bytetrack-better branch from 64efb72 to 2c9a1f6 Compare April 10, 2024 21:58
@rolson24
Copy link
Contributor Author

Ok I believe I have fixed the issue. The problem was that each track was being re-assigned a new id when it was found after being lost. There also was an issue with the default case of minimum_consective_frames = 1 producing slightly fewer tracks than the current release version because it would only return tracks that had already existed for 1 frame before it got assigned an external id. I fixed this so now it produces the exact same output as the release version.
This is the end of the test video from the release version of ByteTrack. You can see that the track_ids are lower than what I originally had:
add_min_frames_test_video_release

And this is the new version with minimum_consecutive_frames = 1. You can see that the track_ids are exactly the same:
add_min_frames_test_video_1_frame_final

And finally here is the new version with minimum_consecutive_frames = 10. You can see that the track_ids are much lower because the short tracks have been filtered out. The track_ids are also lower than the previous version due to fixing the re-assigning of track_ids every time a track is found after being lost. Additionally, the bottom middle person is not tracked yet because the track has not reached 10 frames long.

add_min_frames_test_video_15_frames_final

@rolson24
Copy link
Contributor Author

Just wanted to bump this. I think I fixed the issues we were running into.

@SkalskiP
Copy link
Collaborator

Hi @rolson24 👋🏻 I just tested the code, and it looks AWESOME! Thanks a lot for your contribution. 🙏🏻 Unfortunately, we already closed supervision-0.20.0 release, so this feature will need to wait till May/June to get released.

@SkalskiP SkalskiP merged commit df4c546 into roboflow:develop Apr 24, 2024
9 checks passed
@rolson24
Copy link
Contributor Author

Good to hear!

@rolson24 rolson24 mentioned this pull request Apr 27, 2024
1 task
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.

[ByteTrack] add minimum_consecutive_frames to limit the number of falsely assigned tracker IDs
3 participants