Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add JPEG Quality Option #3285

Merged
merged 10 commits into from
Sep 16, 2023
Merged

Conversation

mdphillips375
Copy link
Contributor

@mdphillips375 mdphillips375 commented Jul 30, 2023

JPEG is a lossy compression algorithm that trades quality for file size. This update adds an option to the general options which allows the user to set the quality and passes that on in the places where there is a possibility to write an image (either to a file or buffer) in JPEG format. The changes to the calls to QPixmap::save() and QImageWriter::setQuality() are wrapped in if statements to check the format so that these changes do not impact (the probably more common) PNG format, which also uses the same parameter differently (a PNG with maximum compression is still lossless).

This change addresses issue #3216 and #506 by giving the user the choice of a higher quality, but larger file.

{
auto* tobox = new QHBoxLayout();

int timeout =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the "timeout" variable name intentional? If not, I suggest changing it to something more fitting to improve code readability

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not. I copied and pasted code from a similar configuration item to insure consistent formatting and placement and forgot to change that int. Will fix later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix has been pushed.

@mmahmoudian
Copy link
Member

@mdphillips375 Thanks for the PR and the time you have invested in Flameshot. Let's wait for other devs to also go through the code and approve it. Cheers.

@mmahmoudian mmahmoudian added this to the v13 milestone Jul 31, 2023
@jack9603301
Copy link
Contributor

Looks good

@jack9603301
Copy link
Contributor

图片

I think you can clean up those commits, they are pointless for this PR

mmahmoudian and others added 3 commits August 2, 2023 08:31
Add JPEG quality option

Signed-off-by: Mike Phillips <mdphillips375@gmail.com>

Revert "Add JPEG quality option"

This reverts commit 046497d.
@mmahmoudian
Copy link
Member

@mdphillips375 thanks for handling the comments. I set it to run the Actions.

One question though: the range is 0-100. What does 0 and 100 mean? Is 100 without any compression? And what is 0?

@mdphillips375
Copy link
Contributor Author

Looking through the sources to libjpeg, I see several references to 100 being lossless. It certainly has the visual appearance of being lossless. I can say that 0 is pretty bad for most things I would personally use this software for. A screenshot of text is not legible. It will turn a photograph into an image resembling a low resolution 8-bit (or lower) display, which could actually be desirable in some cases (retro gaming look).

Qt5 defaults to setting the quality to 75 if you do not pass a quality parameter, which is the case in the current release, so I set the default to 75.

As for a size comparison of the 100 setting, I took a screenshot test I did with Quality = 100, converted to uncompressed TIF, and PNG with maximum compression using GIMP. It is certainly not uncompressed, although not as small as PNG's lossless compression.

Size Comparison of screens

@mmahmoudian
Copy link
Member

@panpuchkov @veracioux do you see any issues to be discussed here or do you have any objections?

Copy link
Contributor

@veracioux veracioux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also merge master into this branch, so that this PR can be tested against a CI/CD pipeline that has had some fixes applied. There is also a conflict that should be resolved.

src/utils/confighandler.h Outdated Show resolved Hide resolved
src/config/generalconf.cpp Outdated Show resolved Hide resolved
src/config/generalconf.cpp Show resolved Hide resolved
src/utils/confighandler.cpp Show resolved Hide resolved
@veracioux
Copy link
Contributor

@mdphillips375 Thanks for the changes. Please also run clang-format as described here. We require this on all PRs before merge.

@mdphillips375
Copy link
Contributor Author

Fixed. Sorry about that.

@veracioux veracioux merged commit c97c3aa into flameshot-org:master Sep 16, 2023
21 of 22 checks passed
panpuchkov pushed a commit to namecheap/flameshot that referenced this pull request Dec 22, 2023
* fixes flameshot-org#3188 (flameshot-org#3254)

Add JPEG quality option

Signed-off-by: Mike Phillips <mdphillips375@gmail.com>

Revert "Add JPEG quality option"

This reverts commit 046497d.

* Add JPEG Quality Option

* Fix local variable name

* JPEG Quality: Add .ini example and minor fixes.

* JPEG Quality: Add .ini example and minor fixes.

* Fix Formatting

---------

Co-authored-by: Mehrad Mahmoudian <m.mahmoudian@gmail.com>
(cherry picked from commit c97c3aa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants