Refactor icon initialization and make use of CreateIconResourceDirectory() for improved accuracy

src/System.Drawing.Common/src/NativeMethods.txt

@@ -35,6 +35,7 @@ LANG_ARABIC
 LANG_JAPANESE
 LevelsEffectGuid
 LevelsParams
+LookupIconIdFromDirectoryEx
 PALETTEENTRY
 PaletteFlags
 PaletteType

src/System.Drawing.Common/src/System/Drawing/Icon.cs

@@ -17,8 +17,6 @@ namespace System.Drawing;
 [TypeForwardedFrom(AssemblyRef.SystemDrawing)]
 public sealed unsafe partial class Icon : MarshalByRefObject, ICloneable, IDisposable, ISerializable, IIcon
 {
-    private static int s_bitDepth;
-
     // The PNG signature is specified at https://www.w3.org/TR/PNG/#5PNG-file-signature
     private const int PNGSignature1 = 137 + ('P' << 8) + ('N' << 16) + ('G' << 24);
     private const int PNGSignature2 = 13 + (10 << 8) + (26 << 16) + (10 << 24);
@@ -398,6 +396,40 @@ public static Icon FromHandle(IntPtr handle)
         return new Icon((HICON)handle);
     }
 
+    // Creates RT_GROUP_ICON resource directory from ICO file data
+    private static byte[] CreateIconResourceDirectory(ReadOnlySpan<byte> iconData)
+    {
+        var reader = new SpanReader<byte>(iconData);
+
+        reader.TryRead(out ICONDIR icoDir);
+        reader.TryRead(icoDir.idCount, out ReadOnlySpan<ICONDIRENTRY> icoEntries);
+
+        byte[] resDir = GC.AllocateUninitializedArray<byte>(sizeof(GRPICONDIR) + icoDir.idCount * sizeof(GRPICONDIRENTRY));
+        Span<byte> resDirSpan = resDir;
+
+        MemoryMarshal.Write(resDirSpan, new GRPICONDIR { Reserved = 0, ResType = icoDir.idType, ResCount = icoDir.idCount });
+
+        Span<GRPICONDIRENTRY> gpEntries = MemoryMarshal.Cast<byte, GRPICONDIRENTRY>(resDirSpan[sizeof(GRPICONDIR)..]);
+
+        for (int i = 0; i < icoDir.idCount; i++)
+        {
+            ref readonly ICONDIRENTRY icoEntry = ref icoEntries[i];
+            gpEntries[i] = new GRPICONDIRENTRY
+            {
+                Width = icoEntry.bWidth,
+                Height = icoEntry.bHeight,
+                ColorCount = icoEntry.bColorCount,
+                Reserved = icoEntry.bReserved,
+                Planes = icoEntry.wPlanes,
+                BitCount = icoEntry.wBitCount,
+                BytesInRes = icoEntry.dwBytesInRes,
+                IconCursorId = (ushort)i
+            };
+        }
+
+        return resDir;
+    }
+
     // Initializes this Image object. This is identical to calling the image's
     // constructor with picture, but this allows non-constructor initialization,
     // which may be necessary in some instances.
@@ -408,155 +440,48 @@ private void Initialize(int width, int height)
             throw new InvalidOperationException(SR.Format(SR.IllegalState, GetType().Name));
         }
 
-        SpanReader<byte> reader = new(_iconData);
-        if (!reader.TryRead(out ICONDIR dir)
-            || dir.idReserved != 0
-            || dir.idType != 1
-            || dir.idCount == 0)
-        {
-            throw new ArgumentException(SR.Format(SR.InvalidPictureType, "picture", nameof(Icon)));
-        }
+        var reader = new SpanReader<byte>(_iconData);
 
-        // Get the correct width and height.
-        if (width == 0)
+        if (!reader.TryRead(out ICONDIR icoDir) || icoDir.idReserved != 0 || icoDir.idType != 1 || icoDir.idCount < 1)
         {
-            width = PInvokeCore.GetSystemMetrics(SYSTEM_METRICS_INDEX.SM_CXICON);
-        }
-
-        if (height == 0)
-        {
-            height = PInvokeCore.GetSystemMetrics(SYSTEM_METRICS_INDEX.SM_CYICON);
-        }
-
-        if (s_bitDepth == 0)
-        {
-            using var hdc = GetDcScope.ScreenDC;
-            s_bitDepth = PInvokeCore.GetDeviceCaps(hdc, GET_DEVICE_CAPS_INDEX.BITSPIXEL);
-            s_bitDepth *= PInvokeCore.GetDeviceCaps(hdc, GET_DEVICE_CAPS_INDEX.PLANES);
-
-            // If the bit depth is 8, make it 4 because windows does not
-            // choose a 256 color icon if the display is running in 256 color mode
-            // due to palette flicker.
-            if (s_bitDepth == 8)
-            {
-                s_bitDepth = 4;
-            }
+            throw new ArgumentException(SR.Format(SR.InvalidPictureType, "picture", nameof(Icon)));
         }
 
-        byte bestWidth = 0;
-        byte bestHeight = 0;
-
-        if (!reader.TryRead(dir.idCount, out ReadOnlySpan<ICONDIRENTRY> entries))
+        if (!reader.TryRead(icoDir.idCount, out ReadOnlySpan<ICONDIRENTRY> icoEntries))
         {
             throw new ArgumentException(SR.Format(SR.InvalidPictureType, "picture", nameof(Icon)));
         }
 
-        foreach (ICONDIRENTRY entry in entries)
-        {
-            bool fUpdateBestFit = false;
-            uint iconBitDepth;
-            if (entry.bColorCount != 0)
-            {
-                iconBitDepth = 4;
-                if (entry.bColorCount < 0x10)
-                {
-                    iconBitDepth = 1;
-                }
-            }
-            else
-            {
-                iconBitDepth = entry.wBitCount;
-            }
+        // Prepare fake RT_GROUP_ICON resource for LookupIconIdFromDirectoryEx call.
+        var resDir = CreateIconResourceDirectory(_iconData);
 
-            // If it looks like if nothing is specified at this point then set the bits per pixel to 8.
-            if (iconBitDepth == 0)
-            {
-                iconBitDepth = 8;
-            }
-
-            // Windows rules for specifying an icon:
-            //
-            //  1. The icon with the closest size match.
-            //  2. For matching sizes, the image with the closest bit depth.
-            //  3. If there is no color depth match, the icon with the closest color depth that does not exceed the display.
-            //  4. If all icon color depth > display, lowest color depth is chosen.
-            //  5. color depth of > 8bpp are all equal.
-            //  6. Never choose an 8bpp icon on an 8bpp system.
-
-            if (_bestBytesInRes == 0)
-            {
-                fUpdateBestFit = true;
-            }
-            else
-            {
-                int bestDelta = Math.Abs(bestWidth - width) + Math.Abs(bestHeight - height);
-                int thisDelta = Math.Abs(entry.bWidth - width) + Math.Abs(entry.bHeight - height);
-
-                if ((thisDelta < bestDelta)
-                    || (thisDelta == bestDelta
-                        && ((iconBitDepth <= s_bitDepth && iconBitDepth > _bestBitDepth)
-                            || (_bestBitDepth > s_bitDepth && iconBitDepth < _bestBitDepth))))
-                {
-                    fUpdateBestFit = true;
-                }
-            }
-
-            if (fUpdateBestFit)
-            {
-                bestWidth = entry.bWidth;
-                bestHeight = entry.bHeight;
-                _bestImageOffset = entry.dwImageOffset;
-                _bestBytesInRes = entry.dwBytesInRes;
-                _bestBitDepth = iconBitDepth;
-            }
-        }
-
-        if (_bestImageOffset > int.MaxValue)
+        int id = 0;
+        fixed (byte* b = resDir)
         {
-            throw new ArgumentException(SR.Format(SR.InvalidPictureType, "picture", nameof(Icon)));
+            id = PInvoke.LookupIconIdFromDirectoryEx(b, fIcon: true, width, height, IMAGE_FLAGS.LR_DEFAULTSIZE);
         }
 
-        if (_bestBytesInRes > int.MaxValue)
-        {
-            throw new Win32Exception((int)WIN32_ERROR.ERROR_INVALID_PARAMETER);
-        }
+        var entry = icoEntries[id];
+        _bestImageOffset = entry.dwImageOffset;
+        _bestBytesInRes = entry.dwBytesInRes;
+        _bestBitDepth = entry.bColorCount != 0 ? (entry.bColorCount < 16 ? 1u : 4u)
+                                   : (entry.wBitCount > 0 ? entry.wBitCount : 8u);
 
-        uint endOffset;
-        try
-        {
-            endOffset = checked(_bestImageOffset + _bestBytesInRes);
-        }
-        catch (OverflowException)
+        if (_bestBytesInRes > int.MaxValue)
         {
             throw new Win32Exception((int)WIN32_ERROR.ERROR_INVALID_PARAMETER);
         }
 
-        if (endOffset > _iconData.Length)
+        if (checked(_bestImageOffset + _bestBytesInRes) > _iconData.Length)
         {
             throw new ArgumentException(SR.Format(SR.InvalidPictureType, "picture", nameof(Icon)));
         }
 
-        // Copy the bytes into an aligned buffer if needed.
-
-        ReadOnlySpan<byte> bestImage = reader.Span.Slice((int)_bestImageOffset, (int)_bestBytesInRes);
-
-        if ((_bestImageOffset % sizeof(nint)) != 0)
+        // CreateIconFromResourceEx expects an aligned RT_ICON resource buffer.
+        // Just copy best image bytes into a new aligned buffer.
+        fixed (byte* b = _iconData.AsSpan().Slice((int)_bestImageOffset, (int)_bestBytesInRes).ToArray())
         {
-            // Beginning of icon's content is misaligned.
-            using BufferScope<byte> alignedBuffer = new((int)_bestBytesInRes);
-            bestImage.CopyTo(alignedBuffer.AsSpan());
-
-            fixed (byte* b = alignedBuffer)
-            {
-                _handle = PInvoke.CreateIconFromResourceEx(b, (uint)bestImage.Length, fIcon: true, 0x00030000, 0, 0, 0);
-            }
-        }
-        else
-        {
-            fixed (byte* b = bestImage)
-            {
-                _handle = PInvoke.CreateIconFromResourceEx(b, (uint)bestImage.Length, fIcon: true, 0x00030000, 0, 0, 0);
-            }
+            _handle = PInvoke.CreateIconFromResourceEx(b, _bestBytesInRes, fIcon: true, 0x00030000, 0, 0, IMAGE_FLAGS.LR_DEFAULTCOLOR);
         }
 
         if (_handle.IsNull)

src/System.Drawing.Common/tests/System/Drawing/IconTests.cs

@@ -24,7 +24,6 @@
 
 using System.ComponentModel;
 using System.Drawing.Imaging;
-using System.Reflection;
 using Microsoft.DotNet.RemoteExecutor;
 
 namespace System.Drawing.Tests;
@@ -33,7 +32,6 @@ public class IconTests
 {
     [Theory]
     [InlineData("48x48_multiple_entries_4bit.ico")]
-    [InlineData("256x256_seven_entries_multiple_bits.ico")]
     [InlineData("pngwithheight_icon.ico")]
     public void Ctor_FilePath(string name)
     {
@@ -50,8 +48,8 @@ public static IEnumerable<object[]> Size_TestData()
         yield return new object[] { "48x48_multiple_entries_4bit.ico", new Size(-32, -32), new Size(16, 16) };
         yield return new object[] { "48x48_multiple_entries_4bit.ico", new Size(32, 16), new Size(32, 32) };
         yield return new object[] { "256x256_seven_entries_multiple_bits.ico", new Size(48, 48), new Size(48, 48) };
-        yield return new object[] { "256x256_seven_entries_multiple_bits.ico", new Size(0, 0), new Size(32, 32) };
-        yield return new object[] { "256x256_seven_entries_multiple_bits.ico", new Size(1, 1), new Size(256, 256) };
+        yield return new object[] { "256x256_seven_entries_multiple_bits.ico", new Size(0, 0), new Size(64, 32) };
+        yield return new object[] { "256x256_seven_entries_multiple_bits.ico", new Size(1, 1), new Size(16, 16) };
 
         // Unusual size
         yield return new object[] { "10x16_one_entry_32bit.ico", new Size(16, 16), new Size(10, 16) };
@@ -753,10 +751,7 @@ public void CorrectColorDepthExtracted()
         using Bitmap bitmap = icon.ToBitmap();
         Assert.Equal(new Size(32, 32), bitmap.Size);
 
-        int expectedBitDepth;
-        string fieldName = PlatformDetection.IsNetFramework ? "bitDepth" : "s_bitDepth";
-        FieldInfo fi = typeof(Icon).GetField(fieldName, BindingFlags.Static | BindingFlags.NonPublic);
-        expectedBitDepth = (int)fi.GetValue(null);
+        int expectedBitDepth = Windows.Forms.Screen.PrimaryScreen.BitsPerPixel;
 
         // If the first icon entry was picked, the color would be black: 0xFF000000?
 

src/System.Drawing.Common/tests/System/Drawing/ImageConverterTests.cs

@@ -31,7 +31,6 @@ public ImageConverterTest()
 
     [Theory]
     [InlineData("48x48_multiple_entries_4bit.ico")]
-    [InlineData("256x256_seven_entries_multiple_bits.ico")]
     [InlineData("pngwithheight_icon.ico")]
     public void ImageConverterFromIconTest(string name)
     {

src/System.Private.Windows.Core/src/Windows/Win32/UI/WindowsAndMessaging/GRPICONDIR.cs

@@ -0,0 +1,18 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Runtime.InteropServices;
+
+namespace Windows.Win32.UI.WindowsAndMessaging;
+
+// https://devblogs.microsoft.com/oldnewthing/?p=7083
+// https://devblogs.microsoft.com/oldnewthing/?p=108925
+// https://learn.microsoft.com/en-us/windows/win32/menurc/newheader
+
+[StructLayout(LayoutKind.Sequential, Pack = 2)]
+internal struct GRPICONDIR
+{
+    public ushort Reserved;
+    public ushort ResType;
+    public ushort ResCount;
+}

src/System.Private.Windows.Core/src/Windows/Win32/UI/WindowsAndMessaging/GRPICONDIRENTRY.cs

@@ -0,0 +1,23 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Runtime.InteropServices;
+
+namespace Windows.Win32.UI.WindowsAndMessaging;
+
+// https://devblogs.microsoft.com/oldnewthing/?p=7083
+// https://devblogs.microsoft.com/oldnewthing/?p=108925
+// https://learn.microsoft.com/en-us/windows/win32/menurc/resdir
+
+[StructLayout(LayoutKind.Sequential, Pack = 2)]
+internal struct GRPICONDIRENTRY
+{
+    public byte Width;
+    public byte Height;
+    public byte ColorCount;
+    public byte Reserved;
+    public ushort Planes;
+    public ushort BitCount;
+    public uint BytesInRes;
+    public ushort IconCursorId;
+}