4
\$\begingroup\$

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;
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks. Really liked your answer. Yes, I agree about file checks, though because the program is used as a minimalistic examples and not supposed to be used in a large system, I tend to skip the checks as they take the students away from the core of the demo. Your comments on mt19937 is very interesting. I will look into it. L1 caches on my computer 96K / core so I have room, but good pt. Passing by value was also interesting. \$\endgroup\$
    – user18490
    Jul 20 at 13:01
  • \$\begingroup\$ #2: the reason why I am using malloc for now (and not vector or new) is because i'd like to look into memory alignment at some point. Not sure this is possible with vector but I am not knowledgeable about this. But I will look into the functions that are in <memory> to see how I can achieve this with this library. \$\endgroup\$
    – user18490
    Jul 20 at 13:09
  • 1
    \$\begingroup\$ Aligning memory in C++ is relatively easy using alignas(). With std::vector, beware to align the data stored in the vector, and not just the vector object itself. \$\endgroup\$
    – G. Sliepen
    Jul 20 at 17:03
3
\$\begingroup\$

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);
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Of course if you use malloc() you have to test the return value regardless of whether it is used in C or C++. \$\endgroup\$
    – G. Sliepen
    Jul 17 at 14:41
  • \$\begingroup\$ @G.Sliepen so true. \$\endgroup\$
    – pacmaninbw
    Jul 17 at 15:00
  • \$\begingroup\$ Thanks for your input. Very valuable and I appreciate the time. I commented in the other answer that as this is a minimalist implementation to demonstrate a concept, I don't focus on checks as this takes the students away from the core of the demo. But I do appreciate the feedback as it's good to be aware of what people can say about this code regardless of what I decide to integrate later on. Many thx again. \$\endgroup\$
    – user18490
    Jul 20 at 13:04
  • \$\begingroup\$ @user18490 I understand your reasoning but I can't say I agree with it. I've been a software engineer since 1984. There are a lot of students fresh out of college that have the theory but miss some of the basic errors, especially in low level languages like C, because they learn in high level languages like Java, Python or C++. In 1993 I had to integrate shared libraries into an executable and had to debug a lot of other peoples code that had basic errors in it. \$\endgroup\$
    – pacmaninbw
    Jul 20 at 13:20
  • \$\begingroup\$ @pacmaninbw I understand your point (and sympathize)). If the students fresh out of university are not writing production quality code when working for companies indeed this is an issue, but I am not teaching programming but more computer graphics. So it's the essence of the algorithm that is implemented in a given small program that needs to stand out. But again, I will take your comment into consideration and might decide to integrate it. \$\endgroup\$
    – user18490
    Jul 21 at 15:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.