CODEWITHSUNDEEP
c++
Jelle Vos
Jelle Vos

Reputation: 48

Pass thread arguments by reference gives me access violation

I looked up how I can pass thread arguments by reference and I found std::ref so I used that:

void Watermarker::markDirectory(const std::string& targetDirectory, const std::string& watermarkPath)
{
    std::vector<std::thread*> threads;

    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))
    {
        std::thread* thread = new std::thread(mark, std::ref(targetDirectory), std::ref(path), std::ref(watermarkImage));
        threads.push_back(thread);
    }

    for (std::thread* thread : threads)
    {
        thread->join();
        delete thread;
    }
}

This is the function:

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

    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/" + imagePath.filename().string(), image);
}

But when I run this it throws an exception "Access violation writing location. I get these 2 popups: enter image description here

Without the std::ref and & in the function it works fine if I pass it by value, but I want it by reference.

Upvotes: 0

Views: 254

Answers (2)

Ted Lyngmo
Ted Lyngmo

Reputation: 117298

The problem is that you're changing watermarkImage from multiple threads at once in the mark function, making your program have undefined behavior.

Suggestion: Take it as a const& and copy it in the thread.

void Watermarker::mark(const std::string& targetDirectory,
                       const std::filesystem::path& imagePath,
                       const cv::Mat& temp_watermarkImage)   // const added
{
    cv::Mat watermarkImage = temp_watermarkImage;       // make a local copy
    ...
}

Upvotes: 2

Remy Lebeau
Remy Lebeau

Reputation: 596417

I see a number of problems with your code.

  • using new to create std::thread objects, just to delete them before exiting. You don't need new/delete at all in this situation.

  • inside your loop, path is a local variable that refers to a temporary std::filesystem::path object created by the directory_iterator. That temporary will go out of scope and be destroyed when (depending on implementation) the current loop iteration is done, or the directory_iterator is destroyed when the loop is finished. Either way, you are passing that path by reference into your threads. So the threads are operating on path objects that may be destroyed before/while they are still running. There is no good reason to pass a std::filesystem::path object by reference, it is lightweight enough to pass across thread boundaries by value with little overhead. Your threads need path objects that stay alive for the lifetime of the call to mark().

  • if mark() is a non-static method (and this code suggests that it is), then it has a hidden this parameter that you are not giving to each thread to pass into mark(), thus the this parameter (and all of the other parameters) will be corrupted when mark() is actually called.

  • even if all the parameters were being passed properly, you are passing a reference to the same watermarkImage object to all of the threads, and mark() is trying to manipulate that image without any synchronization across the threads.

With that said, try something more like this instead:

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

    std::vector<std::thread> threads;

    // Loop through all the files in the target directory
    for (const auto& path : std::filesystem::directory_iterator(targetDirectory))
    {
        threads.emplace_back(
            &Watermarker::mark, this,
            std::cref(targetDirectory), path, std::cref(watermarkImage)
        );
    }

    for (auto& thread : threads)
    {
        thread.join();
    }
}

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

    cv::Mat new_watermarkImage = watermarkImage;
    cv::resize(my_watermarkImage, my_watermarkImage, image.size());

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

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

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

Upvotes: 2

Related Questions