Skip to main content
added 3370 characters in body
Source Link
Cris Luengo
  • 7k
  • 1
  • 15
  • 37

Edit: I just noticed that you have three Image constructors that take a std::vector with the pixel values as input:

        template<std::same_as<std::size_t>... Sizes>
        Image(std::vector<ElementT>&& input, Sizes... sizes):
            size{sizes...}, image_data(begin(input), end(input))
        {
            if (image_data.size() != (1 * ... * sizes)) {
                throw std::runtime_error("Image data input and the given size are mismatched!");
            }
        }

        Image(std::vector<ElementT>& input, std::vector<std::size_t> sizes):
            size{sizes}, image_data(begin(input), end(input))
        {

        }


        Image(std::vector<ElementT>& input, std::size_t newWidth, std::size_t newHeight)
        {
            size.reserve(2);
            size.emplace_back(newWidth);
            size.emplace_back(newHeight);
            if (input.size() != newWidth * newHeight)
            {
                throw std::runtime_error("Image data input and the given size are mismatched!");
            }
            image_data = std::move(input);              //  Reference: https://stackoverflow.com/a/51706522/6667035
        }

The first one takes the vector by r-value reference, the other two by l-value reference. So the first one has to be invoked by Image(std::move(new_data), 0), the second one by Image(new_data, input.getSize()) (it's the one you call), and the third one by Image(new_data, 0, 0).

Note that the second one is the only one that doesn't check for the given image sizes to match the array size. The first two allow for images of any dimensionality, and the third one is explicitly 2D.

But the worst thing here is what you do with the input vectors: In the first one, where you get a vector that the caller cannot use any more (it's an r-value reference, a reference to a temporary, which means that the user either moved from it, or it was the output of a function that was't stored in a local variable), and you're free to steal the data, but you copy the values from the vector. In the second one you also copy the values. In the third one, you steal the data from the input l-value reference, which the user will not at all expect. If you call this constructor as Image(new_data, 0, 0), you don't expect new_data to no longer be useful after this.

Without regard for how the size is specified, you need to rewrite all of these constructors to take the vector by value, like so:

        Image(std::vector<ElementT> input, std::vector<std::size_t> sizes):
            size{std::move(sizes)}, image_data(std::move(input)) {}

Why? What does this do? When you call this function with an r-value (a temporary), or by moving a vector, then input is this same vector. If you call the function with an l-value, input will be a copy, and the caller's vector will not be unexpectedly broken.

So, Image(new_data, input.getSize()) will copy the vector new_data, but Image(std::move(new_data), input.getSize()) will steal the data from new_data. The user gets to decide!

Read the post you linked in your code again: https://stackoverflow.com/a/51706522/6667035 We're doing (1) here. Alternatively do (2) (one constructor takes a const&, the other a &&). On that page, no constructor takes a & like yours.


Edit: I just noticed that you have three Image constructors that take a std::vector with the pixel values as input:

        template<std::same_as<std::size_t>... Sizes>
        Image(std::vector<ElementT>&& input, Sizes... sizes):
            size{sizes...}, image_data(begin(input), end(input))
        {
            if (image_data.size() != (1 * ... * sizes)) {
                throw std::runtime_error("Image data input and the given size are mismatched!");
            }
        }

        Image(std::vector<ElementT>& input, std::vector<std::size_t> sizes):
            size{sizes}, image_data(begin(input), end(input))
        {

        }


        Image(std::vector<ElementT>& input, std::size_t newWidth, std::size_t newHeight)
        {
            size.reserve(2);
            size.emplace_back(newWidth);
            size.emplace_back(newHeight);
            if (input.size() != newWidth * newHeight)
            {
                throw std::runtime_error("Image data input and the given size are mismatched!");
            }
            image_data = std::move(input);              //  Reference: https://stackoverflow.com/a/51706522/6667035
        }

The first one takes the vector by r-value reference, the other two by l-value reference. So the first one has to be invoked by Image(std::move(new_data), 0), the second one by Image(new_data, input.getSize()) (it's the one you call), and the third one by Image(new_data, 0, 0).

Note that the second one is the only one that doesn't check for the given image sizes to match the array size. The first two allow for images of any dimensionality, and the third one is explicitly 2D.

But the worst thing here is what you do with the input vectors: In the first one, where you get a vector that the caller cannot use any more (it's an r-value reference, a reference to a temporary, which means that the user either moved from it, or it was the output of a function that was't stored in a local variable), and you're free to steal the data, but you copy the values from the vector. In the second one you also copy the values. In the third one, you steal the data from the input l-value reference, which the user will not at all expect. If you call this constructor as Image(new_data, 0, 0), you don't expect new_data to no longer be useful after this.

Without regard for how the size is specified, you need to rewrite all of these constructors to take the vector by value, like so:

        Image(std::vector<ElementT> input, std::vector<std::size_t> sizes):
            size{std::move(sizes)}, image_data(std::move(input)) {}

Why? What does this do? When you call this function with an r-value (a temporary), or by moving a vector, then input is this same vector. If you call the function with an l-value, input will be a copy, and the caller's vector will not be unexpectedly broken.

So, Image(new_data, input.getSize()) will copy the vector new_data, but Image(std::move(new_data), input.getSize()) will steal the data from new_data. The user gets to decide!

Read the post you linked in your code again: https://stackoverflow.com/a/51706522/6667035 We're doing (1) here. Alternatively do (2) (one constructor takes a const&, the other a &&). On that page, no constructor takes a & like yours.

Source Link
Cris Luengo
  • 7k
  • 1
  • 15
  • 37

When you create an empty vector, and repeatedly emplace_back() pixels, you do a lot of reallocations that you can avoid by reserve()ing the right number of pixels before the loop.

Next, you put this vector into your image constructor, which copies the data over. Thus, you are copying the data twice. Consider std::move()ing the vector, and have a constructor that doesn’t copy the vector but moves it.

One more comment: MATLAB’s im2double divides the pixel values by 255, and im2uint8 multiplies them by 255. I’m not saying you should do this, I think it’s wasted compute cycles. But saying “like in MATLAB” could mislead some users.