Asserts when creating an internal TimeSliceThread

[dave96]

Hi all, I have quite a few components that need to render on a background thread but I can't seem to avoid hitting various assertions about leaked Typeface objects or decrementing reference counts below 0 if I create that TimeSliceThread internally and I create more than one instance of the object. Essentially that means each object creates and manages the lifetime of its own TimeSliceThread. There are no problems when using a shared TimeSliceThread between multiple objects.

Now I know its not good practice to internally create TimeSliceThreads, and a user supplied one is a much cleaner approach, but I thought this would be handy for quick GUI elements. It also bugs me because I can't see why this is happening.

I've managed to distill this down to a simple test component that just fills an image with a different colour on the background thread and then a timer signals periodic repaints. Creating one of these with a nullptr to the constructor is fine, create two and the asserts intermittently happen, use a shared TimeSliceThread and pass it to the constructors and the asserts go away.

Code: Select all

#ifndef __TESTSCOPE_H_1771BC75__
#define __TESTSCOPE_H_1771BC75__

#include "../JuceLibraryCode/JuceHeader.h"

//==============================================================================
class TestScope :   public Component,
                    public Timer,
                    public TimeSliceClient
{
public:
    //==============================================================================
    /**
     */
    TestScope (TimeSliceThread* backgroundThreadToUse = nullptr);
   
    /** Destructor. */
    ~TestScope();

    //==============================================================================
    /** @internal */
    void resized();
   
    /** @internal */
    void paint (Graphics& g);

    /** @internal */
    void timerCallback();
   
    /** @internal */
    int useTimeSlice();
   
private:
    //==============================================================================
   
    Image image;
    CriticalSection imageLock;
    Random random;

    OptionalScopedPointer<TimeSliceThread> backgroundThreadToUse;
    //==============================================================================
    void renderImage();
   
    //==============================================================================
    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (TestScope);
};

#endif  // __TESTSCOPE_H_1771BC75__


Code: Select all

#include "TestScope.h"

TestScope::TestScope (TimeSliceThread* backgroundThreadToUse_)
    : backgroundThreadToUse (backgroundThreadToUse_, backgroundThreadToUse_ == nullptr ? true : false)
{
    const ScopedLock sl (imageLock);
    image = Image (Image::RGB, jmax (1, getWidth()), jmax (1, getHeight()), false);
    Graphics g (image);
    g.fillAll (Colours::black);
   
    if (backgroundThreadToUse == nullptr)
    {
        OptionalScopedPointer<TimeSliceThread> newThread (new TimeSliceThread ("Scope Rendering Thread"), true);
        backgroundThreadToUse = newThread;
        backgroundThreadToUse->startThread (1);
    }
       
    backgroundThreadToUse->addTimeSliceClient (this);
   
    startTimer (1000 / 60);
}

TestScope::~TestScope()
{
    stopTimer();
   
    backgroundThreadToUse->removeTimeSliceClient (this);

    if (backgroundThreadToUse.willDeleteObject())
        backgroundThreadToUse->stopThread (500);
}

//==============================================================================
void TestScope::resized()
{
    const ScopedLock sl (imageLock);

    image = Image (Image::RGB, jmax (1, getWidth()), jmax (1, getHeight()), false);
    Graphics g (image);
    g.fillAll (Colours::black);
}

void TestScope::paint (Graphics& g)
{
    const ScopedLock sl (imageLock);

    g.drawImageAt (image, 0, 0);
}

void TestScope::timerCallback()
{
    repaint();
}

int TestScope::useTimeSlice()
{
    renderImage();

    return 10;
}

//==============================================================================
void TestScope::renderImage()
{
    const ScopedLock sl (imageLock);
   
    Graphics g (image);
    // draw something onto the image here

    g.fillAll (Colour (random.nextInt()));
}

I think it must have something to do with the destruction of the TimeSliceThread but I can't see why the reference counting would go haywire when there's no reference to a Typeface or Graphics in the destructor.

Any ideas? It would be enlightening to know and possibly helpful to others. I've created a simple Introjucer project with two instances of the above component which easily demonstrates the issue.

TestScope.zip

Demo Introjucer Project

(65.66 KiB) Downloaded 7 times

[TheVinn]

From what I remember, the GlyphCache is not thread safe.

[dave96]

After some further investigation it doesn't appear to be a problem with my creating internal TimeSliceThreads, more when using multiple TimeSliceThreads. If I modify the example given to use two supplied TimeSliceThreads then I get the asserts and EXE_BAD_ACCESS crashes in CoreGraphicsContext::setFont.

Surely its reasonable to expect two different threads to be able to render graphics in the background? What if you need one set of components to have a higher priority than others?

[TheVinn]

After looking over GlyphCache it seems that Jules made it thread safe. Perhaps there is something else that snuck in? You'll have to dig into the JUCE functions you are calling and find out, if we're going to fix it.

[dave96]

Ok, following the constructor path of a Graphics object it seems to crash when getting as far as getting the default typeface object. Changing Typeface to be a normal atomically counted ReferenceCountedObject seems to fix all my problems. Not a crash after running the example I provided for 10 minutes+.

Jules would it be ok to make this change?

Poking through all the Graphics and Font stuff however there does seem to be an awful lot of SingleThreadedReferenceCountedObjects. Surely this is asking for trouble and while they don't seem to affect my particular use case they may well affect others. Is there any need to use standard primitive counters rather than atomics? Surely there isn't much of a performance increase? Labelling everything 'SingleThreaded' implies these classes should not be used on background threads which must be a problem for anyone doing real-time visual rendering on low priority background threads? Surely we want to keep as much work off the message thread as possible?

[TheVinn]

I'm quite surprised that Jules upgraded GlyphCache but forgot the rest of Graphics and its cousins...

[jules]

there does seem to be an awful lot of SingleThreadedReferenceCountedObjects. Surely this is asking for trouble and while they don't seem to affect my particular use case they may well affect others. Is there any need to use standard primitive counters rather than atomics? Surely there isn't much of a performance increase? Labelling everything 'SingleThreaded' implies these classes should not be used on background threads which must be a problem for anyone doing real-time visual rendering on low priority background threads? Surely we want to keep as much work off the message thread as possible?

Since I've not actually tried to make the graphics code thread-safe, I think that using thread-safe pointers would be sending out the wrong message. IMHO it's better that it all fails quickly and lets you know that something is wrong, rather than having code that mostly works, so that you feel confident in using it, but which then fails with some rare problem when you've released your app.

(But Typeface::Ptr is a slightly different situation, and yes, I'll change that to use a normal ref-counted object).

[TheVinn]

We talked about this and the consensus was that one Graphics per Thread is a useful use-case, and that Graphics should be thread-safe when used in this fashion.

However, multiple Thread accessing the same Graphics is not likely to be supported or needed.

[jules]

We talked about this and the consensus was that one Graphics per Thread is a useful use-case, and that Graphics should be thread-safe when used in this fashion.

Absolutely. But I haven't done it yet, I'd need to really thoroughly review the code to make sure it's ok for use like that.

[dave96]

Since I've not actually tried to make the graphics code thread-safe, I think that using thread-safe pointers would be sending out the wrong message. IMHO it's better that it all fails quickly and lets you know that something is wrong, rather than having code that mostly works, so that you feel confident in using it, but which then fails with some rare problem when you've released your app.

Agreed, I was getting a bit ahead of myself before, I don't actually need (or should anyone really need) Graphics to be thread safe, I'm not sharing a Graphics context at all. I do think any underlying shared resources should however be thread safe e.g Typeface as this throws a real spanner in the works. There may be others but I have yet to uncover them.

(But Typeface::Ptr is a slightly different situation, and yes, I'll change that to use a normal ref-counted object).

Excellent, this (as far as I can tell for now) solves all my problems. I think this would be by far the most common problem for people and has probably come up before on here.