System.Drawing.Bitmap Wrapper Class

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:

• I don't repeat myself creating the overloads

• I'm able to create specialized classes to deal with specifi

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

/// 
/// Used with a scan0 pointer to get/set colors.
/// 
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 = Width) throw new ArgumentOutOfRangeException("x");
        if (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.

StackExchange Code Review Q#93058, answer score: 6