Opened 10 years ago
Last modified 5 years ago
#28634 reopened enhancement
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: | |
Component: | Media | Keywords: | has-patch |
Focuses: | Cc: |
Description
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
Attachments (14)
Change History (72)
#2
@
10 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)?
#3
@
10 years ago
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
#4
@
10 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
@
10 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!
#6
@
10 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
Needs a solution via patch to move forward
#7
@
10 years ago
wp use imagic in core by default? not GD2 extension? Your patch good, but i didnt know about capability.
#8
@
10 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
@
10 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.
#11
@
10 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
@
10 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.
#13
@
10 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.
#14
@
10 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
@
10 years ago
Remember, this filter can only be used if imagick is installed and in usage. This is not related to GD2 here.
#16
@
10 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
@
10 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
@
10 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
@
10 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
@
10 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/
#21
@
10 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
@
10 years ago
Sorry, markoheijnen say close this discussion now.))) Ow.. discussion meant discussion?) no problem. This filter must be live and grow roots.
#24
follow-up:
↓ 25
@
10 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
@
10 years ago
Replying to markoheijnen:
What about renaming the filter to
strip_image_exif
which is defaultfalse
.
agree
#27
@
9 years 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.
9 years ago
#29
@
9 years 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
@
9 years 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.
#32
@
9 years 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 years ago
This ticket was mentioned in Slack in #core-images by markoheijnen. View the logs.
9 years ago
#35
@
9 years 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.
#37
@
9 years 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
@
9 years 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
@
9 years ago
- Milestone changed from Future Release to 4.5
- Owner set to joemcgill
- Status changed from reopened to accepted
#42
@
9 years 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 theimage_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?
This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.
9 years ago
#44
@
9 years 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()
.
#45
@
9 years ago
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.
#46
@
9 years 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:
↓ 48
@
9 years 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
@
9 years 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:
↓ 50
@
9 years 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
@
9 years 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
@
9 years 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
#52
@
9 years 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
@
9 years 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.
#55
@
9 years 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]
This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.
9 years ago
#58
@
6 years ago
- Status changed from accepted to reopened
- Type changed from defect (bug) to enhancement
- Version 3.5 deleted
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
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.