Make WordPress Core

Opened 2 years ago

Last modified 5 months ago

#28634 accepted defect (bug)

Upload images. does not clear Thumbnails metadata (+30kb from camera for each thumbnails)

Reported by: alexufo Owned by: joemcgill
Milestone: Future Release Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch
Focuses: Cc:



add_image_size( 'thumb100x100', 100, 100, true ); 
add_filter( 'jpeg_quality', create_function( '', 'return 30;' ));

to function.php

upload picture to library.

look at upload folder.


How can will be 30kb file size with visible jpg artifacts?
I look like save twice. With 30% compression and with 100% quality

Attachments (14)

P1020094-100x100-GD.jpg (1.4 KB) - added by mikeschroder 2 years ago.
P10200944-100x100-Imagick.jpg (30.4 KB) - added by mikeschroder 2 years ago.
P1020094-noexif.JPG (5.6 MB) - added by mikeschroder 2 years ago.
P1020094-noexif-100x100-Imagick.jpg (1.0 KB) - added by mikeschroder 2 years ago.
28634.patch (490 bytes) - added by juliobox 2 years ago.
Strips an image of all profiles and comments striping off the exif information)
28634-2.patch (551 bytes) - added by juliobox 2 years ago.
Same with a filter to keep a retrocompatibility
28634-3.patch (540 bytes) - added by juliobox 2 years ago.
Add a size parameter, filter in resize() instead of load()
28634.diff (660 bytes) - added by wonderboymusic 13 months ago.
28634.2.diff (1.5 KB) - added by adamsilverstein 12 months ago.
28634.3.diff (1.3 KB) - added by markoheijnen 9 months ago.
28634.4.diff (1.3 KB) - added by markoheijnen 9 months ago.
28634.5.diff (2.7 KB) - added by joemcgill 8 months ago.
28634.6.diff (3.4 KB) - added by joemcgill 8 months ago.
28634.7.diff (2.5 KB) - added by joemcgill 8 months ago.

Change History (70)

#1 @alexufo
2 years ago

  • Severity changed from normal to critical

testing second server, problem available. Perhaps many users are deceived about jpeg compression efficiency in wp. 100x100 size must be about 9kb max! test it save for web in photoshop.

#2 @mikeschroder
2 years ago

  • Severity changed from critical to normal

I'm not able to reproduce this -- sizes are as expected with both Imagick and GD.

Could you please attach the original image you're using to test (before resizing)?

#4 @mikeschroder
2 years ago

I took a look, and basically it looks like, from what I can tell, the original image has a lot of metadata.

This is resulting in larger relative file sizes at small resolutions. (Meaning, it's more noticiable)

With Imagick, we maintain metadata. With GD, it gets thrown out due to engine differences.

If you run a diff on the two files, you can see the additional meta.

If you strip the exif before uploading the image, it resizes to under 2kb, even with Imagick.

Attached files to illustrate.

#5 @alexufo
2 years ago

  • Summary changed from jpeg_quality hook fake compress images to Upload images. does not clear Thumbnails metadata (+30kb from camera for each thumbnails)

of course!how i can forgot it.
hm.. i didn't think wp save metadata to thumbnail. is just preview and it must be clear.

Many users can save photograph from camera directly in site. I think it will be good idea clear metadata at least for thumbnail by default. 30 kb is soooo much.
Shortcode gallery consist of these thumbnail images with metadata. 15 photographs have 450kb metadata! Is bad.

Metadata can have thumbnail for thumbnail!

Last edited 2 years ago by alexufo (previous) (diff)

#6 @wonderboymusic
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Needs a solution via patch to move forward

2 years ago

Strips an image of all profiles and comments striping off the exif information)

2 years ago

Same with a filter to keep a retrocompatibility

#7 @alexufo
2 years ago

wp use imagic in core by default? not GD2 extension? Your patch good, but i didnt know about capability.

#8 @juliobox
2 years ago

There is no capability here, just a filter to avoid that since the patch, exif data are stripped down.

For the choice between imagic or GD2, yes :

$implementations = apply_filters( 'wp_image_editors', array( 'WP_Image_Editor_Imagick', 'WP_Image_Editor_GD' ) );

From https://core.trac.wordpress.org/browser/trunk/src/wp-includes/media.php#L2481

#9 @alexufo
2 years ago

sorry, I mean that imagick isnt installed at hosting, filter will not work. GD library have not strip exif function. I prefer imagick or obvious reasons. but how wp community look on functions works only in imagick.

#10 @juliobox
2 years ago

yes, so if you do not have imagick, do not touch this filter?

#11 @markoheijnen
2 years ago

  • Keywords has-patch added; needs-patch removed

I only have one issue with 28634-2.patch and that is the fact that I think the location should be different. I can see myself using the filter but then only for certain image sizes. Like only for thumbnails. Maybe that is given the default editor to much power since it could be done by subclassing the Imagick editor.

#12 @alexufo
2 years ago

perfect logic.) But i think strip exif function should switch on by default. Exif meta needs only for photographers but not in thumbnail for them too. 30kb very much for all wordpress users. All wordpress living no one ask why thumbs have metadata. All wordpress thumbs in web allready have useless meta data in random kb.
Clearing meta in GD it easy by creation new jpg. All cms that we know do not save meta for thumbs.

Last edited 2 years ago by alexufo (previous) (diff)

#13 @juliobox
2 years ago

@alex: You can not strip exif by default since now. Also, my filter is only in the imagick class, so if not installed the filter won't be triggered ;)
Imagine all photographers, already using exif data, maybe from thumbs, you don't know.
Then, after the patch, all is gone!? They have to apply a filter to retreive exif like before?
Not logical! Even if it's logic to strip it for thumbs, you can not do it by default from now, too late.
@marko: new patch added, tested like that, works:

add_filter( 'keep_image_exif', 'exif_if_size', 10, 2 );
function exif_if_size( $value, $size_data ) {
	return $size_data['height'] >= 500 && $size_data['width'] >= 500;

If any of the sizes are < 500 px, the exif data will be stripped.

2 years ago

Add a size parameter, filter in resize() instead of load()

#14 @markoheijnen
2 years ago

That looks great :). Thanks for your work.

I do agree with Juliobox by not stripping the exif data if possible. Not exactly for those reasoning. This due the fact that Imagick isn't installed on a lot of server. I do believe that most of our users don't care a lot about bandwidth. So it all becomes on making a decision what makes the most sense. I like to have it in there because it's content and we should not destroy content.

#15 @juliobox
2 years ago

Remember, this filter can only be used if imagick is installed and in usage. This is not related to GD2 here.

#16 @mikeschroder
2 years ago

While I have no problem with using a filter or other solution to allow stripping exif, I do not think we should strip exif by default here, since one of the big benefits of Imagick is keeping exif data -- in particular, color profiles.

#17 @alexufo
2 years ago

@DH-Shredder color profiles are not supported official in any browsers. Not all Desktop software can read this in exif.

Guys, we need to do wp better in the details and apply best practice web and seo). when will be time, when we can broke backward capability?)

#18 @juliobox
2 years ago

We can not strip exif by default because of backward capability, even to do a better WP.
I dont think "strip exif" is a best practice or recommanded thing. This is a thing.

#19 @markoheijnen
2 years ago

@alexufo I think we can close this discussion now. We will not strip EXIF data because it's not in the best interest for our users and the web. Browsers do support one way or the other color profiles.

And if you as a user don't need them then you can simply use the filter.

#20 @alexufo
2 years ago

  • Resolution set to invalid
  • Status changed from new to closed

Yes. My mistake. Color managing in new browsers works, last two years i do not check it . I cant see Official articles for users who realy want use icc in web. Instead this Adobe Photoshop add check-box in the save for web mode "convert to sRGB" because today people export own photographs with adobeRGB profile and do not understanding why they little desaturated on some browsers or image viewers.
I all understand. Blame this http://developers.google.com/speed/pagespeed/insights/

Last edited 2 years ago by alexufo (previous) (diff)

#21 @juliobox
2 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Why closed? The filter is a good idea, only the strip by default is not i guess.

#22 @alexufo
2 years ago

Sorry, markoheijnen say close this discussion now.))) Ow.. discussion meant discussion?) no problem. This filter must be live and grow roots.

#23 @johnbillion
2 years ago

  • Version changed from trunk to 3.5

#24 follow-up: @markoheijnen
2 years ago

I never say close :) Also just seeing it now otherwise would have reopened it.
What about renaming the filter to strip_image_exif which is default false.

#25 in reply to: ↑ 24 @alexufo
2 years ago

Replying to markoheijnen:

What about renaming the filter to strip_image_exif which is default false.


#26 @juliobox
14 months ago

Hello guys, what do we need to clode this ticket? Thanks !

#27 @wonderboymusic
13 months ago

  • Keywords needs-docs good-first-bug added
  • Milestone changed from Future Release to 4.4

This ticket was mentioned in Slack in #core by sergey. View the logs.

12 months ago

#29 @SergeyBiryukov
12 months ago

  • Keywords good-first-bug removed

Not sure if 'keep_image_exif' is a good idea, since it won't work for GD. Should be documented well, otherwise seems like a wontfix.

#30 @alexufo
12 months ago

I suggest for GD low level cleaner https://github.com/subabrain/php-jpeg-cleaner/blob/master/jpeg_cleaner_class.php
This is follow jpg standard but but i think it is not tested on big auditory.

#31 @wonderboymusic
12 months ago

  • Milestone changed from 4.4 to Future Release

#32 @adamsilverstein
12 months ago


  • default to not stripping data
  • rename filter imagick_strip_image_exif
  • add some hook docs

This ticket was mentioned in Slack in #core-images by markoheijnen. View the logs.

9 months ago

This ticket was mentioned in Slack in #core-images by markoheijnen. View the logs.

9 months ago

#35 @markoheijnen
9 months ago

  • Keywords needs-docs removed

Updated the patch

  • Rename filter image_strip_exif (no imagick and moved image as first like other filters in the class)
  • Change filter to have size name only instead of $size_data and $orig_image. We never send the image object itself.
  • Moved the strip exif logic inside it's own public method. Normally I don't like to have a method only available for one WP_Image_Editor but it's nice to have public for people using it directly.

I haven't seen a visual difference in removing all, adding back icc or not removing exif at all.
Sizes are 40, 43 and 67kb for a 300x300 crop.

#36 @mikeschroder
8 months ago

In 36700:

Media: Optimize Imagick settings for quality and filesize

  • Resamples and sharpens larger images before resize.
  • Uses imagick::FILTER_TRIANGLE for a smoother resize.
  • Introduces WP_Image_Editor_Imagick::thumbnail_image() protected method to efficiently resize images. Similar to the functionality of Imagick's thumbnailImage().
  • Introduces WP_Image_Editor_Imagick::strip_meta() protected method and image_strip_meta filter that, by default, strip image profiles to reduce file size, while leaving color profiles intact.

See: #33642, #30402, #28634.
Props: joemcgill, dnewton, mikeschroder.

#37 @markoheijnen
8 months ago

I do believe the way my method works is a better since it's less guessing by calling stripImage(). Also I think it's fine to have that one public so others can use and add an empty method to WP_Image_Editor.

Also why the sudden change to have it strip all data by default?

#38 @alexufo
8 months ago

@markoheijnen if it is will not strip data by default this is useless feature. Who will know about it? Exif data ignored by gzip in http. I have no idea how say 99% wordpress users remove useless exif in thumbnails if they not used them. I found this problem then I checked site by google speed page.I think wordpress distributive must show 100/100 out of the box.

And i suppose this is not just feature request, this is a silent bug who generate TB every day.
If anyone using tags from thumbnail they can read new information in wp release.
I think VERY small percent of users reads exif from thumbnails. Anyway it is bad and not popular idea. Otherwise we have TB traffic every day and in future it will be only bigger

#39 @joemcgill
8 months ago

  • Milestone changed from Future Release to 4.5
  • Owner set to joemcgill
  • Status changed from reopened to accepted

#40 @DrewAPicture
8 months ago

In 36846:

Docs: Improve the hook doc summary for the image_strip_meta filter, introduced in [36700].

See: #33642, #30402, #28634. See #35986.

#41 @DrewAPicture
8 months ago

In 36847:

Docs: Improve DocBlock syntax and add a missing @return notation for WP_Image_Editor_Imagick::strip_meta(), introduced in [36700].

See: #33642, #30402, #28634. See #35986.

8 months ago

#42 @joemcgill
8 months ago

28634.5.diff is a refresh of 28634.4.diff after the changes to WP_Image_Editor_Imagick that landed in r36700 with a few notable differences:

  • The strip_meta() function is run by default but can be overridden via the image_strip_meta filter.
  • I've left strip_meta() as a protected method for now but am open to making it public.

I prefer this way of clearing data in since it relies on better supported Imagick methods instead of Imagick::deleteImageProperty() and Imagick::setImageProperty(), which was making the whole resize process fail silently on a test environment for me using older versions of Imagick (v2.2.2).

A few next steps:

Should we handle required methods separately inside this method so Imagick can be used even if stripping meta isn't supported for some reason of should we just add 'getimageprofiles', 'stripimage', and 'profileimage' to the required methods array in WP_Image_Editor_Imagick::test()? I suspect the former.

Should we give users the ability to filter a whitelist of protected profile types that won't be stripped or should we keep this as a simple on/off switch for now?

Last edited 8 months ago by joemcgill (previous) (diff)

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

8 months ago

#44 @mikeschroder
8 months ago

@joemcgill We chatted about a few things RE: the patch in the image chat linked above.

On testing, an additional note is that any calls to Imagick still need to be within try-catch blocks, which looks to have been lost in the move to stripImage().

Last edited 8 months ago by mikeschroder (previous) (diff)

#45 @mikeschroder
8 months ago

Did some testing on this and chatted with @ahockley in terms of finding out what profiles we should whitelist.


  • It looks like the data that are most interesting are within the iptc profile, which includes copyright info, but still excludes the majority of 'extras' that don't need to be in resized images.
  • We're still stripping "Rights Usage Terms," but I haven't tracked down the name of the profile yet.
  • Due to current design, when multi_resize is run, we're searching for profiles in the original every time an image is resized (to re-add them). Thinking we should cache/store that information so that it only needs to be retrieved once.

8 months ago

#46 @joemcgill
8 months ago

28634.6.diff Protects both 'icc' and 'iptc' profiles and adds a filter, named image_protected_profiles to modify the list of profile type that can be protected when strip_meta() is run. This also saves the result of the getImageProfiles() as a static variable to avoid calling it several times during a multi_resize() run. The Imagick functionality is also moved back into a try-catch bock like it was after r36700.

#47 follow-up: @markoheijnen
8 months ago

That filter isn't really needed. The couple of people who want to do that, could just use a custom editor instead. Also it feels when the really need to, that it's a bug on our side.

Also after thinking about it. I'm a bit worried about a scenario where someone wants to store different information per image size. It feels that this can be a common situation where small sizes are free to use but bigger/qualitative sizes aren't. Could be an idea to store the profile and exif information on load. Unsure if this will cause performance issues.

#48 in reply to: ↑ 47 @joemcgill
8 months ago

Replying to markoheijnen:

I wouldn't expect someone who wants to keep specific profile data (e.g., Exif) but wants to dump all of the extra vendor profiles to have to build their own editor in order to do so. Additionally, we could probably pass the current $image to the filter so someone could conditionally keep extra metadata depending on size, which would take care of your second concern.

#49 follow-up: @markoheijnen
8 months ago

That is already the case with your patch. It's always dumping all the Exif data or non at all. You only store the profile data and not the property data.

My second concern can only be fixed in the way we do quality now. Having the 'image_strip_meta' set a protected(maybe public) property in load(). Non of the methods can know the size except multi_resize(). This way you can set the property to the right on in multi_resize().

Personally I would add two new methods: get_profiles() and get_exif_data(). Those method internally caching the data into properties when requested.

#50 in reply to: ↑ 49 @joemcgill
8 months ago

Replying to markoheijnen:

That is already the case with your patch. It's always dumping all the Exif data or non at all. You only store the profile data and not the property data.

I see what you mean about Exif properties. In all the images I tested, Exif data was being saved as a profile as well, which is when adding the filter would make the most sense. However, I think this is all academic at this point, because calling setImageProperties() after stripImage() won't actually preserve anything. We're probably better off doing something similar to what's already in r36700 and delete profiles and properties that we don't care about.

I like the idea of being able to switch off stripping in multi_resize based on the intermediate image size.

#51 @markoheijnen
8 months ago

I will do some testing this week to see if it's normal or not.

I do think that "We don't care about" isn't what this ticket is about. It's about deleting all the Exif information. This can mean two things: first is that the current solution is good enough for now and need to be revisited in 4.6 with stripImage() or we keep doing what is currently in core. I do believe that we shouldn't want to do more then just theimage_strip_meta filter. Our priority is that we need to prevent that images don't lose their quality.

The only thing that worries me is the rotation of images but that's already an long term issue that I will look into for 4.6

8 months ago

#52 @joemcgill
8 months ago

I ran a few tests and it looks like at least iOS and OS X are reading the orientation from the Exif profile and not the properties, so I don't think we can strip Exif profiles until after we've handled orientation. In the mean time, 28634.7.diff protects icc, iptc, and exif metadata unless overridden by a filter. This would allow people who don't care about any of the metadata to strip everything by doing something like:

add_filter( 'image_protected_profiles', function() { 
    return array();
} );

#53 @markoheijnen
8 months ago

Sure we can ;) GD is the most used so having the same situation isn't perse bad. But in 4.5 we really should not add the filter image_protected_profiles. I think it's not smart to add it at all but at least do not add it in 4.5. If we are unsure then we shouldn't strip the data by default for 4.5.

#54 @mikeschroder
8 months ago

In 36891:

Media: Progressive enhancement for Imagick; add profiles to whitelist.

  • Progressive enhancement for optional compression improvements and stripping meta.
  • Whitelist IPTC and XMP profiles to maintain Copyright and Rights Usage Terms.
  • Whitelist EXIF profile to maintain orientation information. If handled on upload in the future, it can be stripped as well.

Fixes #33642. See #28634.
Props joemcgill, juliobox, ahockley, markoheijnen, adamsilverstein, wonderboymusic, mikeschroder.

#55 @joemcgill
8 months ago

  • Milestone changed from 4.5 to Future Release

Following r36891: we're being very conservative in which profiles we're stripping in 4.5. I'm going to leave this open so we can look into allowing people to strip more data in a future release. If we do end up stripping everything from resized images, I think we should allow people to filter specific profiles that they want to hold onto without needing to create their own editor.

The biggest filesize savings will probably come from stripping Exif profiles, which we could make the default if we can first handle orientation data (see #33051).

[Edit 2016-05-28: Noticed that I mistakenly referenced version 4.4 instead of 4.5 in this comment. This has been corrected in order to not confuse anyone (probably myself) trying to follow this thread in the future]

Last edited 5 months ago by joemcgill (previous) (diff)

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

6 months ago

Note: See TracTickets for help on using tickets.