Worker Thread Memory Problem

[jrojfer]

Hi all,

i have a problem in my application. I have a thread that captures images from a camera and I need to store JPGs. For this task, i use one thread for each photo but i don't know how to avoid memory conflicts. The camera returns the bitmap on m_pbitmap and I copy this buffer to another one that I pass to the thread (I thought that If i created one buffer every loop it would have different memory addresses and I wouldn't have any problem). My code is:


UINT CreateJPGThread(LPVOID pParam)
{
unsigned char** JPGBuffer = (unsigned char**) pParam;

...

CreateJPGFile(file, *JPGBuffer, W,L, true, true); //It takes 600ms

delete [] *JPGBuffer; // Free the buffer reserved in the other thread
*JPGBuffer = NULL;

return 0;
}

...

//Inside capturing thread...

while (m_showCamera)
{
if (!theCamera.CaptureImage()) {
theCamera.getDIB(m_pBitmap);
unsigned char* bufferJPG = new unsigned char[W*H*3];
memcpy(bufferJPG,m_pBitmap,W*H*3);
AfxBeginThread(CreateJPGThread, &bufferJPG);
}
Sleep(1000); //(1)Just for testing
}
delete [] m_pBitmap;
...



If I set the Sleep I have no problems with memory but I loose some pictures waiting. But if I comment the Sleep it seems to happen some overlapping memory problem or something and it crashes (not immediately but after some pictures... I think it is when the CreatJPEGthread hasn't finished and another thread begins...)

My idea was to allocate a new buffer for each bitmap captured, pass it to the thread, use it and free it inside this thread. That's way I declared the pointed and allocated the memory inside the while and i use memcpy...

My programming skills are not the best ones, I am a beginner, so I would appreciate any kind of help.

Thank you very much beforehand...

[JVene]

Memory problems can be tough.

I see you're making the assumption that the DIB is a 24bpp bitmap, are you certain that's true? They can be 32bpp - even if alpha channel isn't used.

I suppose you might be building with the non-threaded CRT - check that. Memory allocations are usually serialized (some locking mechanism causes all memory allocations to occur one at a time - they're not usually done in parallel). If, for some reason, you were calling a non-threaded version, perhaps there's no locking of the heap, meaning there could be collision in what malloc/new give out (I've never seen this happen, but then I don't recall using threads in a non-threaded CRT).

A DIB has a larger structure than just the bits, if memory serves. You're allocating enough room for the pixels, assuming it really is 24bpp, but what about the DIB header?

Instead of a raw block of RAM, a DIB copy should be made to the bits of another DIB. Your crash might be nothing more than some function that expects a DIB reading the leading header material and attempting to render nonsense.

[MikeAThon]

Instead of Sleep(1000), do the following:

1. Each time in the loop that you create a new thread, store the thread's handle into the next index of an array of handles.

2. Outside the loop, in place of Sleep(1000), call WaitForMultipleObjects() using the array of handles, with the bWaitForAll flag set to TRUE.

3. When WaitForMultipleObjects() completes, then all threads are finished and it's therefore safe to call delete[] on m_pBitmap.

Mike

[Arjay]

I would structure the code slightly differently, using classes and a division of responsibility. I would move the tasks that create the JPG file into a class called CProcessCapture. This class would contain its own thread proc that writes out the file. It's constructor would retrieve the camera's bmp pointer and perform the copy. I would make a thread denoted by '\\inside capturing thread' as static method of a class (called CaptureMgr). In addition to containing the static capturing thread proc, it would contain a list of the CProcessCapture objects.

The psuedo code would be:


// Capturing thread...
private static CCaptureMgr::CaptureThread( LPARAM lParam )
{
CCaptureMgr* pCaptureMgr = static_cast< CCaptureMgr* > ( lParam );

while (pCaptureMgr->ShowCamera( ) )
{
if( !pCaptureMgr->CaptureImage( ) )
{
CBitmap* pBitMap = pCaptureMgr->GetDIB( );

// CProcessBitmap performs the copy from pBitmap object
CProcessBitmap* pProcessBitmap = new CProcessBitmap( pBitMap );

// Add this item to our list, so we can clean up later
pCaptureMgr->AddProcessBitmapItem( pProcessBitMap );

// Save the file (this spawns the new thread with the CProcessBitmap object
pProcessBitmap->SaveFile( );
}
}

[jrojfer]

Memory problems can be tough.

I see you're making the assumption that the DIB is a 24bpp bitmap, are you certain that's true? They can be 32bpp - even if alpha channel isn't used.


Hi JVene, I think they are 24bit bitmap.


I suppose you might be building with the non-threaded CRT - check that. Memory allocations are usually serialized (some locking mechanism causes all memory allocations to occur one at a time - they're not usually done in parallel). If, for some reason, you were calling a non-threaded version, perhaps there's no locking of the heap, meaning there could be collision in what malloc/new give out (I've never seen this happen, but then I don't recall using threads in a non-threaded CRT).


Really, I don't know what CRT is. My options in compiler, VS 6.0, under code generation tab specifies Multithreaded DLL...


A DIB has a larger structure than just the bits, if memory serves. You're allocating enough room for the pixels, assuming it really is 24bpp, but what about the DIB header?

Instead of a raw block of RAM, a DIB copy should be made to the bits of another DIB. Your crash might be nothing more than some function that expects a DIB reading the leading header material and attempting to render nonsense.

My function returns the pointer to the buffer with the pixel information, I don't use the dib information for representing or whatever... I think my CreateJPGFile based on JPEGLIB just needs the buffer with the pixel information...


Instead of Sleep(1000), do the following:

1. Each time in the loop that you create a new thread, store the thread's handle into the next index of an array of handles.

2. Outside the loop, in place of Sleep(1000), call WaitForMultipleObjects() using the array of handles, with the bWaitForAll flag set to TRUE.

3. When WaitForMultipleObjects() completes, then all threads are finished and it's therefore safe to call delete[] on m_pBitmap.

Mike

Hi Mike,

In the way you explain, while the application is waiting for all the threads for finishing I could loose some pictures... Just imaging that my application could take a picture every 300ms during one year, how many handles do i have to store 100, 1000, 10000...? and then wait for them (for the last ones, the first ones are supposed finished...). In this period i could loose some pictures...



I would structure the code slightly differently, using classes and a division of responsibility. I would move the tasks that create the JPG file into a class called CProcessCapture. This class would contain its own thread proc that writes out the file. It's constructor would retrieve the camera's bmp pointer and perform the copy. I would make a thread denoted by '\\inside capturing thread' as static method of a class (called CaptureMgr). In addition to containing the static capturing thread proc, it would contain a list of the CProcessCapture objects.

The psuedo code would be:


// Capturing thread...
private static CCaptureMgr::CaptureThread( LPARAM lParam )
{
CCaptureMgr* pCaptureMgr = static_cast< CCaptureMgr* > ( lParam );

while (pCaptureMgr->ShowCamera( ) )
{
if( !pCaptureMgr->CaptureImage( ) )
{
CBitmap* pBitMap = pCaptureMgr->GetDIB( );

// CProcessBitmap performs the copy from pBitmap object
CProcessBitmap* pProcessBitmap = new CProcessBitmap( pBitMap );

// Add this item to our list, so we can clean up later
pCaptureMgr->AddProcessBitmapItem( pProcessBitMap );

// Save the file (this spawns the new thread with the CProcessBitmap object
pProcessBitmap->SaveFile( );
}
}


Hi Arjay,

actually I just manipulate buffer (unsigned char) not CBitmaps because my inherited code did like this... Could you explain me why do you difine the thread as static? why not a normal worker thread? (I repeat my knowledge is poor... :o( )
The interesting part is how CprocessBitmap copies the buffer, how it creates the thread, and how it frees resources...


Now for you all, the simplest question in the world, about my code, when the program reach here for first time:

unsigned char* bufferJPG = new unsigned char[W*H*3];

it takes one byte for the pointer and stores the address of the first byte of the allocated buffer...
Imaging the second time it reaches there, the parallel thread is executing (is creating JPEG...) . does it take another different byte for the second pointer than in the previous loop? Inside the thread i am making a delete[] freeing the entire buffer but who is freeing that byte of the pointer that was pointing the buffer? If every loop different memory areas are being taken why do memory confligts appear?
When lock mechanisms, mutex, etc must be used? In my case?

Thank you very much, I really appreciate your help!

JRF

[Arjay]

actually I just manipulate buffer (unsigned char) not CBitmaps because my inherited code did like this... Could you explain me why do you difine the thread as static? why not a normal worker thread? The interesting part is how CprocessBitmap copies the buffer, how it creates the thread, and how it frees resources... I mentioned the term 'division of responsibility' but I didn't explain it very well. The idea is that each class that we create handle creating there own resources (and clean up) and also handle the creation of their thread. The CProcessBitmap object when created, gets the pointer to the bmp buffer, allocates an internal buffer (within the class) and copies this buffer into the internal buffer. It does this in the constructor during this line:


// CProcessBitmap performs the copy from pBitmap object
CProcessBitmap* pProcessBitmap = new CProcessBitmap( pBitMap );


so this line is synchronous and the pBitMap (I know you aren't using the CBitmap object - that's okay) can only be accessed by one thread.

After that the pProcessBitmap object is added to a list in the CCaptureMgr class (from within the CaptureThread proc). This is done so, during destruction, the CCaptureMgr class can clean up all the pProcessBitmap objects that were added to the list.

Once the pProcessBitmap object has been added to the list its SaveFile() method is called. This method creates and executes a thread within the CProcessBitmap object that writes out the file.

Static is used in the thread procs because they are members of their associated classes. The thread functions can't operate on class instance methods, so we use static and pass in the 'this' to the thread proc which allows the thread to access the instance variables of the class that started it. NOTE: The code assumes the usage of _BeginThreadEx.

[MikeAThon]

In the way you explain, while the application is waiting for all the threads for finishing I could loose some pictures... Just imaging that my application could take a picture every 300ms during one year, how many handles do i have to store 100, 1000, 10000...? and then wait for them (for the last ones, the first ones are supposed finished...). In this period i could loose some pictures...
You need to re-think your design, since there's no way that you could start 10000 threads anyway.

Mike

[jrojfer]

After that the pProcessBitmap object is added to a list in the CCaptureMgr class (from within the CaptureThread proc). This is done so, during destruction, the CCaptureMgr class can clean up all the pProcessBitmap objects that were added to the list.

Ok, so Imaging that the while (in the code you posted) is executing for one year, and every 300ms a pProcessBitmap is created (with the memory allocated for every bitmap). If we have to wait to exit the while and destruct the pCaptureMgr object (for destructing all the pProcessBitmap stored in the list), it could be 5 minutes (what means 1000 pProcessBitmap objects were created and stored) or 1 year (what means 105120000 pProcessBitmap objects)... how big should the memory be? Because, is the list infinite?

This is what I do not catch... :( Somehow inside the while we should free the resources which have finished their task meanwhile we create new ones.

Thanks a lot!!!!!!

[Arjay]

If we have to wait to exit the while and destruct the pCaptureMgr object (for destructing all the pProcessBitmap stored in the list), it could be 5 minutes (what means 1000 pProcessBitmap objects were created and stored) or 1 year (what means 105120000 pProcessBitmap objects)

There are many ways to handle this problem and using a list may not be the best way. But if you continue to use a list, you can simply mark the CProcessBitmap object as 'completed' at the end of the SaveFile method. (you'll need to protect this variable with some form of synchronization)

Then each time through the while loop, you can call a RemoveCompletedItemsFromList( ) method.


while (pCaptureMgr->ShowCamera( ) )
{
if( !pCaptureMgr->CaptureImage( ) )
{
CBitmap* pBitMap = pCaptureMgr->GetDIB( );

// CProcessBitmap performs the copy from pBitmap object
CProcessBitmap* pProcessBitmap = new CProcessBitmap( pBitMap );

// Add this item to our list, so we can clean up later
pCaptureMgr->AddProcessBitmapItem( pProcessBitMap );

// Save the file (this spawns the new thread with the CProcessBitmap object
pProcessBitmap->SaveFile( );
}

// Walk the list and remove items that have been completed
pCaptureMgr->RemoveCompletedItemsFromList( );
}

[jrojfer]

Thank you all, I will try to solve it somehow as soon as i can...

I appreciate your help.

JRF

[usman999_1]

Well, I would like to do it like

a. Have ONE worker thread (wrapped in a class) for processing the Image captured from camera.

b. That worker thread class have a critical-section protected list/vector of data (image data received from camera).

d. The worker thread moves in a while loop (probably using WaitFor... for some event triggered by main thread). And on incase the event is set, checks the list, if there is something to process (image data from the camera) takes it out of the list, and processes it and deletes what needs to be deleted & then again goes on to wait.

c. The main thread in a loop gets the data from the camera, pushes it to worker thread list/vector sets the event so that Worker thread wakes up and do the job.

Also in the ~ of thread class, you can delete and/or save the un-processed image data (asking the user to save or just ignore) when the user wants to close the application.
This way you won't miss the images and also you wont be creating new thread for every single image. You can further improve your design by using a message queue to send messages (the data received from the camera) to the worker thread without locking or you can use IOCP as non-locking MQ but that is little bit more involved.

Hope this helps,
Regards,
Usman.

[jrojfer]

Hello Usman,

Thanks for your reply. I like your idea of not creating a thread for every picture...

I think I dont understand how critical section works. Both threads (main and worker) share a buffer protected by a critical section (declared inside the worker), is it like this??? When the main thread takes a new data how does it pass to the thread? If it is protected and the worker thread is doing some processing stuff, the main thread couldn't pass it, could it? So we could miss some pictures...

Being radical, if the creation of one JPG would take 3minutes, just with one worker thread... we should implement a kind of FIFO... or we would miss pictures...

The most important question for me is:

If I have a shared buffer between two threads, how can i sincronize it (locking, critical section,...) without blocking the access?

Thank all very much

JRF

[usman999_1]

Well, as I said earlier in my post, here is some pseudo code that should answer your question



while( nRet = WaitForSingleObject(...) ) // You might have to check here for nRet incase there was some error
{
if( /* Event has been set */ )

{
// Try Acquiring the Lock to Critical Section, incase the main thread has the lock, this thread will wait

// Lock acquired...

// Check the list...

// Take the element OUT of the list

// Release the Critical Section...

// Now work with the image data you got from the list, the main thread can now work with the list (add new images etc etc), you might have to check the list again in a loop may be and if its empty then go on to WaitForSingle...

}
}



So as always only lock/acquire the CriticalSection/Mutex as less time as possible. So here I think with this you won't miss any (or very few & I think is highly unlikely) images.

Or if you want you can use some mechanism to remove all those synch-objects so that the main and worker thread do not lock. But I would say first try with the CriticalSection and if you are not happy with the performance, only then think about improving.

Hope this helps,
Regards,
Usman.