Make WordPress Core

Opened 11 years ago

Last modified 4 years ago

#24380 reopened enhancement

Missing Compression Parameter in WP_Image_Editor_GD

Reported by: muvimotv's profile MuViMoTV Owned by: wonderboymusic's profile wonderboymusic
Milestone: Priority: normal
Severity: normal Version: 3.5.1
Component: Media Keywords: has-patch dev-feedback needs-refresh
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Setting the image quality parameter has no effect on png files.

Going through the wp-includes/class-wp-image-editor-gd.php

I noticed that the compression parameter for the imagepng function call is missing.

the current quality parameter only affects jpeg files.

for jpeg, quality goes from 0->100 from bad to good

for png, from 0->9 from good to bad.

in the elseif block starting at line 337 i have added a variable called compression and changed the code as follow:

elseif ( 'image/png' == $mime_type ) {
			// convert from full colors to index colors, like original PNG.
			if ( function_exists('imageistruecolor') && ! imageistruecolor( $image ) )
				imagetruecolortopalette( $image, false, imagecolorstotal( $image ) );

			$compression = -((9/100*$this->quality)-9);
			
			if ( ! $this->make_image( $filename, 'imagepng', array( $image, $filename, $compression ) ) )
				return new WP_Error( 'image_save_error', __('Image Editor Save Failed') );
		}

This convert the scale 0->100 into a 0->9 scale that matches the parameters from imaging.

This gives back control to the user on image quality for png files...

Attachments (11)

class-wp-image-editor-gd.php (10.4 KB) - added by MuViMoTV 11 years ago.
class-wp-image-editor-gd.diff (777 bytes) - added by MuViMoTV 11 years ago.
class-wp-image-editor-gd.2.diff (818 bytes) - added by markoheijnen 10 years ago.
Spacing and add comment
24380.diff (817 bytes) - added by SergeyBiryukov 10 years ago.
24380.2.patch (2.2 KB) - added by mikemanger 10 years ago.
24380.3.patch (2.2 KB) - added by mikemanger 10 years ago.
Fix mime_type check before applying filter
24380.4.patch (1.2 KB) - added by mikemanger 10 years ago.
Removed my png_compression_level filter as I feel the existing wp_editor_set_quality covers this
24380.2.diff (1.1 KB) - added by ericlewis 10 years ago.
24380.3.diff (1.1 KB) - added by ericlewis 10 years ago.
24380.4.2.patch (1.9 KB) - added by mikemanger 10 years ago.
24380.4.diff (1.8 KB) - added by DrewAPicture 10 years ago.
docs tweaks

Download all attachments as: .zip

Change History (60)

#1 @SergeyBiryukov
11 years ago

  • Component changed from Editor to Media
  • Description modified (diff)

#2 @markoheijnen
11 years ago

  • Type changed from defect (bug) to feature request

I thought we did added it but it was never in core before. Also you need to add a patch and not the whole file.

#3 @MuViMoTV
11 years ago

i wouldn't have a clue how to do a "patch" this is my first bug report and i have zero experience in team work...
can you point me in the right direction?
thanks

#5 @MuViMoTV
11 years ago

Hmmm well it doesn't seems to work...

I checked out the trunk version

svn co http://core.svn.wordpress.org/trunk/ .

Then changed the file manually and finally issued this command

svn diff class-wp-image-editor-gd.php class-wp-image-editor-gd.diff

and got the following :

svn: E150000: '/Users/MuViMoTV/wordpress_trunk/class-wp-image-editor-gd.php' is not under version control

#6 @markoheijnen
11 years ago

That file is in the root and not in wp-includes.

So it should be: svn diff wp-includes/class-wp-image-editor-gd.php > class-wp-image-editor-gd.diff

But I always do: svn diff > class-wp-image-editor-gd.diff

#7 @MuViMoTV
11 years ago

sweet! thank you so much for that...

I finally managed to get it to work...

i feel a bit stupid though having missed the folder part...

I've attached the diff file...

#8 @SergeyBiryukov
11 years ago

  • Keywords has-patch added; needs-patch removed

#9 @SergeyBiryukov
11 years ago

  • Type changed from feature request to enhancement

@markoheijnen
10 years ago

Spacing and add comment

#10 @markoheijnen
10 years ago

  • Milestone changed from Awaiting Review to 3.8

Moving to 3.8. Updated patch to add some spacing and comment.

#11 @valeriosza
10 years ago

  • Keywords 2nd-opinion needs-testing added

#12 @markoheijnen
10 years ago

  • Keywords needs-testing removed

This isn't something that can be tested well.

#13 @nacin
10 years ago

Is this ready to go? This math is confusing, a comment would be useful.

#14 @markoheijnen
10 years ago

I would say yes. Math is a bit weird but thats the logic of GD. So my comment is to short I guess ;)

If quality is 100 - no compression
- ( ( 0.09 * 100 ) - 9 ) = 0

If quality is 0 - with full compression
- ( ( 0.09 * 0 ) - 9 ) = 9

#15 @markoheijnen
10 years ago

Will add a patch with a better comment tomorrow that also rounds the result since the documentation described the field as an integer

#16 @SergeyBiryukov
10 years ago

Looks like this can be simplified to 9 - 0.09 * $this->quality.

#17 @SergeyBiryukov
10 years ago

  • Keywords commit added; 2nd-opinion removed

#18 follow-up: @markoheijnen
10 years ago

Good to me. I guess we don't need better documentation now since the code is now better to read

Last edited 10 years ago by markoheijnen (previous) (diff)

#19 in reply to: ↑ 18 @DrewAPicture
10 years ago

Replying to markoheijnen:

Good to me. I guess we don't need better documentation now since the code is now better to read

We can *always* improve documentation :)

#20 @nacin
10 years ago

  • Milestone changed from 3.8 to Future Release

Per IRC, moving to 3.9 for this enhancement.

I think this could still benefit from a few lines of documentation that explains what it is doing to $this->quality. Seems like it is converting 0-100 (or 1-100?) to 9-1 (or 9-0, or 1-9, or 0-9)? These are magic numbers and it would be nice to know why it needs to do this math.

#21 @SergeyBiryukov
10 years ago

  • Keywords 3.9-early added

@mikemanger
10 years ago

#22 @mikemanger
10 years ago

  • Keywords 2nd-opinion added; 3.9-early removed

I feel that whilst converting 1-100 to 9-0 is technically correct, it is a bit confusing (PNG is lossless, so more compression doesn't mean less quality) and could have some unwanted results (the default value for quality is 90 which would give the worst compression, not the best).

I have updated the GD class to convert quality from 1-100 to 0-9, the code (I hope) reads a bit better and as a bonus this better matches how the ImageMagick library handles the quality value. The main difference is that if a quality value of 1-10 is passed to GD then no compression is applied to the image.

To accompany jpeg_quality, I have also added a new filter called png_compression_level in the main WP_Image_Editor class.

Last edited 10 years ago by mikemanger (previous) (diff)

#23 @SergeyBiryukov
10 years ago

  • Milestone changed from Future Release to 4.0

@mikemanger
10 years ago

Fix mime_type check before applying filter

@mikemanger
10 years ago

Removed my png_compression_level filter as I feel the existing wp_editor_set_quality covers this

@ericlewis
10 years ago

#24 @ericlewis
10 years ago

  • Keywords commit 2nd-opinion removed

attachment:24380.2.diff adds clear inline documentation, and hopefully more self-documenting code.

#25 @DrewAPicture
10 years ago

@ericlewis: Multi-line comments use a slightly different syntax.

@ericlewis
10 years ago

#26 @wonderboymusic
10 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 28640:

In WP_Image_Editor_GD::_save(), pass $compression_level into the args array for ->make_image().

Props MuViMoTV, markoheijnen, SergeyBiryukov, mikemanger, ericlewis.
Fixes #24380.

#27 follow-up: @mikemanger
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Anyone care to explain why we are converting from high to low? The original statement that it goes "0->9 from good to bad" is wrong. PNG compression is lossless so there is no such thing as "bad" compression with regards to image quality, just a trade off between file size and compression speed. Best explanation I can find is in the discussion on the PHP documentation page.

I can run some tests to demonstrate this.

#28 in reply to: ↑ 27 ; follow-up: @ericlewis
10 years ago

  • Keywords 2nd-opinion dev-feedback added; has-patch removed

Replying to mikemanger:

Anyone care to explain why we are converting from high to low? The original statement that it goes "0->9 from good to bad" is wrong. PNG compression is lossless so there is no such thing as "bad" compression with regards to image quality, just a trade off between file size and compression speed. Best explanation I can find is in the discussion on the PHP documentation page.

It seems there's some rather widespread confusion of how the $quality parameter for imagepng() works. Thanks for reopening this, you're completely on point here.

Since actual quality has nothing to do with the operation at hand, would it be useful to create a PNG-specific class property like compression_level_png to use here instead?

Last edited 10 years ago by ericlewis (previous) (diff)

#29 in reply to: ↑ 28 @mikemanger
10 years ago

Replying to ericlewis:

Since actual quality has nothing to do with the operation at hand, would it be useful to create a PNG-specific class property like compression_level_png to use here instead?

Yes, adding a separate property should make the documentation clearer. It could be more specific like [image_]zlib_compression_level, but this might be just needless abstraction.. just wondering what happens if another class gets added with a completely different scale.

If the property goes from 0-9, this would need converting to a 1-100 scale for the ImageMagick class.

Another option could be to add a filter (attachment:24380.3.patch) or just make it obvious what 'quality' means for different image formats in the wp_editor_set_quality filter documentation.

#30 @wonderboymusic
10 years ago

24380.3.patch looks nice to me - can someone else weigh in with a thumbs up? @ericlewis: you or someone like you

#31 @wonderboymusic
10 years ago

  • Focuses docs added
  • Keywords has-patch added; 2nd-opinion dev-feedback removed

@DrewAPicture - review the dox plz

#32 @DrewAPicture
10 years ago

  • Keywords needs-patch added; has-patch removed

So, looking at 24380.3.patch, I've got a couple of questions:

  1. We're converting 1-100 to 0-9 ... oh except it also accepts -1? The -1 information should be at the end so it doesn't create confusion.
  2. In the png_compression_level filter docs, it's mentioned that there are two different contexts possible, yet the filter is only evaluated one place in the codebase and the context isn't dynamic. So where would the second context enter the picture? Also, the contexts should be spelled out for the $context parameter, not in the long description area.

#33 @mikemanger
10 years ago

Replying to DrewAPicture:

  1. In the png_compression_level filter docs, it's mentioned that there are two different contexts possible, yet the filter is only evaluated one place in the codebase and the context isn't dynamic. So where would the second context enter the picture? Also, the contexts should be spelled out for the $context parameter, not in the long description area.

In 24380.4.2.patch I have removed the context parameter. I orignally copied it from the jpeg_quality filter above so the context parameter should probably be removed or deprecated there as well (looks like it came from the old image_resize function).

The patch also changes the conversion documentation order.

Last edited 10 years ago by mikemanger (previous) (diff)

@DrewAPicture
10 years ago

docs tweaks

#35 @DrewAPicture
10 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

24380.4.diff tweaks the docs a bit to make it more clear that we're converting the quality scale for GD compatibility.

It still feels like there might be a more straightforward way to accomplish this. Also, I'm confused as to why the png check falls outside the ! $this->quality check in the get_quality() method.

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


10 years ago

This ticket was mentioned in IRC in #wordpress-dev by doc-bot. View the logs.


10 years ago

#38 @mikemanger
10 years ago

Replying to DrewAPicture:

It still feels like there might be a more straightforward way to accomplish this.

As Eric points out in comment:29, another alternative would be to have a new property for this in the WP_Image_Editor class. This would simplify the GD implementation however we would then need some more logic to convert to a 0-100 scale in the Imagick class.

Version 0, edited 10 years ago by mikemanger (next)

#39 @DrewAPicture
10 years ago

  • Focuses docs removed

We've got the verbiage worked out. Sounds like all that's left is to decide if this the best way to go about it.

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


10 years ago

#41 @DrewAPicture
10 years ago

  • Milestone 4.0 deleted

No consensus yet, and too late for this in 4.0. Punting.

#42 @DrewAPicture
10 years ago

  • Milestone set to Future Release

#43 @kirasong
10 years ago

Thinking we should back [28640] out since there isn't consensus and we're getting close to release.

#44 @kirasong
10 years ago

  • Milestone changed from Future Release to 4.0

Setting back to 4.0 so the change doesn't get lost for 4.0.

#45 @wonderboymusic
10 years ago

In 29345:

Revert [28640] as per @DH-Shredder.

See #24380.

#46 @wonderboymusic
10 years ago

  • Milestone changed from 4.0 to Future Release

#47 @chriscct7
8 years ago

  • Keywords needs-refresh added

#48 @mikemanger
8 years ago

Given the 4.5 ImageMagick improvements, I decided to do some GD testing - I averaged 100 runs for each compression level. It isn't particularly scientific as I haven't removed outliers and, more importantly, haven't profiled the memory usage.

Here are the results I got against a 143KB (146270 bytes) screenshot of this page saved in MS paint:

Level Time (seconds) Size (bytes)
0 0.0552 3722124
1 0.044 146909
2 0.0439 145171
3 0.044 143488
4 0.0589 128725
5 0.0603 127968
6 0.0642 127312
7 0.0666 126933
8 0.0959 125587
9 0.1903 125195

Not passing a quality level gives the same result as 6, which is the default on most servers (it can be changed at PHP compile time or php.ini).

I tried some larger images and got more or less the same results - the difference between 8/9 can be much bigger but so can the process time (sometimes 3 times longer).

I guess the decision is to either:

  • leave it is as it is
  • change the default to something else
  • and/or allow for people to filter the value.

I'm honestly not sure it matters as even then Google PageSpeed will likely still complain with most images as I was able to compress the same image down to 90KB using optipng.

#49 @birgire
4 years ago

#48215 was marked as a duplicate.

Note: See TracTickets for help on using tickets.