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

Gui: Add default JPEG save quality #7737

Merged
merged 2 commits into from
Nov 11, 2022
Merged

Conversation

vernzimm
Copy link
Sponsor Contributor

@vernzimm vernzimm commented Nov 7, 2022

Set jpg screenshot capture to 100% quality vs qimage defaulting to -1
Save Picture/Export PDF quality issues

  • Your pull request is confined strictly to a single module.
  • In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
  • Your branch is rebased on latest master git pull --rebase upstream master
    Working in github only. Do I need to do this?
  • All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
    Working in github only. I honestly don't know how to do this -_-
  • All commit messages are well-written
  • Your pull request is well written and has a good description, and its title starts with the module name
  • Commit messages include issue # or fixes # where is the issue ID number from our Issues database in case a particular commit solves or is related to an existing issue.

@github-actions github-actions bot added the Mod: Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Nov 7, 2022
@adrianinsaval
Copy link
Member

how much does this affect the generated file size?

@chennes
Copy link
Member

chennes commented Nov 8, 2022

Thanks for the PR, @vernzimm -- the code itself doesn't quite work (C++ doesn't permit named arguments like that), but I understand what you're driving at. I did a quick test, @adrianinsaval, and the two attached JPEGs are 122kb (pre-PR) and 347kb (post-PR). So in this case it's nearly a 3x size increase, for a pretty minimal quality increase. @vernzimm can you attach a test case that better illustrates what you are trying to improve?

DemoTestOnePrePR

DemoTestOnePostPR

@freecadci
Copy link

pipeline status for feature branch PR_7737. Pipeline 688941178 was triggered at 7944108. All CI branches and pipelines.

@vernzimm
Copy link
Sponsor Contributor Author

vernzimm commented Nov 8, 2022

Hello everyone, I appreciate all of the feedback, and I apologize if this is wasting anyone's time.

@adrianinsaval @chennes here is the discussion about this and a few other things
Save Picture/Export PDF quality issues

Basically, I'm considering 300KB to be negligible. I would guess that the vast majority of cases do not have a consideration of file size at that level, or if they do, they can downgrade. Currently I can not upgrade (I have begun exporting to .png and then converting to .jpg). As you know, many programs do include a quality option for .jpg export. I do not possess the ability to implement that :(

The compromise may be, due to the very nature of the .jpg compression, we go with something like 90%. It is 145KB with very little noticeable artifacts.
90

Text is also a major issue, but 90% seems to keep that well defined.
90 text

@chennes about the code, I was working from QT docs that someone looked up for me. I think I was reading the doc formatting wrong :). I'm guessing it would be simply
image.save(&buffer, "JPG", 100);?

@vernzimm
Copy link
Sponsor Contributor Author

vernzimm commented Nov 8, 2022

@adrianinsaval @chennes I was just looking at the "checks" that failed. They did fail for that code. Is there a way I can run those before sending PR?

@chennes
Copy link
Member

chennes commented Nov 8, 2022

Yes, this is the correct code:

image.save(&buffer, "JPG", 100);

(or, as you suggest, use 90 instead of 100). Do you know how to edit your previous commit and force-push the change? If not, you can just add a new commit with the fix and I will squash when merging.

Set jpg screenshot capture to 100% quality vs qimage defaulting to -1
Fixed original version and use quality 90% as compromise between file size and fidelity.
Copy link
Sponsor Contributor Author

@vernzimm vernzimm left a comment

Choose a reason for hiding this comment

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

Correct previous attempt and change to 90% from 100%

@vernzimm
Copy link
Sponsor Contributor Author

vernzimm commented Nov 8, 2022

@chennes I think I got it? Rebase from master, edit the file, and commit that into vernzimm-patch-1 branch. It's not really editing the original pull request though, in which case no, I don't know 😆

@freecadci
Copy link

pipeline status for feature branch PR_7737. Pipeline 689248893 was triggered at 3952f73. All CI branches and pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_7737. Pipeline 689252408 was triggered at 549398b. All CI branches and pipelines.

@chennes
Copy link
Member

chennes commented Nov 9, 2022

It's not necessary for you to do anything else, I am happy to squash and merge on my end, but if you are interested in learning, the operation you want to do is an "interactive rebase" which you will use to "squash" the commits and then "force push" the resulting single commit, overwriting the original commits. On the command line, it goes like this (in the case of two commits being combined into one):

git rebase -i HEAD~2

This will pop up an editor window that lists your two commits, looking like this:

pick 3952f7393c Update SoFCOffscreenRenderer.cpp
pick 549398b340 Add default quality to JPG export 

There are some instructions in a comment section below, but what you want to do is change the second pick to squash (or just s):

pick 3952f7393c Update SoFCOffscreenRenderer.cpp
s 549398b340 Add default quality to JPG export 

Save the file and close it. Another editor will pop up offering to let you create a commit message for the new, combined commit. Set it to whatever you want (probably just the commit message you wrote for the first commit). Save and close. Finally, you have to overwrite the commits in your branch to just consist of the new commit:

git push -f

If you refresh the GitHub page for your PR, you'll see that there is now only the single commit.

@luzpaz luzpaz changed the title GUI : Add default Jpeg save quality Gui: Add default JPEG save quality Nov 9, 2022
@chennes chennes merged commit 5f4dbf7 into FreeCAD:master Nov 11, 2022
@donovaly
Copy link
Member

Can this be backported for FC 0.20.2? To me this seems like it improves the situation for average users without adding new functionality. If you agree, it fulfills the criterion for backporting.

@chennes
Copy link
Member

chennes commented Nov 11, 2022

Sure, should just be a simple cherry pick. I'll take care of it later today.

donovaly pushed a commit that referenced this pull request Nov 11, 2022
* Update SoFCOffscreenRenderer.cpp

Set jpg screenshot capture to 100% quality vs qimage defaulting to -1

* Add default quality to JPG export

Fixed original version and use quality 90% as compromise between file size and fidelity.
@donovaly
Copy link
Member

Thanks, I backported it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mod: Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants