Choose smaller poster image size based on actual dimensions #1595

swissspidy · 2024-10-16T11:04:00Z

Summary

On my test post with 7 large videos, the videos were displayed ~610px wide on desktop. With this PR, the full image URLs (which were 1920px wide) are replaced with 767px wide versions.

Image file sizes before this PR: 738 KB

After: 175 KB (!)

Relevant technical choices

github-actions · 2024-10-16T11:09:40Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter

Love this!! A few suggestions.

plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php

westonruter · 2024-10-16T16:11:27Z

plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php

+		$poster_id = attachment_url_to_postid( $poster );
+
+		if ( $poster_id > 0 && $max_element_width > 0 ) {
+			$smaller_image_url = wp_get_attachment_image_url( $poster_id, array( (int) $max_element_width, 0 ) );
+			$processor->set_attribute( 'poster', $smaller_image_url );
+		}


One possible consideration to make here is that if there are no URL Metrics gathered for the largest breakpoint group that in this case the poster should not be reduced in size. The reason being is that if there are only mobile visitors from whom URL Metrics have been gathered, as soon as a desktop visitor visits they could see a poster that is much lower-resolution than it should be.

Maybe this is an edge case that we shouldn't worry about right now, but it's something to keep in mind.

Great point, I think this is relevant to consider. I'd say we should be cautious in such a case and prioritize a "not broken" experience over better performance. Basically, unless we have data for all three viewports, we cannot reasonably make a decision to optimize.

Related to this is https://github.com/WordPress/performance/pull/1596/files#r1803569058 where I realize that lazy-loading may currently be incorrectly applied to an element on desktop if it is not visible on mobile and only URL Metrics for mobile have been collected. But if a site only ever gets mobile traffic, should the lack of desktop data be a blocker?

I think a low-res poster image on desktop if there's a lack of desktop traffic is acceptable.

But we can be more strict for now and then see how well it works. So how can I easily check whether values exist for every viewport? :-)

The OD_URL_Metric_Group_Collection::is_every_group_populated() method will indicate whether all groups have at least one URL Metric. But for some sites all viewports may never get populated, such as if no tablet visitors show up. See #1404.

So should this only check if the largest group (desktop) is populated?

In reality, the last item in iterator_to_array( $context->url_metric_group_collection ) will have the widest viewport since OD_URL_Metric_Group_Collection sorts the breakpoints before constructing the groups.

It would probably make sense to add a helper to OD_URL_Metric_Group_Collection which returns the widest group (i.e. desktop). But we can do that later.

OK I can give that a try tomorrow.

I didn't want to lead you astray, so I went ahead and tested it out in 3521f9e. It seems to work! There are two test cases, one for when desktop URL metrics are collected and another when they are absent.

Amazing, thanks a lot!

plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php

felixarntz

@swissspidy Looks great to me, other than what @westonruter already outlined.

Co-authored-by: Weston Ruter <westonruter@google.com>

plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php

westonruter

Just a couple minor suggestions left. So I'm pre-approving!

Co-authored-by: Weston Ruter <westonruter@google.com>

swissspidy added 3 commits October 16, 2024 12:38

Fix typo

906f5f7

Add new method for reducing poster image size

7a9d556

Add test case

c2b2026

swissspidy added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) labels Oct 16, 2024

swissspidy requested review from westonruter, felixarntz and adamsilverstein as code owners October 16, 2024 11:04

swissspidy requested a review from thelovekesh October 16, 2024 11:04

swissspidy added this to the image-prioritizer n.e.x.t milestone Oct 16, 2024

Lint fixes

e127685

swissspidy added 4 commits October 16, 2024 14:43

Extract other logic to own method too

ca0f538

Fix docblock

071dc58

Fix tests

c723f15

Add native return type hints

d664d43

swissspidy mentioned this pull request Oct 16, 2024

Lazy load videos and video posters #1596

Merged

3 tasks

westonruter requested changes Oct 16, 2024

View reviewed changes

felixarntz reviewed Oct 16, 2024

View reviewed changes

swissspidy and others added 2 commits October 16, 2024 22:45

Apply suggestions from code review

5ceb8fa

Co-authored-by: Weston Ruter <westonruter@google.com>

Use new method

e48c16c

westonruter reviewed Oct 16, 2024

View reviewed changes

plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php Outdated Show resolved Hide resolved

swissspidy and others added 3 commits October 16, 2024 23:30

Pass poster as argument

fc2efdb

Return void

5411888

Avoid downsizing video poster unless desktop URL metrics are collected

3521f9e

westonruter reviewed Oct 16, 2024

View reviewed changes

plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php Outdated Show resolved Hide resolved

westonruter reviewed Oct 16, 2024

View reviewed changes

plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php Outdated Show resolved Hide resolved

westonruter reviewed Oct 16, 2024

View reviewed changes

plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php Show resolved Hide resolved

westonruter approved these changes Oct 16, 2024

View reviewed changes

Apply suggestions from code review

69c717a

Co-authored-by: Weston Ruter <westonruter@google.com>

swissspidy merged commit ecceec8 into trunk Oct 17, 2024
13 checks passed

swissspidy deleted the fix/1575-poster-size branch October 17, 2024 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Choose smaller poster image size based on actual dimensions #1595

Choose smaller poster image size based on actual dimensions #1595

swissspidy commented Oct 16, 2024

github-actions bot commented Oct 16, 2024 •

edited

Loading

westonruter left a comment

westonruter Oct 16, 2024

felixarntz Oct 16, 2024

westonruter Oct 16, 2024

swissspidy Oct 16, 2024

westonruter Oct 16, 2024

westonruter Oct 16, 2024

westonruter Oct 16, 2024

swissspidy Oct 16, 2024

westonruter Oct 16, 2024

swissspidy Oct 17, 2024

felixarntz left a comment

westonruter left a comment

Choose smaller poster image size based on actual dimensions #1595

Choose smaller poster image size based on actual dimensions #1595

Conversation

swissspidy commented Oct 16, 2024

Summary

Relevant technical choices

github-actions bot commented Oct 16, 2024 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024 •

edited

Loading