| broulik | |
| cfeck | |
| ngraham |
| Frameworks | |
| VDG |
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.
Before:
After:
| Lint Skipped |
| Unit Tests Skipped |
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.
Unfortunately, the QImageReader documentation is not very clear about the effect of setQuality(..). There is a reference to https://doc.qt.io/qt-5/qimagereader.html#setScaledSize, 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.
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.
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.
Less is more, removing the line appears to be a solution as well. Assuming QImageReader uses sane defaults.
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 !
https://github.com/openwebos/qt/blob/master/src/gui/image/qimagereader.cpp#L1202 is the only use of quality in ImageReader, its default is -1 so it depends on its https://doc.qt.io/qt-5/qimageiohandler.html 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 https://community.kde.org/Policies/Commit_Policy to automatically close the bug when it lands.
imageReader.setQuality(0); was added in https://phabricator.kde.org/R320:c249f8547f2d1913e0570d78a0240c5f8865c336 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.
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.
Having had a look at https://code.woboq.org/qt5/qtbase/src/plugins/imageformats/jpeg/qjpeghandler.cpp.html#_M/HIGH_QUALITY_THRESHOLD, 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.
https://code.woboq.org/qt5/qtbase/src/plugins/imageformats/jpeg/qjpeghandler.cpp.html#337
https://code.woboq.org/qt5/qtbase/src/plugins/imageformats/jpeg/qjpeghandler.cpp.html#432
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.
https://code.woboq.org/qt5/qtbase/src/plugins/imageformats/jpeg/qjpeghandler.cpp.html#297
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 Can you provid an email address so we can land this patch with correct authorship information?