Improve the icon parsing for files with a slightly wrong BMP header.

The reason it failed was that we always expected the height property
of the BMP header to be double the height of the icon. The
kde_favicon.ico did not fulfill this requirement.  We can fix that by
simply reading from the ICONDIR entry instead, since that has always
the correct height.

Task-number: 229829
Reviewed-by: alexis

5 files changed, 58 insertions, 32 deletions

diff --git a/src/plugins/imageformats/ico/qicohandler.cpp b/src/plugins/imageformats/ico/qicohandler.cpp
index aa53370..da5ae15 100644
--- a/src/plugins/imageformats/ico/qicohandler.cpp
+++ b/src/plugins/imageformats/ico/qicohandler.cpp
@@ -80,7 +80,7 @@ typedef struct
 typedef struct {                    // BMP information header
     quint32 biSize;                // size of this struct
     quint32 biWidth;               // pixmap width
-    quint32 biHeight;              // pixmap height
+    quint32 biHeight;              // pixmap height     (specifies the combined height of the XOR and AND masks)
     quint16 biPlanes;              // should be 1
     quint16 biBitCount;            // number of bits per pixel
     quint32 biCompression;         // compression method
@@ -108,7 +108,7 @@ private:
     bool readHeader();
     bool readIconEntry(int index, ICONDIRENTRY * iconEntry);
 
-    bool readBMPHeader(ICONDIRENTRY & iconEntry, BMP_INFOHDR * header);
+    bool readBMPHeader(quint32 imageOffset, BMP_INFOHDR * header);
     void findColorInfo(QImage & image);
     void readColorTable(QImage & image);
 
@@ -343,7 +343,7 @@ bool ICOReader::readHeader()
     return headerRead;
 }
 
-bool ICOReader::readIconEntry(int index, ICONDIRENTRY * iconEntry)
+bool ICOReader::readIconEntry(int index, ICONDIRENTRY *iconEntry)
 {
     if (iod) {
         if (iod->seek(startpos + ICONDIR_SIZE + (index * ICONDIRENTRY_SIZE))) {
@@ -355,37 +355,12 @@ bool ICOReader::readIconEntry(int index, ICONDIRENTRY * iconEntry)
 
 
 
-bool ICOReader::readBMPHeader(ICONDIRENTRY & iconEntry, BMP_INFOHDR * header)
+bool ICOReader::readBMPHeader(quint32 imageOffset, BMP_INFOHDR * header)
 {
-    memset(&icoAttrib, 0, sizeof(IcoAttrib));
     if (iod) {
-        if (iod->seek(startpos + iconEntry.dwImageOffset)) {
+        if (iod->seek(startpos + imageOffset)) {
             if (readBMPInfoHeader(iod, header)) {
-
-                icoAttrib.nbits = header->biBitCount ? header->biBitCount : iconEntry.wBitCount;
-                icoAttrib.h = header->biHeight / 2; // this height is always double the iconEntry height (for the mask)
-                icoAttrib.w = header->biWidth;
-
-                switch (icoAttrib.nbits) {
-                case 32:
-                case 24:
-                case 16:
-                    icoAttrib.depth = 32;
-                    break;
-                case 8:
-                case 4:
-                    icoAttrib.depth = 8;
-                    break;
-                default:
-                    icoAttrib.depth = 1;
-                }
-
-                if ( icoAttrib.depth == 32 )                // there's no colormap
-                    icoAttrib.ncolors = 0;
-                else                    // # colors used
-                    icoAttrib.ncolors = header->biClrUsed ? header->biClrUsed : 1 << icoAttrib.nbits;
-                        //qDebug() << "Bits:" << icoAttrib.nbits << "Depth:" << icoAttrib.depth << "Ncols:" << icoAttrib.ncolors;
-                    return TRUE;
+                return TRUE;
             }
         }
     }
@@ -548,7 +523,28 @@ QImage ICOReader::iconAt(int index)
         if (readIconEntry(index, &iconEntry)) {
 
             BMP_INFOHDR header;
-            if (readBMPHeader(iconEntry, &header)) {
+            if (readBMPHeader(iconEntry.dwImageOffset, &header)) {
+                icoAttrib.nbits = header.biBitCount ? header.biBitCount : iconEntry.wBitCount;
+
+                switch (icoAttrib.nbits) {
+                case 32:
+                case 24:
+                case 16:
+                    icoAttrib.depth = 32;
+                    break;
+                case 8:
+                case 4:
+                    icoAttrib.depth = 8;
+                    break;
+                default:
+                    icoAttrib.depth = 1;
+                }
+                if (icoAttrib.depth == 32)                // there's no colormap
+                    icoAttrib.ncolors = 0;
+                else                    // # colors used
+                    icoAttrib.ncolors = header.biClrUsed ? header.biClrUsed : 1 << icoAttrib.nbits;
+                icoAttrib.w = iconEntry.bWidth;
+                icoAttrib.h = iconEntry.bHeight;
 
                 QImage::Format format = QImage::Format_ARGB32;
                 if (icoAttrib.nbits == 24)
diff --git a/tests/auto/qimagereader/baseline/35floppy.ico b/tests/auto/qimagereader/baseline/35floppy.ico
new file mode 100644
index 0000000..59fd37e
--- /dev/null
+++ b/tests/auto/qimagereader/baseline/35floppy.ico
diff --git a/tests/auto/qimagereader/baseline/kde_favicon.ico b/tests/auto/qimagereader/baseline/kde_favicon.ico
new file mode 100644
index 0000000..15bcdbb
--- /dev/null
+++ b/tests/auto/qimagereader/baseline/kde_favicon.ico
diff --git a/tests/auto/qimagereader/baseline/semitransparent.ico b/tests/auto/qimagereader/baseline/semitransparent.ico
new file mode 100644
index 0000000..dd23de9
--- /dev/null
+++ b/tests/auto/qimagereader/baseline/semitransparent.ico
diff --git a/tests/auto/qimagereader/tst_qimagereader.cpp b/tests/auto/qimagereader/tst_qimagereader.cpp
index 3841111..8f7094c 100644
--- a/tests/auto/qimagereader/tst_qimagereader.cpp
+++ b/tests/auto/qimagereader/tst_qimagereader.cpp
@@ -153,6 +153,9 @@ private slots:
 
     void autoDetectImageFormat();
     void fileNameProbing();
+
+    void pixelCompareWithBaseline_data();
+    void pixelCompareWithBaseline();
 };
 
 // Testing get/set functions
@@ -1368,5 +1371,32 @@ void tst_QImageReader::fileNameProbing()
     QCOMPARE(r.fileName(), name);
 }
 
+void tst_QImageReader::pixelCompareWithBaseline_data()
+{
+    QTest::addColumn<QString>("fileName");
+
+    QTest::newRow("floppy (16px,32px - 16 colors)") << "35floppy.ico";
+    QTest::newRow("semitransparent") << "semitransparent.ico";
+    QTest::newRow("slightlybroken") << "kde_favicon.ico";
+}
+
+void tst_QImageReader::pixelCompareWithBaseline()
+{
+    QFETCH(QString, fileName);
+
+    QImage icoImg;
+    // might fail if the plugin does not exist, which is ok.
+    if (icoImg.load(QString::fromAscii("images/%1").arg(fileName))) {
+        QString baselineFileName = QString::fromAscii("baseline/%1").arg(fileName);
+#if 0
+        icoImg.save(baselineFileName);
+#else
+        QImage baseImg;
+        QVERIFY(baseImg.load(baselineFileName));
+        QCOMPARE(baseImg, icoImg);
+#endif
+    }
+}
+
 QTEST_MAIN(tst_QImageReader)
 #include "tst_qimagereader.moc"