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

[alexufo]

add

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.

​https://dl.dropboxusercontent.com/u/3013858/P1030174-100x100.jpg

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

[alexufo]

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.

[mikeschroder]

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)?

[alexufo]

original: ​https://dl.dropboxusercontent.com/u/3013858/P1020094.JPG

I am trying to set jpg quality to 2 and all thumbs still not below 30 kb

​http://data3.floomby.com/files/share/25_6_2014/23/oP8WKavCtUirT0So4Skw.png

[mikeschroder]

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.

[alexufo]

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!

[wonderboymusic]

Needs a solution via patch to move forward

[juliobox]

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

[juliobox]

Same with a filter to keep a retrocompatibility

[alexufo]

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

[juliobox]

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

[alexufo]

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.

[juliobox]

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

[markoheijnen]

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.

[alexufo]

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.

[juliobox]

@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.

[juliobox]

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

[markoheijnen]

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.

[juliobox]

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

[mikeschroder]

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.

[alexufo]

@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?)

[juliobox]

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.

[markoheijnen]

@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.

[alexufo]

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/

[juliobox]

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

[alexufo]

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

[markoheijnen]

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.

[alexufo]

Replying to markoheijnen:

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

agree

[juliobox]

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

[SergeyBiryukov]

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.

[alexufo]

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.

[adamsilverstein]

28634.2.diff​

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

[markoheijnen]

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.

[mikeschroder]

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.

[markoheijnen]

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?

[alexufo]

@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

[DrewAPicture]

In 36846:

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

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

[DrewAPicture]

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.

[joemcgill]

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?

[mikeschroder]

@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().

[mikeschroder]

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

Notes:

• 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.

[joemcgill]

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.

[markoheijnen]

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.

[joemcgill]

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.

[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.

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.

[joemcgill]

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.

[markoheijnen]

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

[joemcgill]

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();
} );

[markoheijnen]

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.

[mikeschroder]

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.

[joemcgill]

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]

[fliespl]

I think it makes sense to rethink adding filter to allow users to decided whether to strip all exif / icc data (probably false by default).

I have comparison with thumb resized from wordpress and the one proposed with gtmetrix.

From the diff it's quite obvious that gtmetrix expects thumbnail to have exif stripped. How about allowing call to stripImage() by filter in imagick class?

1c1
< Image: gtmetrix.jpg
---
> Image: orig.jpg
67c67
<   Orientation: Undefined
---
>   Orientation: TopLeft
69,70c69,130
<     date:create: 2019-01-05T19:29:29+01:00
<     date:modify: 2019-01-05T15:15:36+01:00
---
>     date:create: 2019-01-05T19:29:21+01:00
>     date:modify: 2018-12-28T01:09:47+01:00
>     exif:ApertureValue: 262144/65536
>     exif:BitsPerSample: 8, 8, 8
>     exif:ColorSpace: 1
>     exif:ComponentsConfiguration: 1, 2, 3, 0
>     exif:CustomRendered: 0
>     exif:DateTime: 2017:12:28 15:17:03
>     exif:DateTimeDigitized: 2000:01:01 00:01:04
>     exif:DateTimeOriginal: 2000:01:01 00:01:04
>     exif:ExifImageLength: 1000
>     exif:ExifImageWidth: 1441
>     exif:ExifOffset: 304
>     exif:ExifVersion: 48, 50, 51, 48
>     exif:ExposureBiasValue: 0/1
>     exif:ExposureMode: 0
>     exif:ExposureProgram: 2
>     exif:ExposureTime: 1/60
>     exif:Flash: 16
>     exif:FlashPixVersion: 48, 49, 48, 48
>     exif:FNumber: 4/1
>     exif:FocalLength: 70/1
>     exif:FocalPlaneResolutionUnit: 2
>     exif:FocalPlaneXResolution: 5472000/1436
>     exif:FocalPlaneYResolution: 3648000/956
>     exif:GPSInfo: 1252
>     exif:GPSVersionID: 2, 3, 0, 0
>     exif:ImageLength: 3648
>     exif:ImageWidth: 5472
>     exif:InteroperabilityOffset: 1220
>     exif:ISOSpeedRatings: 1600
>     exif:Make: Canon
>     exif:MaxApertureValue: 4/1
>     exif:MeteringMode: 5
>     exif:Model: Canon EOS 6D
>     exif:Orientation: 1
>     exif:PhotometricInterpretation: 2
>     exif:ResolutionUnit: 2
>     exif:SamplesPerPixel: 3
>     exif:SceneCaptureType: 0
>     exif:ShutterSpeedValue: 393216/65536
>     exif:Software: Adobe Photoshop CC 2015.5 (Windows)
>     exif:SubSecTime: 00
>     exif:SubSecTimeDigitized: 00
>     exif:SubSecTimeOriginal: 00
>     exif:thumbnail:Compression: 6
>     exif:thumbnail:InteroperabilityIndex: R98
>     exif:thumbnail:InteroperabilityVersion: 48, 49, 48, 48
>     exif:thumbnail:JPEGInterchangeFormat: 1366
>     exif:thumbnail:JPEGInterchangeFormatLength: 11585
>     exif:thumbnail:ResolutionUnit: 2
>     exif:thumbnail:XResolution: 72/1
>     exif:thumbnail:YResolution: 72/1
>     exif:UserComment: 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
>     exif:WhiteBalance: 0
>     exif:XResolution: 72/1
>     exif:YCbCrPositioning: 2
>     exif:YResolution: 72/1
>     icc:copyright: Copyright (c) 1998 Hewlett-Packard Company
>     icc:description: sRGB IEC61966-2.1
>     icc:manufacturer: IEC http://www.iec.ch
>     icc:model: IEC 61966-2.1 Default RGB colour space - sRGB
73a134,144
>     unknown: 2
>   Profiles:
>     Profile-8bim: 56 bytes
>     Profile-exif: 12957 bytes
>     Profile-icc: 3144 bytes
>     Profile-iptc: 44 bytes
>       City[1,90]: 0x00000000: 254700                                        -%
>       unknown[2,0]: 
>       Created Date[2,55]: 20000101
>       Created Time[2,60]: 000104+0000
>     Profile-xmp: 3869 bytes
75c146
<     filename: gtmetrix.jpg
---
>     filename: orig.jpg
78c149
<   Filesize: 132KB
---
>   Filesize: 152KB
82c153
<   Elapsed time: 0:01.010
---
>   Elapsed time: 0:01.009