5
\$\begingroup\$

I'm using some resources in the code below to pin an array in memory and construct a Bitmap using that buffer. I'm not sure if I'm handling all of the resourced in the proper way.

I'm interested if the way I'm handling the GCHandle and other resources in the constructor in a safe manner, so there are no resource leaks.

using System.Drawing;
using System.Drawing.Imaging;
using System.IO;
using System.Runtime.InteropServices;

namespace Model
{
    public class FastImage
    {
        private const int BytesPerPixel = 4;
        private readonly byte[] _buffer;
        private const int ExtraSpace = 0;

        public int Width { get; set; }
        public int Height { get; set; }

        public FastImage(int width, int height)
        {
            Width = width;
            Height = height;
            _buffer = new byte[Width * Height * BytesPerPixel];
        }

        public FastImage(Bitmap from)
        {
            Width = from.Width;
            Height = from.Height;
            _buffer = new byte[Width * Height * BytesPerPixel];
            GCHandle handle = GCHandle.Alloc(_buffer);
            try
            {
                var address = Marshal.UnsafeAddrOfPinnedArrayElement(_buffer, 0);
                var stride = BytesPerPixel * Width;
                var pixelFormat = PixelFormat.Format32bppArgb;
                using (var fastBitmap = new Bitmap(Width, Height, stride, pixelFormat, address))
                {
                    using (var fastBitmapGraphics = Graphics.FromImage(fastBitmap))
                        fastBitmapGraphics.DrawImageUnscaled(from, 0, 0);
                }
            }
            finally
            {
                handle.Free();
            }
        }

        public Color GetPixel(int x, int y)
        {
            var i = y * BytesPerPixel + x;
            return Color.FromArgb(_buffer[i], _buffer[i + 1], _buffer[i + 2], _buffer[i + 3]);
        }

        public void SetPixel(int x, int y, Color color)
        {
            var i = y * BytesPerPixel + x;
            _buffer[i] = color.A;
            _buffer[i + 1] = color.R;
            _buffer[i + 2] = color.G;
            _buffer[i + 3] = color.B;
        }

        public void Save(string filename)
        {
            GCHandle handle = GCHandle.Alloc(_buffer);
            try
            {
                var address = Marshal.UnsafeAddrOfPinnedArrayElement(_buffer, 0);
                var stride = BytesPerPixel * Width;
                var pixelFormat = PixelFormat.Format32bppArgb;
                using (var fastBitmap = new Bitmap(Width, Height, stride, pixelFormat, address))
                {
                    fastBitmap.Save(filename);
                }
            }
            finally
            {
                handle.Free();
            }
        }

        public void Save(MemoryStream stream, ImageFormat format)
        {
            GCHandle handle = GCHandle.Alloc(_buffer);
            try
            {
                var address = Marshal.UnsafeAddrOfPinnedArrayElement(_buffer, 0);
                var stride = BytesPerPixel * Width;
                var pixelFormat = PixelFormat.Format32bppArgb;
                using (var fastBitmap = new Bitmap(Width, Height, stride, pixelFormat, address))
                {
                    fastBitmap.Save(stream, format);
                }
            }
            finally
            {
                handle.Free();
            }
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

I cannot complain about your code much. You declare variables as readonly where appropriate and you use the try/finally block to free the resources.

The only things I don't like are these properties

public int Width { get; set; }
public int Height { get; set; }

you don't want them to be settable. The setter should be private.


and the magic numbers here (and in other places).

_buffer[i + 1] = color.R;
_buffer[i + 2] = color.G;
_buffer[i + 3] = color.B;

I find you should use a helper class for them:

static class ColorOffset
{
    public const int Alpha = 0;
    public const int Red = 1;
    public const int Green = 2;
    public const int Blue = 3;
}

_buffer[i + ColorOffset.Red] = color.R;
_buffer[i + ColorOffset.Green] = color.G;
_buffer[i + ColorOffset.Blue] = color.B;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.