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

Correctly convert thumbnail colour space to sRGB #1306

Merged
merged 16 commits into from
Jan 14, 2016
Merged

Conversation

theefer
Copy link
Contributor

@theefer theefer commented Sep 25, 2015

This applies the same conversions as we do with crops to thumbnails.

863e0b9694bc31edbb4baf16db730dbe66b07da1 is a good example of where it can go horribly wrong currently.

TODO

  • Review desired quality and image refinement flags with @paperboyo
  • Review performance on TEST vs current

@paperboyo
Copy link
Contributor

Record image colourModel in fileMetadata

Cool! What do we use to read that info? GM? Then, it does not give us an option to also read colour profile name (and switching from GM>IM, which does, is probably to much). But if not GM, maybe we could read and store colour profile name as well? Also, if we ever surface that info in the UI, it should say that it pertains to the original image (cause the crops will be sRGB/whatever RGB).

If we just apply current pipeline, the thumbs will grow by 2,6KB*no. of thumbs. We can alleviate that by converting them to e.g. AdobeRGB or TinysRGB (~600b).

desired quality and image refinement flags
It's always gonna ba a compromise between quality and filesize, but my (limited) tests show that we may as well aim for, maybe, halving the filesize.

@theefer
Copy link
Contributor Author

theefer commented Sep 25, 2015

What do we use to read that info? GM?

Yes, it's that identify -format '%[JPEG-Colorspace-Name]' command.

Then, it does not give us an option to also read colour profile name (and switching from GM>IM, which does, is probably to much).

Not sure what you mean by that. Do you mean the ICC Color space or Profile Description?

If we just apply current pipeline, the thumbs will grow by 2,6KB*no. of thumbs.

I'm not sure that's true, currently I don't think we touch the profile so the thumbnail contains however much ICC profile information there is in the image (potentially 400KB), AFAIK.

@theefer
Copy link
Contributor Author

theefer commented Sep 25, 2015

But yes we could optimise the embedded profile, as long as it remains visually correct.

@paperboyo
Copy link
Contributor

GM, then. As per #1281 (comment) it doesn't give us what IM would give us. A name of the profile (taken from one of the fields IM describes the profile with).

I'm not sure that's true

Yes, you're right. That's how will grow only the thumbs of images without embedded profiles.

as long as it remains visually correct

Always!

@paperboyo
Copy link
Contributor

GM, then. As per #1281 (comment) it doesn't give us what IM would give us. A name of the profile (taken from one of the fields IM describes the profile with).

Sorry, I’m an idiot. Ignore that and all above blabbing about storing colour profile identity. I’ve learned that we already do it. Doh!

@jamesgorrie
Copy link
Contributor

👍 to code 👍 to testing perf.

@theefer
Copy link
Contributor Author

theefer commented Jan 12, 2016

I want to merge #1621 into this and then this can go out, likely on Thursday. Latency looks similar to the current one, but with correct colour space conversion and reduced thumbnail file size (often half the size).

…+order

Additional optimisations to thumbnail generation and quality
theefer added a commit that referenced this pull request Jan 14, 2016
Correctly convert thumbnail colour space to sRGB
@theefer theefer merged commit 0743b6a into master Jan 14, 2016
@theefer theefer deleted the sc-correct-thumbnail branch January 14, 2016 15:46
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.

3 participants