Refine logic to select smaller image file in the frontend based on `webp_uploads_prefer_smaller_image_file` filter by jjgrainger · Pull Request #302 · WordPress/performance

jjgrainger · 2022-04-17T18:13:28Z

Summary

Fixes #292

Relevant technical choices

Refines logic to select the smaller image file, regardless of mime type, when the webp_uploads_prefer_smaller_image_file filter is enabled.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

felixarntz

@jjgrainger Mostly looks good, it needs a few small refinements. Most importantly though, I think we should remove the function previously introduced since it's not really helpful or accurate anyway. The new approach here works good just by itself.

modules/images/webp-uploads/load.php

felixarntz

@jjgrainger This looks close, two follow-up comments though. One is actually critical, the PR currently doesn't cover the full sized image yet.

Last but not least, can you also add tests for the new logic?

modules/images/webp-uploads/load.php

mitogh

Besides the comment from @felixarntz this looks good to me @jjgrainger

Thanks.

felixarntz

@jjgrainger Production logic now looks good to me, just one nit-pick on the filter and a potential issue with the test.

modules/images/webp-uploads/load.php

felixarntz · 2022-04-27T20:14:41Z

tests/modules/images/webp-uploads/load-tests.php

+		// File produces larger webp image, original jpg is smaller.
+		$attachment_id = $this->factory->attachment->create_upload_object( TESTS_PLUGIN_DIR . '/tests/testdata/modules/images/leafs.jpg' );
+		$metadata      = wp_get_attachment_metadata( $attachment_id );
+		$tag           = wp_get_attachment_image( $attachment_id, 'full', false, array( 'class' => "wp-image-{$attachment_id}" ) );
+		$updated_tag   = webp_uploads_img_tag_update_mime_type( $tag, 'the_content', $attachment_id );

-		$mime_types = webp_uploads_get_mime_types_by_filesize( array( 'image/jpeg', 'image/webp' ), $attachment_id, 'the_content' );
+		// The full size WebP is smaller.
+		$expected_tag = str_replace( $metadata['sources']['image/jpeg']['file'], $metadata['sources']['image/webp']['file'], $tag );

-		$this->assertIsArray( $mime_types );
-		$this->assertNotContains( 'image/invalid', $mime_types );
-		$this->assertSame( array( 'image/jpeg', 'image/webp' ), $mime_types );
+		$this->assertSame( $expected_tag, $updated_tag );


Is this test in fact doing what the comment above says? It says this test covers the case where JPEG is smaller than WebP, but then for the $expected_tag it replaces the JPEG version with WebP, so that seems not right. If the JPEG version is smaller, the test should expect that the JPEG version is still used even though a WebP version exists.

Thanks @felixarntz

I've updated the comments to make things clearer and reviewed the logic and can confirm it is correct. This image produces a smaller WebP file size of the full image. All sub sizes have a smaller JPG.

I've taken an example of the image code when the webp_uploads_prefer_smaller_image_file filter is set to true here:

<img loading="lazy" width="1080" height="720" src="http://localhost:8888/wp-content/uploads/2022/04/leafs.webp" alt="" class="wp-image-15" srcset="http://localhost:8888/wp-content/uploads/2022/04/leafs.webp 1080w, http://localhost:8888/wp-content/uploads/2022/04/leafs-300x200.jpg 300w, http://localhost:8888/wp-content/uploads/2022/04/leafs-1024x683.jpg 1024w, http://localhost:8888/wp-content/uploads/2022/04/leafs-768x512.jpg 768w" sizes="(max-width: 1080px) 100vw, 1080px">

From the srcset you can see all sub sizes use the JPG version as they are smaller than their WebP version.

The replace updates the original image tag, which uses only JPG, to replace the full size with the WebP version in order to match the output of webp_uploads_img_tag_update_mime_type when the webp_uploads_prefer_smaller_image_file is set to true.

I've also created a shorthand version of the metadata array to compare filesizes and this is correct.

Array ( [file] => 2022/04/leafs.jpg [full] => Array ( [image/jpeg] => 336884 [image/webp] => 328322 ) [medium] => Array ( [image/jpeg] => 19820 [image/webp] => 22332 ) [large] => Array ( [image/jpeg] => 237115 [image/webp] => 257974 ) [thumbnail] => Array ( [image/jpeg] => 8082 [image/webp] => 9008 ) [medium_large] => Array ( [image/jpeg] => 135786 [image/webp] => 153226 ) )

Ah, thanks for clarifying. That makes sense, in that case indeed the updated comments look better.

felixarntz

Great work @jjgrainger

felixarntz · 2022-05-02T22:22:00Z

@adamsilverstein @mitogh Can one of you please review this as well so we can get it merged?

adamsilverstein

Change looks good to me (although I think webp_uploads_prefer_smaller_image_file could start true here), I'll work on adding something similar to the core patch.

adamsilverstein · 2022-05-03T21:54:21Z

modules/images/webp-uploads/load.php

-	if ( apply_filters( 'webp_uploads_prefer_smaller_image_file', false ) ) {
-		$target_mimes = webp_uploads_get_mime_types_by_filesize( $target_mimes, $attachment_id );
-	}
+	$prefer_smaller_image_file = apply_filters( 'webp_uploads_prefer_smaller_image_file', false );


working on adding to core patch so this could be true

@adamsilverstein Not sure we should add this to the initial core patch as it feels like an extra enhancement. The main patch has a ton of code already and this enhancement could easily be separate.

Ok, I'll add it as a separate patch

adamsilverstein · 2022-05-03T22:01:52Z

modules/images/webp-uploads/load.php

+		if (
+			$prefer_smaller_image_file &&
+			! empty( $metadata['sources'][ $target_mime ]['filesize'] ) &&
+			! empty( $metadata['sources'][ $original_mime ]['filesize'] ) &&


Looking at this part I realized that there could potentially be several mime types in the sources array (eg. JPEG, WebP, AVIF), we should probably check all of them for the smallest size, rather than just the original and the mapped type.

What do you think @felixarntz ? Fine to address in a follow up so we can get this merged since it covers our primary use case as is.

@adamsilverstein That's a good point, I think eventually that would make sense. It would require a bit more complex reworking though indeed, for example right now we only have a single $target_mime, and then there will need to be a list, all of which we would need to go through.

+1 to doing it in a separate issue.

I'm taking the "look thru every mime type for the smallest" approach for the (follow up) core patch, basically go thru all sources to find the smallest and use that:

foreach( $metadata['sources'] as $source ) { if ( empty( $source['filesize'] ) ) { continue; } if ( $source['filesize'] < $target_file['filesize'] ) { $target_file = $source; } }

add logic to select smaller image size when enabled

dae0793

jjgrainger added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Images no milestone PRs that do not have a defined milestone for release labels Apr 17, 2022

jjgrainger requested a review from felixarntz April 17, 2022 18:13

jjgrainger requested review from adamsilverstein and getsource as code owners April 17, 2022 18:13

felixarntz reviewed Apr 18, 2022

View reviewed changes

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved

mitogh reviewed Apr 18, 2022

View reviewed changes

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved

modules/images/webp-uploads/load.php Show resolved Hide resolved

jjgrainger added 3 commits April 25, 2022 13:42

check filesizes are set and not zero

e248666

remove unused function

94f359e

Merge branch 'trunk' into feature/292-select-smaller-image-size

a2719bb

jjgrainger force-pushed the feature/292-select-smaller-image-size branch from 8a72560 to a2719bb Compare April 25, 2022 12:46

remove tests

2088d42

felixarntz reviewed Apr 26, 2022

View reviewed changes

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved

modules/images/webp-uploads/load.php Show resolved Hide resolved

mitogh reviewed Apr 27, 2022

View reviewed changes

jjgrainger added 3 commits April 27, 2022 11:18

move filter outside of foreach loop

a2a3fa6

do not replace full size if smaller image size preferred

3196a21

add tests for preferr smaller image size

09c953e

jjgrainger requested review from felixarntz and mitogh April 27, 2022 12:59

felixarntz reviewed Apr 27, 2022

View reviewed changes

update comments

806114d

felixarntz approved these changes May 2, 2022

View reviewed changes

adamsilverstein approved these changes May 3, 2022

View reviewed changes

adamsilverstein reviewed May 3, 2022

View reviewed changes

felixarntz merged commit 90ec7bf into trunk May 3, 2022

felixarntz deleted the feature/292-select-smaller-image-size branch May 3, 2022 22:45

felixarntz changed the title ~~add logic to select smaller image size when enabled~~ Refine logic to select smaller image file in the frontend based on webp_uploads_prefer_smaller_image_file filter May 3, 2022

felixarntz added this to the 1.1.0 milestone May 3, 2022

felixarntz removed the no milestone PRs that do not have a defined milestone for release label May 3, 2022

westonruter mentioned this pull request May 8, 2024

Update PHPStan level to 6 #1203

Merged

Conversation

jjgrainger commented Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mitogh left a comment

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

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!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

felixarntz commented May 2, 2022

Uh oh!

adamsilverstein left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jjgrainger commented Apr 17, 2022 •

edited

Loading