Improve the performance of wp_make_content_images_responsive()

[joemcgill]

In an effort to make sure the content filter introduced in 34855 runs as efficiently as possible, we should pass around data like attachment metadata when possible, to avoid redundant calls to the same functions and make sure we use cached data when possible (see #34359 for example).

[joemcgill]

My first approach passes both the metadata and the original image source to the other internal functions, which avoids the need to call internal media functions at all, but makes the function signatures a bit weird. In my tests, the performance of the content filter with 50 images in a post were almost twice as fast with this patch. Would love some feedback on the approach.

[azaozz]

Replying to joemcgill:

Yep, it's moving in the right direction. Got some ideas from the patch and went ahead with more changes/fixes.

In 34430.1.diff:

• Change wp_get_attachment_image_srcset_array() and wp_get_attachment_image_sizes() to accept the image size (from the image tag), image meta and attachment ID as arguments. These are the actual bits that are needed to generate srcset and sizes.
• Change so the image size argument is always an array of width and height. This makes it more precise and avoids any edge cases when the default image sizes have been changed by the user. There is an optional function to "translate" named sizes into arrays, not used for now and not sure if it's needed.
• Changed so no (other) image API functions are used while generating srcset and sizes.
• Several logic fixes and improvements.
• Some names changed to be (hopefully) more descriptive.
• Fixed/updated the tests.

Needs more testing that it didn't break anything.

[azaozz]

In 34430.2.diff: go back to priming the cache. Prime only the post_meta cache with update_meta_cache(). As far as I see it makes sense to do that when two or more local images are found in a post.

[azaozz]

In 34430.3.diff: refresh after r35402, r35404 and r35405.

[azaozz]

#34460 was marked as a duplicate.

[joemcgill]

@azaozz 34430.4.diff​ is a riff off of your last patch with the following changes:

1. Generally, I'd prefer leaving the signature for our srcset and sizes functions as is and add the image meta as a third, optional parameter. My thinking here is that we should optimize the parameters for developers who would use functions like wp_get_attachment_image_srcset() in a theme or plugin, in which case, passing $attachment_id and $size would be consistent with the other wp_get_attachment_image_ functions. This change would have no affect on the performance of our display filter.

2. In wp_get_attachment_image_srcset_array() calling path_join() can be a bit of a drag on performance, so we should only call it when absolutely necessary.

3. In wp_make_content_images_responsive() we can't assume that all images with the same ID are using the same markup (e.g., when the same image is embedded at two sizes on the same page, the same image using different alignments, etc.). Doing so will result in some images not getting filtered. This was causing Tests_Media:test_wp_make_content_images_responsive() to fail on my end.

[joemcgill]

34430.5.diff​ is another iteration which introduces a few notable changes.

First, wp_get_attachment_image_srcset_array() has been removed in favor of the lower level function, _wp_calculate_image_srcset() which accepts a URL, image meta, and a size array comprised of a width and height value.

The generation of these parameters are now handled in wp_get_attachment_image_srcset() which is meant to be used in templates but is no longer used directly in core when adding a srcset attribute to images in post content or to image markup generated by wp_get_attachment_image() on the front end. Instead, wp_get_attachment_image() and wp_image_add_srcset_and_sizes() both rely on _wp_calculate_image_srcset(). All former filter hooks remain in place.

The other notable change is that the order of parameters for wp_get_attachment_image_sizes() has changed so that $attachment_id can be an optional parameter. The order of the parameters in the wp_get_attachment_image_sizes filter have also been updated to reflect this change.

All tests have been updated accordingly.

[azaozz]

In 35412:

Responsive images:

• Introduce wp_calculate_image_srcset() that replaces wp_get_attachment_image_srcset_array() and is used as lower level function for retrieving the srcset data as array.
• Use the new function when generating srcset and sizes on the front-end. This is faster as no (other) image API functions are used.
• Change the wp_get_attachment_image_srcset(). Now it is meant for use in templates and is no longer used in core.
• A few logic fixes and improvements.
• Some names changed to be (hopefully) more descriptive.
• Fixed/updated tests.

Props joemcgill, jaspermdegroot, azaozz.
See #34430.

[DrewAPicture]

Following [35412], we need better descriptions than "Private, do not use." for _wp_upload_dir_baseurl() and _wp_get_image_size_from_meta(). We use these docs internally too, remember :-)

If we don't want it to be used outside core, e.g. "publicly", we should use @ignore in the DocBlocks to skip parsing, but that shouldn't preclude adequately documenting the functions including parameters and returns.

[swissspidy]

Following [35412], we need better descriptions than "Private, do not use." for _wp_upload_dir_baseurl() and _wp_get_image_size_from_meta(). We use these docs internally too, remember :-)

That's what I thought too this morning, so 34430.docs.diff​ is a first pass at adding proper docs.

I wonder if we should make _wp_upload_dir_baseurl() public, as something similar was requested in #33963.

[jaspermdegroot]

Following [35412], in 34430.6.diff​:

• Various inline docs corrections and improvements
• Correction of two filter names
• Consistent type casting in the sizes function
• Consistency in max_srcset_image_width filter and variable name

[jaspermdegroot]

Replaces 34430.6.diff

[jaspermdegroot]

34430.7.diff contains all changes from 34430.6.diff plus:

• A few more corrections in inline docs.
• Check if wp_get_attachment_metadata() has returned an array: Users that allow SVGs in their WordPress installation reported errors in the RICG responsive images plugin repo (see ​https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/issues/208 for example) which can be prevented by checking if wp_get_attachment_metadata has returned an array.
• Only add the srcset attribute if there will be a sizes attribute as well: We already did check if we can set both attributes in wp_image_add_srcset_and_sizes() but not in wp_get_attachment_image().

[jaspermdegroot]

@swissspidy - In your patch 34430.docs.diff on line 897 @return array|false should be @return array|bool

[joemcgill]

Replying to swissspidy:

I wonder if we should make _wp_upload_dir_baseurl() public, as something similar was requested in #33963.

I'm wondering the same thing. This function addresses the main issue brought up in #34359 so we also need to make sure the concerns mentioned in that ticket have been addressed. The approach @azaozz took for using a static variable inside _wp_upload_dir_baseurl() should allow us to avoid any collisions with persistent cache. I'm not sure about situations where a site is conditionally filtering the uploads directory or if that is even a concert in our context here.

[azaozz]

In 35419:

Responsive images:

• Check if wp_get_attachment_metadata() has returned an array to prevent errors when using SVGs.
• Only add the srcset attribute if there will be a sizes attribute.
• Better filter names.
• Some more inline docs fixes.

Props jaspermdegroot.
See #34430.

[azaozz]

In 35426:

Responsive images: add inline docs for private functions.

Props swissspidy.
See #34430.

[jaspermdegroot]

Following [35426], in 34430.8.diff:

• A few more corrections in inline docs
• Removed unwanted blank lines
• Renamed a confusing variable name in wp_get_attachment_image_sizes

[SergeyBiryukov]

#34464 was marked as a duplicate.

[azaozz]

After ​today's chat, in 34430.9.diff:

• Merge wp_image_srcset_attr() into wp_calculate_image_srcset().
• Remove the wp_image_srcset filter. Keep the wp_calculate_image_srcset filter for the array containing the data for creating the srcset value.
• Fix the tests to reflect these changes.

[azaozz]

In 35464:

Responsive images:

• Merge wp_image_srcset_attr() into wp_calculate_image_srcset().
• Remove the wp_image_srcset filter.
• Fix the tests for the above changes.

See #34430.

[azaozz]

In 35465:

Responsive images: few more inline docs fixes.

Props jaspermdegroot.
See #34430.

[jaspermdegroot]

Following [35465], in 34430.10.diff:

• Correction in inline doc: The $attachment_id param of wp_get_attachment_image_srcset() is not optional.
• Added a more comprehensive description of the $sources param of the wp_calculate_image_srcset filter.
• In wp_get_attachment_image_srcset(): Check if the meta data array contains sizes instead of just checking if it's an array. Consistent with what we do in wp_image_add_srcset_and_sizes().
• In wp_get_attachment_image_srcset(): Check if wp_get_attachment_metadata() has returned an array to prevent errors when using SVGs.
• In wp_get_attachment_image_sizes(): call get_post_meta() directly like we do in wp_get_attachment_image() and wp_make_content_images_responsive().
• In wp_get_attachment_image_sizes(): Removed the check if $size is a number since it only accepts an array.

[azaozz]

Replying to jaspermdegroot:

Think there is an inconsistency in wp_get_attachment_image_srcset() in 34430.10.diff. First we want to check if the $image_meta was passed (as it's optional), and get the meta if not. Then we want to see if $image_meta['sizes'] is not empty. Will fix that and commit.

[azaozz]

Actually not sure we need to check if $image_meta['sizes'] is not empty in both wp_get_attachment_image_srcset() and wp_calculate_image_srcset(). It doesn't make any difference in the former and is the first conditional in the latter.

[azaozz]

In 35491:

Responsive images:

• More fixes to inline docs.
• Replace the last wp_get_attachment_metadata() with get_post_meta().
• For consistency only accept array or named size in wp_get_attachment_image_sizes().

Props jaspermdegroot.
See #34430.

[azaozz]

In 34430.11.diff:

• Fix _wp_upload_dir_baseurl() to cache by blog_id.
• Replace path_join() with trailingslashit(). We are joining URLs, there's no need for the extra checks in path_join() (and it's much slower).
• Rename $image_url to $image_src for consistency (used at about 50 other places).
• Couple of tests fixes.

[azaozz]

In 35498:

Responsive images:

• Fix _wp_upload_dir_baseurl() to cache by blog_id.
• Replace path_join() with trailingslashit(), it's much faster.
• Rename $image_url to $image_src for consistency (used at about 50 other places).
• Couple of tests fixes.

See #34430.

[joemcgill]

Once #34359 is resolved, we can open a new ticket for removing _wp_upload_dir_baseurl().