6
\$\begingroup\$

For a project of mine, which is targeting java 6, I had been in the need to have different algorithms/methods to change a BufferdImage with a ColorSpace.TYPE_RGB to a plain ole black and white one.

To test these implementations I therefore created an interface

public interface BlackAndWhiteConverter {
    BufferedImage toBlackAndWhite(BufferedImage image);
    void setThreshold(int threshold);
}  

and the, for me, most successful implementation is up for review

import java.awt.color.ColorSpace;
import java.awt.image.BufferedImage;
import java.awt.image.DataBuffer;
import java.awt.image.DataBufferInt;
import java.awt.image.Raster;
import java.awt.image.WritableRaster;

public class BlackAndWhiteRedThresholdConverter implements BlackAndWhiteConverter {

    @Override
    public BufferedImage toBlackAndWhite(final BufferedImage image) {

        final int imageType = image.getColorModel().getColorSpace().getType();

        if (imageType != ColorSpace.TYPE_RGB) {
            throw new IllegalArgumentException();
        }

        final int width = image.getWidth();
        final int height = image.getHeight();
        final int upperLimit = width * height;

        final DataBuffer buffer = image.getRaster().getDataBuffer();
        final int[] source = ((DataBufferInt) buffer).getData();

        byte[] destination = new byte[upperLimit];

        for (int i = 0; i < upperLimit; i++) {

            int red = (source[i] >> 16) & 0x000000FF;

            if (red > threshold) {
                destination[i] = -1;
            }
        }

        WritableRaster raster = Raster.createPackedRaster(DataBuffer.TYPE_BYTE, width, height, 1, 2, null);
        raster.setDataElements(0, 0, width, height, destination);

        BufferedImage destinationImage = new BufferedImage(width, height, BufferedImage.TYPE_BYTE_BINARY);
        destinationImage.setData(raster);
        return destinationImage;
    }

    private int threshold = 110;

    @Override
    public void setThreshold(int threshold) {
        this.threshold = threshold;
    }


}

Please comment on anything that springs into your face. No matter what, I can't use a java version higher than 6.

\$\endgroup\$
3
  • \$\begingroup\$ I like your verbose style of declaring local variables final, yet many times I heard people complaining about it. For consistency, you should declare also raster and friends final. \$\endgroup\$ – coderodde Jan 18 '17 at 17:27
  • \$\begingroup\$ Just to make this clear, you only care about the red level in the original image? So an image of a green circle will turn completely black? \$\endgroup\$ – JS1 Jan 18 '17 at 18:12
  • \$\begingroup\$ @JS1 if another algorithmn will get me the same result than thats fine. I need this converter to get black letters on grey background for ocr. \$\endgroup\$ – Heslacher Jan 18 '17 at 18:17
6
\$\begingroup\$

Input validation

If the received image is not RGB, throwing an IllegalArgumentException without a custom message is not very friendly. Users would have to look at the code to understand what went wrong exactly.

The long chain image.getColorModel().getColorSpace().getType() is extracted to an imageType local variable that's only used for validation. I would be good to move the validation to a dedicated helper method, let's say validateImageType.

Java conventions

Member variables should be declared at the top, followed by constructors, followed by methods. Very roughly speaking. So move the declaration of threshold higher.

Naming

As the interface name already implies the conversion to black and white, the method name toBlackAndWhite is redundant. I'd rename it to simply convert.

Misc

0x000000FF is the same as 0xFF, so you can shorten it.

\$\endgroup\$
1
  • \$\begingroup\$ I disagree with changing the method name to simply convert. It's fine as it is. Many future interfaces could use the signature BufferedImage convert(BufferedImage) because it's so generic. \$\endgroup\$ – Michael Jan 19 '17 at 11:53
4
\$\begingroup\$

Taking into account other colors

In case your input images aren't just grayscale images, you may need to take into account colors other than red. The standard RGB to grayscale formula looks like this:

gray = (red * 0.3 + green * 0.59 + blue * 0.11)

You can approximate that without using floating point like this:

gray = (red*3 + green*6 + blue) / 10

So your loop would look like this:

    int grayThreshold = threshold * 10;

    for (int i = 0; i < upperLimit; i++) {

        int red   = (source[i] >> 16) & 0xFF;
        int green = (source[i] >>  8) & 0xFF;
        int blue  = (source[i])       & 0xFF;
        int gray  = red*3 + green*6 + blue;

        if (gray > grayThreshold) {
            destination[i] = -1;
        }
    }
\$\endgroup\$
0
3
\$\begingroup\$

My first thought is that this isn't very object-oriented. There's only one field and you can very easily refactor the class so that it doesn't need it:

public abstract class BlackAndWhiteRedThresholdConverter implements BlackAndWhiteConverter {

    public static BufferedImage toBlackAndWhite(final BufferedImage image, final int threshold) {
        // convert
    }

    @Override
    public static BufferedImage toBlackAndWhite(final BufferedImage image) {
        return toBlackAndWhite(image, 110);
    }
}

This class is slightly easier to use than yours. There's no need to instantiate, and I can still use either my own threshold or the default one. Basically though it's no different. However, it's easier to see from this class definition that it's not object-oriented. It's procedural. It ends with "-er" and that's often bad.

A better solution might involve extending BufferedImage. Now when I instantiate a BlackAndWhiteImage I get a proper object who is actually useful.

public class BlackAndWhiteImage extends BufferedImage
{
    private static final int DEFAULT_THRESHOLD = 110;

    public static BlackAndWhiteImage make(final BufferedImage image, final int threshold)
    {
        return toBlackAndWhite(image, threshold);
    }

    public static BlackAndWhiteImage make(final BufferedImage image)
    {
        return make(image, DEFAULT_THRESHOLD);
    }

    /* Private constructor to create underlying BufferedImage */ 
    private BlackAndWhiteImage(ColorModel colourModel, WritableRaster raster, boolean alphaPremult)
    {
        super(colourModel, raster, alphaPremult, null);
    }

    private static BlackAndWhiteImage toBlackAndWhite(final BufferedImage image, final int threshold)
    {
        /* Conversion stuff is all the same */
        return new BlackAndWhiteImage( destinationImage.getColorModel(),
                                       destinationImage.getRaster(),
                                       destinationImage.isAlphaPremultiplied() );
    }
}

The static factory methods (make) are unfortunate but I couldn't get around using them.

Example usage:

BufferedImage img = new BufferedImage(50, 50, BufferedImage.TYPE_INT_RGB);
BlackAndWhiteImage bw = BlackAndWhiteImage.make(img);

File outputfile = new File("");
ImageIO.write(bw, "jpg", outputfile); 
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thanks for your effort. Sure it seems preferable to remove the one and only field but then I need to passs each time the threshold wether it had changed or not. Because I use SIMON I need to use an interface so the methods can be called remotely hence extending BufferedImage is not an option because both the server and the client would need to know about this concrete implementation. Nevertheless +1 \$\endgroup\$ – Heslacher Jan 19 '17 at 15:37
  • \$\begingroup\$ About the -er, I agree on vague names like Controller, Manager etc. including Converter but having a BlackAndWhiteConverter is IMO ok. \$\endgroup\$ – Heslacher Jan 19 '17 at 15:38
  • \$\begingroup\$ The issue is that a "converter" is not really an object. It doesn't represent anything. It's conceptually no different from a function that converts one object to something else. I highly recommend the other articles on the site that I linked - he has some very interesting, if controversial, ideas. \$\endgroup\$ – Michael Jan 19 '17 at 15:45

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.