-
Notifications
You must be signed in to change notification settings - Fork 294
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
Return VideoBuffer to VideoBufferPool when shared pointer is destroyed #282
Conversation
Current code was just a factory, it didn't return the object to the pool when it was released |
include/VideoBufferPool.h
Outdated
pool->enqueue(p); | ||
} else { | ||
//Delete it | ||
delete (buffer); |
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.
I think this should be changed to delete p
. For example, if the shared_ptr.reset(new VideoBuffer) is called, the deleter should not always try to delete the pointer when the shared_ptr was constructed.
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.
other than this, lgtm
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.
rigth
//Try to get a reference to the pool, as it may have been already deleted | ||
auto pool = weak.lock(); | ||
//Ensure we are not overallocating | ||
if (maxallocate && pool && pool->size_approx() < maxallocate) |
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.
If pool can go out of scope then can this->maxallocated also be out of scope?
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.
right, I thought about this, but not sure why I concluded that the =
in the capture was going to make a copy of it. Capturing it explicitly now.
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.
lgtm
No description provided.