Remove copies in image and video implementation#104
Conversation
|
I don't see new tests for checking the copy parameters. Are our current tests sufficient to make sure we are not losing image content anywhere? Our typical checks involve metadata verification. I guess that concern applies to the old code too but just thought of it now. |
|
What are the pros/cons of merging the ImageData/Image classes? That code was reviewed well back when and this change kind of breaks consistency across how video and descriptors were structured. Is that not true? |
ea80b48 to
48bd510
Compare
These changes have been moved to another branch and will be part of a future pull request |
We have tests for both, the metadata an pixels. We have many tests that check every single pixel of the image and video and compare it with direct OpenCV results. The copies in the images are tests through the video implementation that is using those copy and move constructors. All tests are passing. |
vishakha041
left a comment
There was a problem hiding this comment.
Just make sure it is thread safe and remove that all caps TODO comment. Rest looks fine.
|
|
||
| Image::~Image() | ||
| { | ||
| delete _image; |
There was a problem hiding this comment.
Just noticed. Is this never null? I think we talked about this but I am not remembering why there is no null check here.
There was a problem hiding this comment.
it is never null.
src/vcl/ImageData.cc
Outdated
| _tdb = NULL; | ||
|
|
||
| int start; | ||
| // TODO IS THIS REALLY NEEDED???? |
There was a problem hiding this comment.
Its better to not have comments like that. Either you can resolve it or file an issue and remove the comment
| _cv_type = cv_img.type(); | ||
|
|
||
| _cv_img = cv_img.clone(); | ||
| _cv_img = cv_img; // shallow copy |
There was a problem hiding this comment.
Probably a silly question but is this still thread safe? That would be hard to catch with our tests.
There was a problem hiding this comment.
none of this is thread safe. these changes do not affect that.
|
|
||
| for (auto& frame : frames) { | ||
| outputVideo << frame.get_cvmat(); | ||
| outputVideo << frame.get_cvmat(false); |
There was a problem hiding this comment.
I can see why the shallow copy would be important here!
include/vcl/Image.h
Outdated
| * @return A new Image object that is only the requested area | ||
| */ | ||
| Image get_area(const Rectangle &roi) const; | ||
| Image get_area(const Rectangle &roi); |
There was a problem hiding this comment.
move this change to the remove layer PR
|
|
||
| Image::~Image() | ||
| { | ||
| delete _image; |
There was a problem hiding this comment.
it is never null.
include/vcl/Image.h
Outdated
| * @return An OpenCV Mat | ||
| */ | ||
| cv::Mat get_cvmat() const; | ||
| cv::Mat get_cvmat(bool copy=false); |
There was a problem hiding this comment.
this should be true.
src/vcl/ImageData.cc
Outdated
| } | ||
| else { | ||
| img->copy_cv(cv::imread(_fullpath, cv::IMREAD_ANYCOLOR)); | ||
| auto img_read = cv::imread(_fullpath, cv::IMREAD_ANYCOLOR); |
include/vcl/Image.h
Outdated
| */ | ||
| Image(const cv::Mat &cv_img); | ||
|
|
||
| Image(cv::Mat &cv_img); |
There was a problem hiding this comment.
see if we can use a parameter instead
| if (front->get_type() == OperationType::READ) { | ||
| start = 1; | ||
| copy_cv(cv::imread(img._image_id, cv::IMREAD_ANYCOLOR)); | ||
| cv::Mat img_read = cv::imread(img._image_id, cv::IMREAD_ANYCOLOR); |
There was a problem hiding this comment.
check if this hurts or not.
| _cv_type = cv_img.type(); | ||
|
|
||
| _cv_img = cv_img.clone(); | ||
| _cv_img = cv_img; // shallow copy |
There was a problem hiding this comment.
none of this is thread safe. these changes do not affect that.
luisremis
left a comment
There was a problem hiding this comment.
Comments during code review with @philiplantz
d3efb8a to
5184192
Compare
|
All comments applied, all tests passing, noticeable improvement on video performance by avoing copies. Same should be good images that is now avoiding multiple copies. looks good to go! |
* change owner after each job
Optimized video and image code, avoiding copies of cvmats. This increase performance of video read and several image operations by up to 25%.