-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
new_audit: efficient-animated-content - use videos instead of gifs #4885
Conversation
@@ -35,13 +35,13 @@ module.exports = [ | |||
extendedInfo: { | |||
value: { | |||
results: { | |||
length: 18, | |||
length: 19, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to bump these as I added another request to the dbw tester file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we finally just convert this to '>15'
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! not a whole lot to this one :)
const WebInspector = require('../../lib/web-inspector'); | ||
|
||
// the threshold for the size of GIFs wich we flag as unoptimized | ||
const GIF_BYTE_THRESHOLD = 10 * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1024? :)
return { | ||
name: 'uses-optimized-animated-images', | ||
informative: true, | ||
description: 'Use a video format for animated content', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/a format/formats
name: 'uses-optimized-animated-images', | ||
informative: true, | ||
description: 'Use a video format for animated content', | ||
helpText: 'no help here, your on your own!', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my take: "Large GIFs are inefficient for delivering animated content. Consider using MPEG4/WebM videos for animations and PNG/WebP for static images instead of GIF to save network bytes. Learn more."
wikipedia link
@@ -35,13 +35,13 @@ module.exports = [ | |||
extendedInfo: { | |||
value: { | |||
results: { | |||
length: 18, | |||
length: 19, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we finally just convert this to '>15'
or something?
const WebInspector = require('../../lib/web-inspector'); | ||
|
||
// the threshold for the size of GIFs wich we flag as unoptimized | ||
const GIF_BYTE_THRESHOLD = 10 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make it 100kb.
we just looked at 3000 GIFs and encoded 2200 of them (the valid ones).
and then did an analysis. looks like 92% of gifs above 100k are animated. and of those, only 2% of them get bigger as a video.
SO WE GOOD.
4893f20
to
4a6083e
Compare
4a6083e
to
978758a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I think there's just some 🚲 🏠 left to do on learn more link, and audit naming
description: 'Use video formats for animated content', | ||
helpText: 'Large GIFs are inefficient for delivering animated content. Consider using ' + | ||
'MPEG4/WebM videos for animations and PNG/WebP for static images instead of GIF to save ' + | ||
'network bytes. [Learn more](https://en.wikipedia.org/wiki/Video_alternative_to_GIF)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulirish @addyosmani any better link here? when does that post go live :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it wont be live within the next week. But hopefully soon after that? cc @malchata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. That's the plan. I want to blast through feedback in the next couple days and get it into a PR. Knowing the PR process, it'd be next week.
*/ | ||
static get meta() { | ||
return { | ||
name: 'uses-optimized-animated-images', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to 🚲 🏠 this name, people seemed to be real confused with our last "optimized" images audit
uses-optimized-animated-content
uses-efficient-animations
optimizes-animated-content
none of those really seem better to me than what you've got, but others may have ideas :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call. since the recommendation is to use video, it's kinda weird to use this current name.
efficient-animated-content
is my proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go for an opportunity!
Over here I left a fn we can use to estimate savings for each file.
we can then use the byteEfficiency method for computing overall savings.
wdyt?
*/ | ||
static get meta() { | ||
return { | ||
name: 'uses-optimized-animated-images', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call. since the recommendation is to use video, it's kinda weird to use this current name.
efficient-animated-content
is my proposal.
@@ -290,6 +291,7 @@ module.exports = { | |||
{id: 'screenshot-thumbnails', weight: 0}, | |||
{id: 'mainthread-work-breakdown', weight: 0, group: 'perf-info'}, | |||
{id: 'font-display', weight: 0, group: 'perf-info'}, | |||
{id: 'uses-optimized-animated-images', weight: 0, group: 'perf-info'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this a perf-hint? feels pretty equivalent to optimized-images, which is an opportunity.
(will also need to change informative
)
}); | ||
|
||
const headings = [ | ||
{key: 'url', itemType: 'url', text: 'Url'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL
052b08c
to
1ac3263
Compare
89f43e6
to
8cd3119
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % nits
return { | ||
name: 'efficient-animated-content', | ||
scoreDisplayMode: ByteEfficiencyAudit.SCORING_MODES.NUMERIC, | ||
description: 'Use a video formats for animated content', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/a video formats/video formats/
;)
/** | ||
* Calculate savings percentage | ||
* @param {number} bytes | ||
* @see https://github.com/GoogleChrome/lighthouse/issues/4696#issuecomment-380296510} bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo'd }
} | ||
|
||
/** | ||
* Calculate savings percentage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calculate rough savings percentage based on 1000 real gifs transcoded to video
* @see https://github.com/GoogleChrome/lighthouse/issues/4696#issuecomment-380296510} bytes | ||
*/ | ||
static getPercentSavings(bytes) { | ||
return (29.1 * Math.log10(bytes) - 100.7) / 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you throw a math.round() around the numerator?
const WebInspector = require('../../lib/web-inspector'); | ||
const ByteEfficiencyAudit = require('./byte-efficiency-audit'); | ||
|
||
// the threshold for the size of GIFs wich we flag as unoptimized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo (wich), but let's rewrite this anwyay
If GIFs are above this size, we'll flag them
See #4885 (comment) and #4696 (comment)
description: 'Use a video formats for animated content', | ||
helpText: 'Large GIFs are inefficient for delivering animated content. Consider using ' + | ||
'MPEG4/WebM videos for animations and PNG/WebP for static images instead of GIF to save ' + | ||
'network bytes. [Learn more](https://en.wikipedia.org/wiki/Video_alternative_to_GIF)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
55f5185
to
50e3289
Compare
@paulirish I believe everything is fixed |
b597d17
to
5f9c066
Compare
Indeed, no idea why locally it's always green |
@paulirish @patrickhulce said we should go for this implementation and he was going to look at it later. Sorry if I misunderstood @patrickhulce 😛 |
items: [ | ||
{ | ||
url: 'http://localhost:10200/dobetterweb/lighthouse-rotating.gif', | ||
totalBytes: 934285, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 yeah @paulirish I told ward I'd tackle a more fine-grained assertion when I finish the other lantern opportunity rejiggering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm i like the look of this
Fixes #4696
Only number 1 is covered by this one
So I check mimetype and webinspector type as mentioned in our meeting. If you could give some pointers on the metadata part and maybe the audit name 😛 I would appreciate that.
@addyosmani @patrickhulce @paulirish