Memory leak help - having a multi-threaded queue, char buffer and structure

Question

So, I have a high-queue class that helps multi-threaded describe 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; } 

my application only needs one frame in the queue.

The answer here on how to remove the structure does not help in this case, because the application is overwhelming.

I have similar code for an 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 works in a 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 transfer media to url I use the ffmpeg function.

BTW: here is the 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 clear AddSampleToQueue and AddFrameToQueue so that they do not leak memory?

BTW: I am completely new to this whole thing C + + shared / auto. So to speak a beginner. So please provide code examples that work, at least in my code, because I have provided all my code. Therefore, if you know what to do, try integrating your knowledge into my example. Thanks. And if you can show me how to do this without using general / automatic ptrs, I will be very happy.

Answer 1

change your function to below and make similar changes in another place where you allocate 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); 

}

Also, I can’t stop asking this question. When you need only one item in a queue, why do you need a queue at all.

Answer 2

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

Answer 3

Use Shared_ptr

Answer 4

If, when adding a frame to the queue, the owner of the data array is transferred to the sample, frees or removes [] it in the sample of the destructor.

You can also use the move constructor so that you can have the ConcurrentQueue<VideoSample> rather than the ConcurrentQueue<VideoSample*> , which would make the samples that you queue and automatically cancel.

Or, if you control what pushes data in a queue, use a vector or boost :: array instead of a C-style array.

It's also a little weird to use a queue if you really only want one thing to be in it. Instead, it will have a variable protected by a mutex and a condition variable.

Answer 5

Many other people have suggested common pointers.
I see no reason not to use a shared pointer instead of a queue here. In the end, you only allow one frame. They support locking elegantly, and with few bindings can be made thread safe, as well as simple. You really have no way for the circular links that I see, so you should basically be fine.

Alternatively, this sounds like a job for a good circular buffer. That way you can just avoid the bare char array all together. Boost implements one of them elegantly, and with a bit of primitive synchronization, you should be able to make it suitable for your purposes. Of particular note, this will expand your application for processing online data processing is relatively simple.

If you're interested, I'll crack some sample code.

Answer 6

First I would change the ConcurrentQueue<VideoSample * > VideoSamples; on the

 ConcurrentQueue<VideoSample> VideoSamples; 

You do not need this pointer. Turn the rest of the pointers to smart pointers and you're done!

Answer 7

valgrind helps you find almost any memory leak in your program. Although, as others have pointed out, you should use shared_ptrs.

Answer 8

Your code

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

- memory leak. If try_pop succeeds, it will overwrite the pointer in newVideoSample, and your reference to the instance created earlier will be lost forever!

Answer 9

I'm not sure I understand all this, but I will do it anyway.

AddFrameToQueue Function

Apparently you need one frame in the queue at a time, which means you probably don't need the queue at all. In any case: either there is no frame in the queue, and you must create a new one, or there is, and you must 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 } 

AddSampleToQueue Function

Why is there wait_and_pop at the end of this function: not the pop to be held in UrlWriteData ? I really don't understand this part. If the goal is to have one element in the queue, you probably don't need the queue (episode 2), but I think you could use the same code as AddFrameToQueue .

UrlWriteData Function

Here, the data is actually removed from the queue, so you want to release it as soon as you finish 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; } } } 

This is the best thing that I can’t tell you without having to figure it all out and go for smart pointers, RAII and all those things that C ++ do :)