I have trouble saving cv::Mat into 1D array

Question

I want to save cv::Mat data into a linear array, but I don't know why there are bugs. The image color is grayscale(CV_8UC1). Here is the code:

uchar* imgToArray(cv::Mat& img)
{
    int n = img.rows;
    int m = img.cols;
    uchar* res = new uchar(m * n);
    for(int row = 0; row < m; row++)
    {
        for(int col = 0; col < n; col++)
        {
            res[row * n + col] = img.at<uchar>(row, col);
        }
    }
    return res;
}

The debugging information mentioned,

Program received signal SIGSEGV, Segmentation fault.
0x000055555555c0e9 in imgToArray (img=..., n=512, m=512) at ../conversion.cpp:10
10              res[row * n + col] = img.at<uchar>(row, col);

I am quite confused about this. Thanks to anyone who gives advice in advance!

Answer 1

There are multiple issues in your code, other than the obvious incorrect allocation pointed out by rafix07. A question is whether you really need to have ownership of the array of uchar (if not, then there are much easier ways to go about this), but I'll assume you do. Let's start from the top.

uchar* imgToArray(cv::Mat& img, int n, int m);

1. Returning a raw pointer (uchar*).

First of all, we're returning an array, so let's make that explicit. Raw pointers are prone to memory leaks, and C++11 has been around for a while -- we can do better. Let's make it a std::unique_ptr, which can correctly handle arrays as well.

We can then allocate with std::make_unique<uchar[]>(size), where it's less likely you'll make the kind of mistake you have.

2. Returning a raw array without a size.

The first without the second is asking for trouble if you ever want to work with the returned data somehow. Relying on the user having to call another function to obtain that, or having to calculate it themselves is far from ideal. So, let's pack the size together with the smart pointer using std::pair.

typedef std::pair<std::unique_ptr<uchar[]>, size_t> uchar_array_t;
uchar_array_t imgToArray(cv::Mat& img, int n, int m);

3. Taking img as non-const reference, even when you don't intend to modify it.

If you really care about avoiding the overhead of copying the header, use a const reference to make your intent clear.

4. Arguments int n and int m are redundant. They're also not validated.

There's no reason to give the function anything else than a cv::Mat that corresponds to the area you want to turn into the "linear array". OpenCV already provides an efficient way to get a ROI: cv::Mat::operator(). Let the user use that, and drop the redundant arguments.

typedef std::pair<std::unique_ptr<uchar[]>, size_t> uchar_array_t;
uchar_array_t imgToArray(cv::Mat const& image);

5. Code assumes img is of datatype CV_8UC1 (i.e. a single channel matrix where each element is an unsigned byte), but does not verify that it's indeed the case.

This means when you call it with a different type of image, you will get nonsense out. This is undesireable. You could add some assertions to handle this, but IMHO it is better to make it explicit in the signature. Let's use cv::Mat1b instead of a plain cv::Mat.

typedef std::pair<std::unique_ptr<uchar[]>, size_t> uchar_array_t;
uchar_array_t imgToArray(cv::Mat1b const& image);

6. The code doesn't consider the scenario when it's given an empty matrix.

Let's add an assertion for this precondition: CV_Assert(!image.empty());

7. The incorrect allocation, as pointed out by rafix07.

This becomes moot since we changed our return type. We can rewrite it as such:

uchar_array_t result;
result.second = image.rows * image.cols;
result.first = std::make_unique<uchar[]>(result.second);

8. Copying the data by "manually" iterating over pixels and calling cv::Mat::at.

There are many pessimizations in this part, along with potential crashes or UB we've addressed in the previous points.

• cv::Mat::at involves an unnecessary overhead, when we know we're accessing valid pixels. Even if the matrix is not continuous, we can do a lot better by getting a pointer using cv::Mat::ptr for each row, and incrementing it.
• res[row * n + col] -- we're writing to the array in a sequence, so why recompute the position rather than simply incrementing a pointer?
• Doing this at all -- OpenCV allows us to create a cv::Mat using "foreign data", meaning an array we've allocated ourselves. By design it stores the data in this array in a liner fashion, just as you desire. It also provides a convenient method to copy data from one matrix to another.

So let's take advantage of that:

cv::Mat1b temp(image.rows, image.cols, result.first.get());
image.copyTo(temp);

---

Let's add this all together.

#include <opencv2/opencv.hpp>
#include <memory>

typedef std::pair<std::unique_ptr<uchar[]>, size_t> uchar_array_t;

uchar_array_t to_array(cv::Mat1b const& image)
{
    CV_Assert(!image.empty());

    uchar_array_t result;

    result.second = image.rows * image.cols;
    result.first = std::make_unique<uchar[]>(result.second);

    cv::Mat1b temp(image.rows, image.cols, result.first.get());
    image.copyTo(temp);

    return result;
}

void observe_data(uchar const[], size_t)
{
    // ...
}

void modify_data(uchar[], size_t)
{
    // ...
}

void take_ownership(uchar data[], size_t)
{
    // ...
    delete[] data;
}

int main()
{
    cv::Mat image(cv::imread("itYxy.png", cv::IMREAD_GRAYSCALE));

    uchar_array_t data_1(to_array(image));
    uchar_array_t data_2(to_array(image(cv::Rect(10, 10, 40, 50))));

    observe_data(data_2.first.get(), data_2.second);
    observe_data(data_2.first.get(), data_2.second);
    observe_data(data_2.first.release(), data_2.second);

    return 0;
}

You can use std::unique_ptr::get to observe the array, or std::unique_ptr::release to transfer ownership. Leaks are avoided, we have the sizes available so we can avoid accessing data beyond the bounds of the allocated arrays, preconditions are tested for.

---

Of course, if you don't need to own the array, much of this could be avoided. At most a clone of a matrix (if it's not continuous) followed by a call to cv::Mat::ptr.

Answer 2

You have created one int object with value m * n

uchar* res = new uchar(m * n);

not array, should be

uchar* res = new uchar[m * n];