Avoid creating macros

In C++ you almost never have to resort to macros. You can define constants using constexpr, these can then be used to declare array sizes. For example:

constexpr std::size_t RGB_H = 303;

Macros have lots of problems; two of those are that they don't respect namespaces, and that constants like you defined them don't have a proper type.

Create a separate type for RGB values

Using a three-dimensional array for the image is making things unnecessarily complex. An image has only two dimensions. Consider creating a struct or class to represent the RGB values of a pixel:

struct RGB {
    std::uint8_t r;
    std::uint8_t g;
    std::uint8_t b;
};

That way, you can declare the image like so:

RGB img[RGB_H][RGB_W];

There are libraries that provide vector types that might be helpful, for example GLM's glm::vec3, which also comes with a lot of mathematical functions that can directory work on these types.

Don't hardcode the image size

The biggest problems I've found with this code are:

• The width and height have to be typed in as #defines, so every RGBImage has to have the same size.

That is obviously very undesirable, so just don't do that. Of course the issue then is that you cannot declare a member variable as an array of which you don't know the size at compile time. The solution in C++ is to use a std::vector. You are already using those in your code, so you could have created a 2-dimensional vector the way you did for convolution kernels:

std::vector<std::vector<RGB>> image;

However, nested vectors are very inefficient. It's better to declare a one-dimensional array that is large enough to hold all pixels:

std::vector<RGB> image;

Store the width and height in member variables (you already declared width and height but you never used those). The size of the vector should of course be width * height. Then given x and y coordinates, you can access a pixel using:

image[x + y * width]

Is there a "correct" C++ way to efficiently store the image, with the ability to change its dimensions upon instantiation?

With a std::vector, the dimensions can be changed even after instantiation. Using a one-dimensional std::vector keeps everything tightly packed together in memory, which is usually the most efficient.

Be consistent

I'm not sure what the proper conventions are regarding the image editing itself, editing another image, or simply being a method which operates on images. Currently it's a mix of the first two.

To avoid confusing the user of your class, it would be great if your interface was very consistent. So either have everything modify the image in-place, or have everything return a new image with the original one intact, or possibly provide both options for every operation.

You already have two answers discussing #define, using namespace std, compile-time image size, and a bunch of other things. I will avoid repeating those points and discuss some other things:

1. Documentation: Make sure each function is documented. You'll thank me next year when you don't remember what you meant by thinEdges(). :)

2. Formatting: Some functions don't have space around them, they are declared on the very next line after the previous function. Empty lines around functions make the file more readable. I found it hard to find where you defined conv2d(), for example.

3. Images are most often stored as 8-bit or 16-bit unsigned integers, or as floating-point values. The former is how they come off the sensor, and how they're shown to screen. The floating-point representation is useful for increased precision in computations. unsigned int is a 32-bit unsigned integer, and not terribly useful. If you used (signed) int, you could store the result of a Sobel or Laplace filter (which produce negative values). But I would have chosen float because it's so much easier (e.g. no overflow, division by 0 errors, ), and it takes up the same amount of space! You could also parameterize the type by turning the class into a template, but that's a bit more advanced.

4. height and width are public variables. Someone using the class could change those values, making the object inconsistent and causing out-of-bounds memory access later on. Make these variables private, and add a function to read their values.

5. What should be class methods and what should be free functions? I am not a big fan of image processing functions being methods to the image class. I think things are clearer when these are free functions that take an image as input. This would remove one reason for the inconsistency in your function calls. For example, out = img.canny() is not as pretty to me as out = canny(img). But when multiple images are involved, the problem becomes more obvious. Your combineImgs(img1, img2) uses *this as the output, whereas other functions use *this as the input. As a free function, it's much more natural to define a function that takes two images as input: out = combineImgs(img1, img2).

   Class methods should be functions that work directly on the object, for example accessors, operators, and functions that modify it (e.g. change the image size).

6. combineImgs() adds up two images. Why not make this into an operator? You can declare RGBImage operator+(RGBImage const&, RGBImage const&), then you can use out = img1 + img2. This is (IMO) more expressive than combineImgs(), since there are so many different ways to combine images.

7. Actually, on second reading, it looks like canny() does not use *this at all, it takes an input image as other? Is *this used? If not, why is this a class method?

8. canny() takes sobelx and sobely kernels as input. This would make the function much harder to use: these kernels are fixed and never change, so you are asking the user to always put in the same values here. On the other hand, the original Canny doesn't use Sobel filters, it uses Gaussian derivatives, so you'd expect a sigma parameter for the Gaussian filter to be a parameter to canny().

9. The Canny filter has several stages: derivatives (you've got the conv2d() call here), non-maximum suppression, hysteresis threshold, and thinning (I don't see you using the thinning, this is something you could add). All three of these steps are useful outside the Canny filter. If you make these into separate functions, then the canny() function will be shorter and easier to read, and you'll have added functionality to your library! For example, this is how I implemented Canny in DIPlib, notice how the largest part of that function is the logic to compute thresholds from the inputs provided by the user.

10. Every time you do a triple loop (over i/x, j/y and d/channel) and apply the same operation to the value inside, you could simply use a single loop. unsigned int img[RGB_H][RGB_W][RGB_D] declares a contiguous memory block, you can simply loop over each integer using a single loop.

11. R_COEFF and the other two values are only used in one place. There is one function that converts RGB to grayscale. Declare these constants in that function, so that (1) it's easy to find their definition when you're reading the function, and (2) it's not as hard to find a name (being a global variable it must be unique within the file, inside the function there's a much smaller scope so name clashes are less likely). This becomes increasingly important as your project grows in size.

12. There's a bug in your code: if(i < 0 || j < 0 || i > RGB_H || i > RGB_W) doesn't do the check properly. If i == RGB_W, then you'll be writing out of bounds. It should be if(i < 0 || j < 0 || i >= RGB_H || i >= RGB_W). But instead of using the hard-coded constants, you should be using the height and width variables here. At some point you'll improve your code to have dynamic image sizes, and it'll be a lot more work then to change the code to use your size variables instead of the constants.

13. Another bug: getGrayscale() returns an int instead of an unsigned int.

Others have suggested the use of std::vector<> to store the image data. If you do that, it must be a private member, in the same way that your image sizes must be private members. You'll have a class invariance that is width * height * 3 == img.size(), and you need to be able to guarantee that this is always true. Making these variables private is the only way you can do that.

With this memory being private, you'll have to add pixel access functions. You already have getGrayscale() and setGrayscale(). You could consider, for example, defining unsigned int& operator()(int i, int j, int c). This returns a reference to a value in the image, so you can assign to it, image(4, 8, 0) = 5 would be valid. Of course you could also give it a regular function name, unsigned int& getValue(int i, int j, int c) for example; you'll still be able to assign to it: image.value(4, 8, 0) = 5.

Be very careful not to define a std::vector<std::vector<uint>> though. It is hard to initialize, and very inefficient. Just have a std::vector<uint>, and write your pixel getting/setting functions to compute the index. Outside of your class code you will never know or think about how it's implemented, you will always just use normal indexing. If you make the number of channels a parameter as well, you'll be able to use this class also for gray-scale images, which is what a lot of your code works with anyway.

Though for some advanced algorithms, it is actually really useful to do pointer arithmetic to access a pixel's neighbors. For those algorithms you will want to implement a function that returns a pointer to the first pixel (or to a pixel at a given coordinate), and you will want to have a function that says how many values to skip to get to the next pixel on the left/right (3 in your case) and bottom/top (3 * width in your case).

It has already been said a lot, but I would like to add a word.

We should put the implementation in a namespace to localize the names we introduce.

At the beginning, it may be easier to represent colors by means of a custom data type, like struct Color. It allows us to separate the color and image implementations and vary both independently. We could store Colors in a flat buffer instead of a matrix like it is done commonly.

std::vector is a well established data type. Using vector allows us to omit the image destructor and copy/move-constructor definitions because automatically generated versions of the definitions operate per class member, and std::vector data type has them all implemented.

We may use const and noexcept modifiers of class methods more extensively.

A constructor with a single parameter is treated as conversion constructor, e.g., std::string s = "hello";. Obviously, we don't intend to convert a string to an image. To make the intent explicit, supply the constructor with explicit keyword,

RGBImage img1 = "path"; // error, if the constructor is explicit
RGBImage img2 = RGBImage("path"); // ok

There is no need to call close on the file stream object. The destructor does it for us. But we should verify the file stream state more often to not miss an error.

getGrayscale member function returns a value of the red channel and can't guarantee that the image is actually grayscale. It is meant to be used in computationally intensive algorithms and may slower the program execution due to the ubiquitous bounds checking. Leave the bounds checking responsibility to the calling code.

Moreover, we represent our image as surrounded with the infinite black area. It may cause a lot of subtle computation errors, which in the end can only be determined visually.

The following code is a box filter implementation example. It depends on the header-only cimg library for quick visualization of the results. The header file resides in the github repository. I couldn't make it work with jpg images, probably due to the missing ImageMagick, but it works fine with ppm format. For now, we must hardcode a path to the image.

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <CImg.h>
#include <algorithm>


namespace myimglib
{

static constexpr double pi = 3.14159265358979323846;
static constexpr float eps = 1e-6;

struct Color {
    Color& operator=(float value) noexcept {
        r = g = b = value;
        return *this;
    }

    Color& operator*=(float k) noexcept {
        r *= k;
        g *= k;
        b *= k;
        return *this;
    }

    Color& operator/=(float k) noexcept {
        r /= k;
        g /= k;
        b /= k;
        return *this;
    }

    Color& operator+=(const Color& rhs) noexcept {
        r += rhs.r;
        g += rhs.g;
        b += rhs.b;
        return *this;
    }

    Color& operator-=(const Color& rhs) noexcept {
        r -= rhs.r;
        g -= rhs.g;
        b -= rhs.b;
        return *this;
    }

    friend Color operator/(Color lhs, float k) noexcept {
        return lhs /= k;
    }

    friend Color operator*(float k, Color rhs) noexcept {
        return rhs *= k;
    }

    friend Color operator+(Color lhs, const Color& rhs) noexcept {
        return lhs += rhs;
    }

    friend Color operator-(Color lhs, const Color& rhs) noexcept {
        return lhs -= rhs;
    }

    void clamp() {
        r = (r < eps) ? 0 : (r > 1 + eps) ? 1 : r;
        g = (g < eps) ? 0 : (g > 1 + eps) ? 1 : g;
        b = (b < eps) ? 0 : (b > 1 + eps) ? 1 : b;
    }

    float r;
    float g;
    float b;
};


class RGBImage {
public:
    RGBImage(std::ptrdiff_t height = 0, std::ptrdiff_t width = 0)
        : height_(height), width_(width),
        data_(height_ * width_, { 0, 0, 0 })
    { }

    explicit RGBImage(const std::string& filePath);

    auto height() const noexcept {
        return height_;
    }

    auto width() const noexcept {
        return width_;
    }

    const Color& operator()(std::ptrdiff_t x, std::ptrdiff_t y) const noexcept {
        return data_[y * width_ + x];
    }

    Color& operator()(std::ptrdiff_t x, std::ptrdiff_t y) noexcept {
        return data_[y * width_ + x];
    }

    friend RGBImage readImage(const std::string& filePath);
    friend bool writeImage(const RGBImage& image, const std::string& filePath);

    void dim(float factor) noexcept {
        std::for_each(std::begin(data_), std::end(data_), [factor](Color& c) {
            c *= factor;
        });
    }

    void grayscale(float rk, float gk, float bk) noexcept {
        std::for_each(std::begin(data_), std::end(data_), [rk, gk, bk](Color& c) {
            c = rk * c.r + gk * c.g + bk * c.b;
        });
    }

    void binaryThresh(float threshold) noexcept {
        std::for_each(std::begin(data_), std::end(data_), [threshold](Color& c) {
            c.r = (c.r > threshold + eps) ? 1 : 0;
            c.g = (c.g > threshold + eps) ? 1 : 0;
            c.b = (c.b > threshold + eps) ? 1 : 0;
        });
    }

    RGBImage conv2d(const std::vector<std::vector<int>>& kernel) const;

private:
    std::ptrdiff_t height_;
    std::ptrdiff_t width_;
    std::vector<Color> data_;
};


RGBImage readImage(const std::string& filePath) {
    std::ifstream file(filePath, std::ios::binary);
    if (!file) {
        std::cerr << "Failed to open file " << filePath << '\n';
        return {};
    }
    RGBImage image;
    if (!file.read(reinterpret_cast<char*>(&image.height_), sizeof(image.height_))
        .read(reinterpret_cast<char*>(&image.width_), sizeof(image.width_))) {
        std::cerr << "Failed to read image dimensions\n";
        return {};
    }
    auto n = image.height_ * image.width_;
    image.data_.resize(n);
    if (!file.read(reinterpret_cast<char*>(image.data_.data()), n * sizeof(Color))) {
        std::cerr << "Failed to read image content\n";
        return {};
    }
    return image;
}

RGBImage::RGBImage(const std::string& filePath)
    : RGBImage(readImage(filePath))
{ }

bool writeImage(const RGBImage& image, const std::string& filePath) {
    std::ofstream file(filePath, std::ios::binary);
    if (!file) {
        std::cerr << "Failed to open file " << filePath << '\n';
        return false;
    }
    if (!file.write(reinterpret_cast<const char*>(&image.height_), sizeof(image.height_))
        .write(reinterpret_cast<const char*>(&image.width_), sizeof(image.width_))) {
        std::cerr << "Failed to write image dimensions\n";
        return false;
    }
    if (!file.write(reinterpret_cast<const char*>(image.data_.data()),
        image.data_.size() * sizeof(Color))) {
        std::cerr << "Failed to write image content\n";
        return false;
    }
    return true;
}


RGBImage RGBImage::conv2d(const std::vector<std::vector<int>>& kernel) const {
    using std::ptrdiff_t;

    int ksum = 0;
    for (const auto& row : kernel)
        for (const int value : row)
            ksum += value;
    if (ksum <= 0) 
        ksum = 1;
    RGBImage out(height_, width_);
    const ptrdiff_t kheight = kernel.size();
    const ptrdiff_t hmid = kheight / 2;
    const ptrdiff_t kwidth = kernel[0].size();
    const ptrdiff_t wmid = kwidth / 2;
    for (ptrdiff_t oy = 0; oy < height_; ++oy) {
        for (ptrdiff_t ox = 0; ox < width_; ++ox) {
            Color outColor{ 0, 0, 0 };
            for (ptrdiff_t i = 0, y = oy - hmid; i < kheight && y <= oy + hmid && y < height_; ++i, ++y) {
                if (y < 0)
                    continue;
                for (ptrdiff_t j = 0, x = ox - wmid; j < kwidth && x <= ox + wmid && x < width_; ++j, ++x) {
                    if (x >= 0)
                        outColor += kernel[i][j] * (*this)(x, y);
                }
            }
            out(ox, oy) = outColor / ksum;
        }
    }
    return out;
}

} // myimglib


void boxblur(cimg_library::CImg<unsigned char>& img, int kernelSize) {
    myimglib::RGBImage myimage(img.height(), img.width());
    for (int y = 0; y < img.height(); ++y) {
        for (int x = 0; x < img.width(); ++x) {
            myimage(x, y) = {
                float(img(x, y, 0)) / 255,
                float(img(x, y, 1)) / 255,
                float(img(x, y, 2)) / 255
            };
        }
    }
    std::vector<std::vector<int>> kernel(kernelSize, std::vector<int>(kernelSize, 1));
    auto modified = myimage.conv2d(kernel);
    for (int y = 0; y < img.height(); ++y) {
        for (int x = 0; x < img.width(); ++x) {
            img(x, y, 0) = modified(x, y).r * 255;
            img(x, y, 1) = modified(x, y).g * 255;
            img(x, y, 2) = modified(x, y).b * 255;
        }
    }
}

int main()
{
    using namespace cimg_library;
    const char* imagePath = "YOUR PATH TO IMAGE";
    CImg<unsigned char> image(imagePath), visu(500, 400, 1, 3, 0);
    boxblur(image, 9);

    const unsigned char red[] = { 255,0,0 }, green[] = { 0,255,0 }, blue[] = { 0,0,255 };
    CImgDisplay main_disp(image, "Click a point"), draw_disp(visu, "Intensity profile");
    while (!main_disp.is_closed() && !draw_disp.is_closed()) {
        main_disp.wait();
        if (main_disp.button() && main_disp.mouse_y() >= 0) {
            const int y = main_disp.mouse_y();
            visu.fill(0).draw_graph(image.get_crop(0, y, 0, 0, image.width() - 1, y, 0, 0), red, 1, 1, 0, 255, 0);
            visu.draw_graph(image.get_crop(0, y, 0, 1, image.width() - 1, y, 0, 1), green, 1, 1, 0, 255, 0);
            visu.draw_graph(image.get_crop(0, y, 0, 2, image.width() - 1, y, 0, 2), blue, 1, 1, 0, 255, 0).display(draw_disp);
        }
    }
    return 0;
}