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.

Sign up for GitHub

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

Jump to bottom

Add GIF compression #748

Merged
merged 10 commits into from
Jul 17, 2020
Merged

Add GIF compression #748

merged 10 commits into from
Jul 17, 2020

Conversation

amorriscode
Copy link
Contributor

@amorriscode amorriscode commented Nov 2, 2019 •
edited
Loading

This PR attempts to resolve #117.

It appears the current setup doesn't do lossy compression so I need help/need to tweak that. I can use imagemin-giflossy for guidance.

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

IssueHunt has been backed by the following sponsors. Become a sponsor

@skllcrn skllcrn requested review from sindresorhus and karaggeorge and removed request for sindresorhus November 2, 2019 12:30
@sindresorhus
Copy link
Contributor

sindresorhus commented Nov 6, 2019 •
edited
Loading

Why are you using both gifsicle and giflossy? gifsicle supports lossy compression: https://github.com/kohler/gifsicle/blob/master/NEWS.md#version-192--18apr2019

@amorriscode
Copy link
Contributor Author

@sindresorhus I'll try to rework the PR. I swear I tried using --lossy on gifsicle and had nothing but errors.

@sindresorhus
Copy link
Contributor

Bump, in case you forgot about this. If you're just busy, feel free to ignore the bump :)

@amorriscode
Copy link
Contributor Author

@sindresorhus sorry about that! I'll take a look at this this weekend. 🙂

@amorriscode
Copy link
Contributor Author

@sindresorhus longest weekend of my life! 😅
I removed giflossy and got compression working.

@amorriscode amorriscode force-pushed the add-gif-compression branch from a38fae2 to 4799bb3 Compare March 31, 2020 13:21
@amorriscode
Copy link
Contributor Author

@sindresorhus @karaggeorge I rebased this morning! This is ready for review again. :)

@amorriscode amorriscode force-pushed the add-gif-compression branch from 4799bb3 to 8eaa318 Compare May 26, 2020 02:51
@amorriscode
Copy link
Contributor Author

amorriscode commented May 26, 2020 •
edited
Loading

@karaggeorge Friendly little bump! 👋

sindresorhus
sindresorhus suggested changes Jun 22, 2020
View reviewed changes
main/convert.js Outdated Show resolved Hide resolved
main/convert.js Outdated Show resolved Hide resolved
renderer/components/preferences/categories/general.js Outdated Show resolved Hide resolved
@amorriscode amorriscode force-pushed the add-gif-compression branch from 8eaa318 to a29cd2f Compare June 23, 2020 20:13
@amorriscode
Copy link
Contributor Author

amorriscode commented Jun 23, 2020 •
edited
Loading

@sindresorhus Here are some results I compiled today. Based on this, I think it's better to lower the compression down to 50. I believe the default is 20 according to the gifsicle man page.

Compression With Kap Resize

I made this GIF then exported it from Kap after resizing. Then I used gifsicle to test the compression results.

No Compression - 6.6 MB

comp-off

--lossy=50 - 4.9 MB

comp-50

--lossy=100 - 4.7 MB

comp-100

--lossy=150 - 4.6 MB

comp-150

--lossy=200 - 4.6 MB

comp-200

--lossy=500 - 4.3 MB

comp-500

Compression Without Kap Resize

I made this GIF then exported it from Kap without resizing. Then I used gifsicle to test the compression results.

No Compression - 2.4 MB

noresize-comp-off

--lossy=50 - 1.6 MB

noresize-comp-50

--lossy=100 - 1.6 MB

noresize-comp-100

--lossy=200 - 1.5 MB

noresize-comp-200

--lossy=500 - 1.5 MB

noresize-comp-500

sindresorhus
sindresorhus reviewed Jul 16, 2020
View reviewed changes
renderer/components/preferences/categories/general.js Outdated
key="lossyCompression"
parentItem
title="Lossy compression"
subtitle="Compress GIFs with lossy compression if you prefer small file size over quality"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could make it clearer that the quality is only slightly reduced.

@sindresorhus
Copy link
Contributor

Thanks for testing the different qualities. This PR looks good to me know when the inline comment is resolved and the merge conflict is fixed.

@sindresorhus
Copy link
Contributor

@karaggeorge ?

karaggeorge
karaggeorge reviewed Jul 17, 2020
View reviewed changes
main/convert.js Outdated Show resolved Hide resolved
@amorriscode amorriscode force-pushed the add-gif-compression branch from 1771e47 to 962aeee Compare July 17, 2020 15:18
@amorriscode @sindresorhus
Adjust subtitle based on suggestion
3d04f1e 
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@sindresorhus sindresorhus merged commit cc34315 into wulkano:master Jul 17, 2020
@sindresorhus
Copy link
Contributor

Thanks for working on this, @amorriscode 👍🏻

@mootari mootari mentioned this pull request Jan 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus requested changes

@karaggeorge karaggeorge karaggeorge approved these changes

Assignees
No one assigned
Labels
Do Not Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add lossy compression option to gif export
4 participants
@amorriscode @sindresorhus @karaggeorge @skllcrn