Simple multi-threading setup for tile rendering (C++)

Question

I am writing a small framework that I intend to build upon in the future. I am also using this framework to teach people how to support multi-threading in 3D rendering, and thus wanted to be sure I was exposing them to code that was decently clean.

The goal is to write a simple example in which we split a frame into tiles (smaller chunks of say 32x32 pixels) and get the threads to render the pixels of each tile. I want to use the minimalist approach and thus avoid doing something like a task scheduler. Therefore I have chosen to calculate the number of tiles in the main program, store that in an atomic variable, and use that in the threads to provide each thread with a current tile number. Threads then decrement that number and keep doing so while that total number of tiles is greater or equal to 0.

I have chosen to store the current values for the calculated pixels for each tile in a small buffer managed by the thread and then copy the values of the tile's pixels into the image buffer. I believe this is thread-safe but would love to get your feedback on that.

To make the exercise more fun, I am currently assigning a random color to each tile before I implement the rest of the rendering process. I am using a random number generator from the standard lib for now. I declared the generator thread_local though to be honest unless I actually call the function render() recursively or need other functions called from within the thread function to use the generator I don't think this is necessary. As long as each thread own its own random number generator I believe this is thread safe too.

Your feedback would be greatly appreciated, where here what I am looking for is:

1. Simplicity: easier for people to learn from
2. Correctness: if people learn from that, it better be correct and ideally pass a C++ peer review.
3. Efficiency: while I am not looking to save 2 or 3 cycles if you see things that could definitely impact performance please let me know. One thing I have been thinking about at this point in time is that the way I allocate memory dynamically is not caring about alignment. So that's one possible thing I could add / improve.

What do you think? Here is the code:

#include <thread>
#include <atomic>
#include <iostream>
#include <fstream>
#include <mutex>
#include <random>

struct render_info_t
{
    unsigned int width { 640 };
    unsigned int height { 480 };
    unsigned int tile_size { 32 };
    unsigned int num_tiles_x{}, num_tiles_y{};
    unsigned char *buffer { nullptr };
};

struct thread_info_t
{
    unsigned int id;
    const render_info_t* render_info;
};

void render(const thread_info_t& thread_info, std::atomic_int &count)
{
    int curr_tile {};
    const render_info_t* ri = thread_info.render_info;
    thread_local std::mt19937 gen(std::hash<std::jthread::id>()(std::this_thread::get_id()));
    std::uniform_real_distribution<float> dist(0.0f, 1.f);
    unsigned char *buffer = (unsigned char*)malloc(ri->tile_size * ri->tile_size * 3);
    
    while ((curr_tile = --count) >= 0) {
        unsigned char *curr_pixel = buffer;
        float r = dist(gen);
        float g = dist(gen);
        float b = dist(gen);

        unsigned int curr_tile_y = curr_tile / ri->num_tiles_x;
        unsigned int curr_tile_x = curr_tile - curr_tile_y * ri->num_tiles_x;
        unsigned int x0 = curr_tile_x * ri->tile_size;
        unsigned int x1 = std::min((curr_tile_x + 1) * ri->tile_size, ri->width);
        unsigned int y0 = curr_tile_y * ri->tile_size;
        unsigned int y1 = std::min((curr_tile_y + 1) * ri->tile_size, ri->height);
        for (unsigned int y = y0; y < y1 ; ++y) {
            for (unsigned int x = x0; x < x1; ++x, curr_pixel += 3) {
                /*
                ** TODO trace ray at pixel coordinate x, y
                */
                curr_pixel[0] = (unsigned char)(r * 255);
                curr_pixel[1] = (unsigned char)(g * 255);
                curr_pixel[2] = (unsigned char)(b * 255);
            }
        }

        // copy the tile's pixels into the image buffer
        unsigned char *row = buffer;
        unsigned char *from = ri->buffer + (y0 * ri->width + x0) * 3;
        for (unsigned int y = y0; y < y1 ; ++y, row += ri->tile_size * 3, from += ri->width * 3) { // , from += ri->width * 3
            // the data pointed by the pointer ri->buffer is not part of the struct, so it can be change
            memcpy(from, row, ri->tile_size * 3);
        }
    }
    free(buffer);
}

int main(int argc, char **argv)
{
    unsigned int num_threads = std::jthread::hardware_concurrency();
    std::cout << "Rendering with " << num_threads << " threads" << std::endl;
    std::vector<std::jthread> threads;
    render_info_t ri;
    ri.num_tiles_x = (ri.width + ri.tile_size - 1) / ri.tile_size;
    ri.num_tiles_y = (ri.height + ri.tile_size - 1) / ri.tile_size;
    ri.buffer = (unsigned char*)malloc(ri.width * ri.height * 3);
    std::atomic_int num_tiles = ri.num_tiles_x * ri.num_tiles_y;
    for (unsigned int n = 0; n < num_threads; ++n) {
        thread_info_t thread_info;
        thread_info.id = n;
        thread_info.render_info = &ri;
        threads.emplace_back(render, std::move(thread_info), std::ref(num_tiles));
    }

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

    std::ofstream ofs;
    ofs.open("./output.ppm", std::ios::binary);
    ofs << "P6\n" << ri.width << " " << ri.height << "\n255\n";
    ofs.write((char*)ri.buffer, ri.width * ri.height * 3);
    ofs.close();
    free(ri.buffer);

    return EXIT_SUCCESS;
}

Answer 1

Adding to pacmaninbw's answer:

Use more STL containers

Instead of calling malloc() or even using the proper C++ equivalents, consider using std::vector:

std::vector<unsigned char> buffer(ri->tile_size * ri_tile_size * 3);

Use STL algorithms

The standard library also comes with an extensive set of algorithms. In particular, it has std::copy_n() that is a type-safe replacement for memcpy().

Pass thread_info by value

If you want to pass data to a thread that is unique to that thread, make sure you pass it by value. If you pass it by reference, then you have to worry about whether the object pointed to by the reference is still valid. By the time the thread starts and reads from thread_info, its contents might already have been changed in the first for-loop inside main().

Hidden overhead of std::mt19937

The mt19937 random number generator is very fast and very good, but it only manages to pull that off because of its huge state. Its name actually comes from the fact that it uses 19937 bits of state, which is almost 2.5 kilobytes. This is almost as much as the size of the tile buffer. Even if you have enough RAM, consider that one of the main benefits of tile rendering is that your working set is small enough to fit in L1 cache. So if you do need a RNG in your render threads, consider using a different one that has less state.

Missing error checking

There are many things that can go wrong writing a file, but you are not checking if writing the output file succeeded. The easiest way is just to check if ofs.good() is still true after closing it.

Answer 2

General Observations

Since this is a C++ program, you want to use new and delete rather than malloc() and free(). If this was a C program you are missing the test on whether malloc() successfully returned memory, which is not necessary in C++ since C++ will throw an exception if the memory allocated by new fails.

Since C++ is an object oriented language it might be better to treat the render_info_t struct as an object to make the code reusable and portable.

In both C++ and C it is unnecessary to return EXIT_SUCCESS from main unless you also have code that returns EXIT_FAILURE, the main return value will default to EXIT_SUCCESS.

Magic Numbers

There are Magic Numbers in the struct declaration for render_info_t (640, 480 and 32), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier. It might also be better if there was a constructor for the render_info_t struct that assigned thes3e values to the variables.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

The 3 in the following code is also a magic number:

   ri.buffer = (unsigned char*)malloc(ri.width * ri.height * 3);