-
Notifications
You must be signed in to change notification settings - Fork 121
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
Index ICC Profile information #748
Conversation
Hi, See for example http://en.wikipedia.org/wiki/Color_space:
Go to sleep, paperboyo! Regards |
@paperboyo ah thanks, this occurred to me but was unsure what to call the general term. |
Here is what the file metadata looks like after this change: {
"data": {
"exif": {
"Software": "Adobe Photoshop CS2 Windows",
"Date/Time": "2005:05:14 00:51:18",
"Orientation": "Top, left side (Horizontal / normal)",
"Model": "Canon EOS 300D DIGITAL",
"X Resolution": "180 dots per inch",
"YCbCr Positioning": "Center of pixel array",
"Resolution Unit": "Inch",
"Y Resolution": "180 dots per inch",
"Make": "Canon"
},
"iptc": {},
"getty": {},
"icc": {
"Media White Point": "(0.70840454, 0.73594666, 0.5710449)",
"AToB 2": "mft2(0x6D667432): 41478 bytes",
"Profile Size": "557168",
"Profile Date/Time": "Sat Aug 26 06:41:53 BST 2000",
"XYZ values": "0.9642029 1.0 0.8249054",
"Gamut": "mft1(0x6D667431): 37009 bytes",
"Primary Platform": "Apple Computer, Inc.",
"Signature": "acsp",
"AToB 1": "mft2(0x6D667432): 41478 bytes",
"Profile Connection Space": "Lab",
"AToB 0": "mft2(0x6D667432): 41478 bytes",
"Color space": "CMYK",
"Device manufacturer": "ADBE",
"Class": "Output Device",
"Copyright": "Copyright 2000 Adobe Systems, Inc.",
"BToA 2": "mft1(0x6D667431): 145588 bytes",
"BToA 1": "mft1(0x6D667431): 145588 bytes",
"BToA 0": "mft1(0x6D667431): 145588 bytes",
"Version": "2.1.0",
"Rendering Intent": "Media-Relative Colorimetric",
"Profile Description": "U.S. Web Coated (SWOP) v2",
"CMM Type": "ADBE",
"Tag Count": "10"
},
"xmp": {
"F-Number": "F5.6",
"Aperture Value": "F5.6",
"Model": "Canon EOS 300D DIGITAL",
"Date/Time Original": "Sat Mar 20 12:54:55 GMT 2004",
"Shutter Speed Value": "1/640 sec",
"Date/Time Digitized": "Sat Mar 20 12:54:55 GMT 2004",
"Focal Length": "55.0 mm",
"Exposure Time": "1/640 sec",
"Make": "Canon"
},
"colorModel": {
"id": "CMYK"
},
"exifSub": {
"Date/Time Original": "2004:03:20 14:54:55",
"Metering Mode": "Multi-segment",
"Scene Capture Type": "Standard",
"Exif Image Width": "500 pixels",
"Exif Version": "2.21",
"Exposure Mode": "Auto exposure",
"Sensing Method": "One-chip color area sensor",
"Focal Plane X Resolution": "446/1536000 inches",
"ISO Speed Ratings": "400",
"Flash": "Flash did not fire, auto",
"Shutter Speed Value": "1/640 sec",
"Date/Time Digitized": "2004:03:20 14:54:55",
"Focal Plane Y Resolution": "119/409600 inches",
"Focal Plane Resolution Unit": "Inches",
"Aperture Value": "f/5.6",
"F-Number": "f/5.6",
"Color Space": "Undefined",
"Components Configuration": "YCbCr",
"Exposure Time": "1/640 sec",
"Compressed Bits Per Pixel": "3 bits/pixel",
"FlashPix Version": "1.00",
"Max Aperture Value": "f/5.6",
"Custom Rendered": "Normal process",
"Exif Image Height": "333 pixels",
"Focal Length": "55.0 mm",
"File Source": "Digital Still Camera (DSC)",
"White Balance Mode": "Auto white balance",
"Exposure Bias Value": "0 EV"
}
},
"links": [{
"rel": "image",
"href": "https://api.media.local.dev-gutools.co.uk/images/16a0ccf4aa95fb0e0a853a1296b855164c875bc0"
}]
} |
// This is distinct from reading the ICC Profile Colorspace which may be missing | ||
// while this property is still inferrable from other sources | ||
|
||
def getColorModel(fileLocation: String): Option[ColorModel] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly like this function, but I couldn't find a way to turn a Try
into an Option
while executing a function on the fail case. recover
almost worked but ran into type problems returning an Option[Any]
that I couldn't resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what i'd like to do:
Try { ColorModel((new Info(fileLocation)).getProperty("Colorspace")) } recover {
case e => Logger.error("Oh No ERRORZ")
}.asOption
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can convert Try[T]
into Option[T]
using the toOption
method. I don't know of a way to map the Failure
case of a Try
besides recover
or recoverWith
(you could use a match
but it'd be exhaustive and you then have to map the success case explicitly which is lame). So you can do something like:
> import scala.util.{Try,Failure}
> Try( throw new Error("aa") ).recoverWith { case e: Throwable => println(e); Failure(e) }.toOption
java.lang.Error: aa
res0: Option[Nothing] = None
(You could also use recover
and rethrow e
.)
💎 |
|
||
def getColorModel(fileLocation: String): Option[ColorModel] = { | ||
Try { | ||
ColorModel((new Info(fileLocation)).getProperty("Colorspace")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we trust the Colorspace
property to always be a valid value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in a canonical form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theefer it is difficult to tell what ImageMagick is doing when asked for this property, as it's not well documented and trying to decipher the source code is a bit confusing though i'm pretty sure that the Colorspace
property is not a user specified string but instead is set from this array:
https://github.com/trevor/ImageMagick/blob/master/trunk/MagickCore/option.c#L888
I'm looking for where it is calculated and set and I don't think it's coming from a property:
https://github.com/trevor/ImageMagick/blob/master/trunk/MagickCore/identify.c#L508
It could be coming from here:
That indicates it's relying on the LittleCMS library which from having had a poke around in that codebase is even worst then ImageMagick and i've had enough of trying to decipher C codebases for now.
Note: I've also looked at the GraphicsMagick codebase (which is not conveniently mirrored anywhere in github), and it copies the C code from the ImageMagick codebase, so I would expect its behaviour to be identical:
http://sourceforge.net/p/graphicsmagick/code/ci/default/tree/
From testing this function returns a valid color model even when the image has been stripped of metadata by @paperboyo, and having run through a bunch of test images the naming of color models is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive investigation, 👍 (see also other comment about test coverage)
A few comments, but generally looks good 👍 |
Also: could we add some tests for this new data, I guess update the existing ones to check the expected ICC/colourModel and making sure we cover the different colourModel cases? |
Thanks for the tests 👍 |
@theefer the tests fail in CI. I'm guessing this is because there is no graphicsmagick on the build agents. |
Ah, yeah that would likely be why. You should be able to ssh onto the agents, but not install packages on it. Need to coordinate with websys for that (we really need to have container isolation for this ;_; cc @sihil). |
@theefer i've done a jira (https://jira.gutools.co.uk/browse/WSA-2132). Do you reckon we should turn off tests for the image-loader in CI and merge this for now or wait? |
If you're happy with the result just ship this with tests turned off and we can turn them on asap |
Good to keep an eye on the results in PROD once out too |
@theefer this is an annoying one, we can't turn off the tests without making some code changes as the @andybotting installed I'd rather not make code changes to switch the tests off, but i'm not sure how long it'll take Andy to get the build agents sorted. |
Hi, Could I ask which version of Regards *for now, we do not support any 16bpc image fromats, where, I assume, support for 16 is mandatory. But even now we are doing image manipulations and gamut mapping which produce better results in 16bpc. Even more so, if we decide that resizing is better done in linear colour space, thus introducing two more colour space conversions... |
This doesn't work in our prod environments as imagemagick != graphicsmagick ... still salvageable I think. |
Can confirm this works in the TEST environment. @theefer ok to merg? |
@paperboyo had to drop colorspace detection using graphicsmagick as it doesn't seem to work the same way as imagemagick. So we're back to finding a tool that does the heuristic thing, or figuring it out for ourselves 😭 |
👍 |
Index Colorspace and ICC Profile information
Hmmm, strange, I was using But in future we will have to do much more identification. E.g. to convert PNGs without alpha/transparency to JPGs, so that we do not serve 3,5MB images on live site... |
@paperboyo I've created an issue: #765 I have a desire to fix this now after messing about with it for so long ... |
😆 Thanks, @kenoir! As I said, output is already worlds apart from how it was. I've actually already uploaded production AdobeRGB images without any worry. |
In order to perform "correct" exports and provide visibility into the kinds of images being loaded into the grid we need to index colorspace and profile from an image.