WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED INVALID
28076
[Qt] fast/images/icon-decoding.html needs results for Qt
https://bugs.webkit.org/show_bug.cgi?id=28076
Summary
[Qt] fast/images/icon-decoding.html needs results for Qt
Peter Kasting
Reported
2009-08-07 11:32:07 PDT
Patch coming to skip this test for now.
Attachments
Add to Skipped list
(1.06 KB, patch)
2009-08-07 11:33 PDT
,
Peter Kasting
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Peter Kasting
Comment 1
2009-08-07 11:33:49 PDT
Created
attachment 34298
[details]
Add to Skipped list
Peter Kasting
Comment 2
2009-08-07 11:38:54 PDT
Comment on
attachment 34298
[details]
Add to Skipped list Landed skip update in
r46902
.
Andras Becsi
Comment 3
2009-08-10 01:15:45 PDT
There seems to be a problem with the handling of ico files with multiple entries. I'm working on a solution for this.
Andras Becsi
Comment 4
2009-08-12 01:55:58 PDT
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()) {
Peter Kasting
Comment 5
2009-08-12 10:01:51 PDT
(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.
Andras Becsi
Comment 6
2009-08-12 10:45:58 PDT
> 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.
Peter Kasting
Comment 7
2009-08-12 10:58:19 PDT
(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.
Andras Becsi
Comment 8
2009-08-13 07:43:12 PDT
I have changed my patch according to pkasting's explanation. The patch can be found here:
http://pastebin.com/f293c5c82
Kent Hansen
Comment 9
2010-03-11 07:43:26 PST
(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.
Jesus Sanchez-Palencia
Comment 10
2010-05-13 14:58:58 PDT
(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?
Andras Becsi
Comment 11
2010-05-21 08:04:23 PDT
(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.
Jocelyn Turcotte
Comment 12
2014-02-03 03:12:57 PST
=== 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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug