Remove copies in image and video implementation #104

luisremis · 2019-05-03T22:40:23Z

Optimized video and image code, avoiding copies of cvmats. This increase performance of video read and several image operations by up to 25%.

vishakha041 · 2019-05-10T18:50:27Z

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.

vishakha041 · 2019-05-10T18:53:36Z

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?

luisremis · 2019-05-10T19:01:33Z

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?

These changes have been moved to another branch and will be part of a future pull request

luisremis · 2019-05-10T19:04:28Z

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.

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

Just make sure it is thread safe and remove that all caps TODO comment. Rest looks fine.

vishakha041 · 2019-05-15T06:07:20Z

src/vcl/Image.cc

 void Image::operator=(const Image &img)
 {
    ImageData *temp = _image;
    _image = new ImageData(*img._image);
    delete temp;
 }

-
 Image::~Image()
 {
    delete _image;


Just noticed. Is this never null? I think we talked about this but I am not remembering why there is no null check here.

it is never null.

vishakha041 · 2019-05-16T19:42:24Z

src/vcl/ImageData.cc


    if ( img._tdb != NULL )
        _tdb = new TDBImage(*img._tdb);
    else
        _tdb = NULL;

    int start;
+    // TODO IS THIS REALLY NEEDED????


Its better to not have comments like that. Either you can resolve it or file an issue and remove the comment

vishakha041 · 2019-05-16T19:44:15Z

src/vcl/ImageData.cc


    _cv_type = cv_img.type();

-    _cv_img = cv_img.clone();
+    _cv_img = cv_img; // shallow copy


Probably a silly question but is this still thread safe? That would be hard to catch with our tests.

none of this is thread safe. these changes do not affect that.

vishakha041 · 2019-05-16T19:44:58Z

src/vcl/VideoData.cc

@@ -137,7 +137,7 @@ void VideoData::Write::operator()(VideoData *video)
    std::vector<VCL::Image>& frames = video->get_frames();

    for (auto& frame : frames) {
-        outputVideo << frame.get_cvmat();
+        outputVideo << frame.get_cvmat(false);


I can see why the shallow copy would be important here!

luisremis · 2019-05-17T00:34:45Z

include/vcl/Image.h

@@ -173,14 +185,16 @@ namespace VCL {
         *    y coordinate, height, width)
         *  @return A new Image object that is only the requested area
         */
-        Image get_area(const Rectangle &roi) const;
+        Image get_area(const Rectangle &roi);


move this change to the remove layer PR

luisremis · 2019-05-17T00:42:00Z

src/vcl/Image.cc

 void Image::operator=(const Image &img)
 {
    ImageData *temp = _image;
    _image = new ImageData(*img._image);
    delete temp;
 }

-
 Image::~Image()
 {
    delete _image;


it is never null.

luisremis · 2019-05-17T00:46:15Z

include/vcl/Image.h

         *  @return An OpenCV Mat
         */
-        cv::Mat get_cvmat() const;
+        cv::Mat get_cvmat(bool copy=false);


this should be true.

luisremis · 2019-05-17T00:49:23Z

src/vcl/ImageData.cc

@@ -64,7 +64,8 @@ void ImageData::Read::operator()(ImageData *img)
        img->_channels = img->_tdb->get_image_channels();
    }
    else {
-        img->copy_cv(cv::imread(_fullpath, cv::IMREAD_ANYCOLOR));
+        auto img_read = cv::imread(_fullpath, cv::IMREAD_ANYCOLOR);


luisremis · 2019-05-17T01:08:53Z

include/vcl/Image.h

@@ -86,6 +86,8 @@ namespace VCL {
         */
        Image(const cv::Mat &cv_img);

+        Image(cv::Mat &cv_img);


see if we can use a parameter instead

luisremis · 2019-05-17T01:18:07Z

src/vcl/ImageData.cc

@@ -381,7 +401,8 @@ void ImageData::operator=(const ImageData &img)
        std::shared_ptr<Operation> front = img._operations.front();
        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);


check if this hurts or not.

luisremis · 2019-05-17T01:21:16Z

src/vcl/ImageData.cc


    _cv_type = cv_img.type();

-    _cv_img = cv_img.clone();
+    _cv_img = cv_img; // shallow copy


none of this is thread safe. these changes do not affect that.

luisremis

Comments during code review with @philiplantz

luisremis · 2019-05-17T23:36:42Z

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

luisremis requested review from vishakha041 and philiplantz May 3, 2019 22:40

luisremis force-pushed the optimize_image_video_impl branch from ea80b48 to 48bd510 Compare May 10, 2019 19:00

vishakha041 previously approved these changes May 16, 2019

View reviewed changes

luisremis commented May 17, 2019

View reviewed changes

Remove copies in image and video implementation

5184192

luisremis dismissed vishakha041’s stale review via d3efb8a May 17, 2019 23:25

luisremis force-pushed the optimize_image_video_impl branch 2 times, most recently from d3efb8a to 5184192 Compare May 17, 2019 23:33

luisremis merged commit daacc52 into develop May 17, 2019

luisremis deleted the optimize_image_video_impl branch July 2, 2019 20:35

cwlacewe added a commit to cwlacewe/vdms that referenced this pull request Apr 14, 2023

89 upgrade GitHub actions workflow (IntelLabs#104)

2271a58

* change owner after each job

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove copies in image and video implementation #104

Remove copies in image and video implementation #104

luisremis commented May 3, 2019

vishakha041 commented May 10, 2019

vishakha041 commented May 10, 2019

luisremis commented May 10, 2019

luisremis commented May 10, 2019 •

edited

Loading

vishakha041 left a comment

vishakha041 May 15, 2019

luisremis May 17, 2019

vishakha041 May 16, 2019

vishakha041 May 16, 2019

luisremis May 17, 2019

vishakha041 May 16, 2019

luisremis May 17, 2019

luisremis May 17, 2019

luisremis May 17, 2019

luisremis May 17, 2019

luisremis May 17, 2019

luisremis May 17, 2019

luisremis May 17, 2019

luisremis left a comment

luisremis commented May 17, 2019

Remove copies in image and video implementation #104

Remove copies in image and video implementation #104

Conversation

luisremis commented May 3, 2019

vishakha041 commented May 10, 2019

vishakha041 commented May 10, 2019

luisremis commented May 10, 2019

luisremis commented May 10, 2019 • edited Loading

vishakha041 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisremis left a comment

Choose a reason for hiding this comment

luisremis commented May 17, 2019

luisremis commented May 10, 2019 •

edited

Loading