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

Losslessly optimize out images and documents to save 53MB in the repository #471

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

C0rn3j
Copy link
Contributor

@C0rn3j C0rn3j commented Mar 25, 2024

Exact optimization parameters used can be seen here: https://rys.rs/optimization

The worrying changes are in SVGs, which, if other software relies on random metadata within them, need to be checked.

This PR attempts to losslessly optimize over 10K+ files, achieving massive file size savings at hopefully no cost to any compatibility.

All EXIF data in JPEGs/PNGs was retained.

Winner of this optimization PR is a GameCube SVG that went from 9MB to 7KB:

image

@C0rn3j C0rn3j changed the title Optimize images Losslessly optimize out images and documents to save 53MB in the repository Mar 25, 2024
@fpscan
Copy link
Collaborator

fpscan commented Mar 28, 2024

I am not sure how I can review these changes.

@baxysquare
Copy link
Collaborator

I am a bit nervous merging a PR that affects most of the files on the repository and could have a domino effect on the rest of the project. That said, I can also see the appeal of reducing the repository by 53MB, especially for devices with limited space.

I presume there is a way to rollback the Pull Release if we were to merge it and find that people were having issues with the new smaller files. @fpscan or @RobLoach if you could confirm that rollback is a possibility, that would be great.

If rollback is not an option, I would propose that this PR be closed and request that @C0rn3j consider submitting a new PR that limits the scope of the next PR to a single theme. For example, if the optimization was limited to /xmb/systematic/ and /Systematic/ for now, there is a backup of the original files at baxy-retroarch-themes/bytheme
/Systematic/
and we can restore them back to the current files with ease.

If things go well with the single theme, @C0rn3j could submit a larger PR that contained all the themes except Monochrome and see how it goes. That way, the default files for XMB and Ozone would not be touched, leaving most end users with a functioning system. If that went well, a third and final PR with Monochrome could be submitted.

Long-term, I'd really like to figure out a way that, as new files or themes are added, that optimization was a part of the process. For years, I have been optimizing my PNGs with ImageOptim, but had never thought about optimizing the SVGs.

@RobLoach
Copy link
Member

RobLoach commented Apr 1, 2024

A few things...

  • GitHub allows reverting merge requests. If we use squash, it'll be one commit, so it'll be even easier to see the change in one commit https://github.blog/2016-04-01-squash-your-commits/
  • As baxysquare mentioned, it will break all the other associated Pull Requests. Though those look to be a few years old.
  • I'd prefer for these optimizations to be in a script directly in the repository. Running xmb-convert.sh after this is merged, for instance, will overwrite these optimizations.
  • We've had issues with RetroArch not loading optimized images correctly before, so we'll want to test to make sure the new images load correctly before bringing this in
  • 9MB to 7KB is a win. Would it be worth bringing that GameCube image in as a first step, as it's almost 20% of the size savings?

@baxysquare
Copy link
Collaborator

Thanks @RobLoach for your perspective.

The long-term goal is to eliminate PNGs from the repository and require all themes to have 100% SVG source. The PNGs would be created and optimized via script, and that would dramatically reduce the file size of the repository, while also giving priority and flexibility in the build process. For example, a Vita build may only need 64² pixel icons or an 8K output of Lakka could need a 512² or 1024² set. They'd only have to build what they need, instead of getting everything and the kitchen sink as they do in the current paradigm. For more info, please check out #341.

@C0rn3j Hopefully these perspectives will help you decide what to do next. We really appreciate you going to the effort to make this PR and would appreciate your help with the repository.

@C0rn3j
Copy link
Contributor Author

C0rn3j commented Apr 1, 2024

9MB to 7KB is a win. Would it be worth bringing that GameCube image in as a first step, as it's almost 20% of the size savings?

I have pushed a much smaller PR which runs jpegoptim against the logo file, pdftk decompress/compress on two PDFs and svgo for the handful of the worst SVG files - #472

Repacking TTFs with fontools additionally gets some nice 300KB savings there - #473

Long-term, I'd really like to figure out a way that, as new files or themes are added, that optimization was a part of the process. For years, I have been optimizing my PNGs with ImageOptim, but had never thought about optimizing the SVGs.

I'd prefer for these optimizations to be in a script directly in the repository. Running xmb-convert.sh after this is merged, for instance, will overwrite these optimizations.

I have checked the mentioned xmb-convert.sh and found ANOTHER convert_xmb_assets.sh file which already runs optipng! And so do some other scripts!

optipng is practically superseded by oxipng however, so it should be swapped out.

I have NOT used --zopfli with oxipng for this PR, which gains considerable further savings at the cost of exponentially increased runtime - for this reason it might be a good idea to keep the exported PNGs in the repository as is the state right now.

I will do a --zopfli run for the final PNG changes - it takes a REALLY long time, unfortunately.

I have switched all shell scripts to oxipng, added the missing optimization step to xmb-convert.sh and also added --zopfli, which may be a bit controversial, but the space savings are worth it in my opinion. #474


I am not against splitting the work up further if necessary.
I've also noticed that due to a bug in svgo (which is fixed in master) not everything got processed, and another issue which I am still trying to get a hold of, so that 53MB reduction might be even more substantial in the end.

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