Imaging with getPixel() and loops

Question

I wrote perfectly working code that reads an image, and then does a kind of threshold on it by zone. If the zone has too much white then it turns all the zone into white, otherwise it turns into black.

The problem is that the code was originally from a C++ source with pointers and all instead of GetPixel() etc. Speed was just instant, nothing more. Now that I rewrote "safely" (no pointers thingy), it take like 20 seconds per images, so instant versus 20 seconds = insane difference.

Could any of you help me improve that small code, even if you use back unsafe pointers code etc? I don't really care about that. The part I absolutely need is speed, and it has to be quick as possible because I will deal with huge images library. Instead of taking 30 minutes, it will literally takes days.

    private Bitmap doti(Bitmap imag)
    {
        int threshold = 7;
        int distance = 9;
        int rows, cols, row, col, x, y, count;
        int dhalf = (distance / 2) + 1;
        int Sqdistance = SQ(distance);
        rows = imag.Height;
        cols = imag.Width;
        Bitmap bitmap0 = (Bitmap)imag.Clone();
        Bitmap bitmap = new Bitmap(cols, rows);
        Bitmap outmap = new Bitmap(cols, rows);

        //convert to grayscale of a single byte
        for (row = 0; row < rows; row++)
            for (col = 0; col < cols; col++)
            {
                Color pixel = bitmap0.GetPixel(col, row);
                byte grayish = (byte)Math.Floor((decimal)(pixel.R + pixel.G + pixel.B) / 3);
                bitmap.SetPixel(col, row, Color.FromArgb(grayish, grayish, grayish));
            }

        //check our threshold to set black or white by checking each pixels in a square defined by distance.
        for (row = 0; row < rows; row++)
            for (col = 0; col < cols; col++)
            {
                count = 0;
                //could optimize here heavily maybe, by only checking inside a circle rather than square+dist.
                for (x = Math.Max(col - dhalf, 0); x < Math.Min(col + dhalf, cols); x++)
                    for (y = Math.Max(row - dhalf, 0); y < Math.Min(row + dhalf, rows); y++)
                    {
                      //if inside the square and pixel color is not black count one more not black pixel
                        if ((Sqdistance > DISTANCE(col, row, x, y)) && ((bitmap.GetPixel(x, y).R) > 0)) //this second condition is killing a lot the speed to begin.
                            count++;
                    }

                //if too much count of not black pixel, set it white.
                if (count >= threshold)
                    outmap.SetPixel(col, row, Color.FromArgb(255, 255, 255));
                else
                    outmap.SetPixel(col, row, Color.FromArgb(0, 0, 0));

            }

        return outmap;
    }

SQ() and DISTANCE():

private int SQ(int a) { return ((a) * (a)); }
private int DISTANCE(int a, int b, int c, int d) { return (SQ(a - c) + SQ(b - d)); }

Answer 1

The ´var´ keyword:

From the C# Programming Guide:

The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

int threshold = 7;
Bitmap bitmap0 = (Bitmap)imag.Clone();

would become:

var threshold = 7;
var bitmap0 = (Bitmap)imag.Clone();

Variable names:

Names like bitmap, bitmap0, Sqdistance don't have a useful meaning, not for you, nor for others reading/reviewing your code. Use meaningful names for your variables, better for readability and maintainability.

Curly braces:

Although you're not writing "wrong" code by omitting the braces on the outer loops, it seriously decreases readability. Following is only 2 lines longer and is much easier to understand:

for (row = 0; row < rows; row++)
{
    for (col = 0; col < cols; col++)
    {
        //Code...
    }
}

Other:

if (count >= threshold)
    outmap.SetPixel(col, row, Color.FromArgb(255, 255, 255));
else
    outmap.SetPixel(col, row, Color.FromArgb(0, 0, 0));

can be rewritten to:

var colorBit = count >= treshold ? 255 : 0;
outmap.SetPixel(col, row, Color.FromArgb(colorBit, colorBit, colorBit));

Following code contains too much braces in my opinion:

private int SQ(int a) { return ((a) * (a)); }

Rewrite it as follows:

private int Square(int a)
{ 
    return a * a;
}

or a different approach:

private int Square(int a)
{ 
    return (int)Math.Pow(a, 2);
}

Also for those two last methods, don't make them upper case. Method names in .NET are Pascal case (Capitalization Conventions).

Performance:

Regarding the performance. I didn't take a deep dive in the code or test it, but is there a reason why you perform the loops twice? Isn't there a possibility to execute all the code inse the double for-loop?

I did a performance analysis on your method and it is indeed the GetPixel method that is slowing things done. I did a Google search and apparently a lot of people are dealing with this. Here's an interesting question on StackOverflow that has a possible solution: C# - Faster Alternatives to SetPixel and GetPixel for Bitmaps for Windows Forms App.

Seems that an important part of this approach is the LockBits() method call. More reading about it on MSDN: Bitmap.LockBits Method (Rectangle, ImageLockMode, PixelFormat)

Answer 2

byte grayish = (byte)Math.Floor((decimal)(pixel.R + pixel.G + pixel.B) / 3);

The decimal type is not a good option for this since it costs a lot of performance and the benefits are not needed at all. You should only use it if you have big comma numbers that need a lot of accuracy. Here you're only dealing with natural numbers, no commas at all.

byte grayish = (byte)(((int)pixel.R + pixel.G + pixel.B) / 3);

Math.Floor is not needed since int and byte division do not keep the remaining rest.

Answer 3

Abbas touched on this, but I don't think that he gave it the attention it truly

    for (row = 0; row < rows; row++)
        for (col = 0; col < cols; col++)
        {
            Color pixel = bitmap0.GetPixel(col, row);
            byte grayish = (byte)Math.Floor((decimal)(pixel.R + pixel.G + pixel.B) / 3);
            bitmap.SetPixel(col, row, Color.FromArgb(grayish, grayish, grayish));
        }

    //check our threshold to set black or white by checking each pixels in a square defined by distance.
    for (row = 0; row < rows; row++)
        for (col = 0; col < cols; col++)
        {
            count = 0;
            //could optimize here heavily maybe, by only checking inside a circle rather than square+dist.
            for (x = Math.Max(col - dhalf, 0); x < Math.Min(col + dhalf, cols); x++)
                for (y = Math.Max(row - dhalf, 0); y < Math.Min(row + dhalf, rows); y++)
                {
                  //if inside the square and pixel color is not black count one more not black pixel
                    if ((Sqdistance > DISTANCE(col, row, x, y)) && ((bitmap.GetPixel(x, y).R) > 0)) //this second condition is killing a lot the speed to begin.
                        count++;
                }

            //if too much count of not black pixel, set it white.
            if (count >= threshold)
                outmap.SetPixel(col, row, Color.FromArgb(255, 255, 255));
            else
                outmap.SetPixel(col, row, Color.FromArgb(0, 0, 0));

        }

Always use Curly Braces, people won't notice if you use them, and programmers will be expecting them when you don't use them, especially for for loops.

for (row = 0; row < rows; row++)
{
    for (col = 0; col < cols; col++)
    {
        Color pixel = bitmap0.GetPixel(col, row);
        byte grayish = (byte)Math.Floor((decimal)(pixel.R + pixel.G + pixel.B) / 3);
        bitmap.SetPixel(col, row, Color.FromArgb(grayish, grayish, grayish));
    }
}
//check our threshold to set black or white by checking each pixels in a square defined by distance.
for (row = 0; row < rows; row++)
{
    for (col = 0; col < cols; col++)
    {
        count = 0;
        //could optimize here heavily maybe, by only checking inside a circle rather than square+dist.
        for (x = Math.Max(col - dhalf, 0); x < Math.Min(col + dhalf, cols); x++)
        {
            for (y = Math.Max(row - dhalf, 0); y < Math.Min(row + dhalf, rows); y++)
            {
              //if inside the square and pixel color is not black count one more not black pixel
                if ((Sqdistance > DISTANCE(col, row, x, y)) && ((bitmap.GetPixel(x, y).R) > 0)) //this second condition is killing a lot the speed to begin.
                {
                    count++;
                }   
            }

        //if too much count of not black pixel, set it white.
        if (count >= threshold)
        {
            outmap.SetPixel(col, row, Color.FromArgb(255, 255, 255));
        }
        else
        {
            outmap.SetPixel(col, row, Color.FromArgb(0, 0, 0));
        }
    }
}

---

Curly Braces in For Loops and If/Then statements define the block of code so that the programmer (and subsequent programmers) will not have to think twice about what scope they are adding code (should they need to add code for some new functionality or to fix a logic bug).

If you always use Curly Braces then you know exactly what your scope is, which is especially important when nesting loops inside of each other.

It is highly recommended by the majority of Programmers (C#, Java, etc.) to always use Curly Braces.

Most exceptions to this rule are simple if statements that perform some simple single line of code when the simple condition is satisfied.

the condition in the if statement shown here is so long that simply adding the count++; at the end would prove less readable.

if ((Sqdistance > DISTANCE(col, row, x, y)) && ((bitmap.GetPixel(x, y).R) > 0)) //this second condition is killing a lot the speed to begin.
    count++;

I would agree that Curly Braces are not needed, if it were something more simple like:

if ( x = y ) count++; 

This is a shortcut to the recommended form of an if statement, which is

if ([condition])
{
     //code
}

As stated by MSDN

Both the then-statement and the else-statement can consist of a single statement or multiple statements that are enclosed in braces ({}). For a single statement, the braces are optional but recommended.

---

The same thing goes for For Loops, there is a standard and there are shortcuts to the standard.

MSDN States

The body of the loop consists of a statement, an empty statement, or a block of statements, which you create by enclosing zero or more statements in braces.

Answer 4

I can see one area that might be slowing it down substantially; though I will admit that it has been quite a while since I directly used C#. As a video and interactive media programmer, your repeated calls to getPixel() concern me, as under the hood they may be requesting resources, pulling out a single pixel, and eliminating the same resources every single time. Given the significant lag you're reporting, I would say that that is probable.

The usual response is to retrieve the entire image as a buffer, work with that, and then create the new image from the modified buffer. This is likely why C++ had so many pointers floating around in it. For languages like Java and C#, there are Buffer classes that provide the same native speeds; though as I said, I haven't worked with C# directly in forever and don't know the exact name of them in its standard library.