Merge pull request #792 from brianpopow/feature/BMPv2ColorPaletteFix

JimBobSquarePants · web-flow · commit abaf5581b6e2 · 2018-12-21T00:24:33.000+11:00
WIP: Fix for Windows 2.0 or OS/2 1.x bitmaps only use 3 bytes per color palette entry
diff --git a/src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs b/src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs
@@ -108,7 +108,7 @@ public Image<TPixel> Decode<TPixel>(Stream stream)
         {
             try
             {
-                this.ReadImageHeaders(stream, out bool inverted, out byte[] palette);
+                int bytesPerColorMapEntry = this.ReadImageHeaders(stream, out bool inverted, out byte[] palette);
 
                 var image = new Image<TPixel>(this.configuration, this.infoHeader.Width, this.infoHeader.Height, this.metaData);
 
@@ -137,6 +137,7 @@ public Image<TPixel> Decode<TPixel>(Stream stream)
                                 this.infoHeader.Width,
                                 this.infoHeader.Height,
                                 this.infoHeader.BitsPerPixel,
+                                bytesPerColorMapEntry,
                                 inverted);
                         }
 
@@ -329,18 +330,20 @@ private void UncompressRle8(int w, Span<byte> buffer)
         /// <param name="colors">The <see cref="T:byte[]"/> containing the colors.</param>
         /// <param name="width">The width of the bitmap.</param>
         /// <param name="height">The height of the bitmap.</param>
-        /// <param name="bits">The number of bits per pixel.</param>
+        /// <param name="bitsPerPixel">The number of bits per pixel.</param>
+        /// <param name="bytesPerColorMapEntry">Usually 4 bytes, but in case of Windows 2.x bitmaps or OS/2 1.x bitmaps
+        /// the bytes per color palette entry's can be 3 bytes instead of 4.</param>
         /// <param name="inverted">Whether the bitmap is inverted.</param>
-        private void ReadRgbPalette<TPixel>(Buffer2D<TPixel> pixels, byte[] colors, int width, int height, int bits, bool inverted)
+        private void ReadRgbPalette<TPixel>(Buffer2D<TPixel> pixels, byte[] colors, int width, int height, int bitsPerPixel, int bytesPerColorMapEntry, bool inverted)
             where TPixel : struct, IPixel<TPixel>
         {
             // Pixels per byte (bits per pixel)
-            int ppb = 8 / bits;
+            int ppb = 8 / bitsPerPixel;
 
             int arrayWidth = (width + ppb - 1) / ppb;
 
             // Bit mask
-            int mask = 0xFF >> (8 - bits);
+            int mask = 0xFF >> (8 - bitsPerPixel);
 
             // Rows are aligned on 4 byte boundaries
             int padding = arrayWidth % 4;
@@ -366,7 +369,7 @@ private void ReadRgbPalette<TPixel>(Buffer2D<TPixel> pixels, byte[] colors, int
                         int colOffset = x * ppb;
                         for (int shift = 0, newX = colOffset; shift < ppb && newX < width; shift++, newX++)
                         {
-                            int colorIndex = ((rowSpan[offset] >> (8 - bits - (shift * bits))) & mask) * 4;
+                            int colorIndex = ((rowSpan[offset] >> (8 - bitsPerPixel - (shift * bitsPerPixel))) & mask) * bytesPerColorMapEntry;
 
                             color.FromBgr24(Unsafe.As<byte, Bgr24>(ref colors[colorIndex]));
                             pixelRow[newX] = color;
@@ -576,7 +579,9 @@ private void ReadFileHeader()
         /// <summary>
         /// Reads the <see cref="BmpFileHeader"/> and <see cref="BmpInfoHeader"/> from the stream and sets the corresponding fields.
         /// </summary>
-        private void ReadImageHeaders(Stream stream, out bool inverted, out byte[] palette)
+        /// <returns>Bytes per color palette entry. Usually 4 bytes, but in case of Windows 2.x bitmaps or OS/2 1.x bitmaps
+        /// the bytes per color palette entry's can be 3 bytes instead of 4.</returns>
+        private int ReadImageHeaders(Stream stream, out bool inverted, out byte[] palette)
         {
             this.stream = stream;
 
@@ -596,19 +601,23 @@ private void ReadImageHeaders(Stream stream, out bool inverted, out byte[] palet
             }
 
             int colorMapSize = -1;
+            int bytesPerColorMapEntry = 4;
 
             if (this.infoHeader.ClrUsed == 0)
             {
                 if (this.infoHeader.BitsPerPixel == 1
                     || this.infoHeader.BitsPerPixel == 4
                     || this.infoHeader.BitsPerPixel == 8)
                 {
-                    colorMapSize = ImageMaths.GetColorCountForBitDepth(this.infoHeader.BitsPerPixel) * 4;
+                    int colorMapSizeBytes = this.fileHeader.Offset - BmpFileHeader.Size - this.infoHeader.HeaderSize;
+                    int colorCountForBitDepth = ImageMaths.GetColorCountForBitDepth(this.infoHeader.BitsPerPixel);
+                    bytesPerColorMapEntry = colorMapSizeBytes / colorCountForBitDepth;
+                    colorMapSize = colorMapSizeBytes;
                 }
             }
             else
             {
-                colorMapSize = this.infoHeader.ClrUsed * 4;
+                colorMapSize = this.infoHeader.ClrUsed * bytesPerColorMapEntry;
             }
 
             palette = null;
@@ -627,6 +636,8 @@ private void ReadImageHeaders(Stream stream, out bool inverted, out byte[] palet
             }
 
             this.infoHeader.VerifyDimensions();
+
+            return bytesPerColorMapEntry;
         }
     }
 }
diff --git a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs
@@ -55,6 +55,30 @@ public void BmpDecoder_IsNotBoundToSinglePixelType<TPixel>(TestImageProvider<TPi
             }
         }
 
+        [Theory]
+        [WithFile(WinBmpv2, PixelTypes.Rgba32)]
+        public void BmpDecoder_CanDecodeBmpv2<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : struct, IPixel<TPixel>
+        {
+            using (Image<TPixel> image = provider.GetImage(new BmpDecoder()))
+            {
+                image.DebugSave(provider, "png");
+                image.CompareToOriginal(provider);
+            }
+        }
+
+        [Theory]
+        [WithFile(Bit8Palette4, PixelTypes.Rgba32)]
+        public void BmpDecoder_CanDecode4BytePerEntryPalette<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : struct, IPixel<TPixel>
+        {
+            using (Image<TPixel> image = provider.GetImage(new BmpDecoder()))
+            {
+                image.DebugSave(provider, "png");
+                image.CompareToOriginal(provider);
+            }
+        }
+
         [Theory]
         [InlineData(Car, 24)]
         [InlineData(F, 24)]
diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs
@@ -201,6 +201,10 @@ public static class Bmp
             public const string Bit16 = "Bmp/test16.bmp";
             public const string Bit16Inverted = "Bmp/test16-inverted.bmp";
             public const string Bit32Rgb = "Bmp/rgb32.bmp";
+            
+            // Note: This format can be called OS/2 BMPv1, or Windows BMPv2
+            public const string WinBmpv2 = "Bmp/pal8os2v1_winv2.bmp";
+            public const string Bit8Palette4 = "Bmp/pal8-0.bmp";
             public const string Os2v2Short = "Bmp/pal8os2v2-16.bmp";
 
             public static readonly string[] All
diff --git a/tests/ImageSharp.Tests/TestUtilities/TestEnvironment.Formats.cs b/tests/ImageSharp.Tests/TestUtilities/TestEnvironment.Formats.cs
@@ -34,8 +34,7 @@ internal static IImageFormat GetImageFormat(string filePath)
         {
             string extension = Path.GetExtension(filePath);
 
-            IImageFormat format = Configuration.ImageFormatsManager.FindFormatByFileExtension(extension);
-            return format;
+            return Configuration.ImageFormatsManager.FindFormatByFileExtension(extension);
         }
 
         private static void ConfigureCodecs(
@@ -69,7 +68,7 @@ private static Configuration CreateDefaultConfiguration()
 
             cfg.ConfigureCodecs(
                 BmpFormat.Instance,
-                SystemDrawingReferenceDecoder.Instance,
+                IsWindows ? (IImageDecoder)SystemDrawingReferenceDecoder.Instance : MagickReferenceDecoder.Instance,
                 bmpEncoder,
                 new BmpImageFormatDetector());
 
diff --git a/tests/ImageSharp.Tests/TestUtilities/Tests/TestEnvironmentTests.cs b/tests/ImageSharp.Tests/TestUtilities/Tests/TestEnvironmentTests.cs
@@ -127,7 +127,7 @@ public void GetReferenceEncoder_ReturnsCorrectEncoders_Linux(string fileName, Ty
 
         [Theory]
         [InlineData("lol/foo.png", typeof(MagickReferenceDecoder))]
-        [InlineData("lol/Rofl.bmp", typeof(SystemDrawingReferenceDecoder))]
+        [InlineData("lol/Rofl.bmp", typeof(MagickReferenceDecoder))]
         [InlineData("lol/Baz.JPG", typeof(JpegDecoder))]
         [InlineData("lol/Baz.gif", typeof(GifDecoder))]
         public void GetReferenceDecoder_ReturnsCorrectDecoders_Linux(string fileName, Type expectedDecoderType)
diff --git a/tests/Images/Input/Bmp/pal8-0.bmp b/tests/Images/Input/Bmp/pal8-0.bmp
diff --git a/tests/Images/Input/Bmp/pal8os2v1_winv2.bmp b/tests/Images/Input/Bmp/pal8os2v1_winv2.bmp

Original file line number	Diff line number	Diff line change
`@@ -108,7 +108,7 @@ public Image<TPixel> Decode<TPixel>(Stream stream)`
`108`	`108`	`{`
`109`	`109`	`try`
`110`	`110`	`{`
`111`		`- this.ReadImageHeaders(stream, out bool inverted, out byte[] palette);`
	`111`	`+ int bytesPerColorMapEntry = this.ReadImageHeaders(stream, out bool inverted, out byte[] palette);`
`112`	`112`
`113`	`113`	`var image = new Image<TPixel>(this.configuration, this.infoHeader.Width, this.infoHeader.Height, this.metaData);`
`114`	`114`
`@@ -137,6 +137,7 @@ public Image<TPixel> Decode<TPixel>(Stream stream)`
`137`	`137`	`this.infoHeader.Width,`
`138`	`138`	`this.infoHeader.Height,`
`139`	`139`	`this.infoHeader.BitsPerPixel,`
	`140`	`+ bytesPerColorMapEntry,`
`140`	`141`	`inverted);`
`141`	`142`	`}`
`142`	`143`
`@@ -329,18 +330,20 @@ private void UncompressRle8(int w, Span<byte> buffer)`
`329`	`330`	`/// <param name="colors">The <see cref="T:byte[]"/> containing the colors.</param>`
`330`	`331`	`/// <param name="width">The width of the bitmap.</param>`
`331`	`332`	`/// <param name="height">The height of the bitmap.</param>`
`332`		`- /// <param name="bits">The number of bits per pixel.</param>`
	`333`	`+ /// <param name="bitsPerPixel">The number of bits per pixel.</param>`
	`334`	`+ /// <param name="bytesPerColorMapEntry">Usually 4 bytes, but in case of Windows 2.x bitmaps or OS/2 1.x bitmaps`
	`335`	`+ /// the bytes per color palette entry's can be 3 bytes instead of 4.</param>`
`333`	`336`	`/// <param name="inverted">Whether the bitmap is inverted.</param>`
`334`		`- private void ReadRgbPalette<TPixel>(Buffer2D<TPixel> pixels, byte[] colors, int width, int height, int bits, bool inverted)`
	`337`	`+ private void ReadRgbPalette<TPixel>(Buffer2D<TPixel> pixels, byte[] colors, int width, int height, int bitsPerPixel, int bytesPerColorMapEntry, bool inverted)`
`335`	`338`	`where TPixel : struct, IPixel<TPixel>`
`336`	`339`	`{`
`337`	`340`	`// Pixels per byte (bits per pixel)`
`338`		`- int ppb = 8 / bits;`
	`341`	`+ int ppb = 8 / bitsPerPixel;`
`339`	`342`
`340`	`343`	`int arrayWidth = (width + ppb - 1) / ppb;`
`341`	`344`
`342`	`345`	`// Bit mask`
`343`		`- int mask = 0xFF >> (8 - bits);`
	`346`	`+ int mask = 0xFF >> (8 - bitsPerPixel);`
`344`	`347`
`345`	`348`	`// Rows are aligned on 4 byte boundaries`
`346`	`349`	`int padding = arrayWidth % 4;`
`@@ -366,7 +369,7 @@ private void ReadRgbPalette<TPixel>(Buffer2D<TPixel> pixels, byte[] colors, int`
`366`	`369`	`int colOffset = x * ppb;`
`367`	`370`	`for (int shift = 0, newX = colOffset; shift < ppb && newX < width; shift++, newX++)`
`368`	`371`	`{`
`369`		`- int colorIndex = ((rowSpan[offset] >> (8 - bits - (shift * bits))) & mask) * 4;`
	`372`	`+ int colorIndex = ((rowSpan[offset] >> (8 - bitsPerPixel - (shift * bitsPerPixel))) & mask) * bytesPerColorMapEntry;`
`370`	`373`
`371`	`374`	`color.FromBgr24(Unsafe.As<byte, Bgr24>(ref colors[colorIndex]));`
`372`	`375`	`pixelRow[newX] = color;`
`@@ -576,7 +579,9 @@ private void ReadFileHeader()`
`576`	`579`	`/// <summary>`
`577`	`580`	`/// Reads the <see cref="BmpFileHeader"/> and <see cref="BmpInfoHeader"/> from the stream and sets the corresponding fields.`
`578`	`581`	`/// </summary>`
`579`		`- private void ReadImageHeaders(Stream stream, out bool inverted, out byte[] palette)`
	`582`	`+ /// <returns>Bytes per color palette entry. Usually 4 bytes, but in case of Windows 2.x bitmaps or OS/2 1.x bitmaps`
	`583`	`+ /// the bytes per color palette entry's can be 3 bytes instead of 4.</returns>`
	`584`	`+ private int ReadImageHeaders(Stream stream, out bool inverted, out byte[] palette)`
`580`	`585`	`{`
`581`	`586`	`this.stream = stream;`
`582`	`587`
`@@ -596,19 +601,23 @@ private void ReadImageHeaders(Stream stream, out bool inverted, out byte[] palet`
`596`	`601`	`}`
`597`	`602`
`598`	`603`	`int colorMapSize = -1;`
	`604`	`+ int bytesPerColorMapEntry = 4;`
`599`	`605`
`600`	`606`	`if (this.infoHeader.ClrUsed == 0)`
`601`	`607`	`{`
`602`	`608`	`if (this.infoHeader.BitsPerPixel == 1`
`603`	`609`	`\|\| this.infoHeader.BitsPerPixel == 4`
`604`	`610`	`\|\| this.infoHeader.BitsPerPixel == 8)`
`605`	`611`	`{`
`606`		`- colorMapSize = ImageMaths.GetColorCountForBitDepth(this.infoHeader.BitsPerPixel) * 4;`
	`612`	`+ int colorMapSizeBytes = this.fileHeader.Offset - BmpFileHeader.Size - this.infoHeader.HeaderSize;`
	`613`	`+ int colorCountForBitDepth = ImageMaths.GetColorCountForBitDepth(this.infoHeader.BitsPerPixel);`
	`614`	`+ bytesPerColorMapEntry = colorMapSizeBytes / colorCountForBitDepth;`
	`615`	`+ colorMapSize = colorMapSizeBytes;`
`607`	`616`	`}`
`608`	`617`	`}`
`609`	`618`	`else`
`610`	`619`	`{`
`611`		`- colorMapSize = this.infoHeader.ClrUsed * 4;`
	`620`	`+ colorMapSize = this.infoHeader.ClrUsed * bytesPerColorMapEntry;`
`612`	`621`	`}`
`613`	`622`
`614`	`623`	`palette = null;`
`@@ -627,6 +636,8 @@ private void ReadImageHeaders(Stream stream, out bool inverted, out byte[] palet`
`627`	`636`	`}`
`628`	`637`
`629`	`638`	`this.infoHeader.VerifyDimensions();`
	`639`	`+`
	`640`	`+ return bytesPerColorMapEntry;`
`630`	`641`	`}`
`631`	`642`	`}`
`632`	`643`	`}`
Original file line number	Diff line number	Diff line change
`@@ -34,8 +34,7 @@ internal static IImageFormat GetImageFormat(string filePath)`
`34`	`34`	`{`
`35`	`35`	`string extension = Path.GetExtension(filePath);`
`36`	`36`
`37`		`- IImageFormat format = Configuration.ImageFormatsManager.FindFormatByFileExtension(extension);`
`38`		`- return format;`
	`37`	`+ return Configuration.ImageFormatsManager.FindFormatByFileExtension(extension);`
`39`	`38`	`}`
`40`	`39`
`41`	`40`	`private static void ConfigureCodecs(`
`@@ -69,7 +68,7 @@ private static Configuration CreateDefaultConfiguration()`
`69`	`68`
`70`	`69`	`cfg.ConfigureCodecs(`
`71`	`70`	`BmpFormat.Instance,`
`72`		`- SystemDrawingReferenceDecoder.Instance,`
	`71`	`+ IsWindows ? (IImageDecoder)SystemDrawingReferenceDecoder.Instance : MagickReferenceDecoder.Instance,`
`73`	`72`	`bmpEncoder,`
`74`	`73`	`new BmpImageFormatDetector());`
`75`	`74`