Watermarking an image

Question

I have a small bit of code and I wonder if I can make it more readable. I would appreciate it if some people could take a look at it and give me some feedback. I especially wonder if there is a beter way of doing this:

cv::Mat image = loadImage(path.string());
if (image.data == NULL) { continue; }

This is the bigger code:

cv::Mat Watermarker::loadImage(const std::string& path)
{
    if (!validFile(path))
    {
        if (!std::filesystem::is_directory(path))
        {
            std::cout << path << " is invalid\n";
        }

        return cv::Mat{};
    }

    cv::Mat image = cv::imread(path, cv::IMREAD_UNCHANGED);
    if (image.data == NULL)
    {
        std::cout << "Not enough permissions to read file " << path << "\n";
        return cv::Mat{};
    }

    return image;
}

void Watermarker::mark(const std::string& targetDirectory, const std::string& watermarkPath)
{
    cv::Mat watermarkImage = loadImage(watermarkPath);
    if (watermarkImage.data == NULL) { return; }

    // Loop through all the files in the target directory
    for (const std::filesystem::path& path : std::filesystem::directory_iterator(targetDirectory))
    {
        cv::Mat image = loadImage(path.string());
        if (image.data == NULL) { continue; }

        cv::resize(watermarkImage, watermarkImage, image.size());

        // Give the images 4 channels
        cv::cvtColor(image, image, cv::COLOR_RGB2RGBA);
        cv::cvtColor(watermarkImage, watermarkImage, cv::COLOR_RGB2RGBA);

        // Add the watermark to the original image
        cv::addWeighted(watermarkImage, 0.3, image, 1, 0.0, image);

        saveImage(targetDirectory + "/watermarked_images/" + path.filename().string(), image);
    }
}

Answer 1

Don't ignore errors

You are just skipping any file that cannot be read correctly. But probably, if the user tried to process the images in a given target directory, it wants all of them to be watermarked, and if something cannot be watermarked, the user probably wants to hear about it. So you should not just continue after loading an image failed, you should stop and report an error. There are several ways to do this, for example:

Consider throwing exceptions

I would recommend throwing exceptions here. I assume that not being able to open a file or parsing it correctly is an exceptional event for your program. The advantage of throwing an exception is that the caller of a function that throws doesn't have to handle the exception, it can just let it propagate up the stack until something does handle it, or if nothing handles the exception, the program will abort with an error. The latter can even be desirable. Here is an example of how to throw exceptions:

#include <stdexcept>

cv::Mat Watermarker::loadImage(const std::string& path)
{
    cv::Mat image = cv::imread(path, cv::IMREAD_UNCHANGED);

    if (!image.data)
        throw std::runtime_error("Failed to read image");

    return image;
}

I have removed the check for path being valid as well, since if it isn't valid, trying to call cv::imread() on it will fail as well.

Inside Watermarker::mark(), you can now assume that when loadImage() returns, image.data is always non-NULL, so you don't need a check there. If you really want to continue parsing images, then you can catch the exception, and perhaps print a warning so the user does get to know what was wrong:

try {
    cv::Mat image = loadImage(path.string());
    cv.resize(...);
    ...
} catch (std::runtime_error &e) {
    std::cerr << "Could not load " << path << ": " << e.what() << "\n";
}

Write error messages to std::cerr

Don't write warnings, errors and debug messages to std::cout, but use std::cerr. This is especially important if the program is printing normal output to std::cout, so that if standard output is redirect to a file for example, it will not contain error messages, and the error messages will still be printed to the console.

Avoid converting std::filesystem::paths to std::strings too early

Use std::filesystem::path as the type to pass paths in consistently. Only convert them to/from std::strings when really necessary. You can also make use of implicit conversions between std::string and std::filesystem::path. So for example:

cv::Mat Watermarker::loadImage(const std::filesystem::path& path)
{
    cv::Mat image = cv::imread(path, cv::IMREAD_UNCHANGED);
    ...
}

And:

void Watermarker::mark(const std::fileystem::path& targetDirectory, const std::filesystem::path& watermarkPath)
{
    auto watermarkImage = loadImage(watermarkPath);

    for (auth& path : std::filesystem::directory_iterator(targetDirectory))
    {
        auto image = loadImage(path);
        cv::resize(watermarkImage, watermarkImage, image.size());
        ...
        saveImage(targetDirectory / "watermarked_images" / path.filename(), image);
    }
}