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

fix(website): resize showcase images, tighten CI check #6043

Merged
merged 17 commits into from
Jan 7, 2022

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Dec 3, 2021

Motivation

PR created as requested in: #6042 (comment)

mogrify -filter Triangle -define filter:support=2 -resize 640 -unsharp 0.25x0.25+8+0.065 -dither None -posterize 136 -quality 100 -define png:compression-filter=5 -define png:compression-level=9 -define png:compression-strategy=1 -define png:exclude-chunk=all -interlace none -colorspace sRGB -strip website/src/data/showcase/*.png

Related PRs

#6042

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 3, 2021
@armano2 armano2 marked this pull request as draft December 3, 2021 18:11
@armano2 armano2 changed the title fix(website): resize images to width 640 fix(website): resize images to width 640 (version 2) Dec 3, 2021
@armano2 armano2 closed this Dec 3, 2021
@armano2 armano2 deleted the resize-showcase-images2 branch December 3, 2021 18:12
@netlify
Copy link

netlify bot commented Dec 3, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 75a622d

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61d7d69237b3dd0007193c1a

😎 Browse the preview: https://deploy-preview-6043--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 72
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-6043--docusaurus-2.netlify.app/

@armano2 armano2 restored the resize-showcase-images2 branch December 3, 2021 18:18
@armano2 armano2 reopened this Dec 3, 2021
@armano2 armano2 force-pushed the resize-showcase-images2 branch from 5b1eb79 to bb4664b Compare December 3, 2021 18:21
@armano2 armano2 marked this pull request as ready for review December 3, 2021 18:35
@Josh-Cena
Copy link
Collaborator

image

Just picking up a random image. The original screenshot must have been optimised a lot🧐

@armano2
Copy link
Contributor Author

armano2 commented Dec 4, 2021

Just picking up a random image. The original screenshot must have been optimised a lot🧐

yes, they used 8b colors (256) in it, and due to anti-aliasing we have to use 16b colors (65536), we could update some of them manually, but i'm not sure if that's desired, i also noticed that there is one jpg image in this folder

8b color: 00101
16b color: 00101000


we can't force all of them it to 8b, as its going to produce

image)

instead of

image

in some cases

@Josh-Cena
Copy link
Collaborator

Just jumping in—there's also #3185. Does that one make sense? I'm not a big fan of WebP (not supported by every image viewer) but guess it could be more optimised?

@armano2
Copy link
Contributor Author

armano2 commented Dec 4, 2021

by changing format to webp we can save up 5 more mb (across all images), tho some of them are bigger


size for all images:

original images ~50mb
png/resized: ~10mb
webp/resized: ~5mb

@Josh-Cena
Copy link
Collaborator

Oh, just realized that the ideal image plugin we use probably doesn't support WebP... We can only apply WebP to other images until we fix that on the ideal image side

@armano2
Copy link
Contributor Author

armano2 commented Dec 4, 2021

for reference command for webp (executed on original images)

magick mogrify -filter Triangle -define filter:support=2 -resize 640 -unsharp 0.25x0.25+8+0.065 -dither None -posterize 136 -quality 100 -define webp:lossless=true -interlace none -strip -format webp website/src/data/showcase/*.png
magick mogrify -filter Triangle -define filter:support=2 -resize 640 -unsharp 0.25x0.25+8+0.065 -dither None -posterize 136 -quality 100 -define webp:lossless=true -interlace none -strip -format webp website/src/data/showcase/*.jpg

@Josh-Cena Josh-Cena added the pr: documentation This PR works on the website or other text documents in the repo. label Dec 5, 2021
@slorber
Copy link
Collaborator

slorber commented Dec 6, 2021

Thanks, will look at this later

Just a reminder: the goal is not necessarily to have the best-optimized format for source images, because those are not the images that end up being deployed on the website anyway. The goal is a format that is faster to load/resize multiple times.

Formats like AVIF may be good for some use-cases but afaik are not the fastest to load and require more CPU usage. Just reducing the source image size should be enough, we don't need to aim for maximum compression but good enough read/resize speed. Using advanced techniques that shrink the size more can actually make the situation worst for the use-case that we are trying to solve.

@Josh-Cena Josh-Cena changed the title fix(website): resize images to width 640 (version 2) fix(website): resize images to width 640 Dec 9, 2021
@Josh-Cena Josh-Cena force-pushed the resize-showcase-images2 branch from 0bbb233 to 451243b Compare January 6, 2022 05:37
@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Jan 6, 2022

No resizing method can make this file's size smaller... Can't imagine how it was originally encoded...

image

Edit. just dug up #4901. I think we can incorporate that

@Josh-Cena Josh-Cena changed the title fix(website): resize images to width 640 fix(website): resize showcase images, tighten CI check Jan 6, 2022
@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Jan 6, 2022

There isn't a way for us to conveniently push to a fork in an action, so I decided to just let the check fail if the image's aspect ratio is not good. We are going to require a width of 640, because it would be easy to fix for a maintainer (just run a script)

@slorber What do you think about it now? If throwing an error for foreseeably every showcase PR isn't too frustrating, let's do it :D

@slorber
Copy link
Collaborator

slorber commented Jan 6, 2022

What about requiring size > 640?

We could do one big resize PR every month instead of fixing PRs independently, as long as the resize is now easy and remains possible it should be good enough to accept larger images temporarily.

Why are many images smaller in width but larger in kb? Isn't it supposed to become smaller as we reduce their width?

@Josh-Cena
Copy link
Collaborator

Why are many images smaller in width but larger in kb? Isn't it supposed to become smaller as we reduce their width?

Apparently, the optimization in #4901 was so good that any resizing just makes the file bigger😄 See my screenshot above.

I guess the optimization deleted some metadata, since I don't know how PNGs can be optimized otherwise... But I can't use that tool since my M1 mac forbids me to build that binary 🤦‍♂️ I would fight against it later

@Josh-Cena
Copy link
Collaborator

Great, optimized the images so that all have a size reduction. Will merge this now

@Josh-Cena Josh-Cena merged commit 4578b8b into facebook:main Jan 7, 2022
@armano2 armano2 deleted the resize-showcase-images2 branch March 2, 2023 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: documentation This PR works on the website or other text documents in the repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants