Optimizing Image Processing in OpenCV: Using cv::Mat with Reference Counting Mechanisms and C++ ref qualifiers

Question

I have questions regarding this code:

As the cv::Mat copy constructor involves only pointer copying due to the reference counting mechanism, in the Apply method I return by copy instead of explicitly using std::move. This is because I also wanted to avoid moving the data member from the class.

Also, in the getters, I chose not to return by copy since the client code could modify the internal data by having a pointer. Therefore, I used a const reference. Additionally, for getters, I used reference qualifiers because returning a reference from instances of the rvalue category could cause dangling in some situations.

Am I right? are designs accurate?

EDIT Regarding why not a stand-alone function:

I considered using a class to encapsulate everything instead of having separate functions. Does that make sense?

load an image Apply CLAHE

converted to grayscale.

apply Gaussian blur.

apply Canny edge detection.

apply Hough Line Transform

List if lines it finds Draw lines

// Header

 #ifndef GAUSSIANBLURFILTER_H
 #define GAUSSIANBLURFILTER_H
 
 #include <opencv2/opencv.hpp>
 #include <optional>
 class GaussianBlurFilter
 {
 public:
 GaussianBlurFilter();
 
 // Disables copy and move constructors.
 GaussianBlurFilter(const GaussianBlurFilter&) = delete;
 
 cv::Mat getBlurredImage() &&
 {
 return std::move(m_blurredImage);
 }
 
 const cv::Mat& getBlurredImage() const &
 {
 return m_blurredImage;
 }
 
 [[nodiscard]] std::optional<cv::Mat> Apply(const cv::Mat& src);
 
 private:
 constexpr static double SIGMA = 2.5;
 constexpr static int KERNEL_DIMENSION = 3;
 inline const static cv::Size KERNEL_SIZE = cv::Size(KERNEL_DIMENSION, KERNEL_DIMENSION);
 
 cv::Mat m_blurredImage;
 };
 

// source

 #include "gaussianblurfilter.h"
 
 GaussianBlurFilter::GaussianBlurFilter() :
 m_blurredImage() {}
 
 [[nodiscard]] std::optional<cv::Mat> GaussianBlurFilter::Apply(const cv::Mat& src)
 {
 if (src.empty())
 {
 return std::nullopt;
 }
 
 cv::GaussianBlur(src, m_blurredImage, GaussianBlurFilter::KERNEL_SIZE, GaussianBlurFilter::SIGMA);
 
 return m_blurredImage;
 }

• Toby Speight: Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. It's best to describe what value this code provides to its user.
• sam: @TobySpeight Thank you! Does it look better?
• G. Sliepen: The code as you posted here should not be in a class in my opinion. If you want to do more transforms, then the answer is "maybe", but you should then create a new question on Code Review with the full code you want reviewed.
• sam: @G.Sliepen Sure. I appreciate all of your great help!
• Cris Luengo: Regarding your newly added question: a class only makes sense if you need to manage data or other resources. If you’re applying a sequence of image processing operation, you don’t have any data to manage, you input an image and you get a new image out. That’s a function. There’s a programming philosophy where everything is a class. I think that doesn’t make any sense, and can lead to awkward and overly complex software architecture. if you can write it in a function, prefer that.

Answer 1

The r-value qualified getBlurredImage() is not wrong, but there is a much simpler approach:

Why not create a stand-alone function?

Why is this a class to begin with? I would just make a stand-alone function that blurs an image:

cv::Mat guassianBlur(const cv::Mat& src)
{
 constexpr static double SIGMA = 2.5;
 constexpr static int KERNEL_DIMENSION = 3;
 const static cv::Size KERNEL_SIZE = cv::Size(KERNEL_DIMENSION, KERNEL_DIMENSION);
 
 cv::Mat blurredImage;
 cv::GaussianBlur(src, blurredImage, KERNEL_SIZE, SIGMA);
 return blurredImage;
}

Now you don't need to worry about copying or moving at all; return value optimization will ensure there is only one image constructed, and nothing is moved or copied. So this is optimal, regardless of whether cv::Mat uses reference counting or not.

Other issues

• Instead of returning a std::optional<cv::Mat>, why not return an empty image if src.empty()?
• Why is Apply() marked [[nodiscard]] if the blurred image is kept stored in the GaussianBlurFilter and can be retrieved by getBlurredImage()? I would rather make both getBlurredImage() functions [[noexcept]].
• Why does Apply() start with an upper case letter, but getBlurredImage() with a lower case one? Be consistend with your naming conventions.
• Why delete the copy and move constructors? There doesn't seem anything wrong with keeping them.

• sam: Thank you so much for the response! Very helpful! I just edited the post regarding 'Why not create a stand-alone function?" Does it make sense now to use classes for each step?
• sam: I think we don't have [[noexcept]] attribute. Did you mean just using noexcept the way often used for move operators?
• sam: You're right! I should have followed the 'Rule of Five.' The data member is of a class type and itself takes care of move and copy operations! Thank you!
• sam: I think using std::optional<cv::Mat> in Apply makes it explicit when the operation did not proceed due to the source image being empty. This approach clearly distinguishes between an operation that has been deliberately not performed (hence std::nullopt) and one that has processed but resulted in an empty image due to other reasons. It also prevents any ambiguity about whether the processing was successful or not, as an empty cv::Mat might still be considered a valid but resultless operation.
• G. Sliepen: I agree with using a std::optional (although maybe a std::expected would be even better since C++23) to distinguish between a successful operation and an unsuccessful one. But in this case, I think blurring an empty image should just produce an empty image as a result. It's a degenerate case, but not an invalid one, IMO.