28076 2009-08-07 11:32:07 -0700 [Qt] fast/images/icon-decoding.html needs results for Qt 2014-02-03 03:12:57 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED INVALID Qt, QtTriaged P2 Normal --- 1 pkasting webkit-unassigned abecsi hausmann kenneth kent.hansen manyoso oldest_to_newest 138000 0 pkasting 2009-08-07 11:32:07 -0700 Patch coming to skip this test for now. 138001 1 34298 pkasting 2009-08-07 11:33:49 -0700 Created attachment 34298 Add to Skipped list 138004 2 34298 pkasting 2009-08-07 11:38:54 -0700 Comment on attachment 34298 Add to Skipped list Landed skip update in r46902. 138618 3 abecsi 2009-08-10 01:15:45 -0700 There seems to be a problem with the handling of ico files with multiple entries. I'm working on a solution for this. 139277 4 abecsi 2009-08-12 01:55:58 -0700 Here is a patch for qt which corrects the layout test, but the imlementation should be changed higher up in QIcon and QPixmap. This is only an expected behaviour change, which is more compilant to the ICO file format description (http://www.daubnet.com/en/file-format-ico) which says: "For general purpose ICOs there should be at least one 32x32 image using the 16 Windows colors." diff --git a/src/plugins/imageformats/ico/qicohandler.cpp b/src/plugins/imageformats/ico/qicohandler.cpp index c9e04a4..77c68b7 100644 --- a/src/plugins/imageformats/ico/qicohandler.cpp +++ b/src/plugins/imageformats/ico/qicohandler.cpp @@ -802,7 +802,25 @@ bool QtIcoHandler::canRead(QIODevice *device) bool QtIcoHandler::read(QImage *image) { bool bSuccess = false; - QImage img = m_pICOReader->iconAt(m_currentIconIndex); + + QIODevice *d = device(); + + QList<QImage> imgs = m_pICOReader->read(d); + QImage img = imgs[m_currentIconIndex]; + QSize requestedSize(32,32); + + if ( imgs.size() > 1 ){ + + // if we have multiple icons in the file we return the first with 32x32 resolution because it is the reference size + // FIXME: there should be a proper way of handling all the files in an ICO file higher up in the abstraction + // ie. to fill the caller QIcon with the icons in the ICO file automatically, and let the user decide which to use + for ( int i=0; i<imgs.size(); ++i ) + if ( imgs[i].size() == requestedSize ){ + img = imgs[i]; + break; + } + + } // Make sure we only write to \a image when we succeed. if (!img.isNull()) { 139377 5 pkasting 2009-08-12 10:01:51 -0700 (In reply to comment #4) > Here is a patch for qt which corrects the layout test, but the imlementation > should be changed higher up in QIcon and QPixmap. This is only an expected > behaviour change, which is more compilant to the ICO file format description > (http://www.daubnet.com/en/file-format-ico) which says: > > "For general purpose ICOs there should be at least one 32x32 image using the 16 > Windows colors." That sentence is advisory, to note that if you only include smaller sizes, Windows has to scale them up, and they look bad. However, 32x32 is not a "reference" size, and the first such entry found is not necessarily the best. The layout test is testing that the decoder selects the icon entries in order of largest first, then highest bit depth first after that. This is similar to how Windows itself behaves, especially on newer versions like Vista and 7, where the "typical" icon size is significantly larger than 32x32. Note that for some applications (e.g. displaying favicons in a tab or session history list), you probably want to be able to select a 16x16 icon. 139415 6 abecsi 2009-08-12 10:45:58 -0700 > The layout test is testing that the decoder selects the icon entries in order > of largest first, then highest bit depth first after that. This is similar to > how Windows itself behaves, especially on newer versions like Vista and 7, > where the "typical" icon size is significantly larger than 32x32. > > Note that for some applications (e.g. displaying favicons in a tab or session > history list), you probably want to be able to select a 16x16 icon. Thank you very much for the information! I din't know which is the preferred order of selecting the icons, how ico exactly has to be handled. I will look into that and correct my patch. According to that the behaviour of the Qt infrastructure is totally wrong, because only the 1st element in the ico file is handed upward to create a QIcon or QPixmap or QImage, and no feature is considered. The only way to handle ICO files correctly is to use QImageLoader an specify how to select the icon. In my opinion at least QIcon sould automatically fill himself with all the icons contained in the ico file, and then select which to retunrt on QIcon::pixmap(...) calls, according to the features and the given arguments. Do you agree? WebKit converts the images to NativeImagePtr, which is QPixmap* in case of QtWebKit if I am not mistaken, so the information that it was an ico and had more entries is lost. I could write a patch to work around the test in QtWebKit, and to specifically handle ico files, but I think this is not a QtWebKit bug. 139424 7 pkasting 2009-08-12 10:58:19 -0700 (In reply to comment #6) > According to that the behaviour of the Qt infrastructure is totally wrong, > because only the 1st element in the ico file is handed upward to create a QIcon > or QPixmap or QImage, and no feature is considered. If you're only going to select one image, it should probably be the largest (and highest bit depth). > In my > opinion at least QIcon sould automatically fill himself with all the icons > contained in the ico file, and then select which to retunrt on > QIcon::pixmap(...) calls, according to the features and the given arguments. Do > you agree? I am not familiar with the Qt API, so I can't comment usefully. In the abstract, that sounds correct :) > WebKit converts the images to NativeImagePtr, which is QPixmap* in case of > QtWebKit if I am not mistaken, so the information that it was an ico and had > more entries is lost. Things are only converted to a NativeImagePtr at the time when a client requests a frame at a particular index. But an outside client can query ImageSource::frameCount() for the number of frames in an image, and then use ImageSource::frameSizeAtIndex() to determine which frame it wants before requesting the actual frame data. Making this work depends on having your ImageDecoder return the right values for these calls. If your icon decoder sorts the directory entries by size, and bit depth within size, with the highest quality image first, then callers who blindly grab the first frame will get the highest-quality icon (which is what this layout test is testing), and callers who want to pick an icon can use the functions described above. 139838 8 abecsi 2009-08-13 07:43:12 -0700 I have changed my patch according to pkasting's explanation. The patch can be found here: http://pastebin.com/f293c5c82 198620 9 kent.hansen 2010-03-11 07:43:26 -0800 (In reply to comment #8) > I have changed my patch according to pkasting's explanation. The patch can be > found here: http://pastebin.com/f293c5c82 Could you please submit this patch as a merge request at http://qt.gitorious.org if you haven't already? Additionally you could create a report at http://bugreports.qt.nokia.com and link to it from here, so the dependency becomes explicit. Thanks. 225522 10 jesus 2010-05-13 14:58:58 -0700 (In reply to comment #9) > Could you please submit this patch as a merge request at http://qt.gitorious.org if you haven't already? > Additionally you could create a report at http://bugreports.qt.nokia.com and link to it from here, so the dependency becomes explicit. > Thanks. Was this done? 228917 11 abecsi 2010-05-21 08:04:23 -0700 (In reply to comment #10) > (In reply to comment #9) > > Could you please submit this patch as a merge request at http://qt.gitorious.org if you haven't already? > > Additionally you could create a report at http://bugreports.qt.nokia.com and link to it from here, so the dependency becomes explicit. > > Thanks. > > Was this done? I had a merge request for this on gitorious but there was ongoing work by Milan Burda on Qt image loader and icon handling already, apparently he wanted this done differently so I didn't create a bugreport. Seems that the issue wasn't resolved, I'll recheck. 975012 12 jturcotte 2014-02-03 03:12:57 -0800 === Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines. 34298 2009-08-07 11:33:49 -0700 2009-08-07 11:38:53 -0700 Add to Skipped list patch text/plain 1084 pkasting SW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIExheW91dFRlc3Rz L0NoYW5nZUxvZwkocmV2aXNpb24gNDY5MDApCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHdv cmtpbmcgY29weSkKQEAgLTEsMyArMSwxMyBAQAorMjAwOS0wOC0wNyAgUGV0ZXIgS2FzdGluZyAg PHBrYXN0aW5nQGdvb2dsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI4 MDc2CisgICAgICAgIEFkZCBmYXN0L2ltYWdlcy9pY29uLWRlY29kaW5nLmh0bWwgdG8gUXQgc2tp cCBsaXN0IHVudGlsIGl0IGhhcworICAgICAgICByZXN1bHRzLgorCisgICAgICAgICogcGxhdGZv cm0vcXQvU2tpcHBlZDoKKwogMjAwOS0wOC0wNyAgTWlrZSBGZW50b24gIDxtaWtlLmZlbnRvbkB0 b3JjaG1vYmlsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRXJpYyBTZWlkZWwuCkluZGV4 OiBMYXlvdXRUZXN0cy9wbGF0Zm9ybS9xdC9Ta2lwcGVkDQo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gTGF5b3V0 VGVzdHMvcGxhdGZvcm0vcXQvU2tpcHBlZAkocmV2aXNpb24gNDY4NzApCisrKyBMYXlvdXRUZXN0 cy9wbGF0Zm9ybS9xdC9Ta2lwcGVkCSh3b3JraW5nIGNvcHkpCkBAIC0yMTYsNiArMjE2LDcgQEAg ZmFzdC9mcmFtZXMvaWZyYW1lLXdpbmRvdy1mb2N1cy5odG1sCiBmYXN0L2hpc3RvcnkvY2xpY2tl ZC1saW5rLWlzLXZpc2l0ZWQuaHRtbAogZmFzdC9oaXN0b3J5L2dvLWJhY2stdG8tY2hhbmdlZC1u YW1lLmh0bWwKIGZhc3QvaHRtbC9rZXlnZW4uaHRtbAorZmFzdC9pbWFnZXMvaWNvbi1kZWNvZGlu Zy5odG1sCiBmYXN0L2lubGluZS9kaXJ0eUxpbmVzRm9ySW5saW5lLmh0bWwKIGZhc3QvaW52YWxp ZC9taXNzaW5nLWVuZC10YWcueGh0bWwKIGZhc3QvaW52YWxpZC9yZXNpZHVhbC1zdHlsZS5odG1s Cg==