HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Imaging with getPixel() and loops

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
imagingwithloopsgetpixeland

Problem

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  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)); }

Solution

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)

Code Snippets

int threshold = 7;
Bitmap bitmap0 = (Bitmap)imag.Clone();
var threshold = 7;
var bitmap0 = (Bitmap)imag.Clone();
for (row = 0; row < rows; row++)
{
    for (col = 0; col < cols; col++)
    {
        //Code...
    }
}
if (count >= threshold)
    outmap.SetPixel(col, row, Color.FromArgb(255, 255, 255));
else
    outmap.SetPixel(col, row, Color.FromArgb(0, 0, 0));
var colorBit = count >= treshold ? 255 : 0;
outmap.SetPixel(col, row, Color.FromArgb(colorBit, colorBit, colorBit));

Context

StackExchange Code Review Q#86393, answer score: 13

Revisions (0)

No revisions yet.