Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert images to RGB and strip metadata #37

Merged
merged 2 commits into from
May 20, 2017

Conversation

ausi
Copy link
Member

@ausi ausi commented May 16, 2017

Copy link

@discordier discordier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, however, the upstream PR seems to break stuff.

@aschempp
Copy link
Member

does that work with all implementations? Or will we get an exception e.g. when using GD?

@ausi
Copy link
Member Author

ausi commented May 17, 2017

Yes, it works with all Imagine implementations. In GD strip() and usePalette() just do nothing, because GD strips always and converts always to RGB.

@leofeyer
Copy link
Member

It seems that the GDlib does not handle palette conversions very well:

http://stackoverflow.com/questions/12815305/bad-cmyk-to-rgb-conversion-with-gd

Therefore, do we really need this step?

@ausi
Copy link
Member Author

ausi commented May 17, 2017

With GD, the behavior before and after this pull request is exactly the same, see #37 (comment)

But with Gmagick and Imagick, the image is converted to RGB and profiles and metadata get stripped from the image.

We can make the conversion to RGB configurable, I’m not sure if strip() should be configurable too.

@leofeyer
Copy link
Member

With GD, the behavior before and after this pull request is exactly the same,

Ok, sorry for not reading thoroughly. Then we do not need this to be configurable I think.

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem I see here is that resizer settings should be considered in the cache key.

@ausi
Copy link
Member Author

ausi commented May 17, 2017

The only problem I see here is that resizer settings should be considered in the cache key.

If we don’t make them configurable, they are not “settings” I think and are not needed in the cache key then because they can’t change.

@Toflar
Copy link
Member

Toflar commented May 18, 2017

Fine with me then :) I think I'll only test it on a real life project once the php-imagine/Imagine#564 discussion has found an end, ok?

@ausi
Copy link
Member Author

ausi commented May 18, 2017

I think you can test on real life project already. It looks like the issue from php-imagine/Imagine#564 was related to my installation of ImageMagick because the delegate lcms was missing. You can test that on your system by calling convert -list configure | grep DELEGATES and if lcms shows up, everything should be fine.

@ausi ausi changed the base branch from master to hotfix/0.3.4 May 20, 2017 13:19
@ausi ausi force-pushed the fix/strip-profile-metadata branch from be649eb to ba38061 Compare May 20, 2017 13:22
@ausi ausi merged commit a60e9bc into hotfix/0.3.4 May 20, 2017
@ausi ausi deleted the fix/strip-profile-metadata branch May 20, 2017 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants