System.Drawing.Bitmap Wrapper Class

Question

I have created two classes to help me work with images in C#. I decided to wrap the System.Drawing.Bitmap so I don't have to deal with different file formats (let the framework open/save them), but the Get/Set methods of System.Drawing.Bitmap are far too slow.

I created an abstract class instead of an interface so I don't have to repeat myself creating overloads for trivial methods whenever I decided to create another classes to deal with different PixelFormats. Also for performance, instead of checking the PixelFormat and for every GetColor(...)/SetColor(...) and deciding if I should read 3 or 4 bytes, I just use a class that knows exactly how to read/write a color. Think about it: for a 1000x1000 image I'm saving 1000000 checks.

The GetColor(int,int) method for a 24bpp image would be:

public Color GetColor(int x, int y)
{
    index = (y * stride) + (x * 3);          
    byte b = scan0AsBytePointer[index];
    byte g = scan0AsBytePointer[index + 1];
    byte r = scan0AsBytePointer[index + 2];

    return new Color(r,g,b);
}

and for a grayscale image:

public Color GetColor(int x, int y)
{
    index = (y * stride) + x;          
    byte gray = scan0AsBytePointer[index];

    return new Color(gray, gray, gray);
}

In this very specific case, a 32bpp PixelFormat, I'm able to optimize the code reading an entire integer (4 bytes) as creating a Color struct with it:

    System.Drawing.Color GetColorNoException(int x, int y)
    {
        index = (y * stride) + (x * 4);           

        return Color.FromArgb(scan0AsIntPointer[index/4]);
    }

This trick (reading a integer) may look like unnecessary and somewhat hurts the legibility, but in my testings this yielded a 50% performance gain (over reading 4 bytes and creating a color) for GetColor() calls that doesn't cause a cache miss.

Again, I'm using inheritance:

1. I don't repeat myself creating the overloads
2. I'm able to create specialized classes to deal with specific formats in the fastest possible way (this is the main reason)
3. As a last resort I still can create a static method that checks the PixelFormat of a image, creates the specific FastBitmapXX object and return it as a FastBitmap.

I'm using UNSAFE code instead of marshaling to improve the performance of LoadBits() and SaveBits() method, plus saving on memory.

I tested the classes and they appear to be working correctly, but I'd like to have them reviewed. I'm open to suggestions (especially performance related ones) and critiques. I know a few methods have no commentary, but they are pretty simple ones. I shall comment 'em later. Methods that don't check for exceptions are called only when I'm SURE all parameters are ok.

/// <summary>
/// Abstract class for bitmap-manipulating classes.
/// </summary>
public unsafe abstract class FastBitmap : IMatrix<Color>, IDisposable
{
    #region Fields

    /// <summary>
    /// The bitmapData from the underlaying Bitmap object.
    /// </summary>
    protected BitmapData bmpData;

    /// <summary>
    /// The stride of the underlaying Bitmap.
    /// </summary>
    protected int stride;

    /// <summary>
    /// The pointer to the BitmapData.Scan0
    /// </summary>
    protected IntPtr scan0AsIntPtr;

    /// <summary>
    /// Pointer to the BitmapData.Scan0.
    /// As byte* to support weird PixelFormats.
    /// </summary>
    protected byte* scan0AsBytePointer;

    /// <summary>
    /// A pointer to the Bitmap.Scan0.
    /// As int* to improve the performance of 32bpp format.
    /// </summary>
    protected int* scan0AsIntPointer;

    /// <summary>
    /// Used with a scan0 pointer to get/set colors.
    /// </summary>
    protected int index = 0;

    /// <summary>
    /// A reference to the underlaying Bitmap.
    /// </summary>
    protected Bitmap bmp;

    /// <summary>
    /// Whether the memory of the underlaying Bitmap is currently locked and being used by this FastBitmap.
    /// </summary>
    protected bool memoryLocked = false;

    /// <summary>
    /// A rectangle represeting the size of the underlaying Bitmap.
    /// </summary>
    protected Rectangle validArea;

    /// <summary>
    /// A color used to improve the performance of tight SetColor(...) loops.
    /// </summary>
    protected Color currentColor;

    /// <summary>
    /// The width (number of columns) of the underlaying Bitmap.
    /// </summary>
    public int Width { get; protected set; }

    /// <summary>
    /// The height (number of rows) of the underlaying Bitmap.
    /// </summary>
    public int Height { get; protected set; }

    /// <summary>
    /// The PixelFormat of the underlaying Bitmap.
    /// </summary>
    public PixelFormat PixelFormat { get; protected set; }

    #endregion

    public FastBitmap(Bitmap bmp, PixelFormat pixelFormat)
    {
        if (bmp.PixelFormat != pixelFormat) throw new FormatException("The bitmap has a invalid PixelFormat. Expected " + pixelFormat.ToString() + ".");

        this.bmp = bmp;
        this.Width = bmp.Width;
        this.Height = bmp.Height;
        this.PixelFormat = bmp.PixelFormat;

        this.validArea = new Rectangle(0, 0, Width, Height);

        LoadBits();
    }

    #region Underlaying Bitmap

    public Bitmap GetUnderlayingBitmap()
    {
        Bitmap bitmapReference;

        SaveBits();
        bitmapReference = this.bmp;
        LoadBits();

        return bitmapReference;
    }

    public void UpdateUnderlayingBitmap()
    {
        SaveBits();
        LoadBits();
    }

    public void LoadBits()
    {
        if (memoryLocked)
        {
            return;
        }

        // Lock the bitmap's bits.  
        Rectangle rect = new Rectangle(0, 0, bmp.Width, bmp.Height);
        bmpData = bmp.LockBits(validArea, System.Drawing.Imaging.ImageLockMode.ReadWrite, this.PixelFormat);
        stride = bmpData.Stride;

        // Get the address of the first line.
        scan0AsIntPtr = bmpData.Scan0;            
        scan0AsBytePointer = (byte*)scan0AsIntPtr;
        scan0AsIntPointer = (int*)scan0AsIntPtr;

        memoryLocked = true;
    }

    public void SaveBits()
    {
        if (!memoryLocked)
        {
            return;
        }

        bmp.UnlockBits(bmpData);

        memoryLocked = false;
    }

    #endregion

    #region Set Color

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    protected abstract void SetColorNoException(int x, int y);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    protected void SetColor(int x, int y)
    {
        if (x < 0 || x >= Width) throw new ArgumentOutOfRangeException("x");
        if (y < 0 || y >= Height) throw new ArgumentOutOfRangeException("y");

        SetColorNoException(x, y);
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public abstract void SetColor(int x, int y, Color c);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void SetColor(Point pt, Color c)
    {
        SetColor(pt.X, pt.Y, c);
    }

    public void SetColor(IEnumerable<Point> points, Color c)
    {
        // In case the mofo decides to not inline the calls.
        currentColor = c;

        foreach (var pt in points)
        {
            SetColor(pt.X, pt.Y);
        }
    }

    public void SetColor(Rectangle rect, Color c)
    {
        if (!rect.IntersectsWith(validArea)) throw new ArgumentOutOfRangeException("rect", "The rectangle is outside the image.");

        rect.Intersect(validArea);
        currentColor = c;
        for (int i = rect.X; i < rect.X + rect.Width; i++)
        {
            for (int j = rect.Y; j < rect.Y + rect.Height; j++)
            {
                SetColorNoException(i, j);
            }
        }
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    protected bool TrySetColor(int x, int y)
    {
        if (x < 0 || x >= Width || y < 0 || y >= Height) return false;

        SetColorNoException(x, y);

        return true;
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public bool TrySetColor(int x, int y, Color c)
    {
        if (x < 0 || x >= Width || y < 0 || y >= Height) return false;

        SetColor(x, y, c);

        return true;
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public bool TrySetColor(Point pt, Color c)
    {
        return TrySetColor(pt.X, pt.Y, c);
    }

    public bool TrySetColor(IEnumerable<Point> points, Color c)
    {
        bool sucessForAll = true;

        currentColor = c;
        foreach (var pt in points)
        {
            sucessForAll = sucessForAll && TrySetColor(pt.X, pt.Y);
        }

        return sucessForAll;
    }

    #endregion

    #region Get Color

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    protected abstract Color GetColorNoException(int x, int y);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public Color GetColor(int x, int y)
    {
        if (x < 0 || x >= Width) throw new ArgumentOutOfRangeException("x");
        if (y < 0 || y >= Height) throw new ArgumentOutOfRangeException("y");

        return GetColorNoException(x, y);
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public Color GetColor(Point pt)
    {
        return GetColor(pt.X, pt.Y);
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public bool TryGetColor(int x, int y, ref Color c)
    {
        if (x < 0 || x >= Width || y < 0 || y >= Height) return false;

        c = GetColorNoException(x, y);

        return true;
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public bool TryGetColor(Point pt, ref Color c)
    {
        return (TryGetColor(pt.X, pt.Y, ref c));
    }

    /// <summary>
    /// Returns the colors at each point of the given sequence of points
    /// If a point is outside the image, it is simply discarded
    /// </summary>
    public IEnumerable<Color> GetColorsFromArea(IEnumerable<Point> points)
    {
        foreach (var point in points)
        {
            if (point.X >= 0 && point.X < Width && point.Y >= 0 && point.Y < Height)
            {
                yield return GetColorNoException(point.X, point.Y);
            }
        }
    }

    /// <summary>
    /// Returns the colors at each point of the rectangle
    /// If an area of the rectangle is outside the image, those points are discarded
    /// </summary>
    public IEnumerable<Color> GetColorsFromArea(Rectangle rect)
    {
        rect.Intersect(validArea);

        // if false, will return an empty collection
        if (rect.Width > 0 && rect.Height > 0)
        {
            for (int i = rect.X; i < rect.X + rect.Width; i++)
            {
                for (int j = rect.Y; j < rect.Y + rect.Height; j++)
                {
                    yield return GetColorNoException(i, j);
                }
            }
        }
    }

    #endregion

    #region Set Pixel

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void SetPixel(Pixel px)
    {
        SetColor(px.Location.X, px.Location.Y, px.Color);
    }

    public void SetPixels(IEnumerable<Pixel> pixels)
    {
        foreach (var px in pixels)
        {
            SetColor(px.Location.X, px.Location.Y, px.Color);
        }
    }


    public bool TrySetPixel(Pixel px)
    {
        return TrySetColor(px.Location.X, px.Location.Y, px.Color);
    }

    public bool TrySetPixels(IEnumerable<Pixel> pixels)
    {
        bool sucessForAll = true;

        foreach (var px in pixels)
        {
            sucessForAll = sucessForAll && TrySetColor(px.Location.X, px.Location.Y, px.Color);
        }

        return sucessForAll;
    }

    #endregion

    #region Get Pixel

    /// <summary>
    /// Checks if a point is in the image. If it is, yield return it with its color.
    /// </summary>
    /// <param name="points">Collection of points to be checked.</param>
    /// <returns>Collection of Pixels</returns>
    public IEnumerable<Pixel> GetPixels(IEnumerable<Point> points)
    {
        foreach (var pt in points)
        {
            if (pt.X >= 0 && pt.X < Width && pt.Y >= 0 && pt.Y < Height)
            {
                Color c = GetColorNoException(pt.X, pt.Y);
                yield return new Pixel(pt, c);
            }
        }
    }


    public IEnumerable<Pixel> GetPixels(Rectangle rect)
    {
        rect.Intersect(validArea);

        if (rect.Width > 0 && rect.Height > 0)
        {
            for (int i = rect.X; i < rect.X + rect.Width; i++)
            {
                for (int j = rect.Y; j < rect.Y + rect.Height; j++)
                {
                    Color c = GetColorNoException(i, j);
                    yield return new Pixel(i, j, c);
                }
            }
        }
    }

    #endregion

    #region IMatrix

    /// <summary>
    /// Gets the color in the 'pt' coordinates.
    /// </summary>
    /// <param name="pt">The coordinates of the color.</param>
    /// <returns>The color in the coordinates 'pt'.</returns>
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public Color GetValue(Point pt)
    {
        return GetColor(pt.X, pt.Y);
    }

    /// <summary>
    /// Gets the color in the (x,y) coordinates.
    /// </summary>
    /// <param name="x">The horizontal coordinate.</param>
    /// <param name="y">The vertical coordinate.</param>
    /// <returns>The color in the coordinates (x,y).</returns>
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public Color GetValue(int x, int y)
    {
        return GetColor(x, y);
    }

    /// <summary>
    /// Sets the color in the 'pt' coordinates to 'value'.
    /// </summary>
    /// <param name="pt">The coordinates of the color.</param>
    /// <param name="value">The new color.</param>
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void SetValue(Point pt, Color value)
    {
        SetColor(pt.X, pt.Y, value);
    }

    /// <summary>
    /// Sets the color in the (x,y) coordinates to 'value'.
    /// </summary>
    /// <param name="x">The horizontal coordinate.</param>
    /// <param name="y">The vertical coordinate.</param>
    /// <param name="value">The new color.</param>
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void SetValue(int x, int y, Color value)
    {
        SetColor(x, y, value);
    }

    public IEnumerable<Color> RowMajor()
    {
        for (int i = 0; i < this.Height; i++)
        {
            for (int j = 0; j < this.Width; j++)
            {
                yield return this.GetValue(j, i);
            }
        }
    }

    public IEnumerable<Color> ColumnMajor()
    {
        for (int i = 0; i < this.Width; i++)
        {
            for (int j = 0; j < this.Height; j++)
            {
                yield return this.GetValue(i, j);
            }
        }
    }

    #endregion

    #region IDisposable

    bool _disposed = false;

    // Public implementation of Dispose pattern callable by consumers. 
    /// <summary>
    /// Guarantees that the underlaying bitmap's bits are locked.
    /// Releases the resources used by this class.
    /// </summary>
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected void Dispose(bool disposing)
    {
        if (_disposed)
            return;

        if (disposing)
        {
            if (bmp != null)
            {
                SaveBits();
            }
        }

        // Free any unmanaged objects here. 
        // bytes = null;
        bmpData = null;
        bmp = null;

        _disposed = true;
    }

    #endregion

    #region File IO

    public void SaveToFile(string path)
    {
        UpdateUnderlayingBitmap();
        bmp.Save(path);
    }

    #endregion
}

The concrete implementation:

public unsafe sealed class FastBitmap32 : FastBitmap
    {
        public FastBitmap32(Bitmap bmp)
            : base(bmp, PixelFormat.Format32bppArgb)
        { }

        protected override void SetColorNoException(int x, int y)
        {
            index = (y * stride) + (x * 4);

            scan0AsIntPointer[index/4] = currentColor.ToArgb();
        }

        public override void SetColor(int x, int y, System.Drawing.Color c)
        {
            if (x < 0 || x >= Width) throw new ArgumentOutOfRangeException("x");
            if (y < 0 || y >= Height) throw new ArgumentOutOfRangeException("y");

            index = (y * stride) + (x * 4);
            scan0AsIntPointer[index/4] = c.ToArgb();
        }

        protected override System.Drawing.Color GetColorNoException(int x, int y)
        {
            index = (y * stride) + (x * 4);           

            return Color.FromArgb(scan0AsIntPointer[index/4]);
        }
    }

Here is a Pixel (basically a Point + a Color):

/// <summary>
    /// Represents a pixel in an image
    /// </summary>
    public sealed class Pixel
    {
        // constant value
        private static Color DefaultColor = Color.Black;

        /// <summary>
        /// Gets the position of the pixel in the image
        /// </summary>
        public Point Location { get; private set; }

        /// <summary>
        /// Gets or sets the color of the pixel
        /// </summary>
        public Color Color { get; set; }

        /// <summary>
        /// Instantiates a new pixel in the given location, with a default color
        /// </summary>
        /// <param name="position">Where in the image the pixel is in</param>
        public Pixel(Point position)
            : this(position, DefaultColor) { }

        /// <summary>
        /// Instantiates a new pixel of the given color, in the given location
        /// </summary>
        /// <param name="position">Where in the image the pixel is in</param>
        /// <param name="color">The starting color of the pixel</param>
        public Pixel(Point position, Color color)
        {
            this.Location = position;
            this.Color = color;
        }

        public Pixel(int x, int y, Color c)
            : this(new Point(x, y), c) { }

    }

Last but not least, the IMatrix interface:

public interface IMatrix<T>
    {
        int Width { get; }
        int Height { get; }

        void SetValue(int x, int y, T value);
        void SetValue(Point pt, T value);

        T GetValue(int x, int y);
        T GetValue(Point pt);

        IEnumerable<T> RowMajor();
        IEnumerable<T> ColumnMajor();
    }

Answer 1

Pixel class

• public Point Location { get; private set; } and this.Location = position; doesn't match that good. Why don't you rename either the passed in parameter to location or the property to Position?

• you have a overloaded constructor taking only a Point which calls a constructor with a default color but you only have a constructor which takes (int x, int y, Color c) and none with a no color. IMHO you should provide this also.

---

abstract class FastBitmap

/// <summary>
/// Used with a scan0 pointer to get/set colors.
/// </summary>
protected int index = 0;  

this variable is nowhere used in the abstract class, so it doesn't belong there. You should move it to the implementation and IMHO you won't need this at class level either. Make it a method scoped variable.

---

This

Rectangle rect = new Rectangle(0, 0, bmp.Width, bmp.Height);

in the LoadBits() method isn't used at all and should be removed.

---

unsafe sealed class FastBitmap32

Here you have the magic number 4 in your code. You should use a const for that with a meaningful name. But you also could do better by removing the class at all and change the FastBitmap class to a non abstract one.

You can easiliy (with a simple switch) determine how many bytes is consumed by a color for a passed in PixelFormat. That would completely remove the need to have an abstract class.

Instead of checking the PixelFormat and for every GetColor(...)/SetColor(...) and deciding if I should read 3 or 4 bytes, I just use a class that knows exactly how to read/write a color. Think about it. For a 1000x1000 image I'm saving 1000000 checks.

How about

    public FastBitmap(Bitmap bmp)
        : this(bmp, bmp.PixelFormat)
    { }

    private readonly int bytesPerPixel;
    public FastBitmap(Bitmap bmp, PixelFormat pixelFormat)
    {
        if (bmp.PixelFormat != pixelFormat) throw new FormatException("The bitmap has a invalid PixelFormat. Expected " + pixelFormat.ToString() + ".");

        bytesPerPixel = GetBytesPerPixel(pixelFormat);
        this.bmp = bmp;
        this.Width = bmp.Width;
        this.Height = bmp.Height;
        this.PixelFormat = bmp.PixelFormat;

        this.validArea = new Rectangle(0, 0, Width, Height);

        LoadBits();
    }

    protected FastBitmap(Bitmap bmp, int bytesPerPixel)
    {
        this.bytesPerPixel = bytesPerPixel;
        this.bmp = bmp;
        this.Width = bmp.Width;
        this.Height = bmp.Height;
        this.PixelFormat = bmp.PixelFormat;

        this.validArea = new Rectangle(0, 0, Width, Height);

        LoadBits();
    }

    private int GetBytesPerPixel(PixelFormat pixelFormat)
    {
        switch (pixelFormat)
        {
            case System.Drawing.Imaging.PixelFormat.Format24bppRgb:
                return 3;
            case System.Drawing.Imaging.PixelFormat.Format32bppArgb:
                return 4;
            case System.Drawing.Imaging.PixelFormat.Format16bppArgb1555:
                return 2;
            case System.Drawing.Imaging.PixelFormat.Format48bppRgb:
                return 6;
        }
        throw new NotSupportedException("The pixelformat " + pixelFormat.ToString() + " is not supported");
    }

    private void SetColorNoException(int x, int y)
    {
        index = (y * stride) + (x * bytesPerPixel);
        scan0AsIntPointer[index / bytesPerPixel] = currentColor.ToArgb();
    }

    public void SetColor(int x, int y, System.Drawing.Color c)
    {
        if (x < 0 || x >= Width) throw new ArgumentOutOfRangeException("x");
        if (y < 0 || y >= Height) throw new ArgumentOutOfRangeException("y");

        index = (y * stride) + (x * bytesPerPixel);
        scan0AsIntPointer[index / bytesPerPixel] = c.ToArgb();
    }

    private System.Drawing.Color GetColorNoException(int x, int y)
    {
        index = (y * stride) + (x * bytesPerPixel);
        return Color.FromArgb(scan0AsIntPointer[index / bytesPerPixel]);
    }  

you only have one check in the constructor and it can be used with all the pixelformats you define.

Lets assume you want to add support for Format48bppRgb you still can do it by either changing the switch in the GetBytesPerPixel()method or by extending the class and call the protected constructor with 6 as bytesPerPixel parameter.

---

Regions

Please read are-regions-an-antipattern-or-code-smell

Is there a good use for regions?

No. There was a legacy use: generated code. Still, code generation tools just have to use partial classes instead. If C# has regions support, it's mostly because this legacy use, and because now that too many people used regions in their code, it would be impossible to remove them without breaking existent codebases.

Think about it as about goto. The fact that the language or the IDE supports a feature doesn't mean that it should be used daily. StyleCop SA1124 rule is clear: you should not use regions. Never.