Image processing and sizes

[Anticross]

I write some application which process the image finding the color which match with given color. If it not matched it will be replaced by white color. I know that my method far from perfection. But almost all is OK, except bug with large images. If I proceed small images it works but if image is big the program is stacked and CPU shows the 100% load. So here is the code:

@
void ImageWidget::proccessImage(QColor color)
{
// Search algorithm

QImage image = m_image;
int match = 5;

for(int i = 0; i<image.height(); i++)
{
for(int j = 0; j<image.width(); j++)
{
QRgb currentPixel = image.pixel(i,j);

QColor pixColor = QColor::fromRgb(currentPixel);

if(pixColor.red()-color.red() > match || pixColor.red()-color.red() < - match  && 
   pixColor.green()-color.green() > match || pixColor.green()-color.green() < - match &&
   pixColor.blue()-color.blue() > match || pixColor.blue()-color.blue() < - match)
image.setPixel(i,j,qRgb(255,255,255));

}
}
m_imageLabel->setPixmap(QPixmap::fromImage(image));
}
@

m_imageLabel is QLabel and m_image is given image. Maybe anyone can help me to understand why it happens and give some corrections.

[ZapB]

Why do you think it odd that the cpu uses 100% for large images? The bigger the image the longer it will take.

You can reduce the number of checks you are doing by a factor of 2 by having checks like this:

@
qAbs( pixColor.red() - color.red() ) < match
@

If you wish to keep your GUI alive whilst you are doing these calculations then I suggest that you move them into another thread and emit the modified image in a signal when it is finished.

[Anticross]

I've got a double core cpu with freq 2.3 ghz and the program freeze more than 10 min till processing image 1024*768 and don't unfreezes. So I don't know how long I need wait, maybe it is because I using different image formats: png, bmp, jpeg or different color dept in image or any else.

[ZapB]

As an aside the number of cores is irrelevent here as you are only using one thread. Having said that it should not take that long to process such modestly sized image. Have you tried breaking the execution using the debugger when it freezes to see where it is getting stuck?

[Anticross]

is there a difference between pixel loops in different types of images ?

[ZapB]

What does QImage::format() return for an image that works and one that doesn't?

[Anticross]

I've got massages in output window of my debbuger
QImage::pixel: coordinate (0,2336) out of range
QImage::pixel: coordinate (0,2337) out of range
QImage::pixel: coordinate (0,2338) out of range
QImage::pixel: coordinate (0,2339) out of range
QImage::pixel: coordinate (0,2340) out of range
QImage::pixel: coordinate (0,2341) out of range
...
So it's happens when I processed 3504*2336 image. why it not limited by size of image or I have a mistake in my loops limits?

[ZapB]

You have you're i and j loop counters back to front in the call to image.pixel(). It should be:

@
QRgb currentPixel = image.pixel( j, i );
@

since you are using j to loop over the x-coord and i to iterate over the y coord. See how the debugger helps you ;-)

[ZapB]

You have made the same mistake in the call to setPixel() too.

[ZapB]

So with all the above in place your code should look like this:

@
void ImageWidget::proccessImage(QColor color)
{
// Search algorithm

QImage image = m_image;
int match = 5;

for(int i = 0; i<image.height(); i++)
{
for(int j = 0; j<image.width(); j++)
{
QRgb currentPixel = image.pixel(j,i);

QColor pixColor = QColor::fromRgb(currentPixel);

if(qAbs(pixColor.red()-color.red()) > match && 
   qAbs(pixColor.green()-color.green()) > match &&
   qAbs(pixColor.blue()-color.blue()) > match)
    image.setPixel(j,i,qRgb(255,255,255));

}
}
m_imageLabel->setPixmap(QPixmap::fromImage(image));
}
@

[Anticross]

Damn, it's so easy :) I need to be more attentive. Thanks for the help and some code optimization.

[giesbert]

Hi Anticross,

One idea to make the code a bit more readable und to easier find such problems (use x and y instead of i and j), make some renaming. You can also use QRgb directly.

@
void ImageWidget::proccessImage(QColor color)
{
// Search algorithm

QImage image = m_image;
int difference = 5;

for(int y = 0; y < image.height(); y++)
{
    for(int x = 0; x < image.width(); x++)
    {
        QRgb currentPixel = image.pixel(x,y);

        if( qAbs(qRed(currentPixel) - color.red()) > difference &&
            qAbs(qGreen(currentPixel) - color.green()) > difference &&
            qAbs(qBlue(currentPixel) - color.blue()) > difference)
        {
            image.setPixel(x,y,qRgb(255,255,255));
        }
    }
}
m_imageLabel->setPixmap(QPixmap::fromImage(image));

}
@

[ZapB]

I usually tend to use i for iterating over x and j for iterating over ysince it reminds me of i and j unit vectors. I also tend to use epsilon or delta for small values such as your match or difference variables. It's just taste though and this is my background in mathematics prejudicing me. ;-)

[giesbert]

As we are talking about coordinates, and height and width, it's usually x and y. That's why I changed that. i is usually (with the developers I work with :-) ) a variable for a normal for loop. For cascading, I normally use row and column or s9imilar things, por x and y if they are coordinated (which they are in this case).

But it's a matter of taste.

And match was (for me) just nothing that was clear from the name. match for me would be a boolean (matches or not)

[ZapB]

Yeah for me it also depends upon the context in which the loop(s) occur. If I am iterating over rows then I'll use "row". Usually I try to use something that makes it obvious to which the counter refers.