Making unsafe code safe in C#

Question

I was recently reading through an article on image processing in C#

There is a bit of code in there that I don't really like because it's unsafe, and I would like to know if it can be made safe:

public static bool Invert(Bitmap b)
{
    // GDI+ still lies to us - the return format is BGR, NOT RGB. 
    BitmapData bmData = b.LockBits(new Rectangle(0, 0, b.Width, b.Height), 
        ImageLockMode.ReadWrite, PixelFormat.Format24bppRgb); 
    int stride = bmData.Stride; 
    System.IntPtr Scan0 = bmData.Scan0; 
    unsafe 
    { 
        byte * p = (byte *)(void *)Scan0;
        int nOffset = stride - b.Width*3; 
        int nWidth = b.Width * 3;
        for(int y=0;y < b.Height;++y)
        {
            for(int x=0; x < nWidth; ++x )
            {
                p[0] = (byte)(255-p[0]);
                ++p;
            }
            p += nOffset;
        }
    }

    b.UnlockBits(bmData);

    return true;
}

The line byte* p = (byte*)(void*)Scan0; looks like the culprit, but I have to say I don't really understand what it's doing, or how it could be made safe.

Can anyone shed some light on this please?

If you don't know what it's doing, why do you want to "make it safe"? Are you scared of the word "unsafe"? This code is used to use pointers to directly access a bitmap's pixels in memory. The alternative would be to use `GetPixel()` which marshals each pixel's data and is horribly slow. — CodeCaster, May 29 '15 at 13:34
`Scan0` is an `IntPtr`, so you basically have to use pointers if you want to get the same processing speed. — Lucas Trzesniewski, May 29 '15 at 13:35
"How can I make it not unsafe?" - stop using pointers. If you don't know _why_ its using pinters then no't mess with it (or find someone who _does_ know what it's doing and _why_). — D Stanley, May 29 '15 at 13:36
[Related answer](http://stackoverflow.com/a/1563170/706456) to your question. — oleksii, May 29 '15 at 13:40
@CodeCaster The marshalling actually isn't that much of an overhead with `GetPixel` (it's just `HandleRef`, `int` and that's it - all native, no marshalling needed). There's a lot of argument checking done for each `GetPixel` call, though. And of course, the overhead of having a method call at all. — Luaan, May 29 '15 at 13:54

Luaan · Accepted Answer · 2015-05-29T13:52:04.523

The unsafe code is used for performance reasons, mostly. The basic idea is that you're going byte-by-byte over the image data, and flipping each byte manually (although there's more efficient and simple ways to handle the same thing).

The underlying image is handled by GDI+, which is unmanaged code. So when you're working with the image bytes directly, you have to manipulate unmanaged memory. How safe or unsafe this is is actually surprisingly tricky to determine - it depends a lot on how the unmanaged memory was originally allocated. Given that you're working from managed code, and you probably loaded the bitmap from a file or some stream, it's pretty likely it's not really unsafe, actually - there's no way for you to accidentally overwrite your managed memory, for example. The unsafe keyword's name doesn't come from being inherently dangerous - it comes from allowing you to do very unsafe things. For example, if you allocated the memory for the bitmap on your own managed stack, you could mess things up big time.

Overall, it's a good practice to only use unsafe code if you can actually prove that it's worth the costs. In image processing, this is quite often a good trade-off - you're working with tons of simple data, where overheads in e.g. bounds checking can be significant, even though it's quite easy to verify them only once, rather than in each iteration of the cycle.

If you wanted to get rid of this unsafe code, one way would be to allocate your own byte[] (managed), use Marshal.Copy to copy the image data to this byte[], do your modifications in the managed array, and then copy the results back using Marshal.Copy again. The thing is, this means allocating a byte[] as big as the original image, and then copying it twice (the bounds checking is negligible in this scenario - .NET JIT compiler will optimize it away). And in the end, it's still possible to make a mistake while using the Marshal.Copy that will give you the same issues you'd have with unsafe (not entirely, but that would be for a much longer talk).

For me, the by far most valuable part of having unsafe as a keyword is that it allows you to localize the unsafe stuff you're doing. While a typical unmanaged application is unsafe through and through, C# only allows you to be unsafe in specifically marked parts of the code. While those can still affect the rest of the code (which is one of the reasons you can only use unsafe in a FullTrust environment), it makes them much easier to debug and control. It's a trade-off, as always.

However, the code is actually unsafe in a very different manner - the UnlockBits call may never happen if there's an exception in the middle of the code. You should really use finally clauses to ensure properer cleanup of unmanaged resources.

And as a final note, you probably will not be doing image processing on the CPU anyway, if you want "real" performance, safe or unsafe. Today, it's often safe to assume that the computer you're running on has a GPU that can do the job faster, easier and with total isolation from the code actually running on the computer itself.

Making unsafe code safe in C#

1 Answers1