D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik created this revision. broulik added reviewers: Frameworks, dfaure, ngraham, pali, vonreth, antlarr. broulik requested review of this revision. REVISION SUMMARY Prefer downsampling over upscaling TEST PLAN Dolphin requested 128px thumbnail here. Wine executables have 32, 48, and 256

D14308: [Exe Thumbnailer] Improve icon selection algorithm

ngraham added a comment. What a dramatic set of before-and-after images! It's like we jumped from 1991 to 2018. > Algo is basically copied from Plasma's wallpaper selection Sounds like we should put this in a Framework and re-use it instead of copy-pasting it here. INLINE COMMENTS

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik added a comment. > Sounds like we should put this in a Framework and re-use it instead of copy-pasting it here. It's a five line function, not gonna go through the hoops of finding a framework where this might fit.. INLINE COMMENTS > ngraham wrote in icoutils_common.cpp:33 > Wh

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik updated this revision to Diff 38296. broulik added a comment. - Include `` just in case REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14308?vs=38271&id=38296 REVISION DETAIL https://phabricator.kde.org/D14308 AFFECTED FILES thumbnail/icout

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added inline comments. INLINE COMMENTS > icoutils_common.cpp:28 > +{ > +// compute difference of areas > +const qreal desiredAspectRatio = (desiredHeight > 0) ? desiredWidth / > static_cast(desiredHeight) : 0; Can not see an area calculation here ... > broulik wrote in icoutils_c

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik added inline comments. INLINE COMMENTS > bruns wrote in icoutils_common.cpp:33 > I think the formula is a little bit to ad-hoc, and actually wrong. > > We have 2 different notable cases. `desiredAspectRatio` obviously is a > constant, this leaves us with `candidateAspectRatio` and `delt

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added inline comments. INLINE COMMENTS > broulik wrote in icoutils_common.cpp:33 > I'm open to suggestions … also technically you cannot assume the icon is > square Where do you see any assumption about square icons? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde

D14308: [Exe Thumbnailer] Improve icon selection algorithm

anthonyfieroni added inline comments. INLINE COMMENTS > icoutils_common.cpp:33 > +qreal delta = width - desiredWidth; > +delta = (delta >= 0.0 ? delta : -delta * 2 ); // Penalize for scaling up > + It should be qFuzzyIsNull no? REPOSITORY R320 KIO Extras REVISION DETAIL https://pha

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added inline comments. INLINE COMMENTS > anthonyfieroni wrote in icoutils_common.cpp:33 > It should be qFuzzyIsNull no? @anthonyfieroni Why? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlar

D14308: [Exe Thumbnailer] Improve icon selection algorithm

anthonyfieroni added inline comments. INLINE COMMENTS > bruns wrote in icoutils_common.cpp:33 > @anthonyfieroni Why? Comparing doubles should be done by epsilon not by <= 0.0 REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure, ng

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added a comment. Criteria for a good selection algorithm: 1. If there is an exact fit, use it 2. If all candidates have the same aspect ratio, use the best fit - prefer slight downscaling over slight upscaling - prefer slight upscaling over large downscaling 3. If aspect

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added inline comments. INLINE COMMENTS > anthonyfieroni wrote in icoutils_common.cpp:33 > Comparing doubles should be done by epsilon not by <= 0.0 Not in general. There is nothing inexact here. Also, if difference ~= 0, it does not matter if you scale by 1, 2, -1 or -2. REPOSITORY R32

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added a comment. I think the following yields quite good results: qreal distance(int width, int height, int desiredWidth, int desiredHeight) { auto targetSamples = desiredWidth * desiredHeight; auto xscale = (1.0 * desiredWidth) / width; auto yscale = (1

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik added a comment. The code doesn't compile and I don't understand it well enough to fix that. I tried but now I have it pick 16x16 every time REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, an

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added a comment. qreal distance(int width, int height, int desiredWidth, int desiredHeight) { auto targetSamples = desiredWidth * desiredHeight; auto xscale = (1.0 * desiredWidth) / width; auto yscale = (1.0 * desiredHeight) / height; // clamp to

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik updated this revision to Diff 39462. broulik edited the test plan for this revision. broulik added a comment. - Use algorithm suggested by Stefan, thanks. REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14308?vs=38296&id=39462 REVISION DETAIL h

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik updated this revision to Diff 39519. broulik edited the summary of this revision. broulik added a comment. I found a way to get the actual depth of the icon extracted - Prefer higher DPI as well REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added a comment. Should be "Prefer higher BPP" (bits per pixel), DPI is dots per inch, i.e. spatial resolution. INLINE COMMENTS > icoutils_common.cpp:100 > +// but they store the actual depth of the icon extracted in custom > text: > +// qtbase/src/plugins/imageformats

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik added a comment. > Should be "Prefer higher BPP" That's just the internal changelog on Phabricator. Can we ship this already..? INLINE COMMENTS > bruns wrote in icoutils_common.cpp:100 > qico_handler.cpp (no "n") No, that is the actual file name on qtbase git dev REPOSITORY R

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > broulik wrote in icoutils_common.cpp:100 > No, that is the actual file name on qtbase git dev https://github.com/qt/qtbase/blob/5.11/src/plugins/imageformats/ic

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added a comment. In D14308#308137 , @broulik wrote: > > Should be "Prefer higher BPP" > > That's just the internal changelog on Phabricator. Can we ship this already.. See "Summary: " ... INLINE COMMENTS > icoutils_common.cpp:

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik edited the summary of this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr, bruns Cc: anthonyfieroni, bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added a comment. Bits ber pixel? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr, bruns Cc: anthonyfieroni, bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik updated this revision to Diff 39645. broulik added a comment. - Fix top, should have worn my glasses when reading that REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14308?vs=39519&id=39645 REVISION DETAIL https://phabricator.kde.org/D14308 A

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik edited the summary of this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr, bruns Cc: anthonyfieroni, bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns added inline comments. INLINE COMMENTS > icoutils_common.cpp:99 > +// QtIcoHandler converts all images to 32-bit depth > +// but they store the actual depth of the icon extracted in custom > text: > +// qtbase/src/plugins/imageformats/ico/qicohandler.cpp:455 gramma

D14308: [Exe Thumbnailer] Improve icon selection algorithm

broulik updated this revision to Diff 40131. broulik added a comment. - Update wording REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14308?vs=39645&id=40131 REVISION DETAIL https://phabricator.kde.org/D14308 AFFECTED FILES thumbnail/icoutils_commo

D14308: [Exe Thumbnailer] Improve icon selection algorithm

bruns accepted this revision. This revision is now accepted and ready to land. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D14308 To: broulik, #frameworks, dfaure, ngraham, pali, vonreth, antlarr, bruns Cc: bruns

D14308: [Exe Thumbnailer] Improve icon selection algorithm

This revision was automatically updated to reflect the committed changes. Closed by commit R320:30d274092c15: [Exe Thumbnailer] Improve icon selection algorithm (authored by broulik). REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14308?vs=40131&id=40204 R