WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 13 months ago

#24380 reopened enhancement

Missing Compression Parameter in WP_Image_Editor_GD

Reported by: MuViMoTV Owned by: wonderboymusic
Milestone: Future Release Priority: normal
Severity: normal Version: 3.5.1
Component: Media Keywords: has-patch dev-feedback
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 2 years ago.
class-wp-image-editor-gd.diff (777 bytes) - added by MuViMoTV 2 years ago.
class-wp-image-editor-gd.2.diff (818 bytes) - added by markoheijnen 22 months ago.
Spacing and add comment
24380.diff (817 bytes) - added by SergeyBiryukov 21 months ago.
24380.2.patch (2.2 KB) - added by mikemanger 16 months ago.
24380.3.patch (2.2 KB) - added by mikemanger 16 months ago.
Fix mime_type check before applying filter
24380.4.patch (1.2 KB) - added by mikemanger 16 months 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 15 months ago.
24380.3.diff (1.1 KB) - added by ericlewis 15 months ago.
24380.4.2.patch (1.9 KB) - added by mikemanger 15 months ago.
24380.4.diff (1.8 KB) - added by DrewAPicture 14 months ago.
docs tweaks

Download all attachments as: .zip

Change History (57)

comment:1 @SergeyBiryukov2 years ago

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

comment:2 @markoheijnen2 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.

comment:3 @MuViMoTV2 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

comment:5 @MuViMoTV2 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

comment:6 @markoheijnen2 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

comment:7 @MuViMoTV2 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...

comment:8 @SergeyBiryukov2 years ago

  • Keywords has-patch added; needs-patch removed

comment:9 @SergeyBiryukov2 years ago

  • Type changed from feature request to enhancement

@markoheijnen22 months ago

Spacing and add comment

comment:10 @markoheijnen22 months ago

  • Milestone changed from Awaiting Review to 3.8

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

comment:11 @valeriosza22 months ago

  • Keywords 2nd-opinion needs-testing added

comment:12 @markoheijnen22 months ago

  • Keywords needs-testing removed

This isn't something that can be tested well.

comment:13 @nacin21 months ago

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

comment:14 @markoheijnen21 months 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

comment:15 @markoheijnen21 months ago

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

comment:16 @SergeyBiryukov21 months ago

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

@SergeyBiryukov21 months ago

comment:17 @SergeyBiryukov21 months ago

  • Keywords commit added; 2nd-opinion removed

comment:18 follow-up: @markoheijnen21 months ago

Good to me. I guess we don't need better documentation

Version 0, edited 21 months ago by markoheijnen (next)

comment:19 in reply to: ↑ 18 @DrewAPicture21 months 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 :)

comment:20 @nacin21 months 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.

comment:21 @SergeyBiryukov21 months ago

  • Keywords 3.9-early added

@mikemanger16 months ago

comment:22 @mikemanger16 months 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 16 months ago by mikemanger (previous) (diff)

comment:23 @SergeyBiryukov16 months ago

  • Milestone changed from Future Release to 4.0

@mikemanger16 months ago

Fix mime_type check before applying filter

@mikemanger16 months ago

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

@ericlewis15 months ago

comment:24 @ericlewis15 months ago

  • Keywords commit 2nd-opinion removed

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

comment:25 @DrewAPicture15 months ago

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

@ericlewis15 months ago

comment:26 @wonderboymusic15 months 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.

comment:27 follow-up: @mikemanger15 months 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.

comment:28 in reply to: ↑ 27 ; follow-up: @ericlewis15 months 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 15 months ago by ericlewis (previous) (diff)

comment:29 in reply to: ↑ 28 @mikemanger15 months 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.

comment:30 @wonderboymusic15 months ago

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

comment:31 @wonderboymusic15 months ago

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

@DrewAPicture - review the dox plz

comment:32 @DrewAPicture15 months 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.

@mikemanger15 months ago

comment:33 @mikemanger15 months 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 14 months ago by mikemanger (previous) (diff)

@DrewAPicture14 months ago

docs tweaks

comment:35 @DrewAPicture14 months 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.

comment:36 @ircbot14 months ago

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

comment:37 @ircbot14 months ago

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

comment:38 @mikemanger14 months ago

Replying to DrewAPicture:

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

As Eric points out in comment:28, 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.

Last edited 14 months ago by mikemanger (previous) (diff)

comment:39 @DrewAPicture14 months 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.

comment:40 @ircbot13 months ago

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

comment:41 @DrewAPicture13 months ago

  • Milestone 4.0 deleted

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

comment:42 @DrewAPicture13 months ago

  • Milestone set to Future Release

comment:43 @DH-Shredder13 months ago

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

comment:44 @DH-Shredder13 months ago

  • Milestone changed from Future Release to 4.0

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

comment:45 @wonderboymusic13 months ago

In 29345:

Revert [28640] as per @DH-Shredder.

See #24380.

comment:46 @wonderboymusic13 months ago

  • Milestone changed from 4.0 to Future Release
Note: See TracTickets for help on using tickets.