Parallelizing bitmap methods

Question

I created a class called QuickBitmap to quickly get/set pixels in a Bitmap object. Using intertops I'm able to safely lock/unlock the bits of the Bitmap and with the use of methods like SaveBits, LoadBits, UpdateUnderlayingBitmap, GetPixel and SetPixel. I can quickly and easily manipulate the image.

The problem:

One of my methods changes the bitdepth to 32 bits. If I use it I'm no longer able to quickly determine if the image I'm manipulating was, originally, composed of only black and white colors.

I created the following methods do it:

private IEnumerable<Color> AllPixels()
{
    for (int i = 0; i < Width;i++ )
    {
        for(int j=0; j<Height; j++)
        {
            yield return GetPixel(i, j);
        }
    }
}

public bool IsBlackAndWhiteParallel()
{
    return AllPixels().AsParallel().All(x => x.IsBlackOrWhite());
}

But as you can guess, it's quite a slow method. So I googled a little and found out that I can break a parallel.for loop. So I rewrote my method do this:

public bool IsBlackAndWhiteParallel()
{
    bool ret = true;
    Parallel.For(0, Width, (i, loopstate) =>
    {
        for(int j=0; j<Height; j++)
        {
            if(!GetPixel(i,j).IsBlackOrWhite())
            {
                ret = false;
                loopstate.Stop();
            }
        }
    });

    return ret;
}

I benchmarked the code using the benchmark class (I found it in an old question on SO) with 100 iterations:

public static class Benchmarker
{
    public static void Profile(string description, int iterations, Action func)
    {
        // warm up 
        func();

        Stopwatch watch = new Stopwatch();

        // clean up
        GC.Collect();
        GC.WaitForPendingFinalizers();
        GC.Collect();

        watch.Start();
        for (int i = 0; i < iterations; i++)
        {
            func();
        }
        watch.Stop();
        Console.Write(description);
        Console.WriteLine(" average time: {0} ms", watch.Elapsed.TotalMilliseconds/iterations);
    }
}

Results:

• First code: 1170 ms
• Second code: 99 ms

This is how the getpixe/setpixel methods are implemented for 32 bits image:

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public Color GetPixel(int x, int y)
    {
        int pos;
        int r, g, b, a;

        pos = (y * stride) + (x * 4);

        b = bytes[pos];
        g = bytes[pos + 1];
        r = bytes[pos + 2];
        a = bytes[pos + 3];

        return Color.FromArgb(a, r, g, b);
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void SetPixel(int x, int y, Color c)
    {
        int pos;

        pos = (y * stride) + (x * 4);

        bytes[pos] = c.B;
        bytes[pos + 1] = c.G;
        bytes[pos + 2] = c.R;
        bytes[pos + 3] = c.A;
    }

I'm satisfied with the result, but I'd like to know if you have any suggestions for improvements.

Answer 1

By using BitConverter.ToInt32 you can simplify the GetPixel() method to

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public Color GetPixel(int x, int y)
{
    return Color.FromArgb(BitConverter.ToInt32(bytes, y * stride + x * 4));
}

Also there is no need to place () arround the multiplications because they will be done first.

---

Because GetPixel and SetPixel are public you should add some validation to them. This can be done by adding a private method GetPosition() like

private int GetPosition(int x, int y)
{
    if (x > stride) { throw new ArgumentOutOfRangeException("x"); }

    int position = y * stride + x * 4;

    if (position >= bytes.Length) { throw new ArgumentOutOfRangeException("y"); }
    return position;
}

now the changed GetPixel() method looks like

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public Color GetPixel(int x, int y)
{
    return Color.FromArgb(BitConverter.ToInt32(bytes, GetPosition(x, y)));
}

---

Basically you don't need the Color to determine if a pixel is black or white. You can just check if the argb value of that pixel matches the values for Color.Black or Color.White like

private const int black = -16777216;
private const int white = -1;

public bool IsBlackAndWhite(int x, int y)
{
    int argb = BitConverter.ToInt32(GetPosition(x, y));
    return argb == white || argb == black;
}

---

You should let your variables some space to breathe.

for(int j=0; j<Height; j++)  

should be

for(int j = 0; j < Height; j++)  

Your IDE will usually have a keyboard shortcut for formatting the code.

---

Declaring multiple variables in one line makes your code less readable.

Answer 2

Var

Use the var keyword when defining local variables where the right hand side of the definition makes the type obvious. This looks cleaner and saves time when it comes to changing types during refactoring.

e.g.

bool ret = true;

should be

var ret = true;

You should also use var when defining loop variables.

e.g.

for (int i = 0; i < iterations; i++)

should be

for (var i = 0; i < iterations; i++)

Naming

Avoid using shortened names. The extra characters don't cost anything and can be immensely helpful for a maintenance programmer (or yourself in a few months).

e.g.

bool ret = true;

would be better off named something like result. The same goes for r,g,b and a, although you would be forgiven for leaving them contracted, since they are commonly-used names.

Design

You don't have to declare your variables up at the top. In fact, it's often clearer if you declare your variables as close to where you use them as possible.

e.g.

int pos;
int r, g, b, a;

pos = (y * stride) + (x * 4);

b = bytes[pos];
g = bytes[pos + 1];
r = bytes[pos + 2];
a = bytes[pos + 3];

should be

int pos = (y * stride) + (x * 4);

int b = bytes[pos];
int g = bytes[pos + 1];
int r = bytes[pos + 2];
int a = bytes[pos + 3];

Additionally, I question your use of int here, when bytes should clearly be containing byte. Why the cast when storing it in a variable?