Adding filters to images

Question

Here is some code I wrote as part of a project that I am working on to learn C#. It is a class responsible for applying filters to images. In this case, my code offers filters such as sepia and grayscale (sorry, no Valencia for you Instagram users!). Any input on how it can be improved is appreciated.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Drawing;
using System.IO;
using System.Drawing.Imaging;
using System.Runtime.InteropServices;

namespace ImageFilter
{
public static class ExtBitmap
{
    private static Bitmap GetArgbCopy(Image sourceImage)
    {
        Bitmap bmpNew = new Bitmap(sourceImage.Width, sourceImage.Height, PixelFormat.Format32bppArgb);

        using (Graphics graphics = Graphics.FromImage(bmpNew))
        {
            graphics.DrawImage(sourceImage, new Rectangle(0, 0, bmpNew.Width, bmpNew.Height), new Rectangle(0, 0, bmpNew.Width, bmpNew.Height), GraphicsUnit.Pixel);
            graphics.Flush();
        }

        return bmpNew;
    }

    private static Bitmap ApplyColorMatrix(Image sourceImage, ColorMatrix colorMatrix)
    {
        Bitmap bmp32BppSource = GetArgbCopy(sourceImage);
        Bitmap bmp32BppDest = new Bitmap(bmp32BppSource.Width, bmp32BppSource.Height, PixelFormat.Format32bppArgb);

        using (Graphics graphics = Graphics.FromImage(bmp32BppDest))
        {
            ImageAttributes bmpAttributes = new ImageAttributes();
            bmpAttributes.SetColorMatrix(colorMatrix);

            graphics.DrawImage(bmp32BppSource, new Rectangle(0, 0, bmp32BppSource.Width, bmp32BppSource.Height),
                             0, 0, bmp32BppSource.Width, bmp32BppSource.Height, GraphicsUnit.Pixel, bmpAttributes);

        }

        bmp32BppSource.Dispose();

        return bmp32BppDest;
    }

    public static Bitmap CopyWithTransparency(this Image sourceImage, byte alphaComponent = 100)
    {
        Bitmap bmpNew = GetArgbCopy(sourceImage);
        BitmapData bmpData = bmpNew.LockBits(new Rectangle(0, 0, sourceImage.Width, sourceImage.Height), ImageLockMode.ReadOnly, PixelFormat.Format32bppArgb);

        IntPtr ptr = bmpData.Scan0;

        byte[] byteBuffer = new byte[bmpData.Stride * bmpNew.Height];

        Marshal.Copy(ptr, byteBuffer, 0, byteBuffer.Length);

        for (int k = 3; k < byteBuffer.Length; k += 4)
        {
            byteBuffer[k] = alphaComponent;
        }

        Marshal.Copy(byteBuffer, 0, ptr, byteBuffer.Length);

        bmpNew.UnlockBits(bmpData);

        bmpData = null;
        byteBuffer = null;

        return bmpNew;
    }

    public static Bitmap DrawWithTransparency(this Image sourceImage)
    {
        ColorMatrix colorMatrix = new ColorMatrix(new float[][] 
                                            {
                                                new float[] {1, 0, 0, 0, 0},
                                                new float[] {0, 1, 0, 0, 0},
                                                new float[] {0, 0, 1, 0, 0},
                                                new float[] {0, 0, 0, 0.3f, 0},
                                                new float[] {0, 0, 0, 0, 1}
                                            });

        return ApplyColorMatrix(sourceImage, colorMatrix);
    }


    public static Bitmap CopyAsNegative(this Image sourceImage)
    {
        Bitmap bmpNew = GetArgbCopy(sourceImage);
        BitmapData bmpData = bmpNew.LockBits(new Rectangle(0, 0, sourceImage.Width, sourceImage.Height), ImageLockMode.ReadOnly, PixelFormat.Format32bppArgb);

        IntPtr ptr = bmpData.Scan0;

        byte[] byteBuffer = new byte[bmpData.Stride * bmpNew.Height];

        Marshal.Copy(ptr, byteBuffer, 0, byteBuffer.Length);
        byte[] pixelBuffer = null;

        int pixel = 0;

        for (int k = 0; k < byteBuffer.Length; k += 4)
        {
            pixel = ~BitConverter.ToInt32(byteBuffer, k);
            pixelBuffer = BitConverter.GetBytes(pixel);

            byteBuffer[k] = pixelBuffer[0];
            byteBuffer[k + 1] = pixelBuffer[1];
            byteBuffer[k + 2] = pixelBuffer[2];
        }

        Marshal.Copy(byteBuffer, 0, ptr, byteBuffer.Length);

        bmpNew.UnlockBits(bmpData);

        bmpData = null;
        byteBuffer = null;

        return bmpNew;
    }

    public static Bitmap DrawAsNegative(this Image sourceImage)
    {
        ColorMatrix colorMatrix = new ColorMatrix(new float[][] 
                                            {
                                                new float[] {-1, 0, 0, 0, 0},
                                                new float[] {0, -1, 0, 0, 0},
                                                new float[] {0, 0, -1, 0, 0},
                                                new float[] {0, 0, 0, 1, 0},
                                                new float[] {1, 1, 1, 1, 1}
                                            });

        return ApplyColorMatrix(sourceImage, colorMatrix);
    }

    public static Bitmap CopyAsGrayscale(this Image sourceImage)
    {
        Bitmap bmpNew = GetArgbCopy(sourceImage);
        BitmapData bmpData = bmpNew.LockBits(new Rectangle(0, 0, sourceImage.Width, sourceImage.Height), ImageLockMode.ReadOnly, PixelFormat.Format32bppArgb);

        IntPtr ptr = bmpData.Scan0;

        byte[] byteBuffer = new byte[bmpData.Stride * bmpNew.Height];

        System.Runtime.InteropServices.Marshal.Copy(ptr, byteBuffer, 0, byteBuffer.Length);

        float rgb = 0;

        for (int k = 0; k < byteBuffer.Length; k += 4)
        {
            rgb = byteBuffer[k] * 0.11f;
            rgb += byteBuffer[k+1] * 0.59f;
            rgb += byteBuffer[k+2] * 0.3f;

            byteBuffer[k] = (byte)rgb;
            byteBuffer[k + 1] = byteBuffer[k];
            byteBuffer[k + 2] = byteBuffer[k];

            byteBuffer[k + 3] = 255;
        }

        Marshal.Copy(byteBuffer, 0, ptr, byteBuffer.Length);

        bmpNew.UnlockBits(bmpData);

        bmpData = null;
        byteBuffer = null;

        return bmpNew;
    }

    public static Bitmap DrawAsGrayscale(this Image sourceImage)
    {
        ColorMatrix colorMatrix = new ColorMatrix(new float[][] 
                                            {
                                                new float[] {.3f, .3f, .3f, 0, 0},
                                                new float[] {.59f, .59f, .59f, 0, 0},
                                                new float[] {.11f, .11f, .11f, 0, 0},
                                                new float[] {0, 0, 0, 1, 0},
                                                new float[] {0, 0, 0, 0, 1}
                                            });

        return ApplyColorMatrix(sourceImage, colorMatrix);
    }

    public static Bitmap CopyAsSepiaTone(this Image sourceImage)
    {
        Bitmap bmpNew = GetArgbCopy(sourceImage);
        BitmapData bmpData = bmpNew.LockBits(new Rectangle(0, 0, sourceImage.Width, sourceImage.Height), ImageLockMode.ReadOnly, PixelFormat.Format32bppArgb);

        IntPtr ptr = bmpData.Scan0;

        byte[] byteBuffer = new byte[bmpData.Stride * bmpNew.Height];

        Marshal.Copy(ptr, byteBuffer, 0, byteBuffer.Length);

        byte maxValue = 255;
        float r = 0;
        float g = 0;
        float b = 0;

        for (int k = 0; k < byteBuffer.Length; k += 4)
        {
            r = byteBuffer[k] * 0.189f + byteBuffer[k + 1] * 0.769f + byteBuffer[k + 2] * 0.393f;
            g = byteBuffer[k] * 0.168f + byteBuffer[k + 1] * 0.686f + byteBuffer[k + 2] * 0.349f;
            b = byteBuffer[k] * 0.131f + byteBuffer[k + 1] * 0.534f + byteBuffer[k + 2] * 0.272f;

            byteBuffer[k+2] = (r > maxValue ? maxValue : (byte)r);
            byteBuffer[k + 1] = (g > maxValue ? maxValue : (byte)g);
            byteBuffer[k] = (b > maxValue ? maxValue : (byte)b);
        }

        Marshal.Copy(byteBuffer, 0, ptr, byteBuffer.Length);

        bmpNew.UnlockBits(bmpData);

        bmpData = null;
        byteBuffer = null;

        return bmpNew;
    }

    public static Bitmap DrawAsSepiaTone(this Image sourceImage)
    {
        ColorMatrix colorMatrix = new ColorMatrix(new float[][] 
                                            {
                                                new float[] {.393f, .349f, .272f, 0, 0},
                                                new float[] {.769f, .686f, .534f, 0, 0},
                                                new float[] {.189f, .168f, .131f, 0, 0},
                                                new float[] {0, 0, 0, 1, 0},
                                                new float[] {0, 0, 0, 0, 1}
                                            });

        return ApplyColorMatrix(sourceImage, colorMatrix);
    }
}
}

Answer 1

In the world of Graphics and Bitmaps you can assume that virtually everything's disposable so you should also try to dispose everything you're working with.

One of such examples would be the ImageAttributes that you use in the ApplyColorMatrix and you don't dispose.

You're also not consistent with the using statement. Once you use it and the other time not like here:

private static Bitmap ApplyColorMatrix(Image sourceImage, ColorMatrix colorMatrix)
{
    Bitmap bmp32BppSource = GetArgbCopy(sourceImage);
    Bitmap bmp32BppDest = new Bitmap(bmp32BppSource.Width, bmp32BppSource.Height, PixelFormat.Format32bppArgb);

    using (Graphics graphics = Graphics.FromImage(bmp32BppDest))
    {
        ImageAttributes bmpAttributes = new ImageAttributes();
        bmpAttributes.SetColorMatrix(colorMatrix);

        graphics.DrawImage(bmp32BppSource, new Rectangle(0, 0, bmp32BppSource.Width, bmp32BppSource.Height),
                         0, 0, bmp32BppSource.Width, bmp32BppSource.Height, GraphicsUnit.Pixel, bmpAttributes);

    }

    bmp32BppSource.Dispose();

    return bmp32BppDest;
}

Ideally it should look like this:

private static Bitmap ApplyColorMatrix(Image sourceImage, ColorMatrix colorMatrix)
{
    using (Bitmap bmp32BppSource = GetArgbCopy(sourceImage))
    {
        Bitmap bmp32BppDest = new Bitmap(bmp32BppSource.Width, bmp32BppSource.Height, PixelFormat.Format32bppArgb);

        using (Graphics graphics = Graphics.FromImage(bmp32BppDest))
        using(ImageAttributes bmpAttributes = new ImageAttributes())
        {
            bmpAttributes.SetColorMatrix(colorMatrix);
            graphics.DrawImage(
                image: bmp32BppSource, 
                destRect: new Rectangle(0, 0, bmp32BppSource.Width, bmp32BppSource.Height),
                srcX: 0, 
                srcY: 0, 
                srcWidth: bmp32BppSource.Width, 
                srcHeight: bmp32BppSource.Height,
                srcUnit: GraphicsUnit.Pixel,
                imageAttrs: bmpAttributes
            );

            return bmp32BppDest;
        }
    }        
}

With such a lengthy parameter list you can use their names and write them in separate lines to improve readability.

---

In other places like where you use LockBits there is a danger that if an exception is thrown the bits won't get unclocked anymore because there is no try/finally block e.g. the CopyWithTransparency method should be implemented this way:

public static Bitmap CopyWithTransparency(this Image sourceImage, byte alphaComponent = 100)
{
    Bitmap bmpNew = GetArgbCopy(sourceImage);
    BitmapData bmpData = null;

    try
    {
        bmpData = bmpNew.LockBits(new Rectangle(0, 0, sourceImage.Width, sourceImage.Height), ImageLockMode.ReadOnly, PixelFormat.Format32bppArgb);
        IntPtr ptr = bmpData.Scan0;

        byte[] byteBuffer = new byte[bmpData.Stride * bmpNew.Height];

        Marshal.Copy(ptr, byteBuffer, 0, byteBuffer.Length);

        for (int k = 3; k < byteBuffer.Length; k += 4)
        {
            byteBuffer[k] = alphaComponent;
        }

        Marshal.Copy(byteBuffer, 0, ptr, byteBuffer.Length);

        return bmpNew;
    }
    finally
    {
        if (bmpData != null) bmpNew.UnlockBits(bmpData);
    }
}

The other APIs are very similar so the above suggestions apply to all of them.

---

for (int k = 3; k < byteBuffer.Length; k += 4)
{
    byteBuffer[k] = alphaComponent;
}

Here I don't like the magic 3 and 4 and the variable k. We usually start with i unless the variable has a special meaning. In this case you could even use an a that would be much better (because it stands for alpha) then a k which doesn't mean anything and with two constants the code is much easier to understand:

const int firstAlpha = 3;
const int colorComponentCount = 4;
for (int a = firstAlpha; a < byteBuffer.Length; a += colorComponentCount)
{
    byteBuffer[a] = alphaComponent;
}

The same applies to other loops like here:

for (int k = 0; k < byteBuffer.Length; k += 4)
{
    pixel = ~BitConverter.ToInt32(byteBuffer, k);
    pixelBuffer = BitConverter.GetBytes(pixel);

    byteBuffer[k] = pixelBuffer[0];
    byteBuffer[k + 1] = pixelBuffer[1];
    byteBuffer[k + 2] = pixelBuffer[2];
}

Why + 1 or + 2? Would you know it it two months? Use constnats like greenOffset or blueOffset etc. or create a helper class to store the offsets:

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

that you can use with the arrays:

const int colorCount = 4;
for (int i = 0; i < byteBuffer.Length; i += colorCount)
{
    pixel = ~BitConverter.ToInt32(byteBuffer, i);
    pixelBuffer = BitConverter.GetBytes(pixel);

    byteBuffer[i + ColorOffset.Red] = pixelBuffer[ColorOffset.Red];
    byteBuffer[i + ColorOffset.Green] = pixelBuffer[ColorOffset.Green];
    byteBuffer[i + ColorOffset.Blue] = pixelBuffer[ColorOffset.Blue];
}

Now you or anyone else will swill know in few weeks what's going on here. There are more magic numbers here and there but I think you'll now know how to deal with them.