WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 3 months ago

Last modified 2 months ago

#43524 closed enhancement (fixed)

Add another default image size

Reported by: azaozz Owned by: joemcgill
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-dev-note
Focuses: Cc:
PR Number:

Description

We've had the srcset and sizes attributes for quite some time and they seem to work quite well.

One problem is that we don't have a 2x images for the built-in large size. Currently when displaying images larger than 512 px on the front-end on devices with high pixel density screens, the browser has only one choice: to output the full size/original image. These images are often as large as 6000px x 4000px and 4MB or even larger.

To fix this and not output needlessly large images we need a new size that should be 2x the current "large" size, max 2048px width or height.

Attachments (2)

43524.patch (7.8 KB) - added by pierlo 4 months ago.
43524.2.patch (1.7 KB) - added by azaozz 4 months ago.

Download all attachments as: .zip

Change History (46)

#1 @azaozz
21 months ago

Once we add that, we can consider changing the logic when outputting the srcset attribute and use the new "xlarge" size as the largest available.

Last edited 21 months ago by azaozz (previous) (diff)

#2 follow-up: @joyously
21 months ago

Would you call it "2xlarge" and have it be a computed size?

#3 in reply to: ↑ 2 @azaozz
21 months ago

Replying to joyously:
Hehe, yep, can be any name that makes sense. If we follow t-shirt sizes, 2xlarge should come after xlarge, right? :)

The size should be 2 x large, so 2048px should be the max width (for landscape) or height (for portrait).

Last edited 21 months ago by azaozz (previous) (diff)

#4 follow-up: @birgire
21 months ago

Sounds like xlarge could be a good addition, but then there's the default 1600 maximum image width for the srcset:

/**
 * Filters the maximum image width to be included in a 'srcset' attribute.
 *
 * @since 4.4.0
 *
 * @param int   $max_width  The maximum image width to be included in the 'srcset'. Default '1600'.
 * @param array $size_array Array of width and height values in pixels (in that order).
 */
$max_srcset_image_width = apply_filters( 'max_srcset_image_width', 1600, $size_array );

#5 in reply to: ↑ 4 @azaozz
18 months ago

Replying to birgire:

We can increase that :)

Also been thinking if we would need 2x medium_large size. That'd be 1536px (can round it up to 1600px?). This will be used for the 2x (retina) images in post_content, as the main column in pretty much all themes is not wider than 768px (800px).

Adding these two new sizes will significantly improve the selection of image sizes, so the browsers will be able to pick a better match from the srcset attribute when displaying images on the front-end. However it will also bump up the processing time/resource usage on the server when creating the image sub-sizes after uploading an image.

This may cause more timeout errors on some shared hosting accounts. Ideally we should have a way of regenerating missing image sizes after an image is uploaded. That depends on #40439 and #43525 being done.

#6 follow-up: @joyously
18 months ago

Could the 2x stuff be made optional?
Most of the sites I work with do not need this, and it seems like such a waste of disk space (out of the client's quota), along with not needing to worry about that extra processing causing timeouts, etc.

#7 in reply to: ↑ 6 @azaozz
18 months ago

Replying to joyously:

Could the 2x stuff be made optional?
Most of the sites I work with do not need this

You mean you never use "large" image size on a retina screen? What about your site's visitors, do any of them have larger high-density screens? :)

#8 follow-up: @joyously
18 months ago

I'm saying that most of my client sites have small images and/or themes that don't display wider than about 1200px.

#9 in reply to: ↑ 8 ; follow-up: @azaozz
18 months ago

Replying to joyously:

Right. What happens when you have to display an image at lets say 800px? You choose the "large" size (1024px by default) and all is good. However it displays "fuzzy" on most Macs, laptops and desktops, as they have "retina" screens, right? :)

#10 follow-up: @joyously
18 months ago

So are you suggesting that on an upload of a 800px image, that a 1024px one is made for large and a 2048px one is made for xlarge? That the user has no choice in what images are used?

Those users with retina screens have to deal with their choice of hardware.

#11 in reply to: ↑ 10 ; follow-up: @azaozz
18 months ago

Replying to joyously:

I'm not sure I follow... :) Image sub-sizes depend on the original image. If it is not large enough to create all sub-sizes, we stop at the largest sub-size we can make. That's all.

For the 800px image from the example above, the only sub-sizes that will be created will be "medium" and "thumbnail" (if the user didn't change the settings for these sizes to something over 800px).

Those users with retina screens have to deal with their choice of hardware.

Sorry but I don't think I understand. Should we make WordPress produce sub-standard websites that look bad on new/nicer screens..?

Last edited 18 months ago by azaozz (previous) (diff)

#12 @stephenwp001
18 months ago

Correct me if I'm wrong but srcset just displays different size images for different size displays/devices, its not a retina solution?

#13 in reply to: ↑ description ; follow-up: @ettoredn
16 months ago

Replying to azaozz:

One problem is that we don't have a 2x images for the built-in large size. Currently when displaying images larger than 512 px on the front-end on devices with high pixel density screens, the browser has only one choice: to output the full size/original image. These images are often as large as 6000px x 4000px and 4MB or even larger.

Sorry for being rude, but please support your claim with actual evidence. Out of every image on a website, it seems unlikely to me that most of them are over 4 MiB and display wider than 1024px.

To fix this and not output needlessly large images we need a new size that should be 2x the current "large" size, max 2048px width or height.

or you can just

add_image_size()

Besides, as pointed out by @joyously, it would increase disk space usage thus it becomes a tradeoff between disk space and user experience/bandwidth efficiency. If, as both my and @joyously experience suggest, there are a relative small number of cases where this is necessary, such situations can be dealt with the single line of code above added inside a theme or plugin.

Last edited 16 months ago by ettoredn (previous) (diff)

#14 in reply to: ↑ 9 @ettoredn
16 months ago

Replying to azaozz:

Replying to joyously:

Right. What happens when you have to display an image at lets say 800px? You choose the "large" size (1024px by default) and all is good. However it displays "fuzzy" on most Macs, laptops and desktops, as they have "retina" screens, right? :)

What should happen is that you upload the image at whatever high resolution you need and then use

wp_get_attachment_image(id, 'full')

or similar functions.

Depending on the value of the sizes attribute and the viewport, the browser will choose the best srcset image to display. I believe if left empty, the browser assumes '100vw', but this may depend on the browser.

Passing an argument different than 'full' would force the widest srcset image to be limited to the given image size.

How srcset and sizes work together, and how the browser chooses the image to render thus download, seems to be an often misunderstood topic.

Last edited 16 months ago by ettoredn (previous) (diff)

#15 in reply to: ↑ 11 @ettoredn
16 months ago

Replying to azaozz:

Replying to joyously:

Sorry but I don't think I understand. Should we make WordPress produce sub-standard websites that look bad on new/nicer screens..?

Presentation should be managed by the theme, not (necessarily) by Core (or model + business logic). We all know WordPress is a mess in the separation of concerns, however Core should just provide "sane" defaults and let theme developers deal with presentation issues such us this one!

#16 in reply to: ↑ 13 @azaozz
16 months ago

Replying to ettoredn:

Sorry for being rude, but please support your claim with actual evidence. Out of every image on a website, it seems unlikely to me that most of them are over 4 MiB and display wider than 1024px.

Right. This ticket comes as a result from looking at few websites I have access to :)

The facts:

  • Most modern cell phones create JPEG files of 2 - 2.5MB.
  • Most modern cameras create JPEG files of at least 3MB (up to about 24MB in raw formats).
  • Most users upload these photos without editing them beforehand.
  • Currently WordPress doesn't have a suitable file size for larger high density displays like MacBooks, newer Dells, Surface Pro, 4k or 5k monitors, etc. etc.

The only option now to display non-fuzzy larger images (width of 600-700px+) on the above screens is to load the "full" size images, i.e. at least 2MB per image. This ticket is about creating a suitable size for this case.

Besides, as pointed out by @joyously, it would increase disk space usage thus it becomes a tradeoff between disk space and user experience/bandwidth efficiency.

As far as I see the new size will be about 600-700KB, and will save a lot of bandwidth for most WordPress sites. As disk space is considerably "cheaper" than bandwidth, this would result in lower hosting costs and better user experience for 1/3 of the internet :)

#17 @pento
14 months ago

  • Milestone changed from 5.0 to 5.1

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


11 months ago

#19 @mikeschroder
11 months ago

  • Milestone changed from 5.1 to 5.2

Moving to 5.2 because this depends on #40439 to avoid increased HTTP errors.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


11 months ago

#22 @joemcgill
11 months ago

  • Owner set to joemcgill
  • Status changed from new to assigned

#23 @desrosj
9 months ago

  • Keywords needs-patch added
  • Milestone changed from 5.2 to 5.3

5.2 beta is in a little over a day. Since this still needs a patch, I am going to punt it to 5.3. @joemcgill if you are confident in this for 5.2, feel free to move it back.

@pierlo
4 months ago

#24 follow-ups: @pierlo
4 months ago

In attachment:43524.patch I've added the extra_large subsize which has a width of 2048px. Things to note:

  • $max_srcset_image_width is at 2624px. I'm not sure how the previous 1600px was calculated.
  • WP DB version is incremented by 1.

It's possible that I have not made changes to all areas to support the new subsize.

#25 in reply to: ↑ 24 @azaozz
4 months ago

Replying to pierlo:

Thanks for the patch!

Been meaning to comment here too. After #40439 and #47872 there shouldn't be any technical problems adding more default image sizes (before them any new size would increase the chance of the upload failing).

In that terms thinking it may need two new sizes: one that is 2x "large", 2048px max-width or max-height, and another that is 2x "medium-large", 1536px max-width or max-height. Or perhaps it makes sense to do 1600 and 2000? Thinking best to defer to @joemcgill here as he has a lot more experience :)

In any case the new sizes should not be "user settable" from the UI just like the "medium-large" size. Only themes and plugins should be able to change or disable them (although best to not do that, they will be needed for better support for the srcset attribute).

I'm actually thinking we should remove the image size UI completely and replace it with "Add a custom image size (but only if you really know what you're doing)" in new WP installs. But lets take it one step at a time.

Also related #47873.

Last edited 4 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core-media by azaozz. View the logs.


4 months ago

@azaozz
4 months ago

#27 @azaozz
4 months ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

In 43524.2.patch: Add two more "default" image sizes. These sizes are added to the $_wp_additional_image_sizes array and not stored in options. They are not meant to be changeable/settable by the users.

The new sizes are meant to enhance the way WordPress displays images on the front-end on larger, high-density devices. They make it possible to generate more suitable srcset and sizes attributes when the users upload large images.

These sizes can still be changed or removed by themes and plugins but that is not recommended. The size "names" reflect the image dimensions, so changing the sizes would be quite misleading.

Been wondering if we should make it impossible to change them, only let themes and plugins to remove/unset them. Or perhaps we can detect changes and output "doing-it-wrong"?

#28 in reply to: ↑ 24 ; follow-up: @azaozz
4 months ago

Replying to pierlo:

@pierlo Thanks for the patch! While reviewing it was thinking why we store all these sizes in options and what happens when they get changed... Frankly I think this is a "left over" thing from 2003 :) There is no longer a good reason to have different images with different sub-sizes with the same sub-size name. It's just confusing and may result in generating poor srcset attribute.

Also thinking we should remove the UI for setting of the three "classic" image sizes: thumbnail, medium, large. Currently a user can set a thumbnail to be 1000x1000px and "large" to be 50x50px...

$max_srcset_image_width is at 2624px. I'm not sure how the previous 1600px was calculated.

Yeah, the 1600px value was (mostly) arbitrary, based on current usage. Wondering how you came up with 2624px though. Is that some sort of a standard width somewhere on the internet?

In 43524.2.patch I've changed that to 2560. Thinking to use the same value for the "BIG" image cut-out size, see #47873. But lets set it to something that truly makes sense :)

#29 in reply to: ↑ 28 @pierlo
4 months ago

Replying to azaozz:

Yes I think removing the UI for choosing one of the "classic" image sizes is outdated and should be removed.

There is no longer a good reason to have different images with different sub-sizes with the same sub-size name.

That got me thinking, how do websites dedicated to photography deal with this issue? Using Unsplash as an example, they compress uploads because "it results in dramatically smaller file sizes while providing the same quality photo" (source). Also, they generate srcset and sizes attributes based on the image size, with the last URL in the srcset being the original uploaded image.

With that said, I say we remove these sub-sizes, and instead generate the sub-images based on the image itself. One major concern however is the amount of memory needed for the dynamic resizing of images. Perhaps with #40439 it is no longer necessary, or to let the user know that images greater a certain size cannot be handled by the server.

Yeah, the 1600px value was (mostly) arbitrary, based on current usage. Wondering how you came up with 2624px though. Is that some sort of a standard width somewhere on the internet?

I simply added 1024 to it, no specific reason :)

This ticket was mentioned in Slack in #core-media by pierlo. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-media by azaozz. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


4 months ago

#33 follow-up: @mikeschroder
3 months ago

I like the approach in the most recent patch.

Would it make sense to expand a bit more in the comments for the new function why those sizes are there, as opposed to the location/method that the sizes get added for the other core ones?

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 months ago

#35 in reply to: ↑ 33 @azaozz
3 months ago

Replying to mikeschroder:

Would it make sense to expand a bit more in the comments for the new function why those sizes are there, as opposed to the location/method that the sizes get added for the other core ones?

Good idea :)

Will change the inline docs and commit it (so we can catch any eventual problems early). Please feel free to change or edit it as you like!

Last edited 3 months ago by azaozz (previous) (diff)

#36 @azaozz
3 months ago

In 46066:

Media: Add two new intermediate image sizes, 1536px and 2048px. They are meant to enhance the way WordPress displays images on the front-end on larger, high-density devices. They make it possible to generate more suitable srcset and sizes attributes, and not use the original, often non-optimized image.

Also change the default max_srcset_image_width value to match the new max size.

Props pierlo, azaozz.
See #43524.

#37 @azaozz
3 months ago

  • Keywords 2nd-opinion removed

Left open for eventual inline docs changes/enhancements.

#38 @azaozz
3 months ago

In 46067:

Fix (unrelated) test after [46066].

See #43524.

#39 follow-up: @johnbillion
3 months ago

  • Keywords needs-dev-note added

What's the reason for using the dimensions in the image size name? We don't do that for any other image size.

#40 in reply to: ↑ 39 @azaozz
3 months ago

Replying to johnbillion:

What's the reason for using the dimensions in the image size name?

There was a discussion about sub-sizes "names" in #core-media on Slack. The old practice of "relative" names made sense at the time (early 2000's) for "non-responsive" images. The names also can be quite confusing. A user (or a plugin) may set "thumbnail" to be 500x500px and "large" to be 200x300px.

"Modern" images have srcset and sizes attributes, the old relative sizes don't represent how the images "work" any more. They are great for use in the UI, but don't correspond to using a particular image file. In addition, to be able to generate good srcset, WP depends on having several suitable sub-sizes. In that terms changing the default sub-sizes should be discouraged. This is also the reason the new sizes are added at runtime and not stored in the DB.

There was also a discussion about having something like register_image_size() and WP_Image object at some point in the future. Also removing the size options from Settings->Media, and adding "Add Custom Size" instead.

#41 @mikeschroder
3 months ago

  • Keywords has-patch removed
  • Resolution set to fixed
  • Status changed from assigned to closed

@azaozz Thanks so much! The comments you added to the function look super.

Since this was left open for docs reasons, and that looks to have been taken care of, going to go ahead and close this ticket.
Please feel free to reopen this ticket if there's anything left!

This ticket was mentioned in Slack in #cli by schlessera. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


2 months ago

Note: See TracTickets for help on using tickets.