Fix ToolboxBitmapAttribute to improve error tolerance and better support PNG based ICO embedded resources

[elachlan]

Changes to ToolboxBitmapAttribute were to avoid errors when a bitmap was expected but the icon file was a png based icon. The try catch method was used elsewhere in the class to avoid similar errors.

Used the method outlined in the linked issue and wrote the following python script to automate:

import os
import subprocess
import glob

# Get a list of all .ico files in the current directory
ico_files = [f for f in os.listdir('.') if f.endswith('.ico')]

# Process each .ico file
for ico_file in ico_files:
    try:
        # Convert the .ico file to .png files
        base_name = ico_file.replace('.ico', '')
        subprocess.run(['magick', ico_file, f'{base_name}-%d.png'], check=True)

        # Get the list of created .png files
        png_files = glob.glob(f'{base_name}-*.png')

        if not png_files:
            print(f"No .png files were created for {ico_file}. Skipping.")
            continue

        # Overwrite the original .ico file with the new one
        icotool_command = ['icotool', '-c', '-o', ico_file] + [f'-r{png}' for png in png_files]
        subprocess.run(icotool_command, check=True)

        # Clean up the temporary .png files
        for png in png_files:
            if os.path.isfile(png):
                os.remove(png)

    except subprocess.CalledProcessError:
        print(f"Error processing {ico_file}. Skipping.")
    except Exception as e:
        print(f"Unexpected error processing {ico_file}: {e}. Skipping.")

print("Processing complete.")

Microsoft Reviewers: Open in CodeFlow

[elachlan]

@MichalStrehovsky here is a PR with all the ico files converted. It resulted in errors in ToolboxBitmapAttribute, this pr just adds try catch to swallow those as there was a fallback mechanism already in place. But I suspect we can improve the code to check the file header to check its format to avoid the exception.

Winforms team might be able to help there.

[codecov]

Codecov Report

Attention: Patch coverage is 82.60870% with 12 lines in your changes missing coverage. Please review.

Project coverage is 74.36247%. Comparing base (0aa3a4d) to head (3da25ab).
Report is 57 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11375         +/-   ##
===================================================
+ Coverage   74.26256%   74.36247%   +0.09991%     
===================================================
  Files           3025        3028          +3     
  Lines         626861      627537        +676     
  Branches       46742       46765         +23     
===================================================
+ Hits          465523      466652       +1129     
+ Misses        157993      157531        -462     
- Partials        3345        3354          +9     

Flag	Coverage Δ
Debug	74.36247% <82.60870%> (+0.09991%)	⬆️
integration	17.98035% <0.00000%> (-0.01558%)	⬇️
production	47.16010% <82.60870%> (+0.17419%)	⬆️
test	96.98684% <ø> (ø)
unit	44.14017% <82.60870%> (+0.17398%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[elachlan]

@merriemcgaw Are embedded resources considered a part of the public API? Would a change like this be a breaking change?

[RussKie]

Are embedded resources considered a part of the public API? Would a change like this be a breaking change?

No, it shouldn't be considered a breaking change.
These may be helpful:

• https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-definitions.md
• https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-changes.md

[MichalStrehovsky]

It resulted in errors in ToolboxBitmapAttribute, this pr just adds try catch to swallow those as there was a fallback mechanism already in place. But I suspect we can improve the code to check the file header to check its format to avoid the exception.

What were the errors? It feels like the code this is touching should already handle PNG. Icon.ToBitmap used to throw but support for this was added long time ago (there was even a breaking change notice because the compat bar on .NET Framework is so high that even "something throwing a non-sensical exception" to "something working" is a breaking change, but fortunately we don't have to worry about that here).

[elachlan]

Prior to this change, we would hit an ArgumentException here bubbled up from trying to create a bitmap from the png icon.

[elachlan]

Ultimately we should probably check the type via the stream header and then pass it to the correct function.

[MichalStrehovsky]

Do you have the stack around you could paste here?

I tried this in a small scratch project with one of the icons in this PR and it doesn't throw so I wonder how this could be caused by changing the icon format.

using System.Diagnostics.CodeAnalysis;
using System;
using System.Drawing;

Icon icon = new Icon(@"C:\Users\michals\Downloads\ScrollButtonUp.ico");
Icon sized = new Icon(icon, new Size(32, 32));

var b = sized.ToBitmap();
b = DpiHelper.ScaleBitmapToSize(b, new Size(64, 64));

b.Save(@"C:\Users\michals\Downloads\ScrollButtonUp.bmp");

internal static class DpiHelper
{
    public static Bitmap ScaleBitmapToSize(Bitmap logicalImage, Size deviceImageSize)
    {
        Bitmap deviceImage = new(deviceImageSize.Width, deviceImageSize.Height, logicalImage.PixelFormat);

        using var graphics = Graphics.FromImage(deviceImage);

        graphics.InterpolationMode = System.Drawing.Drawing2D.InterpolationMode.Bilinear;

        RectangleF sourceRect = new(0, 0, logicalImage.Size.Width, logicalImage.Size.Height);
        RectangleF destRect = new(0, 0, deviceImageSize.Width, deviceImageSize.Height);

        // Specify a source rectangle shifted by half of pixel to account for GDI+ considering the source origin the center of top-left pixel
        // Failing to do so will result in the right and bottom of the bitmap lines being interpolated with the graphics' background color,
        // and will appear black even if we cleared the background with transparent color.
        // The apparition of these artifacts depends on the interpolation mode, on the dpi scaling factor, etc.
        // E.g. at 150% DPI, Bicubic produces them and NearestNeighbor is fine, but at 200% DPI NearestNeighbor also shows them.
        sourceRect.Offset(-0.5f, -0.5f);

        graphics.DrawImage(logicalImage, destRect, sourceRect, GraphicsUnit.Pixel);

        return deviceImage;
    }

    /// <summary>
    /// Create and return a new bitmap scaled to the specified size.
    /// Note: this method should be called only inside an if (DpiHelper.IsScalingRequired) clause
    /// </summary>
    /// <param name="logicalImage">The image to scale from logical units to device units</param>
    /// <param name="targetImageSize">The size to scale image to</param>
    [return: NotNullIfNotNull(nameof(logicalImage))]
    public static Bitmap? CreateResizedBitmap(Bitmap? logicalImage, Size targetImageSize)
    {
        if (logicalImage is null)
        {
            return null;
        }

        return ScaleBitmapToSize(logicalImage, targetImageSize);
    }
}

[elachlan]

This exception was originally thrown at this call stack:
 	System.Private.Windows.Core.dll!Windows.Win32.Graphics.GdiPlus.StatusExtensions.ThrowIfFailed(Windows.Win32.Graphics.GdiPlus.Status status) Line 15	C#
	System.Drawing.Common.dll!System.Drawing.Bitmap.Bitmap(System.IO.Stream stream, bool useIcm) Line 63	C#
 	System.Drawing.Common.dll!System.Drawing.Bitmap.Bitmap(System.IO.Stream stream) Line 50	C#
 	System.Drawing.Common.dll!System.Drawing.ToolboxBitmapAttribute.GetBitmapFromResource(System.Type t, string bitmapname, bool large, bool scaled) Line 191	C#
 	System.Drawing.Common.dll!System.Drawing.ToolboxBitmapAttribute.GetImageFromResource(System.Type t, string imageName, bool large, bool scaled) Line 275	C#
 	System.Drawing.Common.dll!System.Drawing.ToolboxBitmapAttribute.GetImageFromResource(System.Type t, string imageName, bool large) Line 222	C#
 	System.Drawing.Common.dll!System.Drawing.ToolboxBitmapAttribute.GetImage(System.Type type, string imgName, bool large) Line 79	C#
 	System.Drawing.Common.dll!System.Drawing.ToolboxBitmapAttribute.GetImage(System.Type type, bool large) Line 72	C#
 	System.Drawing.Common.dll!System.Drawing.ToolboxBitmapAttribute.GetImage(object component, bool large) Line 64	C#
 	System.Windows.Forms.Design.dll!System.Windows.Forms.Design.ComponentTray.TrayControl.UpdateIconInfo() Line 2410	C#
 	System.Windows.Forms.Design.dll!System.Windows.Forms.Design.ComponentTray.TrayControl.TrayControl(System.Windows.Forms.Design.ComponentTray tray, System.ComponentModel.IComponent component) Line 1902	C#
 	System.Windows.Forms.Design.dll!System.Windows.Forms.Design.ComponentTray.AddComponent(System.ComponentModel.IComponent component) Line 699	C#
 	System.Windows.Forms.Design.dll!System.Windows.Forms.Design.DocumentDesigner.OnComponentAdded(object source, System.ComponentModel.Design.ComponentEventArgs ce) Line 941	C#
 	System.Windows.Forms.Design.dll!System.ComponentModel.Design.DesignerHost.AddToContainerPostProcess(System.ComponentModel.IComponent component, System.ComponentModel.IContainer containerToAddTo) Line 296	C#
 	System.Windows.Forms.Design.dll!System.ComponentModel.Design.DesignerHost.PerformAdd(System.ComponentModel.IComponent component, string name) Line 156	C#
 	System.Windows.Forms.Design.dll!System.ComponentModel.Design.DesignerHost.System.ComponentModel.Design.IDesignerHost.CreateComponent(System.Type componentType, string name) Line 971	C#
 	System.Windows.Forms.Design.dll!System.ComponentModel.Design.DesignerHost.System.ComponentModel.Design.IDesignerHost.CreateComponent(System.Type componentType) Line 924	C#
 	DesignSurfaceExt.dll!DesignSurfaceExt.DesignSurfaceExt.CreateComponent<System.Windows.Forms.ContextMenuStrip>(out System.ComponentModel.Design.IDesignerHost host) Line 208	C#
 	DesignSurfaceExt.dll!DesignSurfaceExt.DesignSurfaceExt.CreateComponent<System.Windows.Forms.ContextMenuStrip>() Line 197	C#
 	DesignSurface.dll!TestConsole.MainForm.CreateDesignSurface(int n) Line 154	C#
 	DesignSurface.dll!TestConsole.MainForm.InitFormDesigner() Line 23	C#
 	DesignSurface.dll!TestConsole.MainForm.MainForm_Load(object sender, System.EventArgs e) Line 363	C#

[elachlan]

They use different PInvokes, so I guess that is why.

winforms/src/System.Drawing.Common/src/System/Drawing/Bitmap.cs

Lines 54 to 68 in 74caf32

	public Bitmap(Stream stream, bool useIcm)
	{
	ArgumentNullException.ThrowIfNull(stream);
	using var iStream = stream.ToIStream(makeSeekable: true);
	GpBitmap* bitmap = null;
	Status status = useIcm
	? PInvoke.GdipCreateBitmapFromStreamICM(iStream, &bitmap)
	: PInvoke.GdipCreateBitmapFromStream(iStream, &bitmap);
	status.ThrowIfFailed();
	ValidateImage((GpImage*)bitmap);
	SetNativeImage((GpImage*)bitmap);
	GetAnimatedGifRawData(this, filename: null, stream);
	}

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

Lines 117 to 124 in 74caf32

	public Icon(Stream stream, int width, int height) : this()
	{
	ArgumentNullException.ThrowIfNull(stream);
	_iconData = new byte[(int)stream.Length];
	stream.ReadExactly(_iconData);
	Initialize(width, height);
	}

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

Lines 545 to 562 in 74caf32

	if ((_bestImageOffset % sizeof(nint)) != 0)
	{
	// 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);
	}
	}

[MichalStrehovsky]

Oh, so because these embedded resources don't end in .ico, we take the codepath that tries to create a Bitmap for this, but one cannot create a Bitmap for a PNG icon for whatever reason.

But are both of these try/catches needed? The one I was most puzzled about was the GetIconFromStream (that's the one I extracted into the standalone app and didn't see a problem).

[elachlan]

I was being overly safe there I think.

[elachlan]

@LeafShi1 / @Olina-Zhang can your team please test this? The main test would be to open the new icon and compare it to the old icon. Also if you can trigger the CI to run the tests again that would be great.

[LeafShi1]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[lonitra]

Thanks for taking this on @elachlan! I did a look through and compared the icons before/after change and AFAICT visually the icons look the same. I did, however, spot some differences when comparing sizes and perhaps some of them are expected:

1. There were some icons which had a change in only the bit size e.g. originally 32 bits -> 4 bits. Is this expected? I saw this for icons: colorDialog.ico, RichEdit.ico, RichTextBox.ico, ShieldIcon.ico, Close_left.ico, Open_left.ico, DummyNodeImage.ico
2. Some icons had extra sizes that the original did not have e.g. 148x148 did not exist, but now does. I saw this for icons: 256_1.ico and 256_2.ico
3. Some icons had completely different sizes from the original e.g. original was 33x61 but new is 33x33. I saw this for icons: overflowButton.ico, ToolBarGrip.ico, OrderImages.ico

[elachlan]

@lonitra thank you so much for reviewing these. How are you checking the bit depth and sizing?

[lonitra]

@lonitra thank you so much for reviewing these. How are you checking the bit depth and sizing?

I had opened the icons in Visual Studio and there was a bar on the side that showed the sizing and bits

[elachlan]

Thanks! When I run the ImageMagick Identify command I get different results. It says RichEdit.ico is strangley 8bit before and after the conversion.

here is what it looks like from my VS
Before:

After:

VS seems to have issues displaying the original?

256_1.ico is strange, 148x148 and 47x47 look corrupted when viewed in VS for me. The meta data exists in the original ico file, but with zero file size. 202x123 has a zero size but somehow works in the original and converted versions.

Before:

256_1.ico[0] PNG 505x308 505x308+0+0 8-bit sRGB 13030B 0.000u 0:00.009
256_1.ico[1] ICO 202x123 202x123+0+0 8-bit sRGB 0.000u 0:00.009
256_1.ico[2] PNG 707x431 707x431+0+0 8-bit sRGB 16711B 0.000u 0:00.005
256_1.ico[3] ICO 148x148 148x148+0+0 8-bit sRGB 0.000u 0:00.004
256_1.ico[4] PNG 606x370 606x370+0+0 8-bit sRGB 13120B 0.000u 0:00.002
256_1.ico[5] ICO 47x47 47x47+0+0 8-bit sRGB 0.000u 0:00.002
256_1.ico[6] PNG 808x492 808x492+0+0 8-bit sRGB 805702B 0.000u 0:00.000

After:

256_1.ico[0] PNG 505x308 505x308+0+0 8-bit sRGB 7339B 0.000u 0:00.004
256_1.ico[1] PNG 202x123 202x123+0+0 8-bit sRGB 2277B 0.000u 0:00.003
256_1.ico[2] PNG 707x431 707x431+0+0 8-bit sRGB 10520B 0.000u 0:00.003
256_1.ico[3] PNG 148x148 148x148+0+0 8-bit sRGB 1466B 0.000u 0:00.002
256_1.ico[4] PNG 606x370 606x370+0+0 8-bit sRGB 7745B 0.000u 0:00.001
256_1.ico[5] PNG 47x47 47x47+0+0 8-bit sRGB 449B 0.000u 0:00.001
256_1.ico[6] PNG 808x492 808x492+0+0 8-bit sRGB 40338B 0.000u 0:00.000

If I use Icon tool on the orginal:

--icon --index=1 --width=505 --height=308 --bit-depth=32 --palette-size=0
--icon --index=2 --width=202 --height=123 --bit-depth=32 --palette-size=0
--icon --index=3 --width=707 --height=431 --bit-depth=32 --palette-size=0
--icon --index=4 --width=404 --height=246 --bit-depth=32 --palette-size=0
--icon --index=5 --width=606 --height=370 --bit-depth=32 --palette-size=0
--icon --index=6 --width=303 --height=185 --bit-depth=32 --palette-size=0
--icon --index=7 --width=808 --height=492 --bit-depth=32 --palette-size=0

Edit:
ImageMagick is misinterpreting the resolution, which is causing issues.

I'll do some more work on identifying making the conversion script more error tolerant and add some verification using icontool.

[elachlan]

I've raised an ImageMagick issue at: ImageMagick/ImageMagick#7341

[elachlan]

icotool can extract some of the icons, but errors on a few. I'll put together a new script and push through the changes when I have it sorted.

[Olina-Zhang]

Hi @elachlan, we tested this PR by checking all ico files in your repo: Icon-Resize branch and Winforms repo: Main branch to compare them, found following result:

1. Some icons in your branch display more clearly than Winforms: main branch in Visual Studio, see following screenshot(they are not all we collected, just a part)
   
2. Some icons Size is changed(@lonitra mentioned in above)
3. Some icons Bit is changed(@lonitra mentioned in above)
4. Some icons display are changed with extra pixels around icons, see following screenshot(they are not all we collected, just a part)
   
5. An exception pops up when adding ToolStrip/MenuStrip/StatusStrip/ContextMenuStrip control from toolBox to form designer
   
   Log:
   

[elachlan]

@Olina-Zhang I think the designer issue will need to be looked at by the team. I don't have access to the source code for the designer.

ImageMagick are fixing one of the issues which should resolve some of the sizing corruption.

[elachlan]

@merriemcgaw changing the underlying BMP format to PNG in the ICO files is a breaking change if a user is creating a bitmap directly instead of first creating an Icon. Are we sure that changing resources is not a breaking change?

[Olina-Zhang]

@Olina-Zhang I think the designer issue will need to be looked at by the team. I don't have access to the source code for the designer.

If we didn't replace the binaries built from your PR, that issue doesn't repro.

[elachlan]

ImageMagick fixed the sizing issue. So now the icons have appropriate sizes.

The bit depth is changed on purpose by ImageMagick to save space when there are limited colors in an image.

The artifacts we are seeing look like they are caused by icotool. I am investigating how to use ImageMagick to combine the png files to the ico without converting back to bmp.

[elachlan]

Looks like ImageMagick might not support PNG in ICO files because of specification reasons:
ImageMagick/ImageMagick#2394

@MichalStrehovsky could you live with bitmap based ICO files?
Bitmap: ToolStripDropDown.ico 54kb->22kb
PNG: 54kb->5kb

[MichalStrehovsky]

Looks like ImageMagick might not support PNG in ICO files because of specification reasons: ImageMagick/ImageMagick#2394

@MichalStrehovsky could you live with bitmap based ICO files? Bitmap: ToolStripDropDown.ico 54kb->22kb PNG: 54kb->5kb

In a blank WinForms app, we currently have 48 icons that take over 51 kB each (plus some smaller ones I'm not counting). That's 2.4 MB. A blank WinForms app with AOT and a couple size savings options is 9.2 MB. So these icons are more than 25% of the blank app. If we make them 15% of the blank app, it's certainly an improvement. Still feels a bit excessive. In other places (ASP.NET, Blazor, etc.) we jump on and try to fix anything above 1%.

First thing I'm getting from this is that the VS icon designer cannot be trusted (it shows various artifacts in both before and after versions). I'd ignore the VS icon designer completely and focus on differences that matter - e.g. load the icons as a .NET Icon and save everything as a Bitmap. Compare the Bitmaps. Then look if we can fix the bug where someone tries to open an icon as a Bitmap (the bug you're working around in the code changes here) - can Bitmap detect trying to open an icon and instead open it as Icon and call ToBitmap on it (i.e. move the workaround from there to Bitmap - I assume that would fix the designer issue)?

If the above fails or we don't want to go in that direction - can we instead get the icons out of the app? Then it doesn't matter how big they are. E.g. are these Designer icons? Could they be moved to the Designer assembly? Or could we instruct trimming to drop them based on some condition? What are they used for?

15% leaves a lot on the table. Last week I converted one of my WinForms apps to Native AOT. It's a fully functional and useful app. These icons would still constitute 10% of the app even if we cut their size in half because the whole app size was a little over 10 MB.

[MichalStrehovsky]

Thinking about it more, even in the best case 4+% of the executable size would be icons. It might be the best to look for ways to trim these out of the app. A 4% size saving is still pretty interesting. Assuming these are actually not normally needed - understanding what they are used for would be the best.

[elachlan]

@Olina-Zhang could your team please report the VS artifact issue?

I'll whip up a quick program to do the icon to bitmap conversion and see what before/after renders out to.

With regards to trimming out resources, I am unsure how the trimmer works or how the compiler handles resources. Someone much more skilled them myself would need to investigate that.

[Olina-Zhang]

@Olina-Zhang could your team please report the VS artifact issue?

You mean this problem I listed: An exception pops up when adding ToolStrip/MenuStrip/StatusStrip/ContextMenuStrip control from toolBox to form designer? I'm not quite sure what you mean. Do you request a details about how to repro it or others?

[elachlan]

Sorry about that. Can you please report the issue when viewing the existing icons in visual studio? Visual Studio should display the icons properly.

[Olina-Zhang]

You mean open a Winforms designer bug in Winforms designer repo? That issue just reproduces when replaced these three binaries: System.Drawing.Common.dll, System.Windows.Forms.Design.dll and System.Windows.Forms.dll built from this PR, I am not sure if our testing is a valid validation.

[elachlan]

The icon display issue in visual studio. Not the exception. I am unsure if winforms designer handles viewing/editing icon files.

[Olina-Zhang]

Haha, I see the problem you're referring to. I will open an issue for Visual Studio.

[Olina-Zhang]

Opened an internal VS artifact issue: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2067266.

[MichalStrehovsky]

With regards to trimming out resources, I am unsure how the trimmer works or how the compiler handles resources. Someone much more skilled them myself would need to investigate that.

A couple options, not sure which one is applicable.

• The compiler doesn't analyze which embedded resources are needed, it will keep them all if an assembly is needed. This PR is changing all ico files in the repo. We mostly care about those that end up in System.Windows.Forms. Can some be moved to System.Windows.Forms.Design (normal apps don't reference that assembly)? What are the icons used for? What if we scope this PR down to just the icons from System.Windows.Forms?
• The compiler can accept a list of resource names to trim and then it will trim them, even if the assembly is used. This trimming can be made conditional based on some property. It again begs the question of what are the icons in System.Windows.Forma assembly used for - is there a theme - a feature that might not be needed by everyone?

[elachlan]

Did another quick test, these artifacts are VS specific and don't show if using a browser to display the ico file:

Additionally, if I convert the ico to bmp via .NET and open in VS it displays fine:

[KlausLoeffelmann]

@merriemcgaw changing the underlying BMP format to PNG in the ICO files is a breaking change if a user is creating a bitmap directly instead of first creating an Icon. Are we sure that changing resources is not a breaking change?

I am concerned, to be honest.
@merriemcgaw.

[elachlan]

@KlausLoeffelmann I think we can adjust Bitmap to check the streams leading bytes to establish what file type it is then create the bitmap accordingly.

[KlausLoeffelmann]

I am wondering:
What's the deal in general renaming variable name in something not abbreviated, which get not flagged by the spell checker? Are we changing this in the runtime in the context of touching the code? As an extra PR?

[elachlan]

I've just included it here as a separate commit. As long as its not squashed it should be okay.