7
\$\begingroup\$

The following algorithm works correctly for the purposes that I need, which is to set the top side of an image with a transparent gradient blend. Any tips to optimize it for speed?

void ImageUtil::sectionAlphaGradient(uint32_t* pixelBuffer, const int width, const int gradientHeight)
{
    unsigned int pixelOffsetY, pixelIndex;
    uint8_t A, R, G, B;

    const unsigned short OPAQUE = 255;

    const uint32_t BACKCOLOR = 0x00000000;
    const uint8_t backR = (BACKCOLOR & 0x00FF0000) >> 16;
    const uint8_t backG = (BACKCOLOR & 0x0000FF00) >> 8;
    const uint8_t backB = (BACKCOLOR & 0x000000FF);

    for (unsigned int y = 0; y < gradientHeight; y++)
    {
        A = (uint8_t)((OPAQUE * y) / gradientHeight);

        pixelOffsetY = y * width;

        for (unsigned int x = 0; x < width; x++)
        {
            pixelIndex = pixelOffsetY + x;
            uint32_t pixelValue = pixelBuffer[pixelIndex];

            R = (pixelValue & 0x00FF0000) >> 16;
            G = (pixelValue & 0x0000FF00) >> 8;
            B = (pixelValue & 0x000000FF);

            R = (R * A + backR * (OPAQUE - A)) / OPAQUE;
            G = (G * A + backG * (OPAQUE - A)) / OPAQUE;
            B = (B * A + backB * (OPAQUE - A)) / OPAQUE;

            pixelBuffer[pixelIndex] = (uint32_t)((A << 24) | ((R & 0xFF) << 16) | ((G & 0xFF) << 8) | (B & 0xFF));
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ If you're looking for ultimate performance, you might want to consider rewriting the code to use MMX/SSE intrinsics. \$\endgroup\$ – glampert Nov 14 '14 at 20:55
12
\$\begingroup\$

There are a number of things you could do to make this faster and better.

Use a pointer rather than array references

The code currently computes a pixelIndex and uses it as in pixelValue = pixelBuffer[pixelIndex], but what's actually happening is that each pixel is visited in order. The code can be sped up quite a bit with just this one change.

pixelValue = *pixelBuffer;
// .. do processing
++pixelBuffer;   // advance to the next pixel

Avoid shifting

Right now, the code masks off the pixel value for each color and then shifts it, does calculations and then shifts it back into place. Those shifts are not needed. Instead of this:

const uint8_t backR = (BACKCOLOR & 0x00FF0000) >> 16;
R = (pixelValue & 0x00FF0000) >> 16;
pixelBuffer[pixelIndex] = (uint32_t)((A << 24) | ((R & 0xFF) << 16) 
    | ((G & 0xFF) << 8) | (B & 0xFF));

The code could instead do this:

const uint32_t maskR = 0x00FF0000;
const uint32_t backR = BACKCOLOR & maskR;
R = *pixelBuffer & maskR;
*pixelBuffer++ = (uint32_t)((A << 24) | (R & maskR) 
    | (G & maskG) | (B & maskB));

Use same-sized constants

Using a mix of uint8_t, uint32_t and unsigned short constants makes the compiler work much harder with all of the implicit transformations. Better would be to use all the same size, such as uint32_t, the same as your pixel data. The code generated by the compiler may be simpler, smaller and faster as a result.

Move loop invariants out of the loop

Compilers can often perform this optimization for you, but it's good to give them a hint. This code computes this each time through the inner loop:

R = (R * A + backR * (OPAQUE - A)) / OPAQUE;

But A, backR and OPAQUE don't change within that loop, so better would be to calculate backR * (OPAQUE - A) as part of the outer loop.

Use named constants

This isn't a speed issue, but instead of using the constant 0x00FF0000 multiple places, it's better from a maintenance standpoint to turn that into a named constant maskR to avoid errors. Alternatively, one could create a function or macro that does the masking.

Be careful with signed vs. unsigned

The loop counters x and y are declared as int values, and they're compared to width and gradientHeight, but are you ever really going to want negative numbers for any of those? It seems to me that they'd be better as unsigned.

Is BACKCOLOR really 0?

More of the math could be eliminated if BACKCOLOR is really always supposed to be zero. I assumed instead, that it might be set to some other color, and so I left it in place.

Declare variables as late as possible

Rather than using the old C-style of declaring all variables at the top of a function, use the modern C++-style and declare variables as late as possible. Doing so can sometimes help the compiler figure out register allocation, resulting in faster, smaller code.

Putting it all together

When all of these are applied, the resulting code is shorter, cleaner and faster:

#define maskR(x) (x & 0x00FF0000)
#define maskG(x) (x & 0x0000FF00)
#define maskB(x) (x & 0x000000FF)

void sectionAlphaGradient2(uint32_t* pixelBuffer, 
    const unsigned width, const unsigned gradientHeight)
{
    const uint32_t OPAQUE = 255;
    const uint32_t BACKCOLOR = 0x00000000;
    const uint32_t backR = maskR(BACKCOLOR);
    const uint32_t backG = maskG(BACKCOLOR);
    const uint32_t backB = maskB(BACKCOLOR);

    for (unsigned y = 0; y < gradientHeight; y++)
    {
        uint32_t A = (uint8_t)((OPAQUE * y) / gradientHeight);
        uint32_t shiftedA = A << 24;
        uint32_t altR = backR *(OPAQUE - A);
        uint32_t altG = backG *(OPAQUE - A);
        uint32_t altB = backB *(OPAQUE - A);
        for (unsigned int x = 0; x < width; x++)
        {
            uint32_t R = (maskR(*pixelBuffer) * A + altR) / OPAQUE;
            uint32_t G = (maskG(*pixelBuffer) * A + altG) / OPAQUE;
            uint32_t B = (maskB(*pixelBuffer) * A + altB) / OPAQUE;
            *pixelBuffer++ = shiftedA | maskR(R) | maskG(G) | maskB(B);
        }
    }
}

Results

On my machine, a 64-bit Linux box running g++ 4.8.3 and using test code with an 8000 by 6000 pixel buffer, I get the following results:

original: 240977 microseconds
improved: 125714 microseconds
\$\endgroup\$
  • 3
    \$\begingroup\$ Outstanding review. Not only blows on performance improvement but the explanation is top-notch, I've learned a lot from it. Thanks so much for it. \$\endgroup\$ – PerracoLabs Nov 14 '14 at 16:03
  • 2
    \$\begingroup\$ Happy to have helped. I'm sure you'll do the same for me or others when we post code for review. That's what CodeReview is all about, and I've learned a lot from it, too. \$\endgroup\$ – Edward Nov 14 '14 at 17:24
2
\$\begingroup\$
  1. This could probably be optimized further by dividing by 256 instead of 255 in the X loop. (use ">> 8" or let the compiler handle it.) I doubt the visual difference would be enough to matter.

  2. Reduce the scope of variables as much as possible. (You did this in the for loops, but for example "R", "G", "B", "A" could be declared const within the X loop, "A" could be declared const within the Y loop, etc.)

  3. I dislike some of the capitalization confusion here. Macros should be "UPPER_CASE", functions/namespaces/classes should be "CamelCase", namespace/class methods should be "mixedCase", and variables should be "lower_case".

  4. I applaud the lining up of brackets (I heard all the cool people were doing it), and also separating logical blocks of code with blank lines. Other than the suggestions made in point #3 above, I find it to be a very readable piece of code.

\$\endgroup\$

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.