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.

Sign up for GitHub

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

Choose smaller poster image size based on actual dimensions #1595

Merged
merged 14 commits into from
Oct 17, 2024
Merged

Choose smaller poster image size based on actual dimensions #1595

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
906f5f7
Fix typo
swissspidy Oct 16, 2024
7a9d556
Add new method for reducing poster image size
swissspidy Oct 16, 2024
c2b2026
Add test case
swissspidy Oct 16, 2024
e127685
Lint fixes
swissspidy Oct 16, 2024
ca0f538
Extract other logic to own method too
swissspidy Oct 16, 2024
071dc58
Fix docblock
swissspidy Oct 16, 2024
c723f15
Fix tests
swissspidy Oct 16, 2024
d664d43
Add native return type hints
swissspidy Oct 16, 2024
5ceb8fa
Apply suggestions from code review
swissspidy Oct 16, 2024
e48c16c
Use new method
swissspidy Oct 16, 2024
fc2efdb
Pass poster as argument
swissspidy Oct 16, 2024
5411888
Return void
swissspidy Oct 16, 2024
3521f9e
Avoid downsizing video poster unless desktop URL metrics are collected
westonruter Oct 16, 2024
69c717a
Apply suggestions from code review
swissspidy Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ private function reduce_layout_shifts( OD_Tag_Visitor_Context $context ): void {
$embed_wrapper_xpath = self::get_embed_wrapper_xpath( $processor->get_xpath() );

/**
* Collection of the minimum heights for the element with each group keyed by the minimum viewport with.
* Collection of the minimum heights for the element with each group keyed by the minimum viewport width.
*
* @var array<int, array{group: OD_URL_Metric_Group, height: int}> $minimums
*/
Expand Down
91 changes: 85 additions & 6 deletions plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,96 @@ public function __invoke( OD_Tag_Visitor_Context $context ): bool {
return false;
}

// Skip empty poster attributes and data: URLs.
$poster = trim( (string) $processor->get_attribute( 'poster' ) );
// TODO: If $context->url_metric_group_collection->get_element_max_intersection_ratio( $xpath ) is 0.0, then the video is not in any initial viewport and the VIDEO tag could get the preload=none attribute added.

$poster = $this->get_poster( $context );

if ( null !== $poster ) {
$this->reduce_poster_image_size( $poster, $context );
$this->preload_poster_image( $poster, $context );

return true;
}

swissspidy marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
/**
* Gets the poster from the current VIDEO element.
*
* Skips empty poster attributes and data: URLs.
*
* @since n.e.x.t
*
* @param OD_Tag_Visitor_Context $context Tag visitor context.
* @return non-empty-string|null Poster or null if not defined or is a data: URL.
*/
private function get_poster( OD_Tag_Visitor_Context $context ): ?string {
$poster = trim( (string) $context->processor->get_attribute( 'poster' ) );
if ( '' === $poster || $this->is_data_url( $poster ) ) {
return false;
return null;
}
return $poster;
}

/**
* Reduces poster image size by choosing one that fits the maximum video size more closely.
*
* @since n.e.x.t
*
* @param non-empty-string $poster Poster image URL.
* @param OD_Tag_Visitor_Context $context Tag visitor context, with the cursor currently at a VIDEO tag.
*/
private function reduce_poster_image_size( string $poster, OD_Tag_Visitor_Context $context ): void {
$processor = $context->processor;

$xpath = $processor->get_xpath();

// TODO: If $context->url_metric_group_collection->get_element_max_intersection_ratio( $xpath ) is 0.0, then the video is not in any initial viewport and the VIDEO tag could get the preload=none attribute added.
/*
* Obtain maximum width of the element exclusively from the URL metrics group with the widest viewport width,
* which would be desktop. This prevents the situation where if URL metrics have only so far been gathered for
* mobile viewports that an excessively-small poster would end up getting served to the first desktop visitor.
*/
$max_element_width = 0;
$widest_group = array_reduce(
iterator_to_array( $context->url_metric_group_collection ),
static function ( $carry, OD_URL_Metric_Group $group ) {
return ( null === $carry || $group->get_minimum_viewport_width() > $carry->get_minimum_viewport_width() ) ? $group : $carry;
}
);
foreach ( $widest_group as $url_metric ) {
foreach ( $url_metric->get_elements() as $element ) {
if ( $element['xpath'] === $xpath ) {
$max_element_width = max( $max_element_width, $element['boundingClientRect']['width'] );
break; // Move on to the next URL Metric.
}
}
}

// If the element wasn't present in any URL Metrics gathered for desktop, then abort downsizing the poster.
if ( 0 === $max_element_width ) {
return;
}

$poster_id = attachment_url_to_postid( $poster );

if ( $poster_id > 0 && $max_element_width > 0 ) {
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
$smaller_image_url = wp_get_attachment_image_url( $poster_id, array( (int) $max_element_width, 0 ) );
$processor->set_attribute( 'poster', $smaller_image_url );
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I can give that a try tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Amazing, thanks a lot!

}

/**
* Preloads poster image for the LCP <video> element.
*
* @since n.e.x.t
*
* @param non-empty-string $poster Poster image URL.
* @param OD_Tag_Visitor_Context $context Tag visitor context, with the cursor currently at a VIDEO tag.
*/
private function preload_poster_image( string $poster, OD_Tag_Visitor_Context $context ): void {
$processor = $context->processor;

$xpath = $processor->get_xpath();

// If this element is the LCP (for a breakpoint group), add a preload link for it.
foreach ( $context->url_metric_group_collection->get_groups_by_lcp_element( $xpath ) as $group ) {
Expand All @@ -66,7 +147,5 @@ public function __invoke( OD_Tag_Visitor_Context $context ): bool {
$group->get_maximum_viewport_width()
);
}

return true;
}
}
81 changes: 81 additions & 0 deletions ...rioritizer/tests/test-cases/video-with-large-poster-and-desktop-url-metrics-collected.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php

$full_url = '';
$expected_url = '';

return array(
'set_up' => static function ( Test_Image_Prioritizer_Helper $test_case, WP_UnitTest_Factory $factory ) use ( &$full_url, &$expected_url ): void {
$breakpoint_max_widths = array( 480, 600, 782 );
add_filter(
'od_breakpoint_max_widths',
static function () use ( $breakpoint_max_widths ) {
return $breakpoint_max_widths;
}
);

$element = array(
'isLCP' => false,
'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::VIDEO]',
'boundingClientRect' => $test_case->get_sample_dom_rect(),
);

foreach ( array_merge( $breakpoint_max_widths, array( 1000 ) ) as $viewport_width ) {
OD_URL_Metrics_Post_Type::store_url_metric(
od_get_url_metrics_slug( od_get_normalized_query_vars() ),
$test_case->get_sample_url_metric(
array(
'viewport_width' => $viewport_width,
'elements' => array( $element ),
)
)
);
}

$attachment_id = $factory->attachment->create_object(
DIR_TESTDATA . '/images/33772.jpg',
0,
array(
'post_mime_type' => 'image/jpeg',
'post_excerpt' => 'A sample caption',
)
);

wp_generate_attachment_metadata( $attachment_id, DIR_TESTDATA . '/images/33772.jpg' );

$full_url = wp_get_attachment_url( $attachment_id );
$expected_url = wp_get_attachment_image_url( $attachment_id, array( (int) $element['boundingClientRect']['width'], 0 ) );
},
'buffer' => static function () use ( &$full_url ) {
return "
<html lang=\"en\">
<head>
<meta charset=\"utf-8\">
<title>...</title>
</head>
<body>
<video class=\"desktop\" poster=\"$full_url\" width=\"1200\" height=\"500\" crossorigin>
<source src=\"https://example.com/header.webm\" type=\"video/webm\">
<source src=\"https://example.com/header.mp4\" type=\"video/mp4\">
</video>
</body>
</html>
";
},
'expected' => static function () use ( &$full_url, &$expected_url ) {
return "
<html lang=\"en\">
<head>
<meta charset=\"utf-8\">
<title>...</title>
</head>
<body>
<video data-od-replaced-poster=\"$full_url\" data-od-xpath=\"/*[1][self::HTML]/*[2][self::BODY]/*[1][self::VIDEO]\" class=\"desktop\" poster=\"$expected_url\" width=\"1200\" height=\"500\" crossorigin>
<source src=\"https://example.com/header.webm\" type=\"video/webm\">
<source src=\"https://example.com/header.mp4\" type=\"video/mp4\">
</video>
<script type=\"module\">/* import detect ... */</script>
</body>
</html>
";
},
);
79 changes: 79 additions & 0 deletions ...-prioritizer/tests/test-cases/video-with-large-poster-and-desktop-url-metrics-missing.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?php

$full_url = '';

return array(
'set_up' => static function ( Test_Image_Prioritizer_Helper $test_case, WP_UnitTest_Factory $factory ) use ( &$full_url ): void {
$breakpoint_max_widths = array( 480, 600, 782 );
add_filter(
'od_breakpoint_max_widths',
static function () use ( $breakpoint_max_widths ) {
return $breakpoint_max_widths;
}
);

$element = array(
'isLCP' => false,
'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::VIDEO]',
'boundingClientRect' => $test_case->get_sample_dom_rect(),
);

foreach ( $breakpoint_max_widths as $non_desktop_viewport_width ) {
OD_URL_Metrics_Post_Type::store_url_metric(
od_get_url_metrics_slug( od_get_normalized_query_vars() ),
$test_case->get_sample_url_metric(
array(
'viewport_width' => $non_desktop_viewport_width,
'elements' => array( $element ),
)
)
);
}

$attachment_id = $factory->attachment->create_object(
DIR_TESTDATA . '/images/33772.jpg',
0,
array(
'post_mime_type' => 'image/jpeg',
'post_excerpt' => 'A sample caption',
)
);

wp_generate_attachment_metadata( $attachment_id, DIR_TESTDATA . '/images/33772.jpg' );

$full_url = wp_get_attachment_url( $attachment_id );
},
'buffer' => static function () use ( &$full_url ) {
return "
<html lang=\"en\">
<head>
<meta charset=\"utf-8\">
<title>...</title>
</head>
<body>
<video class=\"desktop\" poster=\"$full_url\" width=\"1200\" height=\"500\" crossorigin>
<source src=\"https://example.com/header.webm\" type=\"video/webm\">
<source src=\"https://example.com/header.mp4\" type=\"video/mp4\">
</video>
</body>
</html>
";
},
'expected' => static function () use ( &$full_url ) {
return "
<html lang=\"en\">
<head>
<meta charset=\"utf-8\">
<title>...</title>
</head>
<body>
<video data-od-xpath=\"/*[1][self::HTML]/*[2][self::BODY]/*[1][self::VIDEO]\" class=\"desktop\" poster=\"$full_url\" width=\"1200\" height=\"500\" crossorigin>
<source src=\"https://example.com/header.webm\" type=\"video/webm\">
<source src=\"https://example.com/header.mp4\" type=\"video/mp4\">
</video>
<script type=\"module\">/* import detect ... */</script>
</body>
</html>
";
},
);
11 changes: 9 additions & 2 deletions plugins/image-prioritizer/tests/test-helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,23 @@ public function data_provider_test_filter_tag_visitors(): array {
* @covers Image_Prioritizer_Background_Image_Styled_Tag_Visitor
*
* @dataProvider data_provider_test_filter_tag_visitors
*
* @param callable $set_up Setup function.
* @param callable|string $buffer Content before.
* @param callable|string $expected Expected content after.
*/
public function test_image_prioritizer_register_tag_visitors( Closure $set_up, string $buffer, string $expected ): void {
$set_up( $this );
public function test_image_prioritizer_register_tag_visitors( callable $set_up, $buffer, $expected ): void {
$set_up( $this, $this::factory() );

$buffer = is_string( $buffer ) ? $buffer : $buffer();
$buffer = preg_replace(
':<script type="module">.+?</script>:s',
'<script type="module">/* import detect ... */</script>',
od_optimize_template_output_buffer( $buffer )
);

$expected = is_string( $expected ) ? $expected : $expected();

$this->assertEquals(
$this->remove_initial_tabs( $expected ),
$this->remove_initial_tabs( $buffer ),
Expand Down