Mandelbrot set visualizer using GTK4, Cairo and POSIX threads in C

Question

I have recently implemented a Mandelbrot set visualizer, which I used to reacquaint myself with multithreading using pthreads.

I'm just wondering whether the way that I wrote the multithreading code is really valid, or are there any edge cases where code could break because there is something wrong with how I organized it. Or, if there are any ways that this code could be simplified to achieve the same result.

The full code is available here: https://git.bonsai.cool/kayprish/mandelbrot-visualiser

Below I will post snippets which this question is focused on, slightly rearranged for readability:

/******* Program data structures *******/

int32_t thread_count;

struct threadInfo {
    int32_t index; // unique number from 0 to thread_count-1
    bool drawing; // marks if we are currently drawing for a given thread
    bool complete; // marks if the drawing that it was supposed to
};

/*
 * Concurrency model explained:
 * One main thread, along with many workers in a thread pool, who split the work of
 * rendering roughly equally. The workers don't do any work until the pixel map which they
 * are meant to work on is marked as available, and they are all singalled to start
 * drawing.
 *
 * Once it is marked as such, this means that the planeView structure is well
 * defined, that pixmap points to a memory region, which is allocated with
 * enough memory for the plain view, and they can all start writing without an
 * issue.
 *
 * Once they are all finished, the last thread to exit the drawing function will signal
 * the main thread to "blit" the finished image to the screen.
 *
 * If any kind of change by the user happens (window resize, panning/zooming (TODO: not
 * implemented yet), the main thread will cause all worker threads to stop working, if
 * they haven't already (they occassionally poll while drawing to see if they should
 * stop). Once they've all stopped, the main thread will update and replace the drawing
 * area, and naturally, signal them to start drawing again.
 */

pthread_mutex_t pixmapMutex;
pthread_cond_t pixmapCond;

struct threadInfo threads[MAX_THREADS];
// this will store an array with status info for all the threads

cairo_surface_t *surface = NULL;
bool pixmapAvailable = false;
unsigned char *pixmap;
struct planeView {
    double scale; // scale is represented as unit/pixel
    int width, height;
    double centerX, centerY;
};
struct planeView mandelbrot = { 1, 0, 0, 0, 0};

/******* Worker thread code *******/

void *worker_thread(void *arg)
{
    struct threadInfo *info = arg;
    while (true) {
        pthread_mutex_lock(&pixmapMutex);
        while (!pixmapAvailable || info->complete) {
            pthread_cond_wait(&pixmapCond, &pixmapMutex);
        }
        pthread_mutex_unlock(&pixmapMutex);
        info->drawing = true;
        draw_mandelbrot(info);
        info->drawing = false;
    }
}

void draw_mandelbrot(struct threadInfo *info)
{
    int h = mandelbrot.height;
    int w = mandelbrot.width;
    int p = info->index, c = thread_count;
    int mod = h % c, div = h / c;
    int a = MIN(p, mod)*(div+1) + (MAX(p, mod)-mod)*div,
        b = p < mod ? a + div + 1 : a + div;
    // [a..b) is the range of rows a given thread is meant to write pixels

    double centerX = mandelbrot.centerX;
    double centerY = mandelbrot.centerY;
    double scale = mandelbrot.scale;

    for (int i = a; i < b; i++) {
        double yRange = (double) i / h * 2.0 - 1.0;
        double y = centerY+yRange*scale;
        pthread_mutex_lock(&pixmapMutex);
        if (!pixmapAvailable) {
            pthread_mutex_unlock(&pixmapMutex);
            return;
            // after this, the thread will again enter the
            // loop in the worker_thread function, and
            // enter a wait state
        }
        pthread_mutex_unlock(&pixmapMutex);
        for (int j = 0; j < w; j++) {
            double xRange = (double) j / w * 2.0 - 1.0;
            double x = centerX+xRange*scale;

            int r, g, b;
            color_from_iteration(&r, &g, &b, x, y);
            pixmap[4*(i*w + j) + 2] = r;
            pixmap[4*(i*w + j) + 1] = g;
            pixmap[4*(i*w + j) + 0] = b;
        }
    }
    pthread_mutex_lock(&pixmapMutex);
    info->complete = true;
    bool allWorkersComplete = true;
    for (int32_t i = 0; i < thread_count; i++) {
        if (!threads[i].complete) {
            allWorkersComplete = false;
        }
    }
    if (allWorkersComplete) {
        cairo_surface_mark_dirty(surface);
        g_main_context_invoke(NULL, queue_redraw_plane, NULL);
    }
    pthread_mutex_unlock(&pixmapMutex);
}

/******* Main thread code *******/

gboolean queue_redraw_plane(void *arg)
{
    (void) arg;
    bool allWorkersHadCompleted = true;
    for (int32_t i = 0; i < thread_count; i++) {
        if (!threads[i].complete) {
            allWorkersHadCompleted = false;
            break;
        }
    }

    if (!allWorkersHadCompleted) {
        return G_SOURCE_REMOVE;
    }

    gtk_widget_queue_draw(GTK_WIDGET(da));
    return G_SOURCE_REMOVE;
}

void blit_plane(GtkDrawingArea *da, cairo_t *cr, int width, int height, gpointer data)
{
    (void) da;
    (void) width;
    (void) height;
    (void) data;
    bool allWorkersHadCompleted = true;
    for (int32_t i = 0; i < thread_count; i++) {
        if (!threads[i].complete) {
            allWorkersHadCompleted = false;
            break;
        }
    }
    if (allWorkersHadCompleted) {
        cairo_set_source_surface(cr, surface, 0, 0);
        cairo_paint(cr);
    }
}

void plane_resize(GtkWidget *widget)
{
    create_surface(widget);
}

void create_surface(GtkWidget *widget)
{
    // Stop all current drawers
    pthread_mutex_lock(&pixmapMutex);
    pixmapAvailable = false; // Mark drawing area as unavailable
    pthread_mutex_unlock(&pixmapMutex);

    bool allWorkersStopped;
    do { // Wait until all writing threads have been stopped
        allWorkersStopped = true;
        for (int32_t i = 0; i < thread_count; i++) {
            if (threads[i].drawing) {
                allWorkersStopped = false;
                break;
            }
        }
    } while (!allWorkersStopped);

    // If all drawers hadn't completed, we need to call
    // cairo_surface_mark_dirty in the main thread
    bool allWorkersHadCompleted = true;
    for (int32_t i = 0; i < thread_count; i++) {
        if (!threads[i].complete) {
            allWorkersHadCompleted = false;
            break;
        }
    }
    if (!allWorkersHadCompleted && surface != NULL) {
        cairo_surface_mark_dirty(surface);
    }

    for (int32_t i = 0; i < thread_count; i++) {
        threads[i].complete = false;
    }

    if (surface) {
        cairo_surface_destroy(surface);
    }

    surface = cairo_image_surface_create(CAIRO_FORMAT_RGB24,
            gtk_widget_get_width(widget),
            gtk_widget_get_height(widget));

    int h = cairo_image_surface_get_height(surface);
    int w = cairo_image_surface_get_width(surface);
    mandelbrot.height = h;
    mandelbrot.width = w;

    // Mark the surface as ready to be drawn by memory access
    // Then notify other threads to start drawing
    // One of these threads will make the corresponding call using
    // cairo_surface_mark_dirty, to notify they are all done drawing
    cairo_surface_flush(surface);
    pixmap = cairo_image_surface_get_data(surface);

    // This lock is unnecessary since all other threads are stopped already
    pthread_mutex_lock(&pixmapMutex);
    pixmapAvailable = true;
    pthread_mutex_unlock(&pixmapMutex);
    pthread_cond_broadcast(&pixmapCond);
}

Answer 1

Improve thread synchronization

You have quite a bit of code for synchronizing the completion of the worker threads. It is more complicated than necessary, there is at least one bug, and in create_surface() there is an avoidable busy-loop waiting for worker threads to stop.

First, in draw_mandelbrot() you have basically created a barrier, which waits for all workers to finish. You can use pthread_barrier_t for that instead: you create one with pthread_barrier_init() with the number of worker threads, and then after completing the work, each worker calls pthread_barrier_wait(). They will wait until all of them have called it, and exactly one of them will get PTHREAD_BARRIER_SERIAL_THREAD as the return value, so you can use that one only to call cairo_surface_mark_dirty().

Second, have draw_mandelbrot() call pthread_cond_signal() so create_surface() can pthread_cond_wait() for the workers to finish without wasting CPU cycles unnecessarily.

Third, you made the mistake of reading threads[i].drawing without having the mutex locked. That means the compiler is allowed to assume threads[i].drawing will not change between iterations of the do…while-loop, and thus move the for-loop out of the do…while-loop, and replace the latter with an if (!allWorkersStopped) while (true) {}. Depending on how the compiler feels, it might optimize the infinite loop away as well. So the problem now is that your code can either:

1. work
2. work most of the time
3. go into an infinite loop
4. skip the whole check for workers having stopped

To avoid any issues, make sure that if a variable is read from or written to in one place with a mutex locked, you also lock that mutex in all other places you read from or write to it.

Thread safety of Cairo and Gtk

Unless you can point to some official documentation from the Gtk and Cairo projects that the functions you are using are thread-safe, assume that they are not. g_main_context_invoke() is thread-safe, but I'm not sure cairo_surface_mark_dirty() is.

I would recommend that you only call cairo_surface_mark_dirty() on the main thread. However, g_main_context_invoke() basically has the effect of calling something on the main thread. So, I would just move the call to cairo_surface_mark_dirty() into queue_redraw_plane().

Avoid code duplication

There are several functions that need to check whether all working threads have completed their work. Instead of duplicating that code, write a function that does it, for example named have_workers_completed().

Consider using double-buffering

Instead of several places having to check whether the pixmap is valid and drawing into it has completed, it might be much nicer to have two pixmaps: one that the worker threads are writing to, and another that the GUI can draw. Once the work is done, the two can be flipped. This reduces the amount of synchronization necessary, makes the code simpler, and might even make your GUI more responsive.