10
\$\begingroup\$

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)); }
\$\endgroup\$
  • \$\begingroup\$ I am guessing that there is a way to turn some of these for loops into foreach loops \$\endgroup\$ – Malachi Apr 9 '15 at 14:45
  • \$\begingroup\$ If you're open to a library solution, you might look into Emgu CV. It's a wrapper for OpenCV, and can give you close to C++ speeds. It can also simplify your code quite a bit. \$\endgroup\$ – Aurelius Apr 9 '15 at 23:54
  • 1
    \$\begingroup\$ GetPixel is super slow, you need to use LockBits it will speed up the whole thing to c++ standards \$\endgroup\$ – Vajura Apr 10 '15 at 5:28
  • \$\begingroup\$ For what it's worth, adding RGB and dividing by 3 doesn't give a very good greyscale, as the perceptual contributions of the 3 colours aren't even. Have a look at Converting RGB to grayscale/intensity for some options. \$\endgroup\$ – Dan Aucott Apr 10 '15 at 10:12
13
\$\begingroup\$

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)

\$\endgroup\$
  • \$\begingroup\$ Thank you for your infos. About the loops, the two first are to loop through all of the image, and the Two seconds are in order to (from each pixel of the image) loop through the pixels of a choosen zone (a square here defined by distance.). \$\endgroup\$ – user3916429 Apr 9 '15 at 13:58
  • \$\begingroup\$ I suspect that SQ and DISTANCE are both C++ macros being expanded into explicit functions in the C# translation. \$\endgroup\$ – Tetsujin no Oni Apr 9 '15 at 14:35
  • \$\begingroup\$ @user3916429 I updated my answer (the performance section) for more information. Hope it helps. \$\endgroup\$ – Abbas Apr 9 '15 at 14:41
  • \$\begingroup\$ @Abbas Just would like to thank you, i am now looking for that BitLocks not much easy to get since in both of your links , it is fully different, the answer from SO is using pointers whereas MSDN is using nothing like that. I will check all those possibilities and try to find my own solution. Thanks again for your time. \$\endgroup\$ – user3916429 Apr 9 '15 at 15:20
  • \$\begingroup\$ Can anyone just enlight me on what are the srcStrideOffset srcTileOffset and dstStrideOffset in your first link please @Abbas ? Because i dont understand at all yet \$\endgroup\$ – user3916429 Apr 9 '15 at 16:17
11
\$\begingroup\$
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.

\$\endgroup\$
  • 2
    \$\begingroup\$ good first answer. \$\endgroup\$ – Malachi Apr 9 '15 at 15:14
4
\$\begingroup\$

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.

\$\endgroup\$
  • 4
    \$\begingroup\$ No-one is going to win an argument about 1-line bracing style in the comments to an answer on Code Review. (unless it is me, that is....). Let's take that sort of debate to chat - The 2nd Monitor. The previous comments are about to get wiped (cc. @Magus) \$\endgroup\$ – rolfl Apr 9 '15 at 15:54
  • \$\begingroup\$ man i understand what you mean about the braces ok. But couldn't you just help me fix my problem instead of wasting times on OPTIONAL braces please? because like i said, i rewrote a C++ code to C# so i just kept the structure. I myself always use braces, even the optional ones, and the point of the topic was IMPROVING SPEED not reading... even if i totally get your point for sure. Even if for once, my own opinion, but double For() iterating on x/y for images is totaly more readable than with braces since nothing is ever done in the first loop. \$\endgroup\$ – user3916429 Apr 9 '15 at 21:54
  • \$\begingroup\$ Everything is done in the first loop. like the second loop happens over and over again in the first loop. \$\endgroup\$ – Malachi Apr 9 '15 at 22:00
  • \$\begingroup\$ Sorry, i meant nothing is done into the first For() of the pair, but always in the second one. I explained myself wrong i guess. \$\endgroup\$ – user3916429 Apr 9 '15 at 22:37
0
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ this has the feel of a really big comment right now, please research a little bit and add to your answer, it sounds like you are going in the right direction. \$\endgroup\$ – Malachi Apr 9 '15 at 19:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.