Fix rectifyRoi when used with binning and/or ROI #378

mintar · 2021-03-04T13:23:14Z

The previous implementation used K_ and P_ (the matrices that include binning and ROI). However, the ROI is specified in unbinned and untransformed raw image coordinates (see REP-104), so rectifyRoi has to use K_full_ and P_full_.

mintar · 2021-03-04T13:32:01Z

Here are some side-by-side comparisons of the effect of this PR. Shown in red is the rawRoi (drawn in the raw image before rectification), green is the rectifiedRoi.

Left: without this PR, Right: with this PR

no binning, no ROI (output is identical)

binning = 2, no ROI (output is identical)

no binning, with ROI (output is fixed by this PR)

with binning, with ROI (output is fixed by this PR)

Code

The following code was used to produce these results:

TEST_F(PinholeTest, rectifiedRoiSize) {
  // --- prepare grid_img
  cv::Mat grid_img(model_.fullResolution(), CV_8UC3, cv::Scalar(0, 0, 0));

  // draw a grid
  const cv::Scalar color = cv::Scalar(255, 255, 255);
  const int thickness = 7;
  const int type = 8;
  for (size_t y = 0; y <= grid_img.rows; y += grid_img.rows / 10) {
    cv::line(grid_img,
             cv::Point(0, y), cv::Point(cam_info_.width, y),
             color, type, thickness);
  }
  for (size_t x = 0; x <= grid_img.cols; x += grid_img.cols / 10) {
    cv::line(grid_img,
             cv::Point(x, 0), cv::Point(x, cam_info_.height),
             color, type, thickness);
  }

  cv::Rect rectified_roi = model_.rectifiedRoi();
  cv::Size reduced_resolution = model_.reducedResolution();
  EXPECT_EQ(0, rectified_roi.x);
  EXPECT_EQ(0, rectified_roi.y);
  EXPECT_EQ(640, rectified_roi.width);
  EXPECT_EQ(480, rectified_roi.height);
  EXPECT_EQ(640, reduced_resolution.width);
  EXPECT_EQ(480, reduced_resolution.height);

  {
    cv::Mat raw_img = grid_img.clone();
    cv::rectangle(raw_img, model_.rawRoi(), cv::Scalar(0, 0, 255), 1);
    raw_img = raw_img(model_.rawRoi());
    cv::resize(raw_img, raw_img, cv::Size(), 1.0 / model_.binningX(), 1.0 / model_.binningY(), cv::INTER_NEAREST);
    cv::Mat rectified_img;
    model_.rectifyImage(raw_img, rectified_img);

    cv::Mat full_rectified_img(model_.fullResolution(), CV_8UC3, cv::Scalar(0, 0, 0));
    cv::resize(rectified_img, rectified_img, cv::Size(), model_.binningX(), model_.binningY(), cv::INTER_NEAREST);
    rectified_img.copyTo(full_rectified_img(model_.rawRoi()));
    cv::rectangle(full_rectified_img, model_.rectifiedRoi(), cv::Scalar(0, 255, 0), 1);
    cv::imwrite("/tmp/img.png", full_rectified_img);
  }

  cam_info_.binning_x = 2;
  cam_info_.binning_y = 2;
  model_.fromCameraInfo(cam_info_);
  rectified_roi = model_.rectifiedRoi();
  reduced_resolution = model_.reducedResolution();
  EXPECT_EQ(0, rectified_roi.x);
  EXPECT_EQ(0, rectified_roi.y);
  EXPECT_EQ(640, rectified_roi.width);
  EXPECT_EQ(480, rectified_roi.height);
  EXPECT_EQ(320, reduced_resolution.width);
  EXPECT_EQ(240, reduced_resolution.height);

  {
    cv::Mat raw_img = grid_img.clone();
    cv::rectangle(raw_img, model_.rawRoi(), cv::Scalar(0, 0, 255), 1);
    raw_img = raw_img(model_.rawRoi());
    cv::resize(raw_img, raw_img, cv::Size(), 1.0 / model_.binningX(), 1.0 / model_.binningY(), cv::INTER_NEAREST);
    cv::Mat rectified_img;
    model_.rectifyImage(raw_img, rectified_img);

    cv::Mat full_rectified_img(model_.fullResolution(), CV_8UC3, cv::Scalar(0, 0, 0));
    cv::resize(rectified_img, rectified_img, cv::Size(), model_.binningX(), model_.binningY(), cv::INTER_NEAREST);
    rectified_img.copyTo(full_rectified_img(model_.rawRoi()));
    cv::rectangle(full_rectified_img, model_.rectifiedRoi(), cv::Scalar(0, 255, 0), 1);
    cv::imwrite("/tmp/img_with_binning.png", full_rectified_img);
  }

  cam_info_.binning_x = 1;
  cam_info_.binning_y = 1;
  cam_info_.roi.x_offset = 100;
  cam_info_.roi.y_offset = 50;
  cam_info_.roi.width = 400;
  cam_info_.roi.height = 300;
  cam_info_.roi.do_rectify = true;
  model_.fromCameraInfo(cam_info_);
  rectified_roi = model_.rectifiedRoi();
  reduced_resolution = model_.reducedResolution();
  EXPECT_EQ(137, rectified_roi.x);
  EXPECT_EQ(82, rectified_roi.y);
  EXPECT_EQ(321, rectified_roi.width);
  EXPECT_EQ(242, rectified_roi.height);
  EXPECT_EQ(321, reduced_resolution.width);
  EXPECT_EQ(242, reduced_resolution.height);

  {
    cv::Mat raw_img = grid_img.clone();
    cv::rectangle(raw_img, model_.rawRoi(), cv::Scalar(0, 0, 255), 1);
    raw_img = raw_img(model_.rawRoi());
    cv::resize(raw_img, raw_img, cv::Size(), 1.0 / model_.binningX(), 1.0 / model_.binningY(), cv::INTER_NEAREST);
    cv::Mat rectified_img;
    model_.rectifyImage(raw_img, rectified_img);

    cv::Mat full_rectified_img(model_.fullResolution(), CV_8UC3, cv::Scalar(0, 0, 0));
    cv::resize(rectified_img, rectified_img, cv::Size(), model_.binningX(), model_.binningY(), cv::INTER_NEAREST);
    rectified_img.copyTo(full_rectified_img(model_.rawRoi()));
    cv::rectangle(full_rectified_img, model_.rectifiedRoi(), cv::Scalar(0, 255, 0), 1);
    cv::imwrite("/tmp/img_with_roi.png", full_rectified_img);
  }

  cam_info_.binning_x = 2;
  cam_info_.binning_y = 2;
  cam_info_.roi.x_offset = 100;
  cam_info_.roi.y_offset = 50;
  cam_info_.roi.width = 400;
  cam_info_.roi.height = 300;
  cam_info_.roi.do_rectify = true;
  model_.fromCameraInfo(cam_info_);
  rectified_roi = model_.rectifiedRoi();
  reduced_resolution = model_.reducedResolution();
  EXPECT_EQ(137, rectified_roi.x);
  EXPECT_EQ(82, rectified_roi.y);
  EXPECT_EQ(321, rectified_roi.width);
  EXPECT_EQ(242, rectified_roi.height);
  EXPECT_EQ(160, reduced_resolution.width);
  EXPECT_EQ(121, reduced_resolution.height);

  {
    cv::Mat raw_img = grid_img.clone();
    cv::rectangle(raw_img, model_.rawRoi(), cv::Scalar(0, 0, 255), 1);
    raw_img = raw_img(model_.rawRoi());
    cv::resize(raw_img, raw_img, cv::Size(), 1.0 / model_.binningX(), 1.0 / model_.binningY(), cv::INTER_NEAREST);
    cv::Mat rectified_img;
    model_.rectifyImage(raw_img, rectified_img);

    cv::Mat full_rectified_img(model_.fullResolution(), CV_8UC3, cv::Scalar(0, 0, 0));
    cv::resize(rectified_img, rectified_img, cv::Size(), model_.binningX(), model_.binningY(), cv::INTER_NEAREST);
    rectified_img.copyTo(full_rectified_img(model_.rawRoi()));
    cv::rectangle(full_rectified_img, model_.rectifiedRoi(), cv::Scalar(0, 255, 0), 1);
    cv::imwrite("/tmp/img_with_binning_and_roi.png", full_rectified_img);
  }
}

The previous implementation used K_ and P_ (the matrices that include binning and ROI). However, the ROI is specified in unbinned and untransformed raw image coordinates (see REP-104), so rectifyRoi has to use K_full_ and P_full_.

Each test runs in its own test fixture, so restoring the original state at the end of a test is not necessary.

mintar · 2021-04-19T07:38:22Z

@mjcarroll : I've rebased this PR onto the latest noetic branch to fix merge conflicts. It's ready for review now.

mintar · 2021-05-04T08:20:46Z

@mjcarroll : Could you review this PR?

This fixes a problem with the following sequence: 1. fromCameraInfo is called with ROI A. | rectified_roi_dirty = true 2. rectifiedRoi is called | rectified_roi_dirty = false 3. fromCameraInfo is called with ROI B. | rectified_roi_dirty = true 4. fromCameraInfo is called with ROI B. | rectified_roi_dirty = false The bug is that rectified_roi_dirty was incorrectly set to `false` by the last line. If now rectifiedRoi is called again, the cached rectified_roi for ROI A will be returned, but it should be recalculated based on ROI B. This commit fixes that.

mintar · 2021-06-04T15:24:40Z

Could somebody review this PR? It's rather small and has tests to prove it's right.

mintar · 2021-10-26T09:29:22Z

Ping again... could somebody merge this please?

flynneva · 2021-10-26T13:45:37Z

@wjwwood @clalancette anything you guys could do here to help get this merged? I've been following this PR....painfully over the last 8 months and it looks like a great contribution.

flynneva · 2021-10-26T13:53:20Z

also maybe @mjcarroll could help out here too?

mintar · 2021-11-22T10:48:10Z

@mjcarroll Monthly reminder to please merge this...

* Fix rectifyRoi when used with binning and/or ROI The previous implementation used K_ and P_ (the matrices that include binning and ROI). However, the ROI is specified in unbinned and untransformed raw image coordinates (see REP-104), so rectifyRoi has to use K_full_ and P_full_. * test: Remove useless restoring of original state Each test runs in its own test fixture, so restoring the original state at the end of a test is not necessary. * Don't overwrite rectified_roi_dirty flag This fixes a problem with the following sequence: 1. fromCameraInfo is called with ROI A. | rectified_roi_dirty = true 2. rectifiedRoi is called | rectified_roi_dirty = false 3. fromCameraInfo is called with ROI B. | rectified_roi_dirty = true 4. fromCameraInfo is called with ROI B. | rectified_roi_dirty = false The bug is that rectified_roi_dirty was incorrectly set to `false` by the last line. If now rectifiedRoi is called again, the cached rectified_roi for ROI A will be returned, but it should be recalculated based on ROI B. This commit fixes that.

mintar added 2 commits April 16, 2021 18:36

Fix rectifyRoi when used with binning and/or ROI

5a2dd76

The previous implementation used K_ and P_ (the matrices that include binning and ROI). However, the ROI is specified in unbinned and untransformed raw image coordinates (see REP-104), so rectifyRoi has to use K_full_ and P_full_.

test: Remove useless restoring of original state

d98101d

Each test runs in its own test fixture, so restoring the original state at the end of a test is not necessary.

mintar force-pushed the fix_rectify_roi branch from 48ff427 to d98101d Compare April 16, 2021 16:37

mintar mentioned this pull request Nov 5, 2021

New Noetic release? #416

Closed

mjcarroll approved these changes Nov 23, 2021

View reviewed changes

mjcarroll merged commit 15ddb06 into ros-perception:noetic Nov 23, 2021

mintar deleted the fix_rectify_roi branch November 24, 2021 12:25

mintar mentioned this pull request Jan 13, 2022

Forward-port PRs from noetic to ros2 #420

Merged

ijnek mentioned this pull request Sep 17, 2022

foxy and galactic branch contains breaking changes since last release #485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rectifyRoi when used with binning and/or ROI #378

Fix rectifyRoi when used with binning and/or ROI #378

mintar commented Mar 4, 2021

mintar commented Mar 4, 2021

mintar commented Apr 19, 2021

mintar commented May 4, 2021

mintar commented Jun 4, 2021

mintar commented Oct 26, 2021

flynneva commented Oct 26, 2021

flynneva commented Oct 26, 2021

mintar commented Nov 22, 2021

Fix rectifyRoi when used with binning and/or ROI #378

Fix rectifyRoi when used with binning and/or ROI #378

Conversation

mintar commented Mar 4, 2021

mintar commented Mar 4, 2021

no binning, no ROI (output is identical)

binning = 2, no ROI (output is identical)

no binning, with ROI (output is fixed by this PR)

with binning, with ROI (output is fixed by this PR)

Code

mintar commented Apr 19, 2021

mintar commented May 4, 2021

mintar commented Jun 4, 2021

mintar commented Oct 26, 2021

flynneva commented Oct 26, 2021

flynneva commented Oct 26, 2021

mintar commented Nov 22, 2021