patterncsharpModerate
Imaging with getPixel() and loops
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
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.
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:
would become:
Variable names:
Names like
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:
Other:
can be rewritten to:
Following code contains too much braces in my opinion:
Rewrite it as follows:
or a different approach:
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
Seems that an important part of this approach is the
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.