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

new_audit: efficient-animated-content - use videos instead of gifs #4885

Merged
merged 15 commits into from
Apr 27, 2018

Conversation

wardpeet
Copy link
Collaborator

Fixes #4696

Only number 1 is covered by this one

Implement an audit that identifies GIFs from network records by contentType and flags all GIFs over 10kb with some title "Use a video format for animated content" (or something much better someone comes up with 😄), description calls out that if your GIF is not animated consider PNG/WebP

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

@@ -35,13 +35,13 @@ module.exports = [
extendedInfo: {
value: {
results: {
length: 18,
length: 19,
Copy link
Collaborator Author

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

Copy link
Collaborator

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?

Copy link
Collaborator

@patrickhulce patrickhulce left a 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;
Copy link
Collaborator

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',
Copy link
Collaborator

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!',
Copy link
Collaborator

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,
Copy link
Collaborator

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;
Copy link
Member

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.

Copy link
Collaborator

@patrickhulce patrickhulce left a 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)',
Copy link
Collaborator

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 :)

Copy link
Member

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

Copy link
Member

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',
Copy link
Collaborator

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 :)

Copy link
Member

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.

Copy link
Member

@paulirish paulirish left a 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',
Copy link
Member

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'},
Copy link
Member

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'},
Copy link
Member

Choose a reason for hiding this comment

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

URL

Copy link
Member

@paulirish paulirish left a 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',
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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;
Copy link
Member

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
Copy link
Member

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)',
Copy link
Member

Choose a reason for hiding this comment

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

@paulirish paulirish changed the title new-audit(optimized-animated-content): use videos instead of gifs new-audit(efficient-animated-content): use videos instead of gifs Apr 23, 2018
@wardpeet
Copy link
Collaborator Author

@paulirish I believe everything is fixed

@paulirish paulirish changed the title new-audit(efficient-animated-content): use videos instead of gifs new_audit(efficient-animated-content): use videos instead of gifs Apr 25, 2018
@paulirish paulirish changed the title new_audit(efficient-animated-content): use videos instead of gifs new_audit: efficient-animated-content - use videos instead of gifs Apr 25, 2018
@paulirish
Copy link
Member

paulirish commented Apr 25, 2018

not sure why smoketests failed on appveyor but not travis
but i saw this same failure on the previous travis run here.

also check out the displayValue. needs some toLocaleString, probably via a util method.

image

ignore the commitlint failure. that's my bug. :/

@wardpeet
Copy link
Collaborator Author

Indeed, no idea why locally it's always green

@wardpeet
Copy link
Collaborator Author

@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,
Copy link
Collaborator

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

Copy link
Member

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

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.

5 participants