WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 8 months ago

#28634 reopened defect (bug)

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

Reported by: alexufo Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.5
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 (7)

P1020094-100x100-GD.jpg (1.4 KB) - added by DH-Shredder 12 months ago.
P10200944-100x100-Imagick.jpg (30.4 KB) - added by DH-Shredder 12 months ago.
P1020094-noexif.JPG (5.6 MB) - added by DH-Shredder 12 months ago.
P1020094-noexif-100x100-Imagick.jpg (1.0 KB) - added by DH-Shredder 12 months ago.
28634.patch (490 bytes) - added by juliobox 12 months ago.
Strips an image of all profiles and comments striping off the exif information)
28634-2.patch (551 bytes) - added by juliobox 12 months ago.
Same with a filter to keep a retrocompatibility
28634-3.patch (540 bytes) - added by juliobox 12 months ago.
Add a size parameter, filter in resize() instead of load()

Change History (32)

comment:1 @alexufo12 months 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.

comment:2 @DH-Shredder12 months 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)?

comment:4 @DH-Shredder12 months 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.

comment:5 @alexufo12 months 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 12 months ago by alexufo (previous) (diff)

comment:6 @wonderboymusic12 months ago

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

Needs a solution via patch to move forward

@juliobox12 months ago

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

@juliobox12 months ago

Same with a filter to keep a retrocompatibility

comment:7 @alexufo12 months ago

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

comment:8 @juliobox12 months 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

comment:9 @alexufo12 months 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.

comment:10 @juliobox12 months ago

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

comment:11 @markoheijnen12 months 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.

comment:12 @alexufo12 months 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 12 months ago by alexufo (previous) (diff)

comment:13 @juliobox12 months 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.

@juliobox12 months ago

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

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

comment:15 @juliobox12 months ago

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

comment:16 @DH-Shredder12 months 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.

comment:17 @alexufo12 months 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?)

comment:18 @juliobox12 months 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.

comment:19 @markoheijnen12 months 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.

comment:20 @alexufo12 months 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 12 months ago by alexufo (previous) (diff)

comment:21 @juliobox12 months 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.

comment:22 @alexufo12 months ago

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

comment:23 @johnbillion11 months ago

  • Version changed from trunk to 3.5

comment:24 follow-up: @markoheijnen10 months 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.

comment:25 in reply to: ↑ 24 @alexufo8 months ago

Replying to markoheijnen:

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

agree

Note: See TracTickets for help on using tickets.