A simple Image class utilizing the XTensor library

Question

I've been teaching myself C++, and I recently created a simple Image class that makes use of the XTensor library. With my Image class, users can easily read and write PNG or JPG images and convert an RGB image to a grayscale image.

If you're interested, you can check out the code for yourself in this repository. Please forgive the extensive comments in the code - I'm still in the process of learning C++.

Declaration of Image Class

To make things easier to read and review here, I've removed some of the extensive comments from my code.

• The declaration of Image class:

#ifndef IMAGE_XTENSOR_HPP
#define IMAGE_XTENSOR_HPP

#include <memory>
#include <string>
#include <xtensor/xarray.hpp>
#include <xtensor/xio.hpp>
#include <xtensor/xview.hpp>

namespace mypackage::image {

struct ImageXTensor {
  ImageXTensor();

  explicit ImageXTensor(std::string file_path);
  explicit ImageXTensor(int c, int h, int w);
  explicit ImageXTensor(const xt::xtensor<double, 3> &input_matrix);
  explicit ImageXTensor(const ImageXTensor &other);
  ImageXTensor(ImageXTensor &&other);

  ImageXTensor &operator=(const ImageXTensor &other);
  ImageXTensor &operator=(ImageXTensor &&other);
  ~ImageXTensor();
  bool operator==(const ImageXTensor &other) const;

  int channels; // aka comp in std_image
  int height;
  int width;
  int size;
  std::unique_ptr<xt::xtensor<double, 3>> pixels;

  bool save(std::string file_path);
  void swap(ImageXTensor &other);
};

ImageXTensor rgb_to_grayscale_xtensor(const ImageXTensor &img);
xt::xarray<double> rgb_to_grayscale_xtensor(const xt::xarray<double> &pixels);

} // namespace mypackage::image

#endif

I hope you find that the declaration of the Image class is easy to understand. However, I have a question about the move constructor ImageXTensor(ImageXTensor &&other);. If I attach explicit keyword to the move constructor like explicit ImageXTensor(ImageXTensor &&other);, it will lead to a compilation error like below:

/home/lai/cmake_template/src/mypackage/image/image_xtensor.cpp: In function ‘mypackage::image::ImageXTensor mypackage::image::rgb_to_grayscale_xtensor(const mypackage::image::ImageXTensor&)’:
/home/lai/cmake_template/src/mypackage/image/image_xtensor.cpp:309:10: error: no matching function for call to ‘mypackage::image::ImageXTensor::ImageXTensor(mypackage::image::ImageXTensor&)’
  309 |   return gray;
      |          ^~~~
/home/lai/cmake_template/src/mypackage/image/image_xtensor.cpp:18:1: note: candidate: ‘mypackage::image::ImageXTensor::ImageXTensor()’
   18 | ImageXTensor::ImageXTensor()
      | ^~~~~~~~~~~~
/home/lai/cmake_template/src/mypackage/image/image_xtensor.cpp:18:1: note:   candidate expects 0 arguments, 1 provided
make[2]: *** [src/mypackage/image/CMakeFiles/image_xtensor.dir/build.make:76: src/mypackage/image/CMakeFiles/image_xtensor.dir/image_xtensor.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:506: src/mypackage/image/CMakeFiles/image_xtensor.dir/all] Error 2
make: *** [Makefile:146: all] Error 2

I tried to understand this error from this SO question, but I'm still not quite sure why this is happening. I understand explicit is used to prevent compilers do implicit conversion, and as a believer of explicit is always better than implicit, I think using explicit everywhere should be always preferred. Thus, I would appreciate any insights or suggestions you might have.

Definition of Image class

Here is the complete definition of the Image class.

#include <cassert>
#include <cmath>
#include <filesystem>
#include <iostream>
#include <mypackage/image/image_xtensor.hpp>
#include <utility>
#include <xtensor/xadapt.hpp>
#include <xtensor/xarray.hpp>
#include <xtensor/xio.hpp>
#include <xtensor/xtensor.hpp>
#include <xtensor/xview.hpp>

#include <stb/stb_image.h>
#include <stb/stb_image_write.h>

namespace mypackage::image {

ImageXTensor::ImageXTensor()
    : channels{0}, height{0}, width{0}, size{0}, pixels{nullptr} {
  std::clog << "The default constructor takes no paramters.\n";
}

ImageXTensor::ImageXTensor(std::string file_path) {
  std::clog << "The constructor takes a file path.\n";

  unsigned char *img_data =
      stbi_load(file_path.c_str(), &width, &height, &channels, 0);
  if (img_data == nullptr) {
    const char *error_msg = stbi_failure_reason();
    std::cerr << "Failed to load image: " << file_path.c_str() << "\n";
    std::cerr << "Error msg (stb_image): " << error_msg << "\n";
    std::exit(1);
  }
  pixels = std::make_unique<xt::xtensor<double, 3>>(
      xt::zeros<double>({channels, height, width}));
  size = pixels->size();
  std::clog << "The image shape: " << channels << " x " << height << " x "
            << width << '\n';
  assert(size == channels * height * width);

  for (int x = 0; x < width; x++) {
    for (int y = 0; y < height; y++) {
      for (int c = 0; c < channels; c++) {
        // PNG's pixels order is mysterious for me.
        std::size_t src_idx = y * width * channels + x * channels + c;
        // Rescale uint8 to float 0-1.
        (*pixels)(c, y, x) = img_data[src_idx] / 255.;
      }
    }
  }
  if (channels == 4)
    channels = 3; // ignore alpha channel

  stbi_image_free(img_data);
}

ImageXTensor::ImageXTensor(int c, int h, int w)
    : channels{c}, height{h}, width{w}, size{c * h * w},
      pixels{std::make_unique<xt::xtensor<double, 3>>(
          xt::zeros<double>({c, h, w}))} {}

ImageXTensor::ImageXTensor(const xt::xtensor<double, 3> &input_matrix) {
  channels = input_matrix.shape(0);
  height = input_matrix.shape(1);
  width = input_matrix.shape(2);
  size = input_matrix.size();

  pixels = std::make_unique<xt::xtensor<double, 3>>(input_matrix);
}

ImageXTensor::ImageXTensor(const ImageXTensor &other)
    : channels{other.channels}, height{other.height}, width{other.width},
      size{other.size}, pixels{std::make_unique<xt::xtensor<double, 3>>(
                            xt::zeros<double>(
                                {other.channels, other.height, other.width}))} {
  std::clog << "Copy Constructor\n";
  *pixels = *other.pixels;
}

ImageXTensor &ImageXTensor::operator=(const ImageXTensor &other) {
  std::clog << "Copy Assignment Operator\n";
  if (this != &other) {
    channels = other.channels;
    height = other.height;
    width = other.width;
    size = other.size;

    pixels = std::make_unique<xt::xtensor<double, 3>>(
        xt::zeros<double>({other.channels, other.height, other.width}));
    *pixels = *other.pixels;
  }
  return *this;
}

ImageXTensor::ImageXTensor(ImageXTensor &&other)
    : channels{other.channels}, height{other.height}, width{other.width},
      size{other.size}, pixels{std::move(other.pixels)} {
  std::clog << "Move Constructor\n";
  swap(other);
}

ImageXTensor &ImageXTensor::operator=(ImageXTensor &&other) {
  std::clog << "Move Assignment Operator\n";
  swap(other);

  return *this;
}

ImageXTensor::~ImageXTensor() { std::clog << "Destruct Image.\n"; }

bool ImageXTensor::operator==(const ImageXTensor &other) const {
  return (width == other.width) && (height == other.height) &&
         (channels == other.channels) && (size == other.size) &&
         (*pixels == *other.pixels);
}

bool ImageXTensor::save(std::string file_path) {
  auto file_extension = std::filesystem::path(file_path).extension();
  unsigned char *out_data = new unsigned char[width * height * channels];
  /** NOTE: There seems to be no easy way to unfold a 3D array into a 1D array
   * with the desired order.
   */
  for (auto x = 0; x < width; x++) {
    for (auto y = 0; y < height; y++) {
      for (auto c = 0; c < channels; c++) {
        int dst_idx = y * width * channels + x * channels + c;
        // Fill out_data with uint8 values range 0-255.
        out_data[dst_idx] = std::roundf((*pixels)(c, y, x) * 255.);
      }
    }
  }

  bool success{false};
  if (file_extension == std::string(".jpg") ||
      file_extension == std::string(".JPG")) {
    auto quality = 100;
    success = stbi_write_jpg(file_path.c_str(), width, height, channels,
                             out_data, quality);
  } else if (file_extension == std::string(".png") ||
             file_extension == std::string(".png")) {
    auto stride_in_bytes = width * channels;
    success = stbi_write_png(file_path.c_str(), width, height, channels,
                             out_data, stride_in_bytes);
  } else {
    std::cerr << "Unsupported file format: " << file_extension << "\n";
  }
  if (!success)
    std::cerr << "Failed to save image: " << file_path << "\n";

  delete[] out_data;
  return true;
}

void ImageXTensor::swap(ImageXTensor &other) {
  std::swap(channels, other.channels);
  std::swap(height, other.height);
  std::swap(width, other.width);
  std::swap(size, other.size);
  std::swap(pixels, other.pixels);
}

ImageXTensor rgb_to_grayscale_xtensor(const ImageXTensor &img) {
  assert(img.channels >= 3);
  ImageXTensor gray(1, img.height, img.width);

  xt::xarray<double> red = xt::view(*img.pixels, 0, xt::all(), xt::all());
  xt::xarray<double> green = xt::view(*img.pixels, 1, xt::all(), xt::all());
  xt::xarray<double> blue = xt::view(*img.pixels, 2, xt::all(), xt::all());

  xt::view(*gray.pixels, 0, xt::all(), xt::all()) =
      0.299 * red + 0.587 * green + 0.114 * blue;

  return gray;
}

xt::xarray<double> rgb_to_grayscale_xtensor(const xt::xarray<double> &pixels) {
  assert(pixels.shape(0) >= 3);
  auto height = pixels.shape(1);
  auto width = pixels.shape(2);
  xt::xarray<double>::shape_type shape = {1, height, width};
  xt::xarray<double> gray(shape);

  xt::xarray<double> red = xt::view(pixels, 0, xt::all(), xt::all());
  xt::xarray<double> green = xt::view(pixels, 1, xt::all(), xt::all());
  xt::xarray<double> blue = xt::view(pixels, 2, xt::all(), xt::all());

  xt::view(gray, 0, xt::all(), xt::all()) =
      0.299 * red + 0.587 * green + 0.114 * blue;

  return gray;
}

} // namespace mypackage::image

I hope you find the implementation of the Image class to be straightforward.

Usage of Image class (You can find the code here)

• Pass Image object to rgb_to_grayscale_xtensor:

bool rgb2gray_image_xtensor(std::string input, std::string output) {
  mypackage::image::ImageXTensor in_img{input};
  auto out_img = mypackage::image::rgb_to_grayscale_xtensor(in_img);
  out_img.save(output);
  return 0;
}

• Pass xt::xarray<double> to rgb_to_grayscale_xtensor:

bool rgb2gray_image_xtensor_PassByTensor(std::string input,
                                         std::string output) {
  mypackage::image::ImageXTensor in_img{input};
  mypackage::image::ImageXTensor out_img{in_img.channels, in_img.height,
                                         in_img.width};
  *out_img.pixels = mypackage::image::rgb_to_grayscale_xtensor(*in_img.pixels);
  out_img.save(output);
  return 0;
}

Unit test: (You can find the code here)

• Test constructor and assignment operator:

#include <gtest/gtest.h>
#include <iostream>
#include <mypackage/image/image_xtensor.hpp>
#include <vector>
#include <xtensor/xadapt.hpp>

TEST(ImageXTensor, ClassAssertion) {
  mypackage::image::ImageXTensor test_img1(2, 3, 4);
  int counter = 0;
  for (int x = 0; x < test_img1.width; x++) {
    for (int y = 0; y < test_img1.height; y++) {
      for (int c = 0; c < test_img1.channels; c++) {
        (*test_img1.pixels)(c, y, x) = counter;
        ++counter;
      }
    }
  }
  std::clog << "test_img1:\n" << (*test_img1.pixels) << '\n';

  std::clog << "Test Copy Constructor.\n";
  mypackage::image::ImageXTensor test_img2{test_img1};
  std::clog << "test_img2:\n" << (*test_img2.pixels) << '\n';
  EXPECT_EQ(test_img2, test_img1);

  std::clog << "Test Copy Assignment Operator.\n";
  mypackage::image::ImageXTensor test_img3;
  test_img3 = test_img1;
  std::clog << "test_img3\n" << (*test_img3.pixels) << '\n';
  EXPECT_EQ(test_img3, test_img1);

  std::clog << "Test Move Assignment Operator.\n";
  mypackage::image::ImageXTensor test_img4;
  test_img4 = std::move(test_img1);
  // test_img1 becomes unspecified after it's moved from.
  EXPECT_EQ(test_img1.pixels, nullptr);
  EXPECT_EQ(test_img4, test_img2);
}

As I'm still learning C++, I'm trying to focus on modern C++ and adhere to the best coding practices. If it's not too much trouble, I would appreciate it if you could review the code with modern C++ (especially those from C++17 and beyond) in mind. Additionally, any tips on following C++ best coding practices would be greatly appreciated. Thank you.

Answer 1

Overview

My main question is if pixels actually needs to be a std::unique_ptr? Does xt::xtensor not support the empty set of pixels efficiently? If it does not support the empty set OR there is an efficiency concern then sure std::unique_ptr is a reasonable approach, otherwise it is overkill and you should remove it.

Since I don't know about xt::xtensor I am going to assume unique_ptr is a valid technique. BUT if not then your class be significantly simplified and all the compiler generated methods will work perfectly.

Answers to Questions / Comments

To make things easier to read and review here, I've removed some of the extensive comments from my code.

Comments are also subject to review usually.
I have seen some really bad omments that needed to be removed.

I understand explicit is used to prevent compilers do implicit conversion, and as a believer of explicit is always better than implicit, I think using explicit everywhere should be always preferred.

Really on needed for constructors that take 1 parameter as an argument. This will prevent the compiler auto converting the object by using the constructor implicitly where you might not have expected.

would appreciate it if you could review the code with modern C++ (especially those from C++17 and beyond) in mind.

I still use (param) when constructing an object a lot. Some people prefer {param}. There are a few corner case it helps solve. I tend to reserve '{}' for initializer lists.

Code Review

You have made this a struct:

struct ImageXTensor {

This means all members are public by default. That is probably not what you want because it looks like these memebrs:

  int channels; // aka comp in std_image
  int height;
  int width;
  int size;
  std::unique_ptr<xt::xtensor<double, 3>> pixels;

maintain a valid state of the object. You may want to provide some form of validation or checking on member variables before they can be modified.

---

Minor syntax quible:

const xt::xtensor<double, 3> &variable

You use a C style of declaration. Putting the & beside the variable name mimiks the style used in C where the * is placed next to the variable. The more normal way is to put all type information in one place (left side) and then place the variable on the right.

const xt::xtensor<double, 3>&   variable;  // This is more normal in c++

---

The usage of explicit on constructors is only really needed on single argument constructors.

  explicit ImageXTensor(std::string file_path);
  explicit ImageXTensor(int c, int h, int w);
  explicit ImageXTensor(const xt::xtensor<double, 3> &input_matrix);
  explicit ImageXTensor(const ImageXTensor &other);

The idea is to prevent the compiler auto converting objects of one type into an ImageXTensor when the engineer is not paying attention.

So I would remove explicit from:

  explicit ImageXTensor(int c, int h, int w);
  explicit ImageXTensor(const ImageXTensor &other);

---

Looks like you the default constructor ImageXTensor() could be dropped and replaced by ImageXTensor(int c = 0, int h = 0, int w = 0) where you use default arguments.

---

The more normal way to define the assignment operator in a class that uses both move and copy is to define a single method that utilizes the move/copy constructor.

So:

  ImageXTensor &operator=(const ImageXTensor &other);
  ImageXTensor &operator=(ImageXTensor &&other);

Can be replaced by the single method:

  ImageXTensor& operator=(ImageXTensor other);

I will go into more detail below. But using the value parameter forces a copy in situations where the parameter is an L-Value and a move in situations when the parameter is an R-Value. Thus the body of the assignment simply needs to swap the current content with the parameter to get an effective assignment.

---

Not sure why this is a stand a lone method.

ImageXTensor rgb_to_grayscale_xtensor(const ImageXTensor &img);
xt::xarray<double> rgb_to_grayscale_xtensor(const xt::xarray<double> &pixels);

Could have been a member of ImageXTensor.

---

ImageXTensor::ImageXTensor(std::string file_path) {

  ....

  if (img_data == nullptr) {

    ...
    // If an object fails to construct correctly then throw an exception.
    // At exit will not unwind the stack. An exception will unwind the
    // stack and if not caught cause the application to exit.

    // I would replace this with a throw.
    // Put the error message in the exception and
    // print the error message from the exception in main()
    std::exit(1);
    
  }

---

Another minor syntactical thing:

  for (int x = 0; x < width; x++) {

This again looks like C. In C++ I would write this as:

  for (int x = 0; x < width; ++x) {
                    //       ^^.   prefer prefix increment.

The code generated will not be different in this situation. But we often use this kind f loop with iterators. And here there may be a minor efficiency issue (probably not). But by using the prefix increment you always get the more effecient version (assuming the standard pattern for implementing iterators).

So if this is not an iterator why is it important. 1: Consistency. 2: If you use it all the time you will always use the correct increment. 3: A lot of times in C++ code we simply change the code by changing a type (so your integer may change to an iterator without you noticing). If you use the pre increment you don't need to go check usage of objects after you change the type.

---

Another minor thing:

        (*pixels)(c, y, x) = img_data[src_idx] / 255.;

Don't like the 255. hard to spot that this is a floating point number. Just add the zero. 255.0.

---

Curious why you ignore alpha by default.

  if (channels == 4)
    channels = 3; // ignore alpha channel

Also add the braces to prevent accidental scope issues:

  if (channels == 4) {
    channels = 3; // ignore alpha channel
  }

---

Where you allocate and then release a resource. I would always use RAII to make sure that the resource is freed even when exceptions happen (read above about stack unwinding).

  stbi_image_free(img_data);

I would change the code to:

{
    using ImageDate = unsigned char*;
    using ImageRes  = std::unique_ptr<ImageData, decltype(&stbi_image_free)>;

    ImageRes img{
        stbi_load(file_path.c_str(), &width, &height, &channels, 0),
        stbi_image_free
    };


    // Stuff that could throw.


    // We will call stbi_image_free() when
    // img goes out of scope at the end of this function
    // automatically.
}

---

Prefer to use the initializer list:

ImageXTensor::ImageXTensor(const xt::xtensor<double, 3> &input_matrix)
    // In all the other constructors
    // You initialized the members here.
    // ----
    // You should prefer to do it here.
    // If the types are complex and use a constructor
    // You would be constructing them here
    // then using the assignment operator below
    // thus effectively constructing them twice.
{
  channels = input_matrix.shape(0);
  height = input_matrix.shape(1);
  width = input_matrix.shape(2);
  size = input_matrix.size();

  pixels = std::make_unique<xt::xtensor<double, 3>>(input_matrix);
}

Like this:

ImageXTensor::ImageXTensor(const xt::xtensor<double, 3> &input_matrix)
  : channels(input_matrix.shape(0))
  , height(input_matrix.shape(1))
  , width(input_matrix.shape(2))
  , size(input_matrix.size())
  , pixels(std::make_unique<xt::xtensor<double, 3>>(input_matrix))
{}

---

Bug:

ImageXTensor::ImageXTensor(ImageXTensor &&other)
    : channels{other.channels}, height{other.height}, width{other.width},
      size{other.size}, pixels{std::move(other.pixels)} {
  std::clog << "Move Constructor\n";


  // This looks like a bug.
  // You have already moved the pointer in `pixels`
  // With this swap you are putting it back.
  swap(other);
}

---

So this is bad practice (testing for assignment to self).

ImageXTensor &ImageXTensor::operator=(const ImageXTensor &other) {
  std::clog << "Copy Assignment Operator\n";
  if (this != &other) {
    channels = other.channels;
    height = other.height;
    width = other.width;
    size = other.size;

    pixels = std::make_unique<xt::xtensor<double, 3>>(
        xt::zeros<double>({other.channels, other.height, other.width}));
    *pixels = *other.pixels;
  }
  return *this;
}

This means you are de-optimizing the most common situation (assignment not to self). Sure we have to cope with self assignment, but it happens so very rarely. So we try and optimize for the normal situaiton (not the none normal).

So we have developed the technique of copy and swap for copy assignment. This uses the copy constructor.

ImageXTensor &ImageXTensor::operator=(ImageXTensor const& rhs) {
  ImageXTensor other(rhs)
  swap(other);

  return *this;
}

---

Now if we look at the move assignment:

ImageXTensor &ImageXTensor::operator=(ImageXTensor &&other) {
  swap(other);

  return *this;
}

It looks like the copy the only difference is the internal variable being swapped into. But if we change this assignment into:

ImageXTensor &ImageXTensor::operator=(ImageXTensor rhs)
{
  swap(rhs);
  return *this;
}

Now this function handles both move and copy effeciently. If the parameter is an L-Value then it is copyied into rhs, alternatively if the parameter is an R-Value then it is moved into rhs. We can then simply swap rhs and the content of the current object. When rhs goes out of scope it cleans everything.

---

Sure for logging:

ImageXTensor::~ImageXTensor() { std::clog << "Destruct Image.\n"; }

But normally you don't want to define the destructor if it does nothing.

---

If a method does not have a chance of throwing an exception it should be marked no-except. This is especially true if the method is move constructor. Also swap comes into that.

---