Improved quality of JPEG thumbnails

Authored by chroniceel on Tue, Jan 14, 2:05 AM.



BUG: 411262
FIXED-IN: 20.04

Simple tweak to increase quality of JPEG thumbnails. For some reason the thumbnailer was set to scale them down at the minimum possible quality, and this led to some rather ugly results. This is, of course, no longer the case. See the attached images for a comparison. The image was created in gimp, and shows a near worst-case for the thumbnailer before the fix.



Diff Detail

R320 KIO Extras
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
chroniceel created this revision.Tue, Jan 14, 2:05 AM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptTue, Jan 14, 2:05 AM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
chroniceel requested review of this revision.Tue, Jan 14, 2:05 AM
ngraham added a subscriber: ngraham.

Presumably this was for space saving reasons; there's a big size difference between a 0 quality JPEG and 100 quality. Maybe set it to 80 as a compromise? These are only thumbnails, after all. We don't want to take up all the space on a user's disk for thumbnails!

This does not affect the size of the created thumbnail, as all thumbnails are stored as png images regardless of source image type. This simply allows the thumbnailer to downscale the image without the artifacting displayed in the screenshots. I have checked and double checked that the resultant size of the thumbnails remains unchanged.

bruns added a subscriber: bruns.Tue, Jan 14, 3:20 AM

Unfortunately, the QImageReader documentation is not very clear about the effect of setQuality(..). There is a reference to, and that in turn has a reference to QImage::scale() (which no longer exists in Qt5) and Qt::SmoothScaling.

Looking a the images (which lack any legend, so are hard to interpret), a quality of 0 seems to trigger something like nearest neighbor scaling.

For very large images, a fast approximation may still be better.

This comment was removed by chroniceel.

It appears to me that the setQuality() function is meant to, in this use case, to simply decide what downscaler to use. Higher values mean better quality downscaling.

Sorry about the images, I did not realize I should have labelled them better. They are named before and after if you download them.

I'm going to delete that line and see what happens.

chroniceel edited the summary of this revision. (Show Details)Tue, Jan 14, 3:35 AM

Okay, removing the line appears to have a similar effect to what I already did. I checked over on the imagecreator.cpp file, which appears to handle the thumbnailing of most other image types, and despite also using a QImageReader object, it does not call setQuality. My best guess is that QImageReader uses a sane value by default, and the imagecreator.cpp class lets that ride, ergo good thumbnails. I have no clue why this was set here, nor why it was set to 0.

chroniceel updated this revision to Diff 73480.Tue, Jan 14, 3:58 AM

Less is more, removing the line appears to be a solution as well. Assuming QImageReader uses sane defaults.

cfeck added a subscriber: cfeck.Tue, Jan 14, 9:02 AM

Did you time a performance difference? I think the idea was to get the downscaled result as fast as possible, by using integer IDCT instead of float IDCT, and by using only the DC coefficient instead of performing the IDCT at all for scale factors <= 1/8.

Nice first patch @chroniceel ! is the only use of quality in ImageReader, its default is -1 so it depends on its to choose the default behavior and its documentation is not more precise either.
We probably should pick ourselves a value so that if Qt changes its default we are not affected and to set the performance level ourselves. 80 or 90 seems like reasonable values as mentioned by @ngraham before.

This line was added in c249f8547f2d1913e0570d78a0240c5f8865c336
So @volkov maybe you have some input ?

You can add FIXED-IN: 20.04 to the commit comment, following our commit policy to automatically close the bug when it lands.

@meven, this isn't about save quality, but load quality.

imageReader.setQuality(0); was added in when I ported JpegCreator to QImageReader.
But as I mentioned there, it doesn't really influence on the decoding speed, and it was only needed to preserve the old behavior as much as possible.
This change looks good to me, +1.

bruns added a comment.Tue, Jan 14, 3:33 PM

But as I mentioned there, it doesn't really influence on the decoding speed, and it was only needed to preserve the old behavior as much as possible.

Can we have some quick benchmarks of e.g. generating Thumbnails for typical 24MP smartphone images? Only having to decode DC and scale down should be significantly faster CPU wise, and probably also saves IO.

It's not overly scientific, but with this little fix, it can gen ~10 thumbnails a second on my system (1.6 Ghz pentium, ~10MP). This seems on par with png files of a similar size. As far as I can tell, there is not a super huge performance decrease. Please note, I use a potato.

chroniceel edited the summary of this revision. (Show Details)Tue, Jan 14, 4:22 PM

Ping! I think this got buried. Unless someone else is testing this?

bruns added a comment.Thu, Jan 16, 4:24 PM

Having had a look at, the jpeg handler actually know two different quality settings, < 50 and > 50, the default being 75, i.e. high.

High quality means floating point DCT and bilinear upsampling (Qt::SmoothTransformation), while low means IDCT and nearest neighbor upsampling.

In either case, it uses libjpeg's capability to reconstruct smaller versions of the image, only decoding it partially, up to DC components only (i.e. 1/8 scale). So for a 24MPixel (6000x4000), libjpeg returns a 750x500 Pixel image, which is then downscaled by Qimage to 256x171 Pixel. Using SmoothTransformation for this size should be not to heavy.

I think best is to set the quality explicitly and mention in a comment what intended effect the setting has (so if the jpeghandler behavior changes, the code can be adjusted to have the wanted effect).

chroniceel updated this revision to Diff 73722.Thu, Jan 16, 5:03 PM

Updated diff. It's setting the quality now, and added a brief comment.

cfeck accepted this revision.Thu, Jan 16, 5:11 PM

Thanks for the detailed investigation, Stefan!

This revision is now accepted and ready to land.Thu, Jan 16, 5:11 PM
ngraham accepted this revision.Thu, Jan 16, 5:13 PM

@chroniceel Can you provid an email address so we can land this patch with correct authorship information?

... unless you wanted that in the summary?

no in a comment is fine

This revision was automatically updated to reflect the committed changes.

Very nice first patch. May it be the first of many! :)