[Exe Thumbnailer] Improve icon selection algorithm
ClosedPublic

Authored by broulik on Jul 23 2018, 6:58 PM.

Details

Summary

Prefer downsampling over upscaling, and also prefer higher BPP

Test Plan

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
broulik requested review of this revision.Jul 23 2018, 6:58 PM
broulik created this revision.

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

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

No idea, really.

broulik updated this revision to Diff 38296.Jul 24 2018, 6:59 AM
  • Include <limits> just in case
bruns added a subscriber: bruns.Jul 24 2018, 2:42 PM
bruns added inline comments.
thumbnail/icoutils_common.cpp
28

Can not see an area calculation here ...

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 delta

  1. all candiates have exactly the same aspect ratio. The difference below is constant, thus has no influence on the sorting. Sorting is determined by deltaalone (good).
  2. candidates have different aspect ratios. Any candidate with a slightly different aspect ratio is immediately heavily penalized, and thus discarded (bad).

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.

broulik added inline comments.Jul 30 2018, 1:53 PM
thumbnail/icoutils_common.cpp
33

I'm open to suggestions … also technically you cannot assume the icon is square

bruns added inline comments.Jul 30 2018, 3:57 PM
thumbnail/icoutils_common.cpp
33

Where do you see any assumption about square icons?

anthonyfieroni added inline comments.
thumbnail/icoutils_common.cpp
33

It should be qFuzzyIsNull no?

bruns added inline comments.Jul 30 2018, 4:11 PM
thumbnail/icoutils_common.cpp
33
anthonyfieroni added inline comments.Jul 30 2018, 4:18 PM
thumbnail/icoutils_common.cpp
33

Comparing doubles should be done by epsilon not by <= 0.0

bruns added a comment.Jul 30 2018, 4:21 PM

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 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.

bruns added inline comments.Jul 30 2018, 4:28 PM
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;
}
broulik updated this revision to Diff 39462.Aug 11 2018, 3:46 PM
broulik edited the test plan for this revision. (Show Details)
  • Use algorithm suggested by Stefan, thanks.
broulik updated this revision to Diff 39519.Aug 12 2018, 1:23 PM
broulik edited the summary of this revision. (Show Details)

I found a way to get the actual depth of the icon extracted

  • Prefer higher DPI as well
bruns added a comment.Aug 13 2018, 7:02 PM

Should be "Prefer higher BPP" (bits per pixel), DPI is dots per inch, i.e. spatial resolution.

thumbnail/icoutils_common.cpp
91

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

No, that is the actual file name on qtbase git dev

bruns requested changes to this revision.Aug 13 2018, 8:55 PM
bruns added inline comments.
This revision now requires changes to proceed.Aug 13 2018, 8:55 PM
bruns added a comment.Aug 13 2018, 8:59 PM

Should be "Prefer higher BPP"

That's just the internal changelog on Phabricator. Can we ship this already..

See "Summary: " ...

thumbnail/icoutils_common.cpp
89

either Qt handler for "Ico" files or qicohandler

broulik edited the summary of this revision. (Show Details)Aug 13 2018, 9:00 PM
bruns added a comment.Aug 13 2018, 9:00 PM

Bits ber pixel?

broulik updated this revision to Diff 39645.Aug 13 2018, 9:01 PM
  • Fix top, should have worn my glasses when reading that
broulik edited the summary of this revision. (Show Details)Aug 13 2018, 9:01 PM
bruns added inline comments.Aug 18 2018, 12:37 AM
thumbnail/icoutils_common.cpp
90

grammatical nitpick:

QtIcoHandler converts all images to 32 bit depth,
but it stores the actual depth of the icon extracted in custom text:

i.e. missing , in the first line, and third person

otherwise it's good to go

broulik updated this revision to Diff 40131.Aug 21 2018, 9:34 AM
  • Update wording
bruns accepted this revision.Aug 21 2018, 4:46 PM
This revision is now accepted and ready to land.Aug 21 2018, 4:46 PM
This revision was automatically updated to reflect the committed changes.