OpenGL Demo Race Condition? And Accessing Message Thread Data in renderOpenGL()

[timart]

Is the following line a race condition between the OpenGL thread and the JUCE MessageThread?

In the file juce/examples/GUI/OpenGLDemo.h, line 848 (in develop branch), in the function renderOpenGL():

glViewport (0, 0, roundToInt (desktopScale * (float) getWidth()), roundToInt (desktopScale * (float) getHeight()));

This line accesses juce::Component methods, getWidth() and getHeight() while running on the OpenGL thread via the renderOpenGL() callback.

I set a breakpoint here and went back in the stack trace to OpenGLContext.cpp -> renderFrame() and I saw that the MessageManager::Lock, mmLock local variable had its member lockWasSuccessful set to false, meaning that the OpenGL thread is not holding a Message Thread lock when accessing Component data. The Component data could simultaneously be modified by the MessageThread, causing a race condition.

The screenshots below show what I saw in the debugger.

---

In the past, it was my understanding that the OpenGL thread always holds a MessageManager::Lock in the renderOpenGL() callback. But, this is not always true. The OpenGL thread only holds a MessageManager::Lock when the associated OpenGLContext is attached to render juce::Components, not purely GL code, and any of those Components are marked to be redrawn by the GL thread. If no Components need to be redrawn, and there is other rendering being done by the GL thread in a renderOpenGL() callback, then the GL thread will not be holding a MessageManager::Lock.

I learned this from this post: OpenGL Thread MessageManager Lock - #4 by fr810

---

Lastly, I see, also in OpenGLDemo.h line 679 (develop branch), a MessageManagerLock is requested before running a bit of code on the GL thread:

const MessageManagerLock mml (ThreadPoolJob::getCurrentThreadPoolJob());
if (! mml.lockWasGained())
    return false;

g.drawFittedText (String (Time::getCurrentTime().getMilliseconds()), image.getBounds(), Justification::centred, 1);

I am not sure why this particular code is guarded by the lock while many Graphics calls above, for an Image, are not guarded. (I am not entirely sure which JUCE methods/objects are unsafe to access on a thread other than the Message Thread.)

If I were to access juce Message Thread data in a renderOpenGL() callback, should I request a MessageManagerLock in the same fashion?

const MessageManagerLock mml (ThreadPoolJob::getCurrentThreadPoolJob());
if (! mml.lockWasGained())
    return false;

Thanks for any help and clarity on this topic!

[timart]

@reuk Any chance you could speak to this? Thank you for any help and for your time!

[reuk]

Thanks, this is on my backlog to investigate, but it might have to wait a little bit while we prep the upcoming release.

[chkn]

@timart

Lastly, I see, also in OpenGLDemo.h line 679 (develop branch), a MessageManagerLock is requested before running a bit of code on the GL thread:

The drawing text-routines need the MML, imho because of some internal routines which require the message-thread.

Font::getStringWidth on non-message thread

Would have been a good time to check this out: Rail

besides of this, I would suggest that the gl-context should create cached versions of getWidth()/Height() which can be safefly accesd from the gl-callback because they are fundamentally useful.
Its really important that the GL-Thread can run as independent as possible.

[timart]

Great to hear, thanks! Best wishes for the upcoming release.

[timart]

Thanks, that definitely makes it clear we need a MessageManagerLock to access Font related methods.

In the program I am writing, one of my OpenGLRenderers uses CachedValues to set various OpenGL Uniforms in the renderOpenGL() callback. I understand that CachedValues are Message Thread specific objects since they receive synchronous value updates from the Message Thread.

I assume if I were to access any of my CachedValues in the renderOpenGL() callback, I would need to first obtain a MessageManagerLock as described in the original post.

[reuk]

Thanks for your patience. The following commits should resolve thread sanitizer issues in the OpenGL demos in the main repository:

OpenGL Demos: Fix Thread Sanitizer warnings

OpenGLContext: Avoid reading from component on background thread

Please let us know if you run into any new issues with these changes.

[timart]

Wow this is great, thank you very much @reuk!

So, I gather from the first commit that anyone who is writing custom OpenGL code using OpenGLRenderer cannot assume automatic thread safety between the Message Thread and the GL Thread (or pool). Thread safety between these threads is not managed for the user by JUCE.

In a developer’s implemented OpenGLRenderer callbacks, if they wish to use any Message Thread data such as Components (their bounds, mouse states, etc.), ValueTrees, etc., the developer will need to handle thread safety with the Message Thread themselves.

If my understanding is correct, maybe the documentation for OpenGLRenderer should be updated to more strongly reflect this. At the moment, the docs make it sound like if you attach your OpenGLContext to a Component and register your OpenGLRenderer, then your renderer callbacks are thread-safe because the MessageManager will be locked. But from what I understand, any use of OpenGLRenderer will need to handle its own thread-safety with the Message Thread, even if it is attached to a component (as demonstrated in my original post with the OpenGLDemo).

Quote from the OpenGLRenderer::renderOpenGL docs:

If the context is attached to a component in order to do component rendering, then the MessageManager will be locked when this callback is made.

If no component rendering is being done, then the MessageManager will not be locked, and you’ll need to make sure your code is thread-safe in any interactions it has with your GUI classes.

Maybe these confusing lines should be removed and it should just say: “You’ll need to make sure your code is thread-safe in any interactions it has with your GUI classes.”

Am I understanding correctly?

[reuk]

I think you’re right, but with one caveat. It looks like if setContinuousRepainting is false, but a custom render is still being used, then the message manager is pretty much guaranteed to be locked during the callback. In this case, render calls will only be made when a component needs to repaint.

If you’re using continuous repainting, then you can’t rely on the message manager to be locked, and you will need to add synchronization to avoid races on any data that you need to access from both the message thread and the render callback. As mentioned in the docs, you should not take the message manager lock yourself inside the render callback, as this is likely to cause deadlocks on macOS.

[timart]

Ah interesting, thank you for your insight and help!

When checking the docs on develop, I came across that new warning you refer to: not to use MessageManagerLock in the renderOpenGL() callback. I assume this is why you removed the MessageManagerLock from the OpenGLDemo in your commit. That is very handy to know.

So, if a developer were using a custom OpenGLRenderer with setContinuousRepainting as true, they would need to handle thread safety themselves. When handling safety with the Message Thread, they would not be able to use MessageManagerLock, but could use a other safety solutions.

Some thread safety solutions, as demonstrated in your commit, include:

• Caching data when it changes and guarding access of that data with a mutex (as you demonstrated with caching Component bounds)
• Caching data into an atomic when it changes (as you demonstrated with caching the Component mouseDown state)

Are there any other thread safety options between the GL Thread and Message Thread that come to mind? These are the only tools I am aware of (besides other varieties of atomics and mutexes).

---

In the JUCE GL app I am working on at the moment, I am accessing CachedValues in renderOpenGL(). I believe these CachedValues get updated synchronously on the Message Thread via a ValueTree. So my access of these CachedValues in renderOpenGL() is a race condition because the Message Thread may update the value while the GL thread reads the value.

To remedy this, I could use a bunch of std::atomics instead of CachedValues, and then manually set the atomics in a ValueTree::Listener callback. Alternatively, I could use a mutex to lock access to these pieces of data when they are accessed or written to in the ValueTree::Listener callback. Further, I could create some kind of wrapper class that obfuscates these approaches but essentially does the same stuff.

Does it sound like I am diagnosing this issue properly and going about a solution in the right direction?

Thank you again for all your help! This has brought me lots of clarity.

[reuk]

@timart

Are there any other thread safety options between the GL Thread and Message Thread that come to mind? These are the only tools I am aware of (besides other varieties of atomics and mutexes).

You could also use lock-free strucutres such as queues (i.e. produce updates on the main thread, add them to the queue, and then read the updates inside the render callback). Using a mutex or an atomic will probably be simplest, though.

@timart

Does it sound like I am diagnosing this issue properly and going about a solution in the right direction?

Yes, I think so. Reading a CachedValue inside the render callback is dangerous because it might be concurrently updated on the main thread. You’ll need to find some way of synchronizing access to the CachedValues in order to avoid races. Of the techniques you suggested, I think using atomics will be far simpler. Using locks to synchronize access to the CachedValues would require you to take the lock before making any ValueTree modifications. If you’re using JUCE classes to manage the value tree (e.g. the AudioProcessorValueTreeState) it might not be possible to add locks in all the necessary locations.

[timart]

Amazing! Thank you for your confirmation and advice.
I appreciate all of your swift help with this issue.
Thanks again for your time and congrats on the recent JUCE release!

[timart]

@reuk I’ve found another misleading documentation comment regarding thread safety with OpenGL. (In addition to the docs for OpenGLRenderer::renderOpenGL mentioned earlier in this thread.)

The docs for OpenGLContext::executeOnGLThread() say:

This function is useful when you need to execute house-keeping tasks such as allocating, deallocating textures or framebuffers. As such, the functor will execute without locking the message thread. Therefore, it is not intended for any drawing commands or GUI code. Any GUI code should be executed in the OpenGLRenderer::renderOpenGL callback instead.

https://docs.juce.com/develop/classOpenGLContext.html#a48062bbab36b278a5ff95831dc8c333d

From what I have learned in this thread, GUI code should NOT be executed in OpenGLRenderer::renderOpenGL() as this could cause data races between the GL thread and the JUCE Message Thread, unless setContinuousRepainting is false.

[timart]

On latest JUCE develop (past 6.1.2), I am still getting MacOS thread sanitizer warnings when using OpenGL in the JUCE DemoRunner. I have never actually been able to use OpenGL with JUCE without getting some thread sanitizer warnings, so this is not unexpected, and it may not actually be anything to worry about. But, after your previous post, @reuk, I thought maybe all thread sanitizer warnings with OpenGL in JUCE might have been resolved.

@reuk

The following commits should resolve thread sanitizer issues in the OpenGL demos in the main repository:

OpenGL Demos: Fix Thread Sanitizer warnings · juce-framework/JUCE@a87efde · GitHub OpenGLContext: Avoid reading from component on background thread · juce-framework/JUCE@f0d11f9 · GitHub

I have noticed that these commits (and possibly other JUCE OpenGL changes) have reduced the number of sanitizer warnings I am receiving. I was getting 9 warnings on JUCE 6.0.8. Now with JUCE 6.1.2 I’m getting 5 warnings.

Do I even need to worry about these thread sanitizer warnings? I’m sure tons of plugins have shipped with these sanitizer issues without any real problem for the users. When the warnings are triggered for my plugin, and I am using the debugger, the audio cuts out. I was worried about this at first, but I haven’t experienced audio dropouts when running without a debugger. My bet is the logging of the thread sanitizer in the debugger causes the audio dropouts.

Thread Sanitizer Warnings

I get 5 thread sanitizer warnings in the JUCE DemoRunner the first time I access any GL related demo or if I go to Settings and set the Renderer to OpenGL Renderer. This spurt of warnings seems to only occur the first time GL is used during the lifetime of the app.

Here are the five warnings:

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSView frame] must be used from main thread only

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSView window] must be used from main thread only

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSView superview] must be used from main thread only

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSView convertRect:toView:] must be used from main thread only

JUCE/modules/juce_gui_basics/native/juce_mac_NSViewComponentPeer.mm: runtime:
UI API called from background thread:
-[NSWindow convertRectToScreen:] must be used from main thread only

Specs

• JUCE develop (past 6.1.2 on commit 428260a6fdf274c8f1721d61deda079110a3fa2b)
• MacOS 10.15.7
• MacBook Pro (Retina, 15-inch, Late 2013)

[reuk]

Thanks for reporting, the issue should be fixed here:

OpenGLContext: Fix thread/consistency warnings on macOS

As always, please let us know if you discover any further issues.

[dataexcessaudio]

Reading this thread with great interest. I also have a Component implementing OpenGLRenderer, and it is updating some uniform variables used by the openGl renderer method in a custom-made parameterCallback class

void GraphicsCanvas::registerParameterCallbacks()
{
    m_params.finalMappedZoomChangedCallback = [this](float value) { 
        m_openGLContext->makeActive();
        if (!OpenGLHelpers::isContextActive()) return;
        m_shaderProgram->use();
        m_zoom->set(value);
    };
}

std::unique_ptr<OpenGLShaderProgram::Uniform> m_zoom;

The ParameterCallback class is essentially a wrapper around the AudioParameterFloat class and invokes a std::function whenever valueChanged is called. I wonder if what I am doing is thread-safe. And I believe it is since the AudioParameterFloat class is using an atomic for it’s value. Could someone confirm this? I guess @timart was using “regular / non-paramerer” valueTree values and these are not atomic by default.

I do sometimes get small audio dropouts when I am wildly changing GUI sliders which are attached to the AudioParameterFloats which in turn set the openGl uniform variables and I wonder if it has to do with this race-condition (if there is one - I think the atomic fixes it) ?

[chkn]

Without knowing the details of your implementation, your approach does not seem ideal to me.

Doing “heavy weight” things like glContext->makeActive() (btw: the comments mention “You should never need to call this in normal use” ) inside a parameter callback (might be also called directly from the audio thread, depends on the wrapper) looks troublesome. It will possibly block the audio thread.

Better find a lock free synchronisation, maybe set a flag, so that in the openGL renderCallback or the message-Thread the updates can be processed, without blocking the audio-thread.

[dataexcessaudio]

I am trying to understand this (I am still not super familliar with multithreading concepts). So you think that the audio dropouts are due to the fact that the audio-thread is pushing the changes and basically setting the atomic values used in the audioParameters and then since they are atomic they internally use a lock, and so when the openGL context uses it it locks it and when this happens the audio-thread is being blocked?

So basically I should have a mechanism in the openGLrenderer method that makes sure that the parameter value is currently not being set by the audio-thread. But when this is the case, it can ignore and simply not set the uniform variable (it is ok if the opengl renderer skips one or a few frames and uses the previous value). I think I did this kind of thing before with a TryLock. Should I use this? Or with a flag? Is there an example somehwere with a flag? Does the flag need to be atomic aswell?

Thank you so much!

[zsliu98]

@dataexcessaudio

the audio dropouts are due to the fact that the audio-thread is pushing the changes and basically setting the atomic values used in the audioParameters and then since they are atomic they internally use a lock

AFAIK those are atomic floats, which should be lock-free on most platforms. You may verify it with std::atomic<T>::is_always_lock_free.

Besically you need to make the audio thread real-time safe. If there are some suspicious heavy things like makeActive() (I am not familiar with OpenGLContext), you may dispatch it to the message thread.

[dataexcessaudio]

Wrapping the body of the callback inside a MessageManager::callAsync method fixed the audio dropouts!!!

Thank you so much!