[llvm-rc] Read the Planes/BitCount fields from BITMAPINFOHEADER for icons

[mstorsjo]

Summary

Previously these fields were only read from this header for cursors, while Planes was hardcoded to 1 for icons (with a comment that it was unknown why this was needed) and BitCount was left at the value read originally in the RESDIRENTRY.

This fixes the single byte that was differing for the icon/cursor test compared to rc.exe.

This is based on research/testing by Nico Weber.

[amccarth]

tools/llvm-rc/ResourceFileWriter.cpp

929 ↗

(On Diff #146542)

Most icons also store two bitmaps (the mask and the "color"). I wonder why this adjustment is needed only for cursors.

950 ↗

(On Diff #146542)

Is there a more-specific test to make sure this is a PNG icon and nut just a corrupted file? For example, BMPHeader->biCompression should have a special value for PNG.

[zturner]

tools/llvm-rc/ResourceFileWriter.cpp

945–946 ↗

(On Diff #146542)

How about just sizeof(BitmapInfoHeader)?

[mstorsjo]

tools/llvm-rc/ResourceFileWriter.cpp

929 ↗

(On Diff #146542)

No idea about the "why" here...

945–946 ↗

(On Diff #146542)

Sure, that probably works.

950 ↗

(On Diff #146542)

For png, this actually isn't a bmpheader at all, but the png header. I could check if the size field (the first field of the struct) matches the png magic interpreted as ulittle32_t.

[thakis]

lgtm, but please credit where at least parts of this are from :-)

[mstorsjo]

Changed to check specifically for the PNG magic header, giving credit where it's due in the commit message :-)

[amccarth]

tools/llvm-rc/ResourceFileWriter.cpp

950 ↗

(On Diff #146542)

There are three possibilities: BMP, PNG, or the wrong type of file. The original change tested to see if it was a valid BMP and assumed PNG otherwise, discounting the possibility that it could be neither. This just flips that around.

If we're going to ignore the possibility of a corrupt/incorrect input, then the original code was clearer, since it was actually testing the size.

[mstorsjo]

Back to the original form of the check, but using sizeof(BitmapInfoHeader) instead of a separate constant.

llvm/trunk/test/tools/llvm-rc/tag-icon-cursor.test

 ; CHECK-NEXT: Data version: 0
 ; CHECK-NEXT: Memory flags: 0x1030
 ; CHECK-NEXT: Language ID: 1033
 ; CHECK-NEXT: Version (major): 0
 ; CHECK-NEXT: Version (minor): 0
 ; CHECK-NEXT: Characteristics: 0
 ; CHECK-NEXT: Data size: 62
 ; CHECK-NEXT: Data: (
-; CHECK-NEXT:   0000: 00000100 04001010 00000100 00006804  |..............h.|
+; CHECK-NEXT:   0000: 00000100 04001010 00000100 20006804  |............ .h.|
 ; CHECK-NEXT:   0010: 00000300 18180000 01002000 88090000  |.......... .....|
 ; CHECK-NEXT:   0020: 04002020 00000100 2000A810 00000500  |..  .... .......|
 ; CHECK-NEXT:   0030: 30300000 01002000 A8250000 0600      |00.... ..%....|
 ; CHECK-NEXT: )
 
 ; CHECK-DAG:  Resource type (int): 1
 ; CHECK-NEXT: Resource name (int): 7
 ; CHECK-NEXT: Data version: 0

llvm/trunk/tools/llvm-rc/ResourceFileWriter.cpp

for (size_t ID = 0; ID < NumItems; ++ID) {
     RETURN_IF_ERROR(Reader.readArray(Image, ItemEntries[ID].Size));
     SingleIconCursorResource SingleRes(Type, ItemEntries[ID], Image);
     SingleRes.setName(IconCursorID + ID);
     RETURN_IF_ERROR(visitSingleIconOrCursor(&SingleRes));
   }
 
   // Now, write all the headers concatenated into a separate resource.
   for (size_t ID = 0; ID < NumItems; ++ID) {
-    if (Type == IconCursorGroupType::Icon) {
-      // rc.exe seems to always set NumPlanes to 1. No idea why it happens.
-      ItemEntries[ID].Planes = 1;
-      continue;
-    }
-
-    // We need to rewrite the cursor headers.
+    // We need to rewrite the cursor headers, and fetch actual values
+    // for Planes/BitCount.
     const auto &OldHeader = ItemEntries[ID];
-    ResourceDirEntryStart NewHeader;
+    ResourceDirEntryStart NewHeader = OldHeader;
+
+    if (Type == IconCursorGroupType::Cursor) {
       NewHeader.Cursor.Width = OldHeader.Icon.Width;
       // Each cursor in fact stores two bitmaps, one under another.
       // Height provided in cursor definition describes the height of the
       // cursor, whereas the value existing in resource definition describes
       // the height of the bitmap. Therefore, we need to double this height.
       NewHeader.Cursor.Height = OldHeader.Icon.Height * 2;
 
+      // Two WORDs were written at the beginning of the resource (hotspot
+      // location). This is reflected in Size field.
+      NewHeader.Size += 2 * sizeof(uint16_t);
+    }
+
     // Now, we actually need to read the bitmap header to find
     // the number of planes and the number of bits per pixel.
     Reader.setOffset(ItemOffsets[ID]);
     const BitmapInfoHeader *BMPHeader;
     RETURN_IF_ERROR(Reader.readObject(BMPHeader));
+    if (BMPHeader->Size == sizeof(BitmapInfoHeader)) {
       NewHeader.Planes = BMPHeader->Planes;
       NewHeader.BitCount = BMPHeader->BitCount;
-
-    // Two WORDs were written at the beginning of the resource (hotspot
-    // location). This is reflected in Size field.
-    NewHeader.Size = OldHeader.Size + 2 * sizeof(uint16_t);
+    } else {
+      // A PNG .ico file.
+      // https://blogs.msdn.microsoft.com/oldnewthing/20101022-00/?p=12473
+      // "The image must be in 32bpp"
+      NewHeader.Planes = 1;
+      NewHeader.BitCount = 32;
+    }
 
     ItemEntries[ID] = NewHeader;
   }
 
   IconCursorGroupResource HeaderRes(Type, *Header, std::move(ItemEntries));
   HeaderRes.setName(ResName);
   RETURN_IF_ERROR(visitIconOrCursorGroup(&HeaderRes));