TMI-150: Fix for 24 bit images with bitmask.

imageio/imageio-bmp/src/main/java/com/twelvemonkeys/imageio/plugins/bmp/BitmapDescriptor.java

@@ -43,6 +43,7 @@ abstract class BitmapDescriptor {
     protected final DIBHeader header;
 
     protected BufferedImage image;
+    protected BitmapMask mask;
 
     public BitmapDescriptor(final DirectoryEntry pEntry, final DIBHeader pHeader) {
         Validate.notNull(pEntry, "entry");
@@ -69,4 +70,17 @@ abstract class BitmapDescriptor {
     protected final int getBitCount() {
         return entry.getBitCount() != 0 ? entry.getBitCount() : header.getBitCount();
     }
+
+    @Override
+    public String toString() {
+        return getClass().getSimpleName() + "[" + entry + ", " + header + "]";
+    }
+
+    public final void setMask(final BitmapMask mask) {
+        this.mask = mask;
+    }
+
+    public final boolean hasMask() {
+        return header.getHeight() == getHeight() * 2;
+    }
 }

imageio/imageio-bmp/src/main/java/com/twelvemonkeys/imageio/plugins/bmp/BitmapIndexed.java

@@ -28,8 +28,6 @@
 
 package com.twelvemonkeys.imageio.plugins.bmp;
 
-import com.twelvemonkeys.image.InverseColorMapIndexColorModel;
-
 import java.awt.image.BufferedImage;
 import java.awt.image.DataBuffer;
 import java.awt.image.IndexColorModel;
@@ -46,8 +44,6 @@ class BitmapIndexed extends BitmapDescriptor {
     protected final int[] bits;
     protected final int[] colors;
 
-    private BitmapMask mask;
-
     public BitmapIndexed(final DirectoryEntry pEntry, final DIBHeader pHeader) {
         super(pEntry, pHeader);
         bits = new int[getWidth() * getHeight()];
@@ -65,7 +61,7 @@ class BitmapIndexed extends BitmapDescriptor {
         // This is slightly obscure, and should probably be moved..
         Hashtable<String, Object> properties = null;
         if (entry instanceof DirectoryEntry.CUREntry) {
-            properties = new Hashtable<String, Object>(1);
+            properties = new Hashtable<>(1);
             properties.put("cursor_hotspot", ((DirectoryEntry.CUREntry) this.entry).getHotspot());
         }
 
@@ -89,8 +85,6 @@ class BitmapIndexed extends BitmapDescriptor {
 
         raster.setSamples(0, 0, getWidth(), getHeight(), 0, bits);
 
-        //System.out.println("Image: " + image);
-
         return image;
     }
 
@@ -100,40 +94,40 @@ class BitmapIndexed extends BitmapDescriptor {
     IndexColorModel createColorModel() {
         // NOTE: This is a hack to make room for transparent pixel for mask
         int bits = getBitCount();
-        
+
         int colors = this.colors.length;
-        int trans = -1;
+        int transparent = -1;
 
         // Try to avoid USHORT transfertype, as it results in BufferedImage TYPE_CUSTOM
         // NOTE: This code assumes icons are small, and is NOT optimized for performance...
         if (colors > (1 << getBitCount())) {
-            int index = findTransIndexMaybeRemap(this.colors, this.bits);
+            int index = findTransparentIndexMaybeRemap(this.colors, this.bits);
 
             if (index == -1) {
                 // No duplicate found, increase bitcount
                 bits++;
-                trans = this.colors.length - 1;
+                transparent = this.colors.length - 1;
             }
             else {
-                // Found a duplicate, use it as trans
+                // Found a duplicate, use it as transparent
-                trans = index;
+                transparent = index;
                 colors--;
             }
         }
 
         // NOTE: Setting hasAlpha to true, makes things work on 1.2
-        return new InverseColorMapIndexColorModel(
+        return new IndexColorModel(
-                bits, colors, this.colors, 0, true, trans,
+                bits, colors, this.colors, 0, true, transparent,
                 bits <= 8 ? DataBuffer.TYPE_BYTE : DataBuffer.TYPE_USHORT
         );
     }
 
-    private static int findTransIndexMaybeRemap(final int[] pColors, final int[] pBits) {
+    private static int findTransparentIndexMaybeRemap(final int[] colors, final int[] bits) {
         // Look for unused colors, to use as transparent
-        final boolean[] used = new boolean[pColors.length - 1];
+        boolean[] used = new boolean[colors.length - 1];
-        for (int pBit : pBits) {
+        for (int bit : bits) {
-            if (!used[pBit]) {
+            if (!used[bit]) {
-                used[pBit] = true;
+                used[bit] = true;
             }
         }
 
@@ -144,38 +138,35 @@ class BitmapIndexed extends BitmapDescriptor {
         }
 
         // Try to find duplicates in colormap, and remap
-        int trans = -1;
+        int transparent = -1;
         int duplicate = -1;
-        for (int i = 0; trans == -1 && i < pColors.length - 1; i++) {
+        for (int i = 0; transparent == -1 && i < colors.length - 1; i++) {
-            for (int j = i + 1; j < pColors.length - 1; j++) {
+            for (int j = i + 1; j < colors.length - 1; j++) {
-                if (pColors[i] == pColors[j]) {
+                if (colors[i] == colors[j]) {
-                    trans = j;
+                    transparent = j;
                     duplicate = i;
                     break;
                 }
             }
         }
 
-        if (trans != -1) {
+        if (transparent != -1) {
             // Remap duplicate
-            for (int i = 0; i < pBits.length; i++) {
+            for (int i = 0; i < bits.length; i++) {
-                if (pBits[i] == trans) {
+                if (bits[i] == transparent) {
-                    pBits[i] = duplicate;
+                    bits[i] = duplicate;
                 }
             }
         }
 
-        return trans;
+        return transparent;
     }
 
     public BufferedImage getImage() {
         if (image == null) {
             image = createImageIndexed();
         }
+
         return image;
     }
-
-    public void setMask(final BitmapMask pMask) {
-        mask = pMask;
-    }
 }

imageio/imageio-bmp/src/main/java/com/twelvemonkeys/imageio/plugins/bmp/BitmapMask.java

@@ -38,19 +38,19 @@ import java.awt.image.BufferedImage;
  * @version $Id: BitmapMask.java,v 1.0 25.feb.2006 00:29:44 haku Exp$
  */
 class BitmapMask extends BitmapDescriptor {
-    protected final BitmapIndexed mask;
+    protected final BitmapIndexed bitMask;
 
     public BitmapMask(final DirectoryEntry pParent, final DIBHeader pHeader) {
         super(pParent, pHeader);
-        mask = new BitmapIndexed(pParent, pHeader);
+        bitMask = new BitmapIndexed(pParent, pHeader);
     }
 
     boolean isTransparent(final int pX, final int pY) {
         // NOTE: 1: Fully transparent, 0: Opaque...
-        return mask.bits[pX + pY * getWidth()] != 0;
+        return bitMask.bits[pX + pY * getWidth()] != 0;
     }
 
     public BufferedImage getImage() {
-        return mask.getImage();
+        return bitMask.getImage();
     }
 }

imageio/imageio-bmp/src/main/java/com/twelvemonkeys/imageio/plugins/bmp/BitmapRGB.java

@@ -28,7 +28,9 @@
 
 package com.twelvemonkeys.imageio.plugins.bmp;
 
+import java.awt.*;
 import java.awt.image.BufferedImage;
+import java.awt.image.WritableRaster;
 
 /**
  * Describes an RGB/true color bitmap structure (16, 24 and 32 bits per pixel).
@@ -43,6 +45,38 @@ class BitmapRGB extends BitmapDescriptor {
     }
 
     public BufferedImage getImage() {
+        // Test is mask != null rather than hasMask(), as 32 bit (w/alpha)
+        // might still have bitmask, but we don't read or use it.
+        if (mask != null) {
+            image = createMaskedImage();
+            mask = null;
+        }
+
         return image;
     }
+
+    private BufferedImage createMaskedImage() {
+        BufferedImage masked = new BufferedImage(getWidth(), getHeight(), BufferedImage.TYPE_4BYTE_ABGR);
+
+        Graphics2D graphics = masked.createGraphics();
+        try {
+            graphics.drawImage(image, 0, 0, null);
+        }
+        finally {
+            graphics.dispose();
+        }
+
+        WritableRaster alphaRaster = masked.getAlphaRaster();
+
+        byte[] trans = {0x0};
+        for (int y = 0; y < getHeight(); y++) {
+            for (int x = 0; x < getWidth(); x++) {
+                if (mask.isTransparent(x, y)) {
+                    alphaRaster.setDataElements(x, y, trans);
+                }
+            }
+        }
+
+        return masked;
+    }
 }

imageio/imageio-bmp/src/main/java/com/twelvemonkeys/imageio/plugins/bmp/DIBImageReader.java

@@ -66,14 +66,15 @@ import java.util.List;
 // TODO: Decide whether DirectoryEntry or DIBHeader should be primary source for color count/bit count
 // TODO: Support loading icons from DLLs, see
 // <a href="http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnwui/html/msdn_icons.asp">MSDN</a>
-// Known issue: 256x256 PNG encoded icons does not have IndexColorModel even if stated in DirectoryEntry (seem impossible as the PNGs are all true color)
+// Known issue: 256x256 PNG encoded icons does not have IndexColorModel even if stated in DirectoryEntry
+// (seem impossible as the PNGs are all true color)
 abstract class DIBImageReader extends ImageReaderBase {
     // TODO: Consider moving the reading to inner classes (subclasses of BitmapDescriptor)
     private Directory directory;
 
     // TODO: Review these, make sure we don't have a memory leak
-    private Map<DirectoryEntry, DIBHeader> headers = new WeakHashMap<DirectoryEntry, DIBHeader>();
+    private Map<DirectoryEntry, DIBHeader> headers = new WeakHashMap<>();
-    private Map<DirectoryEntry, BitmapDescriptor> descriptors = new WeakWeakMap<DirectoryEntry, BitmapDescriptor>();
+    private Map<DirectoryEntry, BitmapDescriptor> descriptors = new WeakWeakMap<>();
 
     private ImageReader pngImageReader;
 
@@ -101,7 +102,7 @@ abstract class DIBImageReader extends ImageReaderBase {
             return getImageTypesPNG(entry);
         }
 
-        List<ImageTypeSpecifier> types = new ArrayList<ImageTypeSpecifier>();
+        List<ImageTypeSpecifier> types = new ArrayList<>();
         DIBHeader header = getHeader(entry);
 
         // Use data from header to create specifier
@@ -121,10 +122,13 @@ abstract class DIBImageReader extends ImageReaderBase {
                 specifier = ImageTypeSpecifiers.createFromIndexColorModel(indexed.createColorModel());
                 break;
             case 16:
+                // TODO: May have mask?!
                 specifier = ImageTypeSpecifiers.createFromBufferedImageType(BufferedImage.TYPE_USHORT_555_RGB);
                 break;
             case 24:
-                specifier = ImageTypeSpecifiers.createFromBufferedImageType(BufferedImage.TYPE_3BYTE_BGR);
+                specifier = new BitmapRGB(entry, header).hasMask()
+                            ? ImageTypeSpecifiers.createFromBufferedImageType(BufferedImage.TYPE_4BYTE_ABGR)
+                            : ImageTypeSpecifiers.createFromBufferedImageType(BufferedImage.TYPE_3BYTE_BGR);
                 break;
             case 32:
                 specifier = ImageTypeSpecifiers.createFromBufferedImageType(BufferedImage.TYPE_INT_ARGB);
@@ -184,6 +188,7 @@ abstract class DIBImageReader extends ImageReaderBase {
             }
             else {
                 Graphics2D g = destination.createGraphics();
+
                 try {
                     g.setComposite(AlphaComposite.Src);
                     g.drawImage(image, 0, 0, null);
@@ -316,6 +321,8 @@ abstract class DIBImageReader extends ImageReaderBase {
             descriptors.put(pEntry, descriptor);
         }
 
+        System.err.println("descriptor: " + descriptor);
+
         return descriptor.getImage();
     }
 
@@ -335,7 +342,7 @@ abstract class DIBImageReader extends ImageReaderBase {
         }
 
         BitmapMask mask = new BitmapMask(pBitmap.entry, pBitmap.header);
-        readBitmapIndexed1(mask.mask, true);
+        readBitmapIndexed1(mask.bitMask, true);
         pBitmap.setMask(mask);
     }
 
@@ -370,7 +377,7 @@ abstract class DIBImageReader extends ImageReaderBase {
                 }
             }
 
-            // NOTE: If we are reading the mask, we don't abort or progress
+            // NOTE: If we are reading the mask, we don't abort or report progress
             if (!pAsMask) {
                 if (abortRequested()) {
                     processReadAborted();
@@ -455,7 +462,7 @@ abstract class DIBImageReader extends ImageReaderBase {
         short[] pixels = new short[pBitmap.getWidth() * pBitmap.getHeight()];
 
         // TODO: Support TYPE_USHORT_565 and the RGB 444/ARGB 4444 layouts
-        // Will create TYPE_USHORT_555;
+        // Will create TYPE_USHORT_555
         DirectColorModel cm = new DirectColorModel(16, 0x7C00, 0x03E0, 0x001F);
         DataBuffer buffer = new DataBufferShort(pixels, pixels.length);
         WritableRaster raster = Raster.createPackedRaster(
@@ -480,6 +487,8 @@ abstract class DIBImageReader extends ImageReaderBase {
 
             processImageProgress(100 * y / (float) pBitmap.getHeight());
         }
+
+        // TODO: Might be mask!?
     }
 
     private void readBitmap24(final BitmapDescriptor pBitmap) throws IOException {
@@ -494,16 +503,19 @@ abstract class DIBImageReader extends ImageReaderBase {
                 cs, nBits, false, false, Transparency.OPAQUE, DataBuffer.TYPE_BYTE
         );
 
+        int scanlineStride = pBitmap.getWidth() * 3;
+        // BMP rows are padded to 4 byte boundary
+        int rowSizeBytes = ((8 * scanlineStride + 31) / 32) * 4;
+
         WritableRaster raster = Raster.createInterleavedRaster(
-                buffer, pBitmap.getWidth(), pBitmap.getHeight(), pBitmap.getWidth(), 3, bOffs, null
+                buffer, pBitmap.getWidth(), pBitmap.getHeight(), scanlineStride, 3, bOffs, null
         );
         pBitmap.image = new BufferedImage(cm, raster, cm.isAlphaPremultiplied(), null);
-        
-        for (int y = 0; y < pBitmap.getHeight(); y++) {
-            int offset = (pBitmap.getHeight() - y - 1) * pBitmap.getWidth();
-            imageInput.readFully(pixels, offset, pBitmap.getWidth() * 3);
 
-            // TODO: Possibly read padding byte here!
+        for (int y = 0; y < pBitmap.getHeight(); y++) {
+            int offset = (pBitmap.getHeight() - y - 1) * scanlineStride;
+            imageInput.readFully(pixels, offset, scanlineStride);
+            imageInput.skipBytes(rowSizeBytes - scanlineStride);
 
             if (abortRequested()) {
                 processReadAborted();
@@ -512,6 +524,14 @@ abstract class DIBImageReader extends ImageReaderBase {
 
             processImageProgress(100 * y / (float) pBitmap.getHeight());
         }
+
+        // 24 bit icons usually have a bit mask
+        if (pBitmap.hasMask()) {
+            BitmapMask mask = new BitmapMask(pBitmap.entry, pBitmap.header);
+            readBitmapIndexed1(mask.bitMask, true);
+
+            pBitmap.setMask(mask);
+        }
     }
 
     private void readBitmap32(final BitmapDescriptor pBitmap) throws IOException {
@@ -535,6 +555,9 @@ abstract class DIBImageReader extends ImageReaderBase {
             }            
             processImageProgress(100 * y / (float) pBitmap.getHeight());
         }
+
+        // There might be a mask here as well, but we'll ignore it,
+        // and use the 8 bit alpha channel in the ARGB pixel data
     }
 
     private Directory getDirectory() throws IOException {

imageio/imageio-bmp/src/test/java/com/twelvemonkeys/imageio/plugins/bmp/ICOImageReaderTest.java

@@ -39,7 +39,9 @@ public class ICOImageReaderTest extends ImageReaderAbstractTest<ICOImageReader>
                         new Dimension(16, 16), new Dimension(16, 16), new Dimension(32, 32), new Dimension(32, 32),
                         new Dimension(48, 48), new Dimension(48, 48), new Dimension(256, 256), new Dimension(256, 256),
                         new Dimension(16, 16), new Dimension(32, 32), new Dimension(48, 48), new Dimension(256, 256)
-                )
+                ),
+                // Problematic icon that reports 24 bit in the descriptor, but has separate 1 bit ''mask (height 2 x icon height)!
+                new TestData(getClassLoaderResource("/ico/rgb24bitmask.ico"), new Dimension(32, 32))
         );
     }