Concurent Memory Access problem with multithreading in GTK3

Question

I am trying to implement an Audacity like recording interface.

For that I have a function that will imitate a sine wave it will basically give a float value for a point of the sine wave at given point in time. The buffer that I am filling up is always of size 1024, and so will contain 1024 float values. The calculation and the filling up of that array takes place in a separate thread. To pass the argument to my thread I a using a custom struct called vis_data which contains the array of floats, and a parameter running that tells the thread to run or to stop execution.

Here is how it looks like

    typedef struct
    {
        float *sig;
        int running;
    } vis_data; 

Here is the code for the sine function

    float sine(float volume, float frequency, double time)
    {
        return volume * (sin(frequency * M_PI2 * time));
    }

To do the visual part and the plotting I am using GTK3. I want to be able to draw 1 second of the signal so knowing that my sampling rate is 44100 samples per second, I have defined a macro to save that parameter and modify it if necessary.

    #define LIMIT 44100

So I have a function that draws the signal at each point in time using cairo on a GtkDrawingArea and when the tracing is done it will stroke it. However there is a little problem. Since none of the operations I am dealing with here are atomic,there seems to be some concurrent memory access type problem. I am guessing that the floats in the array that the drawing function is plotting, are recalculated faster by the thread than they can actually be plotted. So I have decided to use a mutex on my thread, but that did not seem to work!

Here is the code for my thread function

    int cpt = 0;
    
    pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
    
    static gpointer
    thread_func(gpointer user_data)
    {
        vis_data *vs =  (vis_data *)user_data;
        float * table = vs->sig;
        g_print("Starting thread %d\n", 1);
    
        while (vs->running)
        {
            pthread_mutex_lock(&mutex);
            int i = cpt % 1024;
            while (i)
            {
                double time = cpt / 44100.0;
                table[i] = sine(1.0, 10, time);
                i = cpt % 1024;
                cpt += 1;
            }
            cpt += 1;
            pthread_mutex_unlock(&mutex);
        }
        g_print("Ending thread %d\n", 1);
        return NULL;
    }

The cpt is used to continuously advance in the time, and the running allows the thread to run.

Here is the function that allows me to do the drawing

    static gboolean
    on_draw(GtkWidget *widget, cairo_t *cr, gpointer user_data)
    {
        vis_data *vs = (vis_data *)user_data;
        int zoom_x = 100;
        int zoom_y = 100;
    
        GdkRectangle da;            /* GtkDrawingArea size */
        gdouble dx = 0.5, dy = 0.5; /* Pixels between each point */
        gdouble i, clip_x1 = 0.0, clip_y1 = 0.0, clip_x2 = 0.0, clip_y2 = 0.0;
    
        GdkWindow *window = gtk_widget_get_window(widget);
        int drawing_area_width = gtk_widget_get_allocated_width(widget);
        int drawing_area_height = gtk_widget_get_allocated_height(widget);
    
        /* Determine GtkDrawingArea dimensions */
        gdk_window_get_geometry(window,
                                &da.x,
                                &da.y,
                                &da.width,
                                &da.height);
    
        /* Draw on a black background */
        cairo_set_source_rgb(cr, 0.0, 0.0, 0.0);
        cairo_paint(cr);
    
        /* Change the transformation matrix */
        // Put the origin of the graph into the botom left corner
        cairo_translate(cr, da.width / 2, da.height / 2);
        cairo_scale(cr, zoom_x, -zoom_y);
    
        /* Determine the data points to calculate (ie. those in the clipping zone */
        cairo_device_to_user_distance(cr, &dx, &dy);
        cairo_clip_extents(cr, &clip_x1, &clip_y1, &clip_x2, &clip_y2);
        cairo_set_line_width(cr, dx);
        cairo_set_source_rgb(cr, 0.0, 1.0, 0.0);
        cairo_move_to(cr, clip_x1, 0.0);
        cairo_line_to(cr, clip_x2, 0.0);
        cairo_move_to(cr, 0.0, clip_y1);
        cairo_line_to(cr, 0.0, clip_y2);
        cairo_stroke(cr);
    
        int cpt = 0;
        float he = 0;
        int num_it = 0;
        for (i = clip_x1; i < -clip_x1; i += (fabs(clip_x1) * 4) / LIMIT)
        {
            if (cpt < 1024)
            {
                he = vs->sig[cpt];
                cairo_line_to(cr, i, he);
            }
            else
            {
                num_it += 1;
                cairo_set_source_rgba(cr, 1, 1, 1, 1);
                cairo_stroke(cr);
                cpt = 0;
            }
            cpt += 1;
        }
        //gtk_widget_queue_draw_area(widget, 0, 0, drawing_area_width, drawing_area_height);
        return G_SOURCE_REMOVE;
    }

Notice that I have commented out the gtk_widget_queue_draw_area function, that was done to see the very first drawing of the signal, since if that one will seem correct so will the others

Here is What is looks like by the way at 10Hz

We can clearly see how the parts of the sine wave are being drawn but than they are cut, and a part of the sine wave of the next iteration is drawn

Just to assure you that the sine is actually correctly calculated here is what the program yields at 220Hz.

Just to make the code information complete here is my main

    int main(int argc, char **argv)
    {
    
        GThread *thread[N_THREADS];
    
        vis_data *vis_d = malloc(sizeof(vis_data));
        vis_d->sig = malloc(sizeof(float) * 1024);
        
        gtk_init(&argc, &argv);
    
        GtkBuilder *builder = gtk_builder_new();
        GError *error = NULL;
        if (gtk_builder_add_from_file(builder, "test.glade", &error) == 0)
        {
            g_printerr("Error loading file: %s\n", error->message);
            g_clear_error(&error);
            return 1;
        }
    
        GtkWindow *window = GTK_WINDOW(gtk_builder_get_object(builder, "window"));
    
        gtk_window_set_title(GTK_WINDOW(window), "Record");
    
        // Drawing Area
        GtkDrawingArea *da = GTK_DRAWING_AREA(gtk_builder_get_object(builder, "da"));
    
        g_object_unref(builder);
    
        g_signal_connect(G_OBJECT(window), "destroy", G_CALLBACK(on_destroy), &vis_d->running);
    
        g_signal_connect(G_OBJECT(da), "draw", G_CALLBACK(on_draw), vis_d);
    
        for (int n = 0; n < N_THREADS; ++n)
            thread[n] = g_thread_new(NULL, thread_func, vis_d);
    
        gtk_widget_show_all(GTK_WIDGET(window));
    
        gtk_main();
    
        for (int n = 0; n < N_THREADS; ++n)
            g_thread_join(thread[n]);
    
        free(vis_d->sig);
        free(vis_d);
    
        return 0;
    }

The on_destroy function

    void on_destroy(GtkWidget *Widget, gpointer user_data)
    {
        int *running = (int *)user_data;
        *running = 0;
        gtk_main_quit();
    }

And the test.glade which I use to build my Gtk from:

    <?xml version="1.0" encoding="UTF-8"?>
    <!-- Generated with glade 3.38.2 -->
    <interface>
      <requires lib="gtk+" version="3.24"/>
      <object class="GtkWindow" id="window">
        <property name="can-focus">False</property>
        <child>
          <object class="GtkDrawingArea" id="da">
            <property name="visible">True</property>
            <property name="can-focus">False</property>
          </object>
        </child>
      </object>
    </interface>

And this is what I am compiling with

    all:
        gcc `pkg-config --cflags gtk+-3.0` main.c -o visualiser `pkg-config --libs gtk+-3.0` -lm -lSDL2 -lasound -pthread -g -fsanitize=address

So my question is, how do I synchronize or block one of my functions (the on_draw or the thread) to correctly draw my progressing sine wave.

EDIT:

Based on the recommendations of @Gerhardh and @Alexander Dmitirev I have basically gotten rid of the array, and just have one value that my sine function now changes every iteration. Here is how the code looks like.

volatile gint cpt = 0;
gfloat val = 0;

pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

void sine(float volume, float frequency, double time)
{
    val = volume * (sin(frequency * M_PI*2 * time)) ;
}

static gpointer
thread_func(gpointer user_data)
{
    vis_data * vs = (vis_data*)user_data;
    g_print("Starting thread %d\n", 1);
    while (vs->running)
    {
        g_atomic_int_add(&cpt,1);
        double time = cpt/44100.0;
        pthread_mutex_lock(&mutex);
        sine(1.0,440,time);
        pthread_mutex_unlock(&mutex);
    }
    g_print("Ending thread %d\n", 1);
    return NULL;
}

So my cpt is now an atomic and there is a gfloat that the sine function changes every time it is called. And here is drawing part, I have changed the main loop of the on_draw function to adapt to those changes.

for (i = clip_x1; i < -clip_x1; i += (fabs(clip_x1) * 4) / 1024)
    {
        pthread_mutex_lock(&mutex);
        cairo_line_to(cr,i,val);
        pthread_mutex_unlock(&mutex);
    }
    cairo_set_source_rgba(cr, 1, 1, 1, 1);
    cairo_stroke(cr);

So if I understood correctly this is the flow of my code:

• thread loop increments cpt and locks the mutex
• the new value of val is calculated and stored in val
• thread loop unlocks the mutex
• the draw function locks the mutex
• draws a cairo line to the wanted point val
• the draw function unlocks the mutex

And this repeats again and again. However the result doesn't seem to be doing that, here is a photo of how the output looks like with a 440Hz frequency

Here is the result with a 4400Hz frequency

I am really not sure what my code is missing and why exactly is it doing that...

Thanks again for the help

Answer 1

Looks like it's time to update a little my q&a about multithreaded GTK applications. As @Gerhardh mentioned, mutex must be taken into account in all threads that access shared resources. Also note that mutexes should be held as little as possible. For example, this snippet:

while (vs->running)
        {
            pthread_mutex_lock(&mutex);
            int i = cpt % 1024;
            while (i)
            {
                double time = cpt / 44100.0;
                table[i] = sine(1.0, 10, time);
                i = cpt % 1024;
                cpt += 1;
            }
            cpt += 1;
            pthread_mutex_unlock(&mutex);
        }

should be rewritten:

while (vs->running)
        {

            int i = cpt % 1024;
            while (i)
            {
                double time = cpt / 44100.0;
                pthread_mutex_lock(&mutex); // access shared data
                table[i] = sine(1.0, 10, time);
                pthread_mutex_unlock(&mutex); // release ASAP
                i = cpt % 1024;
                cpt += 1;
            }
            cpt += 1;
            
        }

Next:

int cpt = 0;
    
    static gpointer
    thread_func(gpointer user_data)
    {
            ...
            int i = cpt % 1024;
            ...
            cpt += 1;
            ...
    }

Here you have a race while accessing cpt. It can be written and read non-atomically. Protected it with mutex or use g_atomic_* functions.

My recommendation is to make generator a separate class and access it's data only not directly but with dedicated methods. This way you can make it thread-safe.