WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 3 months ago

#21668 new enhancement

WordPress still does not save jpeg as progressive jpeg

Reported by: _ck_ Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.1
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:

Description

Every time WordPress does imagejpeg it should be doing imageinterlace beforehand to set progressive mode, but it's not.

http://php.net/manual/en/function.imageinterlace.php

Progressive jpeg has been supported by ALL browsers since 2001 or so.

progressive demo: http://bbshowcase.org/progressive/

Note that even if an ancient browser is listed somewhere as not rendering progressive, progressive jpeg still will be displayed, simply showing it all at once rather than gradual.

IE6 for example WILL load progressive jpeg, it will just not render it progressively, instead it will load the entire image and then render all at once at the end.

Progressive algorithm will also make slightly smaller jpeg as a side effect in most cases.

ie. in media.php
`
imageinterlace($newimage,1);
imagejpeg( $newimage, ...
`

This update is years overdue.

It might be possible to futureproof this by adding a filter to $newimage before the final imagejpeg. I am uncertain if that would work since filters would not pass $newimage by reference and instead make a copy - while newimage is only a pointer to php/gd memory. Can apply_filters be forced to work somehow by reference instead of a copy? Maybe that should be another feature idea.

Attachments (6)

21668.patch (1.6 KB) - added by SergeyBiryukov 20 months ago.
21668.2.patch (2.0 KB) - added by SergeyBiryukov 20 months ago.
21668.3.diff (1.4 KB) - added by Japh 18 months ago.
Refreshed patch for 3.5
21668.4.patch (3.3 KB) - added by pmeenan 10 months ago.
Revised to only apply progressive encoding to JPEG images and to handle both the GD and Imagemagick paths.
21668.5.patch (2.8 KB) - added by derekspringer 9 months ago.
Restored interlacing for .gif and .png images, added 'image_save_progressive' filter.
21668.6.patch (2.6 KB) - added by markoheijnen 5 months ago.
Different approach for Imagick

Download all attachments as: .zip

Change History (36)

comment:1 follow-up: scribu20 months ago

We are currently in the process of adding support for ImageMagick, so not sure if this is relevant or not: #6821

Version 0, edited 20 months ago by scribu (next)

comment:2 in reply to: ↑ 1 _ck_20 months ago

Replying to scribu:

Many servers do not have imagemagick installed, which given the WP install footprint could mean hundreds of thousands of websites.

Why not fix the default mode of WP, considering only 20 characters are needed to be added in three files and do the responsible thing to make the web faster for millions of visitors on all those pages.

Without a filter, there is no way to affect the output image, so changing the core is required. What's interesting is if they added a filter for this or after it, people could even intercept the jpeg output for watermarking, etc.

comment:3 Japh20 months ago

  • Cc japh@… added

comment:4 markoheijnen20 months ago

I don't think progressive images should be the default. I also don't really believe the images will get smaller or there is an extra lost of quality.

About being responsible and that this update is overdue is not really helpful for this ticket. I would love to get some more insight in progressive images and if/how this can also effect PNGs and GIFs.

That said a filter is indeed needed and hopefully it is possible with a reference instead of copying.

SergeyBiryukov20 months ago

comment:5 follow-up: SergeyBiryukov20 months ago

  • Keywords has-patch added

comment:6 in reply to: ↑ 5 _ck_20 months ago

Replying to SergeyBiryukov:

Thanks for making a patch - I would disagree with the patch for wp-admin/includes/image.php because interlace should only be set specifically before imagejpeg and nothing else as gif and png progressive support is spotty.

Basically anywhere imagejpeg is found in WP code, either a filter-by-reference should be done beforehand or imageinterlace set.

I am uncertain if an apply_filters will obey a php function that is declared as function foo(&$var) and really pass it by reference or first make a copy of the variable it's passed and then try to pass that copy by reference to the function. $newimage almost certainly has to be passed by reference and not a copy (but I could be wrong).

We could test this theory easily:
ie. in media.php

add_filter('imagejpeg','setprogressive');
function setprogressive(&$handle) {
  if (function_exists('imageinterlace')) {imageinterlace($handle,1);}
}
...
apply_filters('imagejpeg',$newimage); // no need to return variable if it's by reference?
imagejpeg( $newimage, ...  

If the image produced is progressive, then it works.

SergeyBiryukov20 months ago

comment:9 Japh18 months ago

Re-did the patch for 3.5 taking into account the wp-image-editor for both GD and Imagick, JPG and PNG.

Japh18 months ago

Refreshed patch for 3.5

comment:10 Japh18 months ago

  • Keywords needs-testing added

comment:11 DH-Shredder17 months ago

  • Cc mike.schroder@… added

comment:12 xyzzy14 months ago

  • Cc dennen@… added

comment:13 markoheijnen13 months ago

#19931 was marked as a duplicate.

pmeenan10 months ago

Revised to only apply progressive encoding to JPEG images and to handle both the GD and Imagemagick paths.

comment:14 pmeenan10 months ago

I updated the patch to only encode JPEGs progressively.

Progressive JPEGs are nearly always smaller than baseline in addition to the improved user experience and since the largest images are usually photos we will get most of the bang for our buck there anyway.

PNG's and GIF's tend to inflate the size so I don't recommend touching them.

There are quite a few studies on the benefits of progressive JPEGs from well respected developers and companies:
Yahoo's image optimization post from 2008 (Stoyan is now at Facebook)
Ann Roboson's article for the 2012 web performance calendar
My work at Google and WebPagetest on it
Akamai's image optimization presentation from Velocity 2013

Other than the patch itself, let me know if there is anything I can do to help make this happen (more data, research, testing, whatever).

comment:15 pmeenan10 months ago

  • Cc pmeenan added

comment:16 pmeenan10 months ago

When testing, you can validate that progressive JPEGs are created here.

I tested the patch on an Ubuntu 12.04 system with php 5.3.10 and imagemagick 6.6.9-7 (both the latest from the standard apt sources) and validated both the GD and imagemagick code paths. The GD code path was tested before installing imagemagick and logging was added to both output paths to validate that the writes were done through the expected paths. Progressive JPEGs were correctly created in both cases.

GD
imagemagick

These were photos that I uploaded to my test Wordpress site with the patch (and logging) applied.

comment:17 SergeyBiryukov10 months ago

  • Keywords 3.7-early added
  • Milestone changed from Awaiting Review to Future Release

comment:18 follow-up: DH-Shredder9 months ago

@pmeenan: Thanks for the patch!

@markoheijnen, @derekspringer and I we chatted about this a bit at the Contributor Day at WCSF.

Notes from the conversation:

  • This is still something we want to do.
  • Initially it's something we'd like to introduce as an optional feature via a filter, due to the increase in file sizes, but we could it consider later becoming a default for JPG alone.
  • We'd like to see support for at least PNG as well, since there's no reason not to support it via the framework when it's optional, and PNG is a commonly used format. As you noted, this would not be something done by default either, due to the increase in file size.

derekspringer9 months ago

Restored interlacing for .gif and .png images, added 'image_save_progressive' filter.

comment:19 derekspringer9 months ago

Big thanks to @DH-Shredder for giving me some pointers today while I worked out the latest patch. I was able to successfully test the progressive .jpg creation using the links that @pmeenan provided using gd, but haven't found a reliable way to test successful creation of interlaced .gif & .png or any of the Imagick options.

If someone with access to an install with Imagick would like to test the images, that would be pretty cool :)

comment:20 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:21 in reply to: ↑ 18 pmeenan9 months ago

Replying to DH-Shredder:

@pmeenan: Thanks for the patch!

@markoheijnen, @derekspringer and I we chatted about this a bit at the Contributor Day at WCSF.

Notes from the conversation:

  • This is still something we want to do.
  • Initially it's something we'd like to introduce as an optional feature via a filter, due to the increase in file sizes, but we could it consider later becoming a default for JPG alone.
  • We'd like to see support for at least PNG as well, since there's no reason not to support it via the framework when it's optional, and PNG is a commonly used format. As you noted, this would not be something done by default either, due to the increase in file size.

FWIW, the progressive JPEGs should almost always be smaller than baseline (unlike PNG and GIF). In Stoyan's testing above, 75% of JPEG's that were under 10KB were smaller as progressive and 94% of JPEG's over 10KB were smaller as progressive. That's largely why I killed the PNG and GIF support in my version of the patch and had it turned on by default for JPEG.

comment:22 follow-up: nacin7 months ago

While progressive JPEGs can be smaller, they are not always desired from an aesthetics standpoint.

I am not inclined to support 21668.5.patch as it appears to change functionality, and/or increase the requirements for GD or Imagick to be used. Please correct me if I am mistaken on these points.

comment:23 in reply to: ↑ 22 ; follow-up: pmeenan7 months ago

Replying to nacin:

While progressive JPEGs can be smaller, they are not always desired from an aesthetics standpoint.

I am not inclined to support 21668.5.patch as it appears to change functionality, and/or increase the requirements for GD or Imagick to be used. Please correct me if I am mistaken on these points.

It doesn't change the requirements for GD or Imagick. The only change is to set the progressive flag in cases where GD or Imagick were already being used to save JPEGs (resize or edit).

As far as the asthetics go, I can't really say because I'm not aware of any studies that have been done to evaluate end-user's perception of either baseline (top-down rendering) or progressive. I do know lots of individual developers have strong opinions one way or another and there are plenty in both camps but I'm not aware of any data either way.

I do have data backing the faster visual experience (not just of the blurry initial passes) but that doesn't really address user perceptions and asthetics.

comment:24 nacin7 months ago

  • Milestone changed from 3.7 to Future Release

I am a serious news junkie. Some of my favorite sites do not use WordPress, I can't remember the last time I have seen a progressive JPEG used.

I'm fine with adding filters in place (if they don't already exist) to make it easy for a plugin to flip the interlace bit. But I don't see enough evidence here to change the default.

comment:25 niutech5 months ago

It's been 15 months since this improvement was proposed and still no inclusion into WP core? Come on, it's so simple, it doesn't break anything, but it makes JPGs load faster and it improves Speed Index. Isn't it what should everybody care about in 2013? Provided that only ca. 7% JPGs are progressive, it's high time to change that. Thanks in advance!

markoheijnen5 months ago

Different approach for Imagick

comment:26 in reply to: ↑ 23 markoheijnen5 months ago

Replying to pmeenan:

Replying to nacin:

While progressive JPEGs can be smaller, they are not always desired from an aesthetics standpoint.

I am not inclined to support 21668.5.patch as it appears to change functionality, and/or increase the requirements for GD or Imagick to be used. Please correct me if I am mistaken on these points.

It doesn't change the requirements for GD or Imagick. The only change is to set the progressive flag in cases where GD or Imagick were already being used to save JPEGs (resize or edit).

As far as the asthetics go, I can't really say because I'm not aware of any studies that have been done to evaluate end-user's perception of either baseline (top-down rendering) or progressive. I do know lots of individual developers have strong opinions one way or another and there are plenty in both camps but I'm not aware of any data either way.

I do have data backing the faster visual experience (not just of the blurry initial passes) but that doesn't really address user perceptions and asthetics.

Nacin is right about the possibility to raise the requirements. In case of Imagick the check happens on load. I change that logic now. Not sure what you think about this way.

Since Imagick supports I wonder if we should limit the possibilities in Imagick to only allow it on png, gif and jpg.

comment:27 markoheijnen5 months ago

  • Keywords 3.7-early removed
  • Milestone changed from Future Release to 3.8

comment:28 wonderboymusic5 months ago

  • Milestone changed from 3.8 to Future Release

comment:29 buley3 months ago

For the sake of a more (apparently) performant Internet, saving images as progressive when possible is a default option that I hope WordPress adopts.

At parade.com we used 21668.4.patch for several months on the WP3.5 branch (https://core.trac.wordpress.org/attachment/ticket/21668/21668.4.patch). The improvements in 21668.5.patch (https://core.trac.wordpress.org/attachment/ticket/21668/21668.5.patch ) are nice - allowing filter-based control - but I think the default false option is the wrong one for the Web. The changes in 21668.6 (https://core.trac.wordpress.org/attachment/ticket/21668/21668.6.patch ) seem more complex than required and are to me confusing, especially since it appears to be wrongly checking for a "PNG" mimetype when the $mimetype (correctly) is "image/png".

Unfortunately I was not able to validate any of these patches as working on WordPress 3.8 with both ImageMagick and GD installed using the identify command and the verbose flag set (in my understanding, identify -verbose $path_to_file | grep Interlace should yield "Interlace: JPEG" and not "Interlace: None").

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

comment:30 markoheijnen3 months ago

You are right about the mime type. I didn't check 21668.5.patch good enough for that. It's a bit more complex but it can be required. The solution can be better if it means that we support more ImageMagick/Imagick versions.

I agree with nacin here that having progressive images by default would change the functionality.

Note: See TracTickets for help on using tickets.