CODEWITHSUNDEEP
c++multithreadingboostmemory-leaksqueue
Rella
Rella

Reputation: 66945

Help wanted on memory leak - having a multi-threading queue, char buffer and a structure

So I have a boosted queue class that helps multi-threading described here.

In my class declarations I have

//...
    struct VideoSample
    {   
        const unsigned char * buffer;
        int len;
    };

    ConcurrentQueue<VideoSample * > VideoSamples;

//...

struct AudioSample
{   
    const unsigned char * buffer;
    int len;
};

ConcurrentQueue<AudioSample * > AudioSamples;

//...

In my class I have a function:

void VideoEncoder::AddFrameToQueue(const unsigned char *buf, int size )
{
    VideoSample * newVideoSample = new VideoSample;
    VideoSamples.try_pop(newVideoSample);

    newVideoSample->buffer = buf;
    newVideoSample->len = size;
    VideoSamples.push(newVideoSample);
    //free(newVideoSample->buffer);
    //delete newVideoSample;
}

keeping only one frame in queue is required for my app.

answer provided here on how to delete a structure is not helpful in this case because app crushes.

I have similar code for audio queue.

void VideoEncoder::AddSampleToQueue(const unsigned char *buf, int size )
{
    AudioSample * newAudioSample = new AudioSample;
    newAudioSample->buffer = buf;
    newAudioSample->len = size;
    AudioSamples.push(newAudioSample);
    url_write (url_context, (unsigned char *)newAudioSample->buffer, newAudioSample->len);
    AudioSamples.wait_and_pop(newAudioSample);
    //delete newAudioSample;
}

and a function that runs in separate thread:

void VideoEncoder::UrlWriteData()
{
    while(1){
        switch (AudioSamples.empty()){
        case true : 
            switch(VideoSamples.empty()){
        case true : Sleep(5); break;
        case false :    
            VideoSample * newVideoSample = new VideoSample;
            VideoSamples.wait_and_pop(newVideoSample);
            url_write (url_context, (unsigned char *)newVideoSample->buffer, newVideoSample->len);
            break;
            } break;
        case false :  Sleep(3);     break;
        }
    }
}

BTW: to stream media data to url I use ffmpeg's function.

BTW: here code I use for queues:

#include <string>
#include <queue>
#include <iostream>

// Boost
#include <boost/thread.hpp>
#include <boost/timer.hpp>

template<typename Data>
class ConcurrentQueue
{
private:
    std::queue<Data> the_queue;
    mutable boost::mutex the_mutex;
    boost::condition_variable the_condition_variable;
public:
    void push(Data const& data)
    {
        boost::mutex::scoped_lock lock(the_mutex);
        the_queue.push(data);
        lock.unlock();
        the_condition_variable.notify_one();
    }

    bool empty() const
    {
        boost::mutex::scoped_lock lock(the_mutex);
        return the_queue.empty();
    }

    bool try_pop(Data& popped_value)
    {
        boost::mutex::scoped_lock lock(the_mutex);
        if(the_queue.empty())
        {
            return false;
        }

        popped_value=the_queue.front();
        the_queue.pop();
        return true;
    }

    void wait_and_pop(Data& popped_value)
    {
        boost::mutex::scoped_lock lock(the_mutex);
        while(the_queue.empty())
        {
            the_condition_variable.wait(lock);
        }

        popped_value=the_queue.front();
        the_queue.pop();
    }

    Data& front()
    {
        boost::mutex::scoped_lock lock(the_mutex);
        return the_queue.front();
    }

};

My question is: How to clean up AddSampleToQueue and AddFrameToQueue so that they would not make memory leaks?

BTW: I am quite new to all this C++ shared/auto stuff. So to say a beginner. So please provide code examples that work at least that are integrated into my code - because I provided all my code. So if you know what to do - please try and integrate your knowledge into my example. Thank you. And if you can show me how to do it with no use of shared/auto ptrs I' d be super glad.

Upvotes: 4

Views: 1442

Answers (9)

Jake Kurzer
Jake Kurzer

Reputation: 1552

A lot of other people have suggested shared pointers.
I don't see a reason not to use a shared pointer instead of a queue here. You permit only one frame, after all. They support locking elegantly, and with just a bit of bodging can be made threadsafe and also simple. You don't really have an opportunity for circular references to occur that I can see, so you should be basically okay this way.

Alternatively, this sounds like a job for a nice circular buffer. That way you could just avoid the naked char array all together. Boost implements one elegantly, and with just a bit of primitive synchro you should be able to make it suitable for your purposes. Of particular note is that this would make expanding your application to handle online data processing relatively simple as well.

If you're interested, I'll hack up some example code.

Upvotes: 3

icecrime
icecrime

Reputation: 76755

I'm not sure I understand it all, but I'll give it a shot anyway.

The AddFrameToQueue function

So apparently you want a single frame in queue at a time, which means you probably don't need a queue at all. Anyway : either there isn't already a frame in queue and you should create a new one, or there is and you should overwrite its buffer and len fields :

void VideoEncoder::AddFrameToQueue(const unsigned char *buf, int size )
{
    VideoSample * newVideoSample = 0;
    if (!VideoSamples.try_pop(newVideoSample))
    {
        // Nothing in queue yet : we allocate a whole new VideoSample
        newVideoSample = new VideoSample;
    }
    else
    {
        // Here, you want to release newVideoSample->buffer depending on
        // the way it was allocated in the first place : free if malloc'ed,
        // delete if new'ed...
    }

    newVideoSample->buffer = buf;
    newVideoSample->len = size;
    VideoSamples.push(newVideoSample);

    // The VideoSample pointer has been pushed in the queue : we must no delete
    // it in order for the queue to keep containing a valid pointer
}

The AddSampleToQueue function

Why is there a wait_and_pop at the end of this function : isn't the pop supposed to take place in UrlWriteData ? I really don't understand this part. If the goal is to have a single item in queue, you probably don't need a queue (episode 2), but I guess you could use the same code as AddFrameToQueue.

The UrlWriteData function

Here, data is actually being removed from the queue, so you want to release it as soon as you're done writing.

void VideoEncoder::UrlWriteData()
{
    while(1){
        switch (AudioSamples.empty()){
        case true : 
            switch(VideoSamples.empty()){
            case true : Sleep(5); break;
            case false :    
                VideoSample * newVideoSample;
                VideoSamples.wait_and_pop(newVideoSample);
                url_write (url_context, (unsigned char *)newVideoSample->buffer, newVideoSample->len);

                // Release newVideoSample->buffer using free if malloc'ed, delete
                // if new'ed...

                delete newVideoSample;

                break;
            } break;
        case false :  Sleep(3);     break;
        }
    }
}

That's the best I can't tell you without trashing the whole thing and go for smart pointers, RAII and all those things that make C++ :)

Upvotes: 1

bala sreekanth
bala sreekanth

Reputation: 484

change your function to the below and do similar changes in other place where you are allocating memory.

void VideoEncoder::AddFrameToQueue(const unsigned char *buf, int size ) { 

VideoSample * newVideoSample;
if(!VideoSamples.try_pop(newVideoSample))
{
    newVideoSample  = new VideoSample; 
}
else
{
   delete buff;
}

newVideoSample->buffer = buf; 
newVideoSample->len = size; 
VideoSamples.push(newVideoSample);

}

ANd also I cannot stop myself asking this question.. When you want only one item in queue then why do you need queue at all.

Upvotes: 1

mmmmmmmm
mmmmmmmm

Reputation: 15513

Your code

VideoSample * newVideoSample = new VideoSample;
VideoSamples.try_pop(newVideoSample);

is a memory leak. If try_pop succeeds it would overwrite the pointer in newVideoSample and your reference to the instance created before is lost forever!

Upvotes: 1

Sam Miller
Sam Miller

Reputation: 24174

valgrind will help you find nearly any memory leak in your program. Though as others pointed out, you should be using shared_ptrs.

Upvotes: 1

Philipp
Philipp

Reputation: 11813

First I would change ConcurrentQueue<VideoSample * > VideoSamples; into 

ConcurrentQueue<VideoSample> VideoSamples;

You don't need this pointer. Turn the rest of the pointers to smart pointers and you're all set!

Upvotes: 1

Pete Kirkham
Pete Kirkham

Reputation: 49311

If, when the frame is added to the queue, the ownership of the data array is transferred to the sample, free or delete[] it in the sample's destructor.

You also might want to uses a move constructor so you can have a queue of ConcurrentQueue<VideoSample> rather than ConcurrentQueue<VideoSample*>, which would make the samples you're enqueuing and dequeuing automatic.

Or, if you control what is pushing the data onto the queue use a vector or boost::array instead of a C-style array.

It's also a bit odd to use a queue if you really do only ever want one thing to be in it. Having an variable protected by a mutex and a condition variable would do instead.

Upvotes: 3

ROAR
ROAR

Reputation: 1324

Use Shared_ptr

Upvotes: 3

Anycorn
Anycorn

Reputation: 51465

use smart pointers: http://www.drdobbs.com/184401507

Upvotes: 3

Related Questions