0

My task recently was to find and fix a memory leak in some old code. I stumbled into a fix, and I'm having trouble understanding why the fix works. It seems that the Garbage Collector was not able to do its job, and I'm hoping someone can explain.

The memory leak is in a method that takes a custom object, asks for a bitmap from that object, and displays that bitmap in a Winforms PictureBox. It fires about 24 times a second, so memory leaks are obvious.

    private void updateVideoFrame(VideoFrame frame)
    {
        if (this.displayVideo == false || frame == null)
        {
            this.videoPictureBox.Image= null;
            return;
        }
        try
        {
            Bitmap bitmap = frame.ToBitmap();
            this.videoPictureBox.Image= bitmap;
        }
        catch (Exception e)
        {
            Debug.WriteLine("Update Video Exception: " + e.Message);
            Debug.WriteLine("Update Video Exception: " + e.StackTrace);
        }
    }

Where the call frame.ToBitmap() looks like this:

System::Drawing::Bitmap^ VideoFrame::ToBitmap()
{
    if (this->bytes == nullptr)
        return nullptr;

    try
    {
        return gcnew System::Drawing::Bitmap(this->Width, this->Height, this->RowStride, System::Drawing::Imaging::PixelFormat::Format24bppRgb, IntPtr((void*)this->bytes));
    }
    catch (Exception^ exception)
    {
        System::Diagnostics::Debug::Assert( true, "VideoFrame::ToBitmap() threw an exception\n" + exception->Message );
        return nullptr;
    }  

    return nullptr;
}

The solution was a change to a single line in the updateVideoFrame method:

this.videoPictureBox.Image= bitmap;

becomes

this.videoPictureBox.Image= new Bitmap(bitmap);

So why does changing that bitmap from a soft to a hard copy unblock the garbage collector?

Note: No amount of calls to Dispose() in updateVideoFrame() forced the garbage collector's hand. Dispose() could be called on the bitmap, and the videoPictureBox.Image to no positive effect. The memory leak continued to occur. Calling GC.Collect() also had no effect.

Edit: I've been asked to provide code for the Dispose() calls I tried.

    private Bitmap currentBitmap = null;

    private void updateVideoFrame(VideoFrame frame)
    {
        if (this.displayVideo == false || frame == null)
        {
            this.videoPictureBox.Image = null;
            return;
        }
        try
        {
            if (this.videoPictureBox.Image != null)
            {
                this.videoPictureBox.Image.Dispose();
            }

            if (this.currentBitmap != null)
            {
                this.currentBitmap.Dispose();
            }

            this.currentBitmap = frame.ToBitmap();
            this.videoPictureBox.Image = this.currentBitmap;

            GC.Collect();
        }
        catch (Exception e)
        {
            Debug.WriteLine("Update Video Exception: " + e.Message);
            Debug.WriteLine("Update Video Exception: " + e.StackTrace);
        }
    }

The above code still leaks as fast as it did before. Simply Commenting out

this.videoPictureBox.Image = this.currentBitmap;

prevents the leak, but of course also kills the entire point of the method.

Edit 2: The only additional epiphany I've had that could be a clue is that the method VideoFrame.ToBitmap() is, I believe, dangerous. I believe what it's doing is simply wrapping it's own byte array in a bitmap and returning it. This in turn means that the PictureBox is ALSO using that exact same byte array in memory. This should be a problem since the VideoFrame is manually de-referenced prior to the Bitmap being replaced with the next frame.

That said, the VideoFrame tracks it's own reference count correctly, and has this function that is being called at the correct time, it just doesn't seem to do anything if the PictureBox is displaying the byte array.

/// <summary>
/// Finalizes an instance of the <see cref="VideoFrame"/> class.
/// </summary>
VideoFrame::!VideoFrame()
{
    System::Threading::Interlocked::Increment(finalizeCount);
    if (this->bytes == nullptr)
        return;

    delete[] this->bytes;
    this->bytes = nullptr;
    GC::RemoveMemoryPressure(this->RowStride * this->Height);
}

/// <summary>
/// Disposes an instance of the <see cref="VideoFrame"/> class.
/// </summary>
VideoFrame::~VideoFrame()
{
    System::Threading::Interlocked::Increment(disposeCount);
    this->!VideoFrame();
    GC::SuppressFinalize(this);
}

What I might expect would happen is that the VideoFrame erases those bytes and the PictureBox ends up throwing an exception because the memory was erased out from under it, but that isn't the case. Every cleanup attempt happily runs without exception, and happily does nothing...

unless a hard copy is made, which would mean that the VideoFrame and the PictureBox now have their own duplicate byte arrays. As pointed out, when that happens no Dispose() calls are required. All cleanup happens.

So what principle or paradigm prevents the Garbage Collector doing cleaning up this memory in this situation? I want to understand so that I can recognize, avoid, or fix the overarching problem. Simple creating a Hard Refference for bad code doesn't make me happy, but even if the bad code is fixed, I want to understand why the GC handles this situation the way it does.

Earendil
  • 143
  • 1
  • 8
  • L.B. That question does have to do with garbage collecting, but that's where similarity ends. Dispose() was the solution to the linked issue, it was not the solution to mine. GC was working for him, just not when he expected. For me GC was never collecting. Since I'm asking why changing a soft to a hard copy of a bitmap allowed the Garbage collector to do it's thing, the answer of "use Dispose" is not helpful. Please reopen my question. – Earendil Apr 17 '15 at 16:19
  • Edit your code to show how you where attempting to dispose the bitmaps (instead of just telling us that you tried it) and it will help your chances of being reopened. – Scott Chamberlain Apr 17 '15 at 16:54
  • Okay, added that code. I'm still not sure why that's a big deal when a solution (even if not the best) is to make a hard copy of the Bitmap. No more memory leaks happens after that, which means the GC must be capable of handling this without the need to manually call Dispose(), right? – Earendil Apr 17 '15 at 19:10

0 Answers0