8
\$\begingroup\$

The setPixelColor function below changes the color of pixels. I need some suggestions to optimize this function.

Example:

public static void main(String[] args) throws IOException {
    BufferedImage pic1 = ImageIO.read(new File("Images/Input-1.bmp"));
    setPixelColor(pic1, 34, 177, 76, 127, 127, 127);
}

This changes the left image into the right image.

color substitution before-and-after


 private static void setPixelColor(BufferedImage imgBuf, int red, int green, int blue, int newRed, int newGreen, int newBlue) throws IOException {
    int[] RGBarray;
    int w;
    int h;

    //Declare color arrays
    int[][] alphaPixels;
    int[][] redPixels;
    int[][] greenPixels;
    int[][] bluePixels;

    w = imgBuf.getWidth();
    h = imgBuf.getHeight();


    alphaPixels = new int[h][w];
    redPixels = new int[h][w];
    greenPixels = new int[h][w];
    bluePixels = new int[h][w];

    RGBarray = imgBuf.getRGB(0, 0, w, h, null, 0, w);

    //Bit shift values into arrays
    int i = 0;
    for (int row = 0; row < h; row++) {
        for (int col = 0; col < w; col++) {
            alphaPixels[row][col] = ((RGBarray[i] >> 24) & 0xff);
            redPixels[row][col] = ((RGBarray[i] >> 16) & 0xff);
            greenPixels[row][col] = ((RGBarray[i] >> 8) & 0xff);
            bluePixels[row][col] = (RGBarray[i] & 0xff);
            i++;
        }
    }

    //Set the values back to integers using re-bit shifting
    for (int row = 0; row < h; row++) {
        for (int col = 0; col < w; col++) {
            if (redPixels[row][col] == red && greenPixels[row][col] == green && bluePixels[row][col] == blue) {
                int rgb = (alphaPixels[row][col] & 0xff) << 24 |
                        (redPixels[row][col] & newRed) << 16 |
                        (greenPixels[row][col] & newGreen) << 8 |
                        (bluePixels[row][col] & newBlue);
                imgBuf.setRGB(col, row, rgb);
            }
        }
    }

    //Write back image
    ImageIO.write(imgBuf, "bmp", new File("Images/Output2.bmp"));
}
\$\endgroup\$
5
\$\begingroup\$

I think that changeColor or substColor would be a more appropriate name for this function.

I don't see why the function should write out its result to a file. That's a violation of the Single Responsibility Principle. What if I want to perform multiple color substitutions before writing out the result? What if I want a different output filename, or a format other than BMP?

Avoid empty declarations, when you can declare and initialize values at the same time. It's more readable and less error-prone.

You don't care about the x-y coordinates of each pixel — only the color matters. So, there is no need to construct the 2-D arrays representing the red, green, and blue channels.

/**
 * Changes all pixels of an old color into a new color, preserving the
 * alpha channel.
 */
private static void changeColor(
        BufferedImage imgBuf,
        int oldRed, int oldGreen, int oldBlue,
        int newRed, int newGreen, int newBlue) {

    int RGB_MASK = 0x00ffffff;
    int ALPHA_MASK = 0xff000000;

    int oldRGB = oldRed << 16 | oldGreen << 8 | oldBlue;
    int toggleRGB = oldRGB ^ (newRed << 16 | newGreen << 8 | newBlue);

    int w = imgBuf.getWidth();
    int h = imgBuf.getHeight();

    int[] rgb = imgBuf.getRGB(0, 0, w, h, null, 0, w);
    for (int i = 0; i < rgb.length; i++) {
        if ((rgb[i] & RGB_MASK) == oldRGB) {
            rgb[i] ^= toggleRGB;
        }
    }
    imgBuf.setRGB(0, 0, w, h, rgb, 0, w);
}

public static void main(String[] args) throws IOException {
    BufferedImage pic1 = ImageIO.read(new File(…));
    changeColor(pic1, 34, 177, 76, 127, 127, 127);
    ImageIO.write(pic1, "bmp", new File(…));
}
\$\endgroup\$
  • \$\begingroup\$ Replace with something like Raster or PixelGrabber because they are faster, but I can't figure out how to use it here, do you have any idea? Would be very useful \$\endgroup\$ – J.D Nov 9 '16 at 22:57
  • \$\begingroup\$ I'm surprised if you say this is slower, but it is certainly much more memory efficient than your OP which allocates 4 int buffers to precalculate all the R G and B values. If you run your function on a large file, you might just run out of memory. This algorithm uses zero additional overhead memory apart from a few variables. \$\endgroup\$ – Octopus Nov 9 '16 at 23:01
  • \$\begingroup\$ If we are speaking about the memory, yes that's possible. But if we are speak about spead... I see it's not actually true. If you don't believe me just try to run bot of this functions and you will see \$\endgroup\$ – J.D Nov 9 '16 at 23:06
  • 1
    \$\begingroup\$ It's likely the setRGB call that will loop through every pixel regardless if it's changed or not. Try setting it pixel by pixel inside the if. \$\endgroup\$ – JollyJoker Nov 10 '16 at 15:06
1
\$\begingroup\$

Instead of shifting each and every pixel and testing against each input R, G, and B values, why not shift your input RGB (and implied A) once and then match that against the full four byte value at each pixel location?

Or if you can't make any assumptions about A you can still match the constructed RGB triplet instead of deconstructing, testing and reconstructing. The same goes for the value you want to set it to.

\$\endgroup\$
  • \$\begingroup\$ you are speaking about just this if? if (redPixels[row][col] == red && greenPixels[row][col] == green && bluePixels[row][col] == blue) where i'm checking for all this value? \$\endgroup\$ – J.D Nov 9 '16 at 22:39
  • \$\begingroup\$ No, that "if" by itself is very quick. I'm talking about your general approach. @200_success has already posted a very good answer that covers what I'm talking about. \$\endgroup\$ – Octopus Nov 9 '16 at 22:58

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.