Add option to generate image srcsets by sebastianbenz · Pull Request #772 · ampproject/amp-toolbox

sebastianbenz · 2020-05-13T20:17:42Z

This adds a new OptimizeImages transformer generating srcset attributes for amp-img.

This transformer requires the following option:

imageOptimizer: a function for customizing the srcset generation. The function should return a URL pointing to a version of the src image with the given width. If no image is available, it should return a falsy value. For example: (src, width) => ${src}?width=${width}.

sebastianbenz · 2020-05-18T20:13:43Z

packages/optimizer/lib/transformers/OptimizeImages.js

+  constructor(config) {
+    this.log = config.log;
+    this.enabled = !!config.optimizeImages;
+    this.imageOptimizer = config.imageOptimizer || DEFAULT_IMAGE_OPTIMIZER;


I've also considered to always require users to provide a function and get rid of the optimizeImages flag. Not sure if the default is very useful.

why is that?
My preference would be to either keep it or have a few examples documented somewheres to ease the onboarding

My plan was to go with samples as well. Current thinking is to provide implementations for:

cloudiary

thumbor

static site (via eleventy-img wrapper)

custom sample

If we do this, then it probably makes sense to remove the default.

what would downstream tools (WP, next, etc) do? provide their own functions?

Correct, this is most likely something that users need to provide as frameworks usually don't optimize images by default.

patrickkettner · 2020-05-19T03:11:59Z

packages/optimizer/lib/transformers/OptimizeImages.js

+const MAX_IMG_SIZE = 820;
+const MIN_WIDTH_TO_ADD_SRCSET_IN_RESPONSIVE_LAYOUT = 300;
+const NUM_SRCSET_DPR = [1.0, 2.0, 3.0];
+const SRCSET_WIDTH = [


whole lot of magic numbers, does it make sense to add a comment as to why these numbers are being used?

added comments

packages/optimizer/lib/transformers/OptimizeImages.js

patrickkettner · 2020-05-19T03:15:45Z

packages/optimizer/lib/transformers/OptimizeImages.js

+  /**
+   * Sets the base width, i.e., renderered dimension measured in CSS pixels.
+   * Returns true if srcset is needed, that is, we'll resize the image to at
+   * least 2 legitimate widths.


what is a legitimate width?

added comment to the list of supported srcset widths

I meant like the phrase is confusing - in what way are other widths illegitimate? why are these widths legitimate?

patrickkettner · 2020-05-19T03:22:29Z

packages/optimizer/lib/transformers/OptimizeImages.js

+  }
+
+  /**
+   *  Returns true if there is more legitimate width.


im not sure if this means "returns true is there are additional widths that are also legitimate", or "returns true is there is a different width that is preferable to the current width"

packages/optimizer/lib/transformers/OptimizeImages.js

patrickkettner · 2020-05-19T03:27:51Z

packages/optimizer/lib/transformers/OptimizeImages.js

+  constructor(config) {
+    this.log = config.log;
+    this.enabled = !!config.optimizeImages;
+    this.imageOptimizer = config.imageOptimizer || DEFAULT_IMAGE_OPTIMIZER;


why is that?
My preference would be to either keep it or have a few examples documented somewheres to ease the onboarding

packages/optimizer/lib/transformers/OptimizeImages.js

sebastianbenz · 2020-05-19T06:32:20Z

@patrickkettner PTAL

googlebot · 2020-05-28T14:44:06Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-28T14:57:19Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

patrickkettner · 2020-05-28T16:29:19Z

packages/optimizer/lib/transformers/OptimizeImages.js

+const {skipNodeAndChildren} = require('../HtmlDomHelper');
+const {isValidImageSrcURL} = require('../URLUtils');
+
+// Don't generate srcsets for images larger than MAX_IMG_SIZE


any reason this is the max that is worth documenting? based on stats? device? processing power?

Good point. Leftover from cache implementation. Changed it to the max supported value.

patrickkettner · 2020-05-28T16:32:03Z

packages/optimizer/lib/transformers/OptimizeImages.js

+// All legimate srcset widths.
+const SRCSET_WIDTH = [39, 56, 82, 100, 150, 300, 500, 750, 1000, 1500, 2000, 2500];
+
+// The maximum number of values. We'll take the initial image width and generate more width values by


does "values" in this context mean "maximum number of [srcset sources]"?

yes, rephrased

patrickkettner

lgtm

couple of comment related things, but nothing flagrant.

patrickkettner · 2020-05-28T16:35:56Z

packages/optimizer/lib/transformers/OptimizeImages.js

+    this.log = config.log;
+    this.imageOptimizer = config.imageOptimizer;
+    this.srcsetWidth = new SrcsetWidth();
+    // TODO turn these into options


always cool to link to a tracking bug

googlebot · 2020-05-28T18:26:34Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-28T18:27:24Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-28T18:32:32Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-28T18:32:48Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

sebastianbenz · 2020-05-28T18:38:05Z

Thanks a lot for the great review @patrickkettner !

googlebot added the cla: yes label May 13, 2020

sebastianbenz requested a review from patrickkettner May 15, 2020 21:38

sebastianbenz added the enhancement label May 15, 2020

sebastianbenz marked this pull request as ready for review May 15, 2020 21:39

sebastianbenz force-pushed the image-transformer branch from 5f611a6 to 7683496 Compare May 18, 2020 20:10

sebastianbenz commented May 18, 2020

View reviewed changes

patrickkettner reviewed May 19, 2020

View reviewed changes

sebastianbenz requested a review from patrickkettner May 19, 2020 06:32

sebastianbenz force-pushed the image-transformer branch from 48d9b99 to 2a5161d Compare May 19, 2020 16:18

sebastianbenz force-pushed the image-transformer branch from 2a5161d to 64146e1 Compare May 28, 2020 14:43

googlebot added cla: no and removed cla: yes labels May 28, 2020

sebastianbenz added the toolbox-optimizer label May 28, 2020

sebastianbenz force-pushed the image-transformer branch from 64146e1 to c868d86 Compare May 28, 2020 14:46

sebastianbenz added cla: yes and removed cla: no labels May 28, 2020

patrickkettner reviewed May 28, 2020

View reviewed changes

patrickkettner approved these changes May 28, 2020

View reviewed changes

sebastianbenz force-pushed the image-transformer branch from c868d86 to 395a2ea Compare May 28, 2020 18:26

googlebot added cla: no and removed cla: yes labels May 28, 2020

sebastianbenz added cla: yes and removed cla: no labels May 28, 2020

fix(core): add missing dependency lru-cache

d5bb3a9

merceyz and others added 9 commits May 28, 2020 20:28

fix(optimizer): add missing dependencies

244176c

add srcset transformer

63fa511

Don't provide a default image optimizer + fix docs

9afd191

v2.5.0-alpha.0

3d5366a

Improve srcset generation

e8e3b9c

Improve docs & fine tune srcset values

a66766b

rebase against master and update deps

e1cbebb

align max value with supported srcset values

1196fdd

Clarifying comments + more srcset values

1c8ae22

sebastianbenz force-pushed the image-transformer branch from 395a2ea to 1c8ae22 Compare May 28, 2020 18:32

googlebot added cla: no and removed cla: yes labels May 28, 2020

sebastianbenz added cla: yes and removed cla: no labels May 28, 2020

sebastianbenz merged commit 153f137 into master May 28, 2020

sebastianbenz deleted the image-transformer branch May 28, 2020 18:36

sebastianbenz changed the title ~~add srcset transformer~~ Add option to generate image srcsets May 28, 2020

Conversation

sebastianbenz commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sebastianbenz commented May 19, 2020

Uh oh!

googlebot commented May 28, 2020

Uh oh!

googlebot commented May 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickkettner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented May 28, 2020

Uh oh!

googlebot commented May 28, 2020

Uh oh!

googlebot commented May 28, 2020

Uh oh!

googlebot commented May 28, 2020

Uh oh!

sebastianbenz commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sebastianbenz commented May 13, 2020 •

edited

Loading