levan92 / deep_sort_realtime

A really more real-time adaptation of deep sort
MIT License
166 stars 51 forks source link

Update distance metric module (partial fit) only considers latest feature vectors #41

Closed romarcg closed 1 year ago

romarcg commented 1 year ago

https://github.com/levan92/deep_sort_realtime/blob/f11a5869ccd669338aacf6f19364b7cd3596b89f/deep_sort_realtime/deep_sort/tracker.py#L124

When preparing the list of feature vectors per confirmed track, the track.features list is updated with the latest vector; hence the features vector history is lost in confirmed tracks. When computing the distance, the new detections are compared to the confirmed tracks with at most: 1. the latest 2 feature vectors (an already confirmed track), or 2. min_hit feature vectors (when the track is confirmed the first time).

Do you think this is an intended behavior? The nn_budget parameter is then somehow overridden. Comparing new detections to the few latest vectors in the confirmed tracks reduces the association performance; however, not doing so will increase the tracking time.

levan92 commented 1 year ago

Thanks for raising this issue, I think this might be quite a serious bug which we missed.

In the original implementation, https://github.com/nwojke/deep_sort/blob/280b8bdb255f223813ff4a8679f3e1321b08cdfc/deep_sort/tracker.py#L89 It seems like the history is completely erased (there's actually an open years-old issue raised there), and my current change of taking the last feature was made 5 years ago.

But it does seem like nn_budget is imposed in nn_matching.NearestNeighborDistanceMetric. The historical features for the active tracks are all stored in that metric object (with the nn_budget in mind).

Let me investigate and look into it!

levan92 commented 1 year ago

https://github.com/levan92/deep_sort_realtime/blob/f11a5869ccd669338aacf6f19364b7cd3596b89f/deep_sort_realtime/deep_sort/tracker.py#L124

Upon closer inspection, I think reverting said line 124 back to track.features = [] as originally implemented is correct, though not ideal. Let me explain:

Every update step, the features from each confirmed track is passed into the metric object and stored there, with metric being a deep_sort.nn_matching.NearestNeighborDistanceMetric class. This following method of NearestNeighborDistanceMetric is called with these features as inputs and stored into self.samples in the metric object:

https://github.com/levan92/deep_sort_realtime/blob/f11a5869ccd669338aacf6f19364b7cd3596b89f/deep_sort_realtime/deep_sort/nn_matching.py#L135-L152

For instance, when a track is still tentative (at the start), the track's features are stored within the track object in track.features and not "erased". But once the track is confirmed, these features are passed into the metric object and stored there, then erased from track.features. Our current track.features = [track.features[-1]] line actually will cause the last feature to be double-included each time. nn_budget is still imposed in NearestNeighborDistanceMetric, by limiting its self.samples, though because of this double inclusion, it effectively halves the nn_budget.

I think in the short term, I will push a fix that replaces track.features = [track.features[-1]] with track.features = [] in tracker.py.

However, the reason why I say this short term fix is not ideal, is because then the historical features are not stored in confirmed tracks. These features are potentially useful for downstream tasks (for example doing person re-identification across cameras), and is more intuitively and conveniently accessed via the track objects. Although we can still access them via the metric object now, but I think it isn't ideal.

In the long term, I will consider storing the historical features within the track class instead of within NearestNeighborDistanceMetric, and get the NearestNeighborDistanceMetric to access the respective track features via the track objects only during distance calculation.

Thanks for raising this issue. Let me know if you catch anything wrong with my explanation and feel free to raise more questions.

romarcg commented 1 year ago

You are right. The bug was related to duplicating the latest feature rather than removing the other features in the track's history. All of the previous track's features are indeed saved in NearestNeighborDistanceMetric.

I also agree with adding an additional mechanism to store the track's history in the Track's object. For the use case you mention, storing only the latest feature vector per confirmed track should be enough. A downstream module should save these vectors without relying on the tracking module to keep tracks that were confirmed but are not followed anymore (e.g., deleted confirmed tracks).