Add Compression Quality slider for lossy formats
ClosedPublic

Authored by nrother on Mar 7 2019, 12:46 PM.

Details

Summary

FEATURE: 63151
FIXED-IN: 19.04.0

This adds a slider to set the compression quality for lossy file formats
to the settings page, so users can set it themselves instead of having
to just accept the (in my opinion too low) default value.
The slider defaults to a quality level of 90.

A couple of notes:

-When adding the slider to the 'Save'-page layout I noticed that the 
 bottom of the help text for the filename chooser got cut off because 
 the window was too small. I tried to figure out how to get the settings
 page to scroll but couldn't quite get it working. Instead I resorted to
 simply increasing the default size of the settings window just enough to make it
 fit. If someone with more experience in Qt than me can get the page to scroll OR
 can suggest a better way to display the formatting help text (which I feel
 takes up too much of the page), that would be marvelous.

-When saving an image at full (100) quality it still looks a bit washed out and
 fuzzy, when compared to, for example, saving the image as png and converting it
 to JPG in gimp at full quality. This might be an issue with QImageWriter unless
 this is the desired behaviour? I haven't looked into it too much, just something
 I've noticed when testing
Test Plan

This setting can be found under
Configure -> Save -> Compression Quality


Set the default file type to JPG, adjust the slider and export to an image-viewer
of your choosing (eg. feh) to see how it affects image quality.

Diff Detail

Repository
R166 Spectacle
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9293
Build 9311: arc lint + arc unit
nrother created this revision.Mar 7 2019, 12:46 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptMar 7 2019, 12:46 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
nrother requested review of this revision.Mar 7 2019, 12:46 PM
nrother edited the test plan for this revision. (Show Details)Mar 7 2019, 12:47 PM
nrother added a reviewer: Spectacle.
davidre added a subscriber: davidre.EditedMar 7 2019, 3:00 PM

I think the slider looks a bit disconnected to the file format maybe putting it below the Filename and format would work. Also the slider should only be usable if the selected format is not lossy in my opinion. But I don't know if it is possible to query if the selected format is lossy.

Personally I don't think it's necessary that both the slider and label are member of the class and you don't need the updateQualityValue method. Instead you can directly connect valueChanged signal to markDirty and setNum or connect it to a lambda that calls both functions. I'm not a reviewer but this is how I would do it.

nrother added a comment.EditedMar 7 2019, 3:21 PM

I think the slider looks a bit disconnected to the file format maybe putting it below the Filename and format would work.

I'm not sure I understand where exactly you mean? I feel the current position makes sense because the other two fields are both
related to WHERE the file is saved, whereas this new option is about HOW this file is saved, so it should be separate from those two.

Also the slider should only be usable if the selected format is not lossy in my opinion. But I don't know if it is possible to query if the selected format is lossy.

I disagree, even if you select PNG in the Settings page, you can still save JPEGs by changing the file format in the "Save As" dialogue. It would be confusing if you
can't change the slider setting because of the dropdown setting, when the slider setting still has an effect.

Personally I don't think it's necessary that both the slider and label are member of the class and you don't need the updateQualityValue method. Instead you can directly connect valueChanged signal to markDirty and setNum or connect it to a lambda that calls both functions. I'm not a reviewer but this is how I would do it.

That sounds like a reasonable implementation, the reason that I did it this way is because I'm trying to not stray too far from how it's done in other places in this codebase,
since I'm fairly new to Qt (and C++ for that matter, I prefer C :-) ), so I try to keep away from fancy implementations that might be more modern/elegant, but are more foreign to me.

ngraham added a subscriber: ngraham.Mar 7 2019, 3:23 PM

I'm not a reviewer but this is how I would do it.

BTW, you can definitely review a patch even if you weren't explicitly marked as a reviewer! It's welcomed, in fact.

I think the slider looks a bit disconnected to the file format maybe putting it below the Filename and format would work.

I'm not sure I understand where exactly you mean? I feel the current position makes sense because the other two fields are both
related to WHERE the file is saved, whereas this new option is about HOW this file is saved, so it should be separate from those two.

The current layout just feels weird (cramped?) to me with the slider below the wall of very technical text. I just had the idea maybe we could seperate the filename and format. Because the current label is only Filename and have a seperate row for the format with Label "Default image format" and below the slider. What do you think of this idea?

Also the slider should only be usable if the selected format is not lossy in my opinion. But I don't know if it is possible to query if the selected format is lossy.

I disagree, even if you select PNG in the Settings page, you can still save JPEGs by changing the file format in the "Save As" dialogue. It would be confusing if you
can't change the slider setting because of the dropdown setting, when the slider setting still has an effect.

Ah I didn't think of that that makes sense.

Something like this (quick copy & paste job):

Something like this (quick copy & paste job):

Ah, now I get it, thanks.

Hmm... I must say I prefer having the Image Format to the right of the Filename, since it is the file extension,
it just seems fitting to have it to the right of the filename. Having it above also adds a lot of whitespace to the right of the dropdown.

Maybe simply having it like this could work?

Or without the spacer:

I think I prefer the version the with the spacer.

I prefer the version with the vertical spacer too. What might make sense is to add some inline help text below this slider, saying something like "Choose the image quality when saving with lossy image formats like JPEG". Otherwise novice users might think that this setting affects all image formats.

I also agree that the filename section's UI is not the best. What we'd like to do is port the token chooser UI present in Gwenview to this: https://bugs.kde.org/show_bug.cgi?id=390856

In the meantime, just making the window taller is fine.

nrother updated this revision to Diff 53376.EditedMar 7 2019, 4:58 PM

Adjust slider position

Right, Slider is now positioned between Path and Filename chooser, with extra spacing.

Edit:
Whoops, forgot to add a help text, sorry, doing about five different things at once right now. :)
Just gonna add that too real quick...

nrother updated this revision to Diff 53377.Mar 7 2019, 5:13 PM

Add help-text for quality-slider

ngraham accepted this revision as: VDG.Mar 7 2019, 5:25 PM

Sweet, the UI looks great to me now. I've tested it out and everything seems to work too. I can't reproduce the washed-out image effect. Here's the same image saved four times at various different file formats and compression levels:

JPEG 8%:


JPEG 80%:

JPEG 100%:

PNG:

So, thumbs up there.

@davidre, would you like to do the code review part?

I can't reproduce the washed-out image effect. Here's the same image saved four times at various different file formats and compression levels:

Hmm, here's what it looks like when I do it:

Reference PNG:

100% JPEG saved with GIMP:

100% JPEG saved with Spectacle:

To me, the reference PNG and GIMPs JPEG look identical, but especially when looking at the orange 'new' keyword while switching between the GIMP and Spectacle
images, the colors are definitely less saturated.

Oh I see what you mean. It's very subtle. Probably an issue in Qt unfortunately. :(

...Which is good for this patch, since it means there's probably nothing we can do here. Please do file a bug though: https://bugreports.qt.io/secure/CreateIssue!default.jspa

Oh I see what you mean. It's very subtle. Probably an issue in Qt unfortunately. :(

...Which is good for this patch, since it means there's probably nothing we can do here. Please do file a bug though: https://bugreports.qt.io/secure/CreateIssue!default.jspa

Will do!

@davidre, would you like to do the code review part?

As stated before I would make the label a local variable and remove the update method and connect directly.
With a lambda you would have

connect(mQualitySlider, &QSlider::valueChanged,  [=](int value){
    mQualityValue->setNum(value);
    markDirty();
});

Because we're using only pointers (mQualityValue and implicitly this) from the outer context inside the lambda capturing by value is ok.
Or you could simply connect the signal to two slots. Sadly we need an ugly cast* because setNum is overloaded.

connect(mQualitySlider, &QSlider::valueChanged, this, &SaveOptionsPage::markDirty);
connect(mQualitySlider, &QSlider::valueChanged, mQualityValue, static_cast<void (QLabel::*)(int)>(&QLabel::setNum));

Both options are used already in Spectacle so choose the one that you prefer.
*Qt has qOverload<T>(&function) as a replacement for the ugly static cast but that requires C++14. For C++ 11 you can use instead QOverload<int>::of(&function)
https://doc.qt.io/qt-5/qtglobal.html#qOverload

The rest looks fine to me!

nrother updated this revision to Diff 53385.Mar 7 2019, 6:46 PM

Use lambda expression to update quality label so it doesn't need to be a member variable

Cool, I like it. Feels a lot more elegant. Will have to brush up on lamda expressions in C++,
only ever used them in Java.

davidre added inline comments.Mar 7 2019, 7:27 PM
src/Gui/SettingsDialog/SaveOptionsPage.cpp
39

You have added an empty line here.

74

Put the Brace on the same line as the parameter list - this style is already used in this file

nrother updated this revision to Diff 53390.Mar 7 2019, 7:38 PM

Adjust codestyle

Looks good to me!

davidre accepted this revision.Mar 7 2019, 7:44 PM
This revision is now accepted and ready to land.Mar 7 2019, 7:44 PM
ngraham accepted this revision.Mar 7 2019, 8:34 PM

Fantastic job!

ngraham edited the summary of this revision. (Show Details)Mar 7 2019, 8:35 PM
This revision was automatically updated to reflect the committed changes.

Now that you're familiar with how to do this, would you like to take a crack at doing the same for Gwenview?

https://bugs.kde.org/show_bug.cgi?id=277996

nrother marked 2 inline comments as done.Mar 7 2019, 8:43 PM

Now that you're familiar with how to do this, would you like to take a crack at doing the same for Gwenview?

https://bugs.kde.org/show_bug.cgi?id=277996

Sure, will take a look at that next.

cfeck added a subscriber: cfeck.Apr 3 2019, 10:07 AM

Where is the check that this is done only for lossy formats? Here, PNG is saved uncompressed after this change.

Where is the check that this is done only for lossy formats? Here, PNG is saved uncompressed after this change.

The docs for QImageWriter::setQuality() state that it sets the image quality for lossy formats, giving jpeg as an example.
For the compression of lossless formats, QImageWriter::setCompression() is used. I assume that includes png? Maybe we need to set a sane default there, as I don't know what the default is.
The docs aren't really clear on this and don't seem to include a list of which format uses which value but since it is ignored when it doesn't apply to the format, I don't think we need to check manually
and saving PNG files should be unaffected by this change unless Qt changed something on their end?

It seems for the png implementation setQuality also sets the Compression instead of setCompression:
https://bugreports.qt.io/browse/QTBUG-43618

filipf added a subscriber: filipf.Apr 3 2019, 10:42 AM

Where is the check that this is done only for lossy formats? Here, PNG is saved uncompressed after this change.

Is this then related to a large file size? If so, I remember there was an issue with the preview in D19469 inexplicably coming out 6 MB big.

It seems for the png implementation setQuality also sets the Compression instead of setCompression:
https://bugreports.qt.io/browse/QTBUG-43618

Well that explains it. Unfortunately it seems like the Qt devs don't plan on fixing so I guess it's on us?
Two options that make sense to me here is check if format is png and then either set a sane default value for compression
or take the current quality value and invert it so that higher values for compression quality mean higher png compression.
So setting compression quality to e.g 100 in the UI results in quality being set to 0 for the ImageWriter so that the resulting
image is compressed at level 9 (so the highest level for png).

It seems for the png implementation setQuality also sets the Compression instead of setCompression:
https://bugreports.qt.io/browse/QTBUG-43618

Well that explains it. Unfortunately it seems like the Qt devs don't plan on fixing so I guess it's on us?
Two options that make sense to me here is check if format is png and then either set a sane default value for compression
or take the current quality value and invert it so that higher values for compression quality mean higher png compression.
So setting compression quality to e.g 100 in the UI results in quality being set to 0 for the ImageWriter so that the resulting
image is compressed at level 9 (so the highest level for png).

In my opinion inverting wouldn't make sense. The slider claims only to be for loseless formats and all of sudden I have to change it to 100% (best quality) to get the smallest filesize when using a loseless format.
I think a sane default is the way go here. Because png is a loseless format there shouldn't be visual differences between the different levels when using QImageWriter but best to check again.

I looked into Qt and it actually uses compression. It just falls back to quality if compression is not set. See: https://code.qt.io/cgit/qt/qtbase.git/tree/src/gui/image/qpnghandler.cpp#n1075
I did a quick test P367 and it seems to work. The question now is do we want to special case this for png to not accidentally break another format (would be a quick fix) or to enable it for all formats that support it. In my opinion if we do the latter we should now do testing (maybe automated?) to not run into weird quality/compression interactions for other formats.
@nrother Do you want to fix it for 19.04.1?

I looked into Qt and it actually uses compression. It just falls back to quality if compression is not set. See: https://code.qt.io/cgit/qt/qtbase.git/tree/src/gui/image/qpnghandler.cpp#n1075
I did a quick test P367 and it seems to work. The question now is do we want to special case this for png to not accidentally break another format (would be a quick fix) or to enable it for all formats that support it. In my opinion if we do the latter we should now do testing (maybe automated?) to not run into weird quality/compression interactions for other formats.
@nrother Do you want to fix it for 19.04.1?

Is there actually any supported lossy format other than jpeg that uses quality? Maybe the slider value should ONLY apply to jpeg and the compression value should always be some predetermined value?
Is there any downside to setting the compression value to 100 for all lossless formats? Only thing I can think of is possibly a performance impact on very low power machines, though that requires testing.

JPEG 2000 and WebP are other (somewhat less popular) lossy formats. The Qt format writers switch to lossless mode for quality 100, see https://code.qt.io/cgit/qt/qtimageformats.git/tree/src/plugins/imageformats/webp/qwebphandler.cpp#n251 and https://code.qt.io/cgit/qt/qtimageformats.git/tree/src/plugins/imageformats/jp2/qjp2handler.cpp#n827

JPEG 2000 uses the quality as a file size percentage value, relative to 30% of the uncompressed size. For 2 bit per pixel (1:12 compression), this means a quality value of 28 is sufficient, while WebP needs a larger value to be free of visible artifacts.

In other words, we can only hope that users either stick to a single format and set the slider once, or change the value whenever they change the format. If not, a per-format setting can probably not be avoided.

P370 (Warning may write many files)
All judging from file sizes:
JPG/JPEG: Quality has an effect
PIC: compression 0 disables compression (enabled by default?)
PNG: see above
TIF/TIFF: same as PIC
WEBP: only quality seems to have an effect

19.04.1 tagging is on Monday, I think we should aim to fix it for this release. I don't have much time at the moment but I could put a patch together on Friday/the weekend.

That would be quite lovely. :)