🔒 Locked

Multithreaded Slideshow Crashing

Started by Halma Jul 12, 2006 at 1:14 PM 15 replies 1.9k views

Original Post

Jul 12, 2006 at 1:14 PM

I have a problem that has taken a good 2-3 hours of my time; I was hoping someone would be able to help me out. I wrote a simple, yet nice-looking, slideshow program using OpenGL, similar in appearance to the one that comes with Windows Media Center. It is presented with a list of images to display, and it displays them in order. The images are slowly moved and zoomed in/out while being displayed, and fade slowly into the next image (also moving). Now, since there could be a lot of (relatively big) images, I didn't want to load them all at the beginning. Loading them in the same thread as the game loop resulted in some really bad jerkiness in the smooth movement. So I load the next image in a separate background thread with slightly lower priority. Now, since OpenGL isn't thread-safe (or so I've heard), the image gets loaded into a separate buffer. When the thread is complete, the main thread copies the data into the OpenGL texture using glTexSubImage2D. (I even spread this copying over 50 frames to make sure I didn't miss any frames!) Now here's the problem: For some images, the image loading thread would crash. After several hours of debugging, I narrowed the problem down to an issue with memory. It seemed like I couldn't access some parts of the data array I'd allocated. The image library is something that I've been using in my game engines for months, so I don't think that it should have a problem. I guessed that the problem was in reallocation, so I made it so that the memory for the image wasn't deallocated and reallocated when the image size changed. I just allocated a large amount of memory, equal to the maximum allowed size, and set a flag (data_norealloc), to override delete and new operations. Here's the final reallocation code:


void Image::setsize(int width, int height) {
    if (w==width && h==height && data) return;
    if (width<0) width=0; if (height<0) height=0;

    if (data && !data_norealloc) delete [] data;
    if (width==0 || height==0) {
        w=h=0;
        data=0;
        return;
    }
    w=width; h=height;
    
    if (!data_norealloc)
        data = new Pixel[w*h];
}

void Image::set_constant_allocation(int width, int height) {
    setsize(width, height);
    data_norealloc=true;
}

Anyway, that seemed to fix the problem. But my question is, WHY does that fix the problem? I haven't done much work with multithreaded applications, could it be that memory allocated by one thread cannot be deleted from another thread? I just want to know if I really fixed the problem, or if it will come back to haunt me..

Ravuya

Jul 12, 2006 at 2:08 PM

Are you locking/mutexing this big buffer? I suspect you were reading from and writing to it at the same time from two different threads. Either that or some kind of bizarre behaviour inside new/delete.

Halma

Jul 13, 2006 at 8:44 AM

Thank you, Ravuya, for your reply.

I have a flag that doesn't get set until the background thread is finished loading the file. Only when the flag is set does the main thread begin to read the data in the buffer.

It's very strange that the problem involved images at certain resolutions. For example, it would happen 100% of the time, when writing to the buffer which had previously held a 673x1024 image. But it was fine for 1600x1200, 2000x1500, 1024x768.. Crazy, I'm telling you.

Halma

Jul 16, 2006 at 4:20 AM

No suggestions? Anybody?

Come on, threads like "Hi, I'm a girl" get hundreds of replies! :)

Ravuya

Jul 20, 2006 at 9:35 AM

Maybe you have a buffer overrun (just because of the abnormal size of the texture). How big is your 'Pixel' class? Have you tried running it with a memory debugger?

Other than that, I'm kinda stuck.

Bob Janova

Jul 20, 2006 at 6:04 PM

If you're allocating the memory in one thread and accessing it in another, that can cause problems. In particular, I think the memory must be allocated in the same thread as your OpenGL device context is defined in, which will usually be your main GUI thread.

I'm also suspicious of your data_norealloc. If you try to load a larger image than before, but data_norealloc is true, you'll overrun your data buffer.

Gorg

Jul 21, 2006 at 12:05 AM

Quote:
Original post by Bob Janova
If you're allocating the memory in one thread and accessing it in another, that can cause problems. In particular, I think the memory must be allocated in the same thread as your OpenGL device context.

That's wrong. The only thing with opengl and threads is that a context can only be current in on thread at a time.

Halma, your problem is a basic producer/consumer problem and you problem sounds like a race condition. A thread is trying to read or write to the buffer while it is being allocated/deallocated.

Setting the buffer to the max allowed size gives a zone that can always be read or written to. So you won't ever get crashes. But you could get corrupted image.

You *need* to use mutexes. A flag is not good enough. Setting a flag is not an atomic operation.

And actually, in your case you would need 2 flags. One for the main thread to wait on the loader thread, and one for loader thread to wait for main thread to finish loading the texture.

LordShade

Jul 21, 2006 at 12:28 AM

A read/write lock is what you want to implement but guess what, MS has done it for you. This is 'unsupported' by MS but works great when you want to share buffers between threads.

Can't remember where I found this but it may have been deep in MSDN.

Happy codings.

// .H file// multiple reader/single writer lock in Windows NT#ifndef _RW_LOCK#define _RW_LOCKclass CRWLock{private:	struct RTL_RWLOCK	{		CRITICAL_SECTION	mCS;		HANDLE				mSharedReleaseSemaphore;		DWORD				mSharedWaiters;		HANDLE				mExclusiveReleaseSemaphore;		DWORD				mExclusiveWaiters;		LONG				mNumberActive;		DWORD				mOwningThreadID;		DWORD				mReserved;		PVOID				mDebugInfo;	};	typedef RTL_RWLOCK* PRTL_RWLOCK;	RTL_RWLOCK		m_RWLock;		// the read/write lock	// function prototypes	typedef void (WINAPI* RtlInitializeResource)(PRTL_RWLOCK);	typedef void (WINAPI* RtlDeleteResource)(PRTL_RWLOCK);	typedef BYTE (WINAPI* RtlAcqResourceExclusive)(PRTL_RWLOCK, BYTE);	typedef BYTE (WINAPI* RtlAcqResourceShared)(PRTL_RWLOCK, BYTE);	typedef void (WINAPI* RtlReleaseResource)(PRTL_RWLOCK);#ifdef _DEBUG	typedef void (WINAPI* RtlDumpResource)(PRTL_RWLOCK);#endif	// names for functions as exported	static LPCSTR m_pstrInitName;	static LPCSTR m_pstrDeleteName;	static LPCSTR m_pstrGetExclusiveName;	static LPCSTR m_pstrGetSharedName;	static LPCSTR m_pstrReleaseName;#ifdef _DEBUG	static LPCSTR m_pstrDumpName;#endif	// static members to hold the function pointers	static RtlInitializeResource pfInitProc;	static RtlDeleteResource pfDelProc;	static RtlAcqResourceExclusive pfGetExclusiveProc;	static RtlAcqResourceShared pfGetSharedProc;	static RtlReleaseResource pfReleaseProc;#ifdef _DEBUG	static RtlDumpResource pfDumpProc;#endifpublic:	CRWLock();	~CRWLock();	// public members to lock and release the resource	BYTE GetExclusive(BYTE fWait);	BYTE GetShared(BYTE fWait);	void Release();	// function to dump the current status of the resource to the debugger output#ifdef _DEBUG	void Dump();#endif};#endif

// .CPP file#include "stdafx.h"#define WIN32_LEAN_AND_MEAN#include <windows.h>#include "RWLock.h"// function namesLPCSTR CRWLock::m_pstrInitName = "RtlInitializeResource";LPCSTR CRWLock::m_pstrDeleteName = "RtlDeleteResource";LPCSTR CRWLock::m_pstrGetExclusiveName = "RtlAcquireResourceExclusive";LPCSTR CRWLock::m_pstrGetSharedName = "RtlAcquireResourceShared";LPCSTR CRWLock::m_pstrReleaseName = "RtlReleaseResource";#ifdef _DEBUGLPCSTR CRWLock::m_pstrDumpName = "RtlDumpResource";#endif// function pointersCRWLock::RtlInitializeResource CRWLock::pfInitProc;CRWLock::RtlDeleteResource CRWLock::pfDelProc;CRWLock::RtlAcqResourceExclusive CRWLock::pfGetExclusiveProc;CRWLock::RtlAcqResourceShared CRWLock::pfGetSharedProc;CRWLock::RtlReleaseResource CRWLock::pfReleaseProc;#ifdef _DEBUGCRWLock::RtlDumpResource CRWLock::pfDumpProc;#endifCRWLock::CRWLock(){	// if this is the first object, fill in the statics	static HINSTANCE hNtdll = NULL;	if(!hNtdll)	{		hNtdll = ::LoadLibraryA("ntdll.dll");		pfInitProc = reinterpret_cast<RtlInitializeResource>(::GetProcAddress(hNtdll, m_pstrInitName));		pfDelProc = reinterpret_cast<RtlDeleteResource>(::GetProcAddress(hNtdll, m_pstrDeleteName));		pfGetExclusiveProc = reinterpret_cast<RtlAcqResourceExclusive>(::GetProcAddress(hNtdll, m_pstrGetExclusiveName));		pfGetSharedProc = reinterpret_cast<RtlAcqResourceShared>(::GetProcAddress(hNtdll, m_pstrGetSharedName));		pfReleaseProc = reinterpret_cast<RtlReleaseResource>(::GetProcAddress(hNtdll, m_pstrReleaseName));#ifdef _DEBUG		pfDumpProc = reinterpret_cast<RtlDumpResource>(::GetProcAddress(hNtdll, m_pstrDumpName));#endif	}	// initialize the structure	pfInitProc(&m_RWLock);}CRWLock::~CRWLock(){	// destroy the lock	pfDelProc(&m_RWLock);}	// public members to lock and release the resourceBYTE CRWLock::GetExclusive(BYTE fWait){	return pfGetExclusiveProc(&m_RWLock, fWait);}BYTE CRWLock::GetShared(BYTE fWait){	return pfGetSharedProc(&m_RWLock, fWait);}void CRWLock::Release(){	pfReleaseProc(&m_RWLock);}	// function to dump the current status of the resource to the debugger output#ifdef _DEBUGvoid CRWLock::Dump(){	pfDumpProc(&m_RWLock);}#endif

Halma

Jul 23, 2006 at 11:17 AM

Ravuya: The Pixel class is 4 bytes.

Quote:
I'm also suspicious of your data_norealloc. If you try to load a larger image than before, but data_norealloc is true, you'll overrun your data buffer.

Of course, but they aren't larger. I have a preset hard limit (2048x2048) on the image size, and I allocate for 2048x2048. Not pretty, I know, but I had a deadline and that was the only way I could get it to work.

Quote:

Halma, your problem is a basic producer/consumer problem and you problem sounds like a race condition. A thread is trying to read or write to the buffer while it is being allocated/deallocated.

It's hard for me to believe this. The main thread only reads from the image for the first 50 frames of a new slide. The second thread doesn't exist at this time.

Quote:

Setting the buffer to the max allowed size gives a zone that can always be read or written to. So you won't ever get crashes. But you could get corrupted image.

I have not yet noticed any corrupted images! All of them look perfect.

Quote:

You *need* to use mutexes. A flag is not good enough. Setting a flag is not an atomic operation.

And actually, in your case you would need 2 flags. One for the main thread to wait on the loader thread, and one for loader thread to wait for main thread to finish loading the texture.

I disagree. You are correct for general applications, but my system is very simple and I don't understand why a flag shouldn't work.

Here's how my system works: I have a global flag that is set by the background thread, whose sole purpose is to indicate when the background thread is finished and therefore the image can be read. This flag is set right before "return 0" in the background thread. Anyway, every frame, the main thread looks at the flag, and when the background thread is finished, it copies the data into the OpenGL texture. No writing is performed. The copying takes place over 50 frames; the next image loading happens at least around 500 frames after that. Until then, no other threads use the Image.

I also discovered that if I just always allocate 2048*2048 pixels instead of w*h, it works fine (even without data_norealloc).

Gorg

Jul 23, 2006 at 12:10 PM

Quote:

I have not yet noticed any corrupted images! All of them look perfect.

I said maybe. It would depend on a couple of factor(read speed, read location, etc.) But I made some assumptions on what you were doing that seems wrong from what you explain.

Quote:

The copying takes place over 50 frames;

What do you mean by that. You copy small chunks of image over the next 50 frames?

Quote:

at least around 500 frames after that. Until then, no other threads use the Image.

Does that mean you actually count the number of frames in your code and only start loading a new image after the 500 frames?

I think it would be much simpler if you posted your code.

DrEvil

Jul 23, 2006 at 12:18 PM

Why not use a single critical section? No need for a mutex for stuff in the same process, and they are very simple.

Halma

Jul 23, 2006 at 4:01 PM

Quote:

What do you mean by that. You copy small chunks of image over the next 50 frames?

Exactly. I didn't want to risk missing a frame vsync.

Quote:

Does that mean you actually count the number of frames in your code and only start loading a new image after the 500 frames?

Not exactly. I have two ImageDisplay objects (see code below), each containing an openGL texture and an Image. One corresponds to the current image, and the other is the second image. When the display time for the current image is over, the second image is faded in. At this time, the second thread is initiated, which reads the next image into the ImageDisplay object that is no longer being drawn. This only takes a small fraction of a second, but at worst we'll say 1 second. The data is copied into the texture over 50 frames, a little under a second. Since the image display time is set to between 8 and 10 seconds, that leaves quite a lot of time before the next scheduled image loading or copying.

Quote:

I think it would be much simpler if you posted your code.

OK. It was pretty big and messy, I cleaned it up and removed code irrelevant to the problem.

It's written using my game engine.

This is the Game.h file:

#include <string>#include <list>#include "Image.h"struct ImageDesc {    std::string imagefile, overlayfile;};struct ImageDisplay {    volatile bool done; //done is true when both background thread and loadnext have finished    bool done2;    bool disable;    bool islast;        int copystartframe;    double starttime, dur;    double fadetime_in, fadetime_out;    // [positioning variables omitted]    Texture *imagetex;  //OpenGL Texture    Image image;    ImageDisplay() {imagetex=0; disable=0; islast=0;}    void draw(double curtime);    void loadnext(double start, double infade);        void copychunk(), copyallchunks();};class Game {    enum State {gsNotStarted, gsRunning, gsFinished} state;       ImageDisplay imgdisp1, imgdisp2, *cur, *next;        void startloadinginback();public:    void init();        void handle_key(int key), handle_key_up(int key);    void render();    friend class ImageDisplay;};

And this is the Game.cpp file:

#include "Game.h"std::list<ImageDesc> imagelist;std::list<ImageDesc>::iterator nextimage;bool images_wrapped;double finishtime;ImageDisplay *loadingdisp;volatile bool loadingfinished=1;DWORD WINAPI load_in_background(LPVOID lpParam) {    Sleep(50); //Give calling thread time to lower this thread's priority    ImageDesc *idesc = &(*nextimage);    loadingdisp->image.openbmp(idesc->imagefile.c_str());            loadingfinished=1;    return 0;}void Game::startloadinginback() {    DWORD threadid, param;    loadingdisp=next;    loadingfinished=0;    next->done=0; next->done2=0;    HANDLE thread=CreateThread(NULL, 0, load_in_background, &param, 0, &threadid);    SetThreadPriority(thread, THREAD_PRIORITY_BELOW_NORMAL);}void ImageDisplay::loadnext(double start, double infade) {    if (disable) return;    while (!loadingfinished) Sleep(50);    if (!imagetex) //only happens once...        imagetex=new Texture(2048, 2048, 4, GL_LINEAR, GL_CLAMP);    imagetex->resize(image.getw(), image.geth());        if ((++nextimage) == imagelist.end()) {        nextimage=imagelist.begin();        images_wrapped=1;    }        //timing stuff...    starttime=start;    dur=R(settings.min_disp_time, settings.max_disp_time);    fadetime_in=infade;    fadetime_out=R(settings.min_fade_time, settings.max_fade_time);    dur+=fadetime_out;    //Movement and positioning calculations omitted (irrelevant)    imagetex->activate();    //Render textured quad [omitted]    done=1;        copystartframe=frametimer.getFrame()+1;}void ImageDisplay::copyallchunks() {    if (disable) return;    imagetex->activate();    glTexSubImage2D(GL_TEXTURE_2D, 0, 0,0, image.getw(),image.geth(), GL_RGBA, GL_UNSIGNED_BYTE, image.geto());    done2=1;}void ImageDisplay::copychunk() {  //copy a bit of the image into texture    if (disable) return;    const int nframe=50; //copy over 50 frames    int linesperframe=(image.geth()+nframe-1)/nframe;    int chunk=frametimer.getFrame()-copystartframe;    if (chunk>nframe) {        done2=1; return;    }    int start=chunk*linesperframe;    int end=(chunk+1)*linesperframe;    if (end>image.geth()) end=image.geth();    if (start<end) {        imagetex->activate();        glTexSubImage2D(GL_TEXTURE_2D, 0, 0,start, image.getw(),end-start, GL_RGBA, GL_UNSIGNED_BYTE, image.getrow(start));    }}void ImageDisplay::draw(double curtime) {    if (disable) return;    if (!done || !done2) return;    double t=curtime-starttime;    if (t<=0 || t>dur) return;        //blah, blah, blah, drawing routine..... (omitted)}void Game::init(HDC _hdc) {    state=gsNotStarted;        //load list of images into imagelist, give error if empty    //[omitted]    nextimage=imagelist.begin();    imgdisp1.image.set_constant_allocation(2048, 2048);    imgdisp2.image.set_constant_allocation(2048, 2048);        cur=&imgdisp1 next=&imgdisp2    next=cur;    startloadinginback();    cur->loadnext(frametimer.getTime(), R(settings.min_fade_time, settings.max_fade_time));     cur->copyallchunks();    next=&imgdisp2    startloadinginback();    next->loadnext(cur->starttime+cur->dur-cur->fadetime_out, cur->fadetime_out);    next->copyallchunks();    images_wrapped=0;}void Game::handle_key(int key) {    if (state==gsNotStarted && key==VK_SPACE) {        double dt=frametimer.getTime()-cur->starttime;        cur->starttime+=dt;        next->starttime+=dt;        state=gsRunning;    }}void Game::handle_key_up(int key) {}void Game::render() {    if (state==gsRunning) {        if (frametimer.getTime() > cur->starttime+cur->dur) {            //display time for cur finished            if (cur->islast) {                if (state!=gsFinished) finishtime=frametimer.getTime();                state=gsFinished;            } else {                ImageDisplay *temp=cur; cur=next; next=temp; //swap cur and next                if (!images_wrapped) startloadinginback();                else {                    next->disable=1;                }            }        }        if (loadingfinished && !next->done)  {            next->loadnext(cur->starttime+cur->dur-cur->fadetime_out, cur->fadetime_out);            if (images_wrapped) {                next->fadetime_out=0;                next->islast=1;            }        }        if (next->done && !next->done2 && frametimer.getFrame()>=next->copystartframe)            next->copychunk();        }    //[OpenGL setup omitted]        if (state==gsNotStarted) {        cur->draw(cur->starttime);    }    if (state==gsRunning) {        cur->draw(frametimer.getTime());        next->draw(frametimer.getTime());    }    if (state==gsFinished) {        cur->draw(cur->starttime+cur->dur-0.001);    }}

[Edited by - Halma on July 24, 2006 1:01:45 AM]

Gorg

Jul 24, 2006 at 12:24 AM

From a quick glance, the code seems fine, but I would not trust this opinion so late at night :)

It is a long shot, but which compiler are you using? I am thinking maybe you are using MSVC 6 and, somehow(this is the long shot), your systems runs out of memory and new returns NULL, but your are not checking for that and your loading thread crashes.

I'll have another look at the code tomorrow, which a fresh mind.

Halma

Jul 24, 2006 at 1:26 AM

I've compiled it with both MinGW GCC 3.4.2, and the Visual C++ that's in Visual Studio 2003. Both have the identical problem. I also tried changing code generation from single-threaded to multi-threaded in my VC project settings, but it made no difference.

As for memory, I have 1GB of RAM and 128MB of video RAM, so I don't think that is the issue.

Gorg

Jul 24, 2006 at 9:54 AM

Ok, I have read through your code, and the only thing I see that could cause a race condition are those 2 lines :

 done=1; copystartframe=frametimer.getFrame()+1;

See how you set copystartframe *after* done. This means that copychunk could be called before you set the new copystartframe.

But I am not fully certain how this could cause your problems : because of the check for 50 frames, then you can only get an invalid chunk number if getFrame() overflows, but in your main code your check for getFrame() >= next->copystartframe and this code would be false if getFrame() overflows.

In any case, this is a potential race condition, and should be fixed.

Halma

Jul 24, 2006 at 3:15 PM

Thank you, I really appreciate your taking the time to read my code.

But I don't think that the problem could be from setting done before copystartframe. copychunk won't be called in between, because all of this happens in one thread. All of the Game and ImageDisplay member functions are called from the main thread.

The second thread only loads an image from a BMP file, and it doesn't call any of the Game and ImageDisplay member functions, or look at their variables, other than the image object that it fills with data.

Gorg

Jul 24, 2006 at 6:25 PM

Quote:
Original post by Halma
But I don't think that the problem could be from setting done before copystartframe. copychunk won't be called in between, because all of this happens in one thread. All of the Game and ImageDisplay member functions are called from the main thread.

You are right. Sorry, for some reason I thought it was still in the thread.

I looked at the code again. (Carefuly this time :)) and I really do not know. The code looks fine.

The only other thing I can think of is : Are you sure all your libs and dll are using the same runtime library(all static or all dynamic). I know you said you tried to change the runtime library, but did you make sure they were all the same? Use the multithreaded version for sure.

Though a discrepancy in the runtime library would cause a problem on deallocation, not access and it would die very quickly.

If you are willing, you could post your code somewhere and I would compile it on my machine to see if I can reproduce the problem.

Aside from that, I don't believe I can be of any help.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.