Fixed memory leak in Frame pointer #425
Conversation
|
I am reviewing this PR. While reviewing, I looked little deeper into the memory management and it looks like few very risky things are present in the code. They either need to be verified by UT + fix or at least, documented very loudly. This is not a comment on the code changes in this PR. Please await more details. |
|
We need to put these comments to make sure that no body things its dead code |
|
Another issue is with 
|
|
Another alternative for this is overloading the |
|
|
||
| if (!isOriginal) { | ||
| // The pointer has been moved forward by skipBytes | ||
| ptrdiff_t offset = static_cast<uint8_t*>(pointer->data()) - static_cast<uint8_t*>(pointer->myOrig); |
There was a problem hiding this comment.
we need a check for myOrig not being NULL before this. Ideally at the start of the function. There is currently as pathway possible that myOrig is NULL. That check is present at end of this function but the pgm will crash here itself.
| auto finalHealth = factory->getPoolHealthRecord(); | ||
| LOG_INFO << "Final pool health after pointer increment test: " << finalHealth; | ||
|
|
||
| tracker.printStats("pointer_increment_leak_test"); |
There was a problem hiding this comment.
why not boost assert tracker.hasLeaks() also ?
| } | ||
|
|
||
| // Test 2: Simulate the skipBytes pointer increment issue | ||
| BOOST_AUTO_TEST_CASE(framefactory_pointer_increment_leak_test) |
There was a problem hiding this comment.
Please add these tests at "Frame" class level as well
- Offsetting from front of frame
- Shrinking from end of frame
You can add them here as well
|
|
||
| BOOST_AUTO_TEST_CASE(mp4_reader_memory_leak) | ||
| { | ||
| std::string videoPath = "./data/Mp4_videos/h264_video_metadata/20230514/0011/1686723796848.mp4"; |
There was a problem hiding this comment.
is this video pushed in the repo? test should be reproducible.
|
|
||
| BOOST_AUTO_TEST_CASE(mp4_reader_memory_leak) | ||
| { | ||
| std::string videoPath = "./data/Mp4_videos/h264_video_metadata/20230514/0011/1686723796848.mp4"; |
There was a problem hiding this comment.
make sure the video is in repo so that test is reproducible.
| LOG_INFO<<"Stopping pipeline"; | ||
| p->stop(); | ||
| p->term(); | ||
| p->wait_for_all(); |
There was a problem hiding this comment.
what is the test here ? I dont see any asserts. Please improve this somehow such that manual intervention is not required.
Filename had \r, made it impossible to checkout on windows
…e see no logs. It is being caused due to this line where we try to log before Logger::initLogger(..) is called. The application code should make sure that Logger::initLogger(..) is the first line of application. if it is not, then they should make sure that they are only using std::coud and std::cerr for "Early logs". This should be improved to be a more permanent solution where Logger::initLogger(..) overwrites previous log instances
…bs/ApraPipes into NVR_Release_Mem_Leak_FIx
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #[Issue]
Description
Precise description of the changes in this pull request
Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Type
Type Choose one: (Bug fix | Feature | Documentation | Testing | Other)
Screenshots (if applicable)
Checklist