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

Don't overwrite original if optimized is larger #95

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

R167
Copy link

@R167 R167 commented Aug 5, 2015

Currently, the original image gets overwritten if the optimized version is larger (happens in some cases). Changing it so that only the smaller image is preserved when optimize_image! is called.

@toy
Copy link
Owner

toy commented Aug 5, 2015

Thanks for contribution!

But there is a trick about this: workers are responsible for checking if the size was reduced (check usage of optimized? method in workers) and all but one use it. The only exception currently is jhead, but its task is to rotate jpeg images which have exif orientation set (otherwise further workers will remove exif and the image will not be oriented properly) and it can sometimes make image bigger. Also it is good to get rid of orientation flag and rotate the images so even viewers ignoring the orientation flag will display it properly.
So forbidding bigger output images can be added as an option, but not by default.

@R167
Copy link
Author

R167 commented Aug 5, 2015

Cool. Changed it so that it is now an optional parameter which is disabled by default. The main reason that I am adding this, is that some of the images that we were trying to optimize (all of which were pngs) were growing by a couple hundred to thousand bytes.

@toy
Copy link
Owner

toy commented Aug 6, 2015

It is very strange that png images are growing in size, is it possible for you to share some of them? It looks like a bug.

About the PR: I think it would be more meaningful to call the option replace_bigger or maybe even better to reverse and call it skip_bigger. Also instead of adding it as an option only for this method, it would be better to add it to config so it would be available for optimize_image and for the image_optim script. But first it is better to check why pngs are growing in size.

@R167
Copy link
Author

R167 commented Aug 6, 2015

Well crap, turns out that it was a miscommunication and instead of being a bug with the gem, it was a bug with how I was previously optimizing the images (before switching to image_optim). No one attached the commit SHA to the file compare so I misinterpreted the problem. Sorry about that.

@R167 R167 closed this Aug 6, 2015
@toy
Copy link
Owner

toy commented Aug 6, 2015

This pull request can still be useful for someone using image_optim on jpeg files, so if you want you can finish it and add the option to produce only smaller files :).

@R167 R167 reopened this Aug 7, 2015
README.markdown Outdated
@@ -234,6 +234,9 @@ end
image_optim.optimize_images!(Dir['*.*'])

image_optim.optimize_images_data(datas)

# Pass options like the sanity check
image_optim.optimize_images!(images, :always_replace => false)
Copy link
Owner

Choose a reason for hiding this comment

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

This is a remainder of previous commits.

@toy
Copy link
Owner

toy commented Aug 8, 2015

You've committed lib/.DS_Store. Probably it is better to add .DS_Store to .gitignore, I just have it in my global git ignore file.

@toy
Copy link
Owner

toy commented Jul 9, 2016

Hi Winston, I am sorry for long wait, are you willing to finish this PR?

@@ -3,6 +3,7 @@
## unreleased

* Use quality `0..100` by default in lossy mode of pngquant worker [#77](https://github.com/toy/image_optim/issues/77) [@toy](https://github.com/toy)
* Don't do anything when "optimized" version is larger and `:skip_bigger => true` [@R167](https://github.com/R167)
Copy link
Owner

Choose a reason for hiding this comment

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

Issue number is missing

@R167
Copy link
Author

R167 commented Jul 6, 2018

Do we know if this is still an issue/desirable to fix/merge?

@kaspergrubbe
Copy link
Contributor

I don't know if it is an issue, but if it is, it could be cool to have this functionality.

@toy
Copy link
Owner

toy commented Jul 8, 2018

@R167 It is still possible for jpegs to be overwritten with bigger file if original has exif rotation header, becomes bigger and is badly optimised

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.

None yet

3 participants