itsuki2021 / sort-cpp

C++ implementation of SORT, a simple online and realtime tracking algorithm
MIT License
7 stars 6 forks source link

Bugs and question about license #1

Open simon-hauser opened 2 years ago

simon-hauser commented 2 years ago

Hi, Thanks for your work with this code! I don't know if the code works for you as it is published, but I had to fix two specific bugs to make it work (VS2022): one in convertXToBBox and one when dead trackers are removed. Would you like me to submit a pull request? Also, I noticed that hungarian is published under BSD license, but the rest of the code has no license. Would you mind publishing the rest also under BSD license or similar (MIT)? cheers, Simon

itsuki2021 commented 2 years ago

Hi, Thanks for your work with this code! I don't know if the code works for you as it is published, but I had to fix two specific bugs to make it work (VS2022): one in convertXToBBox and one when dead trackers are removed. Would you like me to submit a pull request? Also, I noticed that hungarian is published under BSD license, but the rest of the code has no license. Would you mind publishing the rest also under BSD license or similar (MIT)? cheers, Simon

Thanks for your kind reply! I write this code for self improving mainly and it works well on test data (det.txt files and some local videos), but I haven't run it in real applications, so there may be some mistakes in my code and glad to know you are willing to help :) Sorry for license missing, MIT license has been added, thank you for your remind!

simon-hauser commented 2 years ago

The current code produced runtime errors for me. In hungarian.cpp, I had to change all the __DBL_EPSILON__ to DBL_EPSILON. Not sure if this might be compiler specific? In kalman_box_tracker.cpp, lines 113-115, I had to switch all the indexes (e.g. (0,1) to (1,0)) because the state vector has dimension (7,1) but the current code treats it as (1,7). In sort.cpp, when you erase trackers on lines 29 and 65, I needed to change it to it = trackers.erase(it); to reset the iterator to the next item, otherwise the iterator becomes invalidated due to the deletion in the loop.

With these changes I can use the code for real applications and it seems to work correctly.

There is one difference though to the original python sort implementation by abewley that I would like to discuss. Do you have his implementation up and running? When e.g. using a webcam for real-time detection, I would initially get ID 0 in both versions. However, leaving the screen and reentering the view assigns an increasing ID number to me in the python code (ID 1 after reentering, leaving and reentering gives me ID 2 and so on). In your code however, reentering after having ID 1 gives me ID 0 again. Having e.g. 3 persons in the view of the camera (IDs 0, 1, 2), when they leave and a new object comes into view, it will have ID 4 in python, but ID 0 in your code. It's not wrong per se, but I couldn't quickly figure out where this difference could come from. I'm curious if you noticed this or if you would know what makes the behavior different? cheers

itsuki2021 commented 2 years ago

In hungarian.cpp, __DBL_EPSILON__ and DBL_EPSILON seems be compiler specific, the former can be compiled in my environment (Ubuntu 20.04 + cmake 3.16.3) but the latter can‘t.

In kalman_box_tracker.cpp, I treat X (state vector) and Z (measurement vector) as column vectors with shape(7, 1) and shape(4, 1), same as cv::KalmanFilter. The shape of bbox variables is (1, 4+), I stack these row vectors to get the output bboxes (M, 4+). All shapes are as follows: variable shape data
bboxesDet (M1, 6) [[xc, yc, w, h, score, class_id]; [...]; ...]
bboxesPred (M2, 6) [[xc, yc, w, h, score, class_id]; [...]; ...]
bboxesPost (M3, 7) [[xc, yc, w, h, score, class_id, tracker_id]; [...]; ...]
xPred (7, 1) [xc; yc; s; r; dx; dy; ds]
xPost (7, 1) [xc; yc; s; r; dx; dy; ds]
Z(temporary variable) (4, 1) [xc; yc; s; r]

In kalman_box_tracker.cpp, line 116, I fixed a problem, please check it.

// hitStreak = 0 ? timeSinceUpdate > 0 : hitStreak;
hitStreak = timeSinceUpdate > 0 ? 0 : hitStreak;  // after fixing

In sort.cpp, It's my wrong use of erase(), thank you for your correct!

"reentering after having ID 1 gives ID 0 again", Yes, this code behaves different. In python code ID will increase infinitely, so I made a small change to avoid it, ID will be recycled when trackers died and these IDs will be assigned to the new trackers. please undo it if this change makes post-processing hard. kalman_box_tracker.cpp


// queue<int> KalmanBoxTracker::idQueue;

KalmanBoxTracker::KalmanBoxTracker(const cv::Mat &bbox)
{   
    /*
    if (!idQueue.empty())
    {
        id = idQueue.front();
        idQueue.pop();
    }
    else
        id = KalmanBoxTracker::count;
    */
    id = KalmanBoxTracker::count;    // assign the new ID directly

    ...
}

KalmanBoxTracker::~KalmanBoxTracker()
{
   // KalmanBoxTracker::count--;
   // idQueue.push(id);
    delete kf;
}

kalman_box_tracker.h

...
// #include <queue>

...

namespace sort
{
    // using std::queue;

    class KalmanBoxTracker
    {
    public:
    private:
        static int count;
        // static queue<int> idQueue;
simon-hauser commented 2 years ago

Hi, Thanks for the quick answer. I didn't write it correctly in my previous message about index swaping, it should have been in kalman_box_tracker.h, lines 113-115. You are converting a state x with shape (7,1) to a bbox with shape (1,4). However, in the assignment of y, w and h, you are accessing the state with state.at<float>(0,1), (0,2) and (0,3), which are invalid for the state vector dimensions. So I switched the indexes here, and this works. I cannot understand why this works in your case?

The hitStreak correction works, although I didn't observe a problem with it before, but maybe I just didn't look out for it.

Thanks for checking with the ID assignment. I implemented the changes but it didn't quite work in my case, I now can have multiple detections with the same ID in my live stream. I will check again after the weekend to do some more tests on that, maybe I find something since I now know where ID assignment is handled.

itsuki2021 commented 2 years ago

Oh, this is my carelessness, it does access the wrong position. I wrote some code to understand why it still works:

cv::Mat x = (cv::Mat_<float>(7, 1) << 0, 1, 2, 3, 4, 5, 6);
cout << "x.size = " << x.size << "\n\n";

for (int i = 0; i < 7; ++i) cout << x.at<float>(i, 0) << " ";
cout << "\n\n";

for (int i = 0; i < 7; ++i) cout << x.at<float>(0, i) << " ";
cout << "\n\n";

/*
output:
---------------
x.size = 7 x 1

0 1 2 3 4 5 6 

0 1 2 3 4 5 6 

*/

maybe opencv has done some inner work to correct it.

"I now can have multiple detections with the same ID in my live stream.", sorry about my omission, KalmanBoxTracker::count should not be reduced when tracker died, previous comment has been updated, please check the code in KalmanBoxTracker::~KalmanBoxTracker().

Would you mind share your results after the test job? a pull request will be greatly appreciated, thank you! :)

simon-hauser commented 2 years ago

Hi, It might depend then on the openCV version if the example code you wrote about accessing the wrong position works. I used 4.5.0 and it threw a runtime error. But in any case, good that the matter could be solved.

With the additional correction about indexing it now works! Maybe this could be handled as an additional parameter how indexing should happen (continuous or restarting). Or maybe it's just me who was interested in this, so you can leave it like this and somebody can just do what you wrote to get the other indexing ;).

I don't have any specific results to share, was just using a live webcam stream for person detection and then passing the detections to your tracker to get tracking IDs. This works nicely now :).

I can do a pull request with your changes, but I'm not sure how you want to handle the indexing thing? cheers

itsuki2021 commented 2 years ago

Glad to know it works!

Seems the restarting indexing brings some confusion to post-processing, so how about the continuous one?

itsuki2021 commented 2 years ago

Hi, It might depend then on the openCV version if the example code you wrote about accessing the wrong position works. I used 4.5.0 and it threw a runtime error. But in any case, good that the matter could be solved.

With the additional correction about indexing it now works! Maybe this could be handled as an additional parameter how indexing should happen (continuous or restarting). Or maybe it's just me who was interested in this, so you can leave it like this and somebody can just do what you wrote to get the other indexing ;).

I don't have any specific results to share, was just using a live webcam stream for person detection and then passing the detections to your tracker to get tracking IDs. This works nicely now :).

I can do a pull request with your changes, but I'm not sure how you want to handle the indexing thing? cheers

Hi, I've submitted a PR named Kuhn munkres to handle the problems in our discussion, please check it! :)