Prefer downsampling over upscaling, and also prefer higher BPP
Details
- Reviewers
dfaure ngraham pali vonreth antlarr bruns - Group Reviewers
Frameworks - Commits
- R320:30d274092c15: [Exe Thumbnailer] Improve icon selection algorithm
Dolphin requested 128px thumbnail here. Wine executables have 32, 48, and 256 icons. It would pick the 48 one:
Afterwards it would use the 256 ones
The effect isn't neccessarily as noticeable with every icon and I hardly have any "real" windows EXE files around to test :)
Diff Detail
- Repository
- R320 KIO Extras
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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.
thumbnail/icoutils_common.cpp | ||
---|---|---|
33 ↗ | (On Diff #38271) | Where does this magic number come from? |
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..
thumbnail/icoutils_common.cpp | ||
---|---|---|
33 ↗ | (On Diff #38271) | No idea, really. |
thumbnail/icoutils_common.cpp | ||
---|---|---|
28 | Can not see an area calculation here ... | |
33 ↗ | (On Diff #38271) | 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 delta
Think of e.g. a requested size of 96x96, and two candidates, 32x32 and 128x64. The small one has a distance of 128, while the second one has a distance 25032. I don't think that's wanted here. |
thumbnail/icoutils_common.cpp | ||
---|---|---|
33 ↗ | (On Diff #38271) | I'm open to suggestions … also technically you cannot assume the icon is square |
thumbnail/icoutils_common.cpp | ||
---|---|---|
33 ↗ | (On Diff #38271) | Where do you see any assumption about square icons? |
thumbnail/icoutils_common.cpp | ||
---|---|---|
33 | It should be qFuzzyIsNull no? |
thumbnail/icoutils_common.cpp | ||
---|---|---|
33 | @anthonyfieroni Why? |
thumbnail/icoutils_common.cpp | ||
---|---|---|
33 | Comparing doubles should be done by epsilon not by <= 0.0 |
Criteria for a good selection algorithm:
- If there is an exact fit, use it
- If all candidates have the same aspect ratio, use the best fit
- prefer slight downscaling over slight upscaling
- prefer slight upscaling over large downscaling
- If aspect ratio of candidates differ, use the best fit
- For all subsets of (approximately) same aspect ratio, (2.) should select one candidate per set
- Determine which of the subsets yielded the best overall candidate
The last point is difficult to answer - prefer upscaling a candidate with matching aspect ratio, or downscaling a nonmatching one.
thumbnail/icoutils_common.cpp | ||
---|---|---|
33 | 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. |
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.0 * desiredHeight) / height; // clamp to the lower of the two scales // also clamp to one, as scaling up adds no effective // samples, only interpolated samples auto sampleScale = min(1, min(xscale, yscale)); // number of effective source samles in the target auto effectiveSamples = width * height * scale * scale; // scale down another time, to account for loss of fidelity when // using a downscaled image, biases towards smaller downscaling ratios effectiveSamples *= scale; return targetSamples - effectiveSamples; }
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
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 the lower of the two scales // also clamp to one, as scaling up adds no effective // samples, only interpolated samples auto sampleScale = std::min(1.0, std::min(xscale, yscale)); // number of effective source samles in the target auto effectiveSamples = width * height * sampleScale * sampleScale; // scale down another time, to account for loss of fidelity when // using a downscaled image, biases towards smaller downscaling ratios effectiveSamples *= sampleScale; return targetSamples - effectiveSamples; }
I found a way to get the actual depth of the icon extracted
- Prefer higher DPI as well
Should be "Prefer higher BPP" (bits per pixel), DPI is dots per inch, i.e. spatial resolution.
thumbnail/icoutils_common.cpp | ||
---|---|---|
91 ↗ | (On Diff #38271) | qico_handler.cpp (no "n") |
Should be "Prefer higher BPP"
That's just the internal changelog on Phabricator. Can we ship this already..?
thumbnail/icoutils_common.cpp | ||
---|---|---|
91 ↗ | (On Diff #38271) | No, that is the actual file name on qtbase git dev |
thumbnail/icoutils_common.cpp | ||
---|---|---|
91 ↗ | (On Diff #38271) |
See "Summary: " ...
thumbnail/icoutils_common.cpp | ||
---|---|---|
89 ↗ | (On Diff #38271) | either Qt handler for "Ico" files or qicohandler |
thumbnail/icoutils_common.cpp | ||
---|---|---|
90 ↗ | (On Diff #38271) | grammatical nitpick:
i.e. missing , in the first line, and third person otherwise it's good to go |