Photon: Auto-generate additional srcset options by kraftbj · Pull Request #3748 · Automattic/jetpack

kraftbj · 2016-04-29T16:17:02Z

WIP
See #2918


This PR generates additional srcset options based on a variety of factors.

Outstanding questions:
- What about $max_srcset_image_width? It is 1600px by default with a filter.
- Why does ^^ exist? What is the rationale for excluding srcset sources over 1600?
- Custom crops? Are we handling these gracefully or do we need some checking?
- Unit tests. I couldn't figure out how to mock it in tests while in flight, so just dove in.
- Per @lancewillett's [comment](https://github.com/Automattic/jetpack/issues/2918#issuecomment-215109837), would using `$content_width` provide more flexibility?

ChrisBegley · 2016-04-29T17:40:05Z

This looks great! I just tested this and it appears to be working as expected on images that are inserted into a post. It doesn't seem to be handling custom crops though. For example, I have 80x80 images on my sidebar, and the images are distorted because they aren't being cropped anymore:

The auto-generation is awesome, but if we wanted more control would it be possible to output our own srcset sources?

kraftbj · 2016-05-06T22:27:41Z

@ChrisBegley - Thanks for the testing and feedback. I believe you're right that I need to add checking to make sure the srcset proportions match the proportions of the original image if we're going to include it. The commit I just pushed does not include that yet.

we wanted more control would it be possible to output our own srcset sources?

Right now, with the PR as it stands, you can use Core's filter before priority 10 to amend the srcset values that Core natively provides, then use the filter in the PR to "short-circuit" the auto-creation.

kraftbj · 2016-05-06T22:31:51Z

In 1494174 :

Add a sizes array filter

Original patch props @aduth
Right now, if there is not something already filtering the sizes array and we have a content_width, we're using that to max out the sizes array.

Since we've been debating this point back and forth, you can disable this filter by adding a ?photontest=nosize query string to the URL for easy testing.

If the content_width is not set, we are using the original Core behavior for now. We can/should add another maximum, but I ran out of time today.

srcset improvements

If there is a content_width set, we will attempt to generate srcset values of 2x and 3x content_width, assuming that'll cover the maximum displayed size on the vast majority of HiDPI devices. (Need to test: What happens when using a 3.5x or 4x device? Does it still grab the full size? Is that okay)

If there is no content_width, we'll generate a few options at 80%, 60%, and 40% of the original image size.

Testing needed

Still needs a ton of testing, not ready for merge. Just to keep the conversation moving.

aduth · 2016-05-09T15:26:26Z

class.photon.php


 		// Responsive image srcset substitution
 		add_filter( 'wp_calculate_image_srcset', array( $this, 'filter_srcset_array' ), 10, 4 );
+		if ( ! has_filter( 'wp_calculate_image_sizes') ) { // To allow themes to fully control the sizes array.


Could we achieve the same effect by setting a lower priority to our filter, since if a theme had defined their own, it would be run and filtered later (assuming default priority of 10)?

I'm open to it. I don't have strong opinions on how we do it, as long as we let theme authors be smarter than we can be globally. I'm trying to think what kind of use case would modifying it before a theme gets it would cause problems; nothing yet.

josephscott · 2016-05-09T21:48:35Z

@ChrisBegley can you provide detailed steps on how to reproduce ( and make sure that you are using the current version of the patch ). I've tried out cropped images ( via image edit in wp-admin ) in posts and so far they appear normal. If there is something we are missing I want to make sure that we get reproduced so we can address it.

kraftbj · 2016-05-09T22:07:38Z

I think I have it duplicated. Used this in a plugin/theme functions:

add_image_size('kraft-hard-crop', 200, 200, true);
add_filter('image_size_names_choose', 'my_image_sizes');
function my_image_sizes($sizes) {
$addsizes = array(
"kraft-hard-crop" => __( "Kraft Hard Crop")
);
$newsizes = array_merge($sizes, $addsizes);
return $newsizes;
}

then inserted the "Kraft Hard Crop" image via wp-admin. The resulting srcset include hard resize Photon args from the core produced sizes (as expected) while the new ones from this PR are soft-cropped, e.g. https://i0.wp.com/kraftbj.wpsandbox.me/wp-content/uploads/2016/05/DSC_1387-1.jpg?w=4439&ssl=1 4439w, https://i0.wp.com/kraftbj.wpsandbox.me/wp-content/uploads/2016/05/DSC_1387-1.jpg?w=3329&ssl=1 3329w, https://i0.wp.com/kraftbj.wpsandbox.me/wp-content/uploads/2016/05/DSC_1387-1.jpg?w=2220&ssl=1 2220w"

ChrisBegley · 2016-05-10T05:58:09Z

@josephscott oops, I think I misunderstood what @kraftbj meant by "custom crops". I haven't tried inserting images that I cropped with WordPress into posts. I was referring to custom thumbnail images that I have in my theme that use crops. Like this:

add_image_size( 'recent-thumbnail', 80, 80, true );

josephscott · 2016-05-10T16:23:34Z

@ChrisBegley that makes sense, when I saw "crop" my mind when immediately to the crop tool in the image editor of wp-admin.

Just so I make sure I've got all of this right, with this patch you end up with http://i1.wp.com/techyourworld.com/wordpress/wp-content/uploads/2014/02/Superman-Man-of-Steel-Logo-Imgur.jpeg?resize=80%2C80 - what does pre-patch crop version look like?

Sorry, it is the http://i1.wp.com/techyourworld.com/wordpress/wp-content/uploads/2014/02/Superman-Man-of-Steel-Logo-Imgur.jpeg?w=576 URLs using the w that changed with this patch. Looks like the pre-patch behavior was to use resize for each version.

ChrisBegley · 2016-05-10T16:57:46Z

@josephscott yes, that's exactly right. Pre-patch it was using resize, so I got the crop and aspect ratio that I expected. Now it's using w, so it's not cropping the thumbnail images.

So I used to get this:
http://i1.wp.com/techyourworld.com/wordpress/wp-content/uploads/2014/02/Superman-Man-of-Steel-Logo-Imgur.jpeg?resize=80,80

Now I get something like this:
http://i1.wp.com/techyourworld.com/wordpress/wp-content/uploads/2014/02/Superman-Man-of-Steel-Logo-Imgur.jpeg?w=576

It's no longer being resized and cropped to the dimensions defined in my theme:
add_image_size( 'recent-thumbnail', 80, 80, true );

kraftbj · 2016-05-11T20:24:45Z

0ef8a63 provides some support for hard-cropped image sizes. @ChrisBegley, can you give it a whirl?

We probably still need to add checks, across the board, to not insert a srcset that is bigger than the full-size image. At this point, Photon will return the original size image in those cases, but not ideal.

(Updated for a new SHA. Left some debug code in previously)

ChrisBegley · 2016-05-11T22:13:36Z

@kraftbj looks great on my end! It automatically added in 2x and 3x versions too, which is awesome. Let me know if there's anything else you want me to take a look at.

kraftbj · 2016-05-11T22:33:15Z

Thanks @ChrisBegley! I added a check to make sure we're not adding entries larger than the full width image.

Before, it was doing things like for an original image size of 150x150:

https://i0.wp.com/kraftbj.wpsandbox.me/wp-content/uploads/2016/05/150150.png?w=150&ssl=1 150w,
https://i0.wp.com/kraftbj.wpsandbox.me/wp-content/uploads/2016/05/150150.png?resize=100%2C100&ssl=1 100w,
https://i0.wp.com/kraftbj.wpsandbox.me/wp-content/uploads/2016/05/150150.png?w=1208&ssl=1 1208w,
https://i0.wp.com/kraftbj.wpsandbox.me/wp-content/uploads/2016/05/150150.png?w=1812&ssl=1 1812w

Now, it'll abort for any variants where the new size is larger than the original, based on width.

kraftbj · 2016-05-12T18:23:09Z

Marking this for 4.0.4 as this behavior is a bug (making Photon less performant than previously).

jeherve · 2016-06-09T14:32:14Z

Made 2 minor changes, but otherwise this looks good to merge to me (and @kraftbj told me it was okay :) )

kraftbj · 2016-06-09T17:00:34Z

Do we want to squash this down until a couple of atomic commits?

samhotchkiss · 2016-06-10T03:18:49Z

Squashing would be great, @kraftbj -- also, Travis needs to pass :)

kraftbj · 2016-06-10T12:13:47Z

Squashed, rebased against master to pick up Travis improvements (this PR was still running the double-set of Travis builds). No tests were failing, just cancelled them due to yesterday's Travis backlog. Will merge/cherry-pick once tests pass.

Photon: Auto-generate additional srcset options

kraftbj added Bug When a feature is broken and / or not performing as intended [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Status] In Progress labels Apr 29, 2016

kraftbj added this to the 4.1 milestone Apr 29, 2016

kraftbj mentioned this pull request Apr 29, 2016

Photon: HiDPI + srcset #2918

Closed

aduth reviewed May 9, 2016
View reviewed changes

kraftbj added the [Focus] Performance label May 11, 2016

kraftbj force-pushed the fix/photon-big-image-srcset branch 2 times, most recently from 9934d41 to 0ef8a63 Compare May 11, 2016 20:33

kraftbj added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels May 12, 2016

kraftbj modified the milestones: 4.0.4, 4.1 May 12, 2016

kraftbj added the Unit Tests label May 12, 2016

jubstuff mentioned this pull request May 21, 2016

Display WordPress Posts Widget: Resize Featured Images #2151

Closed

josephscott mentioned this pull request May 31, 2016

Provide smaller images in the Carousel when the full-size is very large #3801

Merged

kraftbj removed the Unit Tests label Jun 6, 2016

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jun 9, 2016

kraftbj force-pushed the fix/photon-big-image-srcset branch from 244e972 to 888af95 Compare June 10, 2016 12:08

kraftbj added 3 commits June 10, 2016 07:11

Photon: Auto-generate additional srcset options

46e588d

Photon: Filter sizes array and use content_width for srcset

08abb5e

Photon: Update inline docs

aa59aef

kraftbj force-pushed the fix/photon-big-image-srcset branch from 888af95 to aa59aef Compare June 10, 2016 12:12

kraftbj merged commit ad6e38f into master Jun 10, 2016

matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 10, 2016

kraftbj added a commit that referenced this pull request Jun 10, 2016

Merge pull request #3748 from Automattic/fix/photon-big-image-srcset

aca4614

Photon: Auto-generate additional srcset options

kraftbj deleted the fix/photon-big-image-srcset branch June 10, 2016 16:54

jeherve added a commit that referenced this pull request Jun 13, 2016

Release Notes: add #3748, #4081, #3688

5cc06d3

jeherve added a commit that referenced this pull request Jun 14, 2016

Release Notes: add #3748, #4081, #3688

61c9399

octalmage mentioned this pull request Jan 18, 2017

Photon: Use array_replace instead of array_merge for filter_srcset_array. #6126

Merged

Conversation

kraftbj commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisBegley commented Apr 29, 2016

Uh oh!

kraftbj commented May 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kraftbj commented May 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add a sizes array filter

srcset improvements

Testing needed

Uh oh!

aduth May 9, 2016

Choose a reason for hiding this comment

Uh oh!

kraftbj May 9, 2016

Choose a reason for hiding this comment

Uh oh!

josephscott commented May 9, 2016

Uh oh!

kraftbj commented May 9, 2016

Uh oh!

ChrisBegley commented May 10, 2016

Uh oh!

josephscott commented May 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisBegley commented May 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kraftbj commented May 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisBegley commented May 11, 2016

Uh oh!

kraftbj commented May 11, 2016

Uh oh!

kraftbj commented May 12, 2016

Uh oh!

jeherve commented Jun 9, 2016

Uh oh!

kraftbj commented Jun 9, 2016

Uh oh!

samhotchkiss commented Jun 10, 2016

Uh oh!

kraftbj commented Jun 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kraftbj commented Apr 29, 2016 •

edited

Loading

kraftbj commented May 6, 2016 •

edited

Loading

kraftbj commented May 6, 2016 •

edited

Loading

josephscott commented May 10, 2016 •

edited

Loading

ChrisBegley commented May 10, 2016 •

edited

Loading

kraftbj commented May 11, 2016 •

edited

Loading