{"action":"created","comment":{"url":"https://api.github.com/repos/guardian/grid/pulls/comments/40416950","id":40416950,"diff_hunk":"@@ -0,0 +1,100 @@\n+package com.gu.mediaservice.lib.imaging\n+\n+import java.io._\n+\n+import org.im4java.core.IMOperation\n+\n+import com.gu.mediaservice.lib.Files._\n+import com.gu.mediaservice.lib.imaging.im4jwrapper.{ExifTool, ImageMagick}\n+import com.gu.mediaservice.model.{Asset, Bounds, Dimensions, ImageMetadata}\n+import play.api.libs.concurrent.Execution.Implicits._\n+\n+import scala.concurrent.Future\n+\n+\n+case class ExportResult(id: String, masterCrop: Asset, othersizings: List[Asset])\n+\n+object ImageOperations {\n+  import ExifTool._\n+  import ImageMagick._\n+\n+  private def profilePath(fileName: String): String = s\"${play.api.Play.current.path}/$fileName\"\n+\n+  private def profileLocation(colourModel: String): String = colourModel match {\n+    case \"RGB\"       => profilePath(\"srgb.icc\")\n+    case \"CMYK\"      => profilePath(\"cmyk.icc\")\n+    case \"GRAYSCALE\" => profilePath(\"grayscale.icc\")\n+    case model       => throw new Exception(s\"Profile for invalid colour model requested: $model\")\n+  }\n+\n+  private def tagFilter(metadata: ImageMetadata) = {\n+    Map[String, Option[String]](\n+      \"Copyright\" -> metadata.copyright,\n+      \"CopyrightNotice\" -> metadata.copyrightNotice,\n+      \"Credit\" -> metadata.credit,\n+      \"OriginalTransmissionReference\" -> metadata.suppliersReference\n+    ).collect { case (key, Some(value)) => (key, value) }\n+  }\n+\n+  private def applyOutputProfile(base: IMOperation) = profile(base)(profileLocation(\"RGB\"))\n+\n+  def identifyColourModel(sourceFile: File, mimeType: String): Future[Option[String]] = {\n+    val source    = addImage(sourceFile)\n+    // TODO: use mimeType to lookup other properties once we support other formats\n+    val formatter = format(source)(\"%[JPEG-Colorspace-Name]\")\n+    for {\n+      output      <- runIdentifyCmd(formatter)\n+      colourModel  = output.headOption\n+    } yield colourModel\n+  }\n+\n+  // Optionally apply transforms to the base operation if the colour space\n+  // in the ICC profile doesn't match the colour model of the image data\n+  private def correctColour(base: IMOperation)(iccColourSpace: Option[String], colourModel: Option[String]) = {\n+    (iccColourSpace, colourModel) match {\n+      // If matching, all is well, just pass through\n+      case (icc, model) if icc == model => base\n+      // If no colour model detected, we can't do anything anyway so just hope all is well\n+      case (_,   None) => base","path":"common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala","position":58,"original_position":58,"commit_id":"a83bf2b7e2ff81697d4993a1d8a340bf3ab9eb99","original_commit_id":"a83bf2b7e2ff81697d4993a1d8a340bf3ab9eb99","user":{"login":"paperboyo","id":6032869,"avatar_url":"https://avatars.githubusercontent.com/u/6032869?v=3","gravatar_id":"","url":"https://api.github.com/users/paperboyo","html_url":"https://github.com/paperboyo","followers_url":"https://api.github.com/users/paperboyo/followers","following_url":"https://api.github.com/users/paperboyo/following{/other_user}","gists_url":"https://api.github.com/users/paperboyo/gists{/gist_id}","starred_url":"https://api.github.com/users/paperboyo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/paperboyo/subscriptions","organizations_url":"https://api.github.com/users/paperboyo/orgs","repos_url":"https://api.github.com/users/paperboyo/repos","events_url":"https://api.github.com/users/paperboyo/events{/privacy}","received_events_url":"https://api.github.com/users/paperboyo/received_events","type":"User","site_admin":false},"body":"> is it worth logging out these cases?\r\n\r\nI do think so. It's generally interesting to me what is the composition of the whole db in terms of colour models, colour profiles. Not that (all) the metadata isn't interesting ;) ","created_at":"2015-09-25T10:23:56Z","updated_at":"2015-09-25T10:23:56Z","html_url":"https://github.com/guardian/grid/pull/1281#discussion_r40416950","pull_request_url":"https://api.github.com/repos/guardian/grid/pulls/1281","_links":{"self":{"href":"https://api.github.com/repos/guardian/grid/pulls/comments/40416950"},"html":{"href":"https://github.com/guardian/grid/pull/1281#discussion_r40416950"},"pull_request":{"href":"https://api.github.com/repos/guardian/grid/pulls/1281"}}},"pull_request":{"url":"https://api.github.com/repos/guardian/grid/pulls/1281","id":45538062,"html_url":"https://github.com/guardian/grid/pull/1281","diff_url":"https://github.com/guardian/grid/pull/1281.diff","patch_url":"https://github.com/guardian/grid/pull/1281.patch","issue_url":"https://api.github.com/repos/guardian/grid/issues/1281","number":1281,"state":"open","locked":false,"title":"Correct colour space before cropping","user":{"login":"theefer","id":36964,"avatar_url":"https://avatars.githubusercontent.com/u/36964?v=3","gravatar_id":"","url":"https://api.github.com/users/theefer","html_url":"https://github.com/theefer","followers_url":"https://api.github.com/users/theefer/followers","following_url":"https://api.github.com/users/theefer/following{/other_user}","gists_url":"https://api.github.com/users/theefer/gists{/gist_id}","starred_url":"https://api.github.com/users/theefer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/theefer/subscriptions","organizations_url":"https://api.github.com/users/theefer/orgs","repos_url":"https://api.github.com/users/theefer/repos","events_url":"https://api.github.com/users/theefer/events{/privacy}","received_events_url":"https://api.github.com/users/theefer/received_events","type":"User","site_admin":false},"body":"Currently, the cropper fails in at least two cases:\r\n* If cropping an image where the colour space in the ICC profile doesn't match the actual colour model of the image data. e.g. an image with RGB image data with a CMYK profile (note that both such an image is technically incorrect). This is because when trying to convert from CMYK to sRGB profile, GraphicsMagick detects that the image data isn't actually CMYK and it fails with an error about mismatching colour models.\r\n* If cropping an image where the image data is not in RGB (e.g. CMYK or grayscale) and there is no embedded ICC profile. In that case, adding the profile doesn't trigger any conversion; the sRGB profile is simply embedded, but it doesn't match the image data, resulting in an image that doesn't look correct in browsers.\r\n\r\nThe solution is to always read out both the embedded ICC profile colour space and the image data colour model and compare them before any conversion.\r\n\r\nIf they match, great, we can just apply our normal conversion to sRGB profile (from whatever profile is correctly embedded).\r\n\r\nIf they don't match, either because there is no embedded ICC profile, or because the embedded ICC profile is not matching the actual colour model, then we strip any embedded profile and replace it with a generic ICC profile that matches the actual colour model of the image data. Note that there is no guarantee that the profile will exactly match the image data, but it's a best effort estimate with standard ICC profiles. We can then resume the normal flow of converting the image to the desired output profile (sRGB), etc.\r\n\r\nInitial tests with various images with mismatching or missing ICC profiles gave correct results.\r\n\r\nThis will also be used to correct the colour space of thumbnails we generate, in a future PR.\r\n\r\nThanks to @paperboyo for his big help with this!\r\n\r\n## TODO\r\n\r\n- [x] More manual testing\r\n- [x] Think of whether we can write automated tests for this somehow","created_at":"2015-09-21T11:33:05Z","updated_at":"2015-09-25T10:23:56Z","closed_at":null,"merged_at":null,"merge_commit_sha":"4a28a5f9164f008091e622946221170e2f5dc75f","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/guardian/grid/pulls/1281/commits","review_comments_url":"https://api.github.com/repos/guardian/grid/pulls/1281/comments","review_comment_url":"https://api.github.com/repos/guardian/grid/pulls/comments{/number}","comments_url":"https://api.github.com/repos/guardian/grid/issues/1281/comments","statuses_url":"https://api.github.com/repos/guardian/grid/statuses/a83bf2b7e2ff81697d4993a1d8a340bf3ab9eb99","head":{"label":"guardian:sc-correct-colour-space","ref":"sc-correct-colour-space","sha":"a83bf2b7e2ff81697d4993a1d8a340bf3ab9eb99","user":{"login":"guardian","id":164318,"avatar_url":"https://avatars.githubusercontent.com/u/164318?v=3","gravatar_id":"","url":"https://api.github.com/users/guardian","html_url":"https://github.com/guardian","followers_url":"https://api.github.com/users/guardian/followers","following_url":"https://api.github.com/users/guardian/following{/other_user}","gists_url":"https://api.github.com/users/guardian/gists{/gist_id}","starred_url":"https://api.github.com/users/guardian/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/guardian/subscriptions","organizations_url":"https://api.github.com/users/guardian/orgs","repos_url":"https://api.github.com/users/guardian/repos","events_url":"https://api.github.com/users/guardian/events{/privacy}","received_events_url":"https://api.github.com/users/guardian/received_events","type":"Organization","site_admin":false},"repo":{"id":12298112,"name":"grid","full_name":"guardian/grid","owner":{"login":"guardian","id":164318,"avatar_url":"https://avatars.githubusercontent.com/u/164318?v=3","gravatar_id":"","url":"https://api.github.com/users/guardian","html_url":"https://github.com/guardian","followers_url":"https://api.github.com/users/guardian/followers","following_url":"https://api.github.com/users/guardian/following{/other_user}","gists_url":"https://api.github.com/users/guardian/gists{/gist_id}","starred_url":"https://api.github.com/users/guardian/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/guardian/subscriptions","organizations_url":"https://api.github.com/users/guardian/orgs","repos_url":"https://api.github.com/users/guardian/repos","events_url":"https://api.github.com/users/guardian/events{/privacy}","received_events_url":"https://api.github.com/users/guardian/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/guardian/grid","description":"The Guardian’s new image management system","fork":false,"url":"https://api.github.com/repos/guardian/grid","forks_url":"https://api.github.com/repos/guardian/grid/forks","keys_url":"https://api.github.com/repos/guardian/grid/keys{/key_id}","collaborators_url":"https://api.github.com/repos/guardian/grid/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/guardian/grid/teams","hooks_url":"https://api.github.com/repos/guardian/grid/hooks","issue_events_url":"https://api.github.com/repos/guardian/grid/issues/events{/number}","events_url":"https://api.github.com/repos/guardian/grid/events","assignees_url":"https://api.github.com/repos/guardian/grid/assignees{/user}","branches_url":"https://api.github.com/repos/guardian/grid/branches{/branch}","tags_url":"https://api.github.com/repos/guardian/grid/tags","blobs_url":"https://api.github.com/repos/guardian/grid/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/guardian/grid/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/guardian/grid/git/refs{/sha}","trees_url":"https://api.github.com/repos/guardian/grid/git/trees{/sha}","statuses_url":"https://api.github.com/repos/guardian/grid/statuses/{sha}","languages_url":"https://api.github.com/repos/guardian/grid/languages","stargazers_url":"https://api.github.com/repos/guardian/grid/stargazers","contributors_url":"https://api.github.com/repos/guardian/grid/contributors","subscribers_url":"https://api.github.com/repos/guardian/grid/subscribers","subscription_url":"https://api.github.com/repos/guardian/grid/subscription","commits_url":"https://api.github.com/repos/guardian/grid/commits{/sha}","git_commits_url":"https://api.github.com/repos/guardian/grid/git/commits{/sha}","comments_url":"https://api.github.com/repos/guardian/grid/comments{/number}","issue_comment_url":"https://api.github.com/repos/guardian/grid/issues/comments{/number}","contents_url":"https://api.github.com/repos/guardian/grid/contents/{+path}","compare_url":"https://api.github.com/repos/guardian/grid/compare/{base}...{head}","merges_url":"https://api.github.com/repos/guardian/grid/merges","archive_url":"https://api.github.com/repos/guardian/grid/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/guardian/grid/downloads","issues_url":"https://api.github.com/repos/guardian/grid/issues{/number}","pulls_url":"https://api.github.com/repos/guardian/grid/pulls{/number}","milestones_url":"https://api.github.com/repos/guardian/grid/milestones{/number}","notifications_url":"https://api.github.com/repos/guardian/grid/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/guardian/grid/labels{/name}","releases_url":"https://api.github.com/repos/guardian/grid/releases{/id}","created_at":"2013-08-22T13:49:31Z","updated_at":"2015-09-24T18:29:46Z","pushed_at":"2015-09-25T09:46:35Z","git_url":"git://github.com/guardian/grid.git","ssh_url":"git@github.com:guardian/grid.git","clone_url":"https://github.com/guardian/grid.git","svn_url":"https://github.com/guardian/grid","homepage":"","size":23632,"stargazers_count":925,"watchers_count":925,"language":"Scala","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":44,"mirror_url":null,"open_issues_count":57,"forks":44,"open_issues":57,"watchers":925,"default_branch":"master"}},"base":{"label":"guardian:master","ref":"master","sha":"5a06a6b68c72cb35a7196f26e4c1f7c0d96a8cbd","user":{"login":"guardian","id":164318,"avatar_url":"https://avatars.githubusercontent.com/u/164318?v=3","gravatar_id":"","url":"https://api.github.com/users/guardian","html_url":"https://github.com/guardian","followers_url":"https://api.github.com/users/guardian/followers","following_url":"https://api.github.com/users/guardian/following{/other_user}","gists_url":"https://api.github.com/users/guardian/gists{/gist_id}","starred_url":"https://api.github.com/users/guardian/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/guardian/subscriptions","organizations_url":"https://api.github.com/users/guardian/orgs","repos_url":"https://api.github.com/users/guardian/repos","events_url":"https://api.github.com/users/guardian/events{/privacy}","received_events_url":"https://api.github.com/users/guardian/received_events","type":"Organization","site_admin":false},"repo":{"id":12298112,"name":"grid","full_name":"guardian/grid","owner":{"login":"guardian","id":164318,"avatar_url":"https://avatars.githubusercontent.com/u/164318?v=3","gravatar_id":"","url":"https://api.github.com/users/guardian","html_url":"https://github.com/guardian","followers_url":"https://api.github.com/users/guardian/followers","following_url":"https://api.github.com/users/guardian/following{/other_user}","gists_url":"https://api.github.com/users/guardian/gists{/gist_id}","starred_url":"https://api.github.com/users/guardian/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/guardian/subscriptions","organizations_url":"https://api.github.com/users/guardian/orgs","repos_url":"https://api.github.com/users/guardian/repos","events_url":"https://api.github.com/users/guardian/events{/privacy}","received_events_url":"https://api.github.com/users/guardian/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/guardian/grid","description":"The Guardian’s new image management system","fork":false,"url":"https://api.github.com/repos/guardian/grid","forks_url":"https://api.github.com/repos/guardian/grid/forks","keys_url":"https://api.github.com/repos/guardian/grid/keys{/key_id}","collaborators_url":"https://api.github.com/repos/guardian/grid/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/guardian/grid/teams","hooks_url":"https://api.github.com/repos/guardian/grid/hooks","issue_events_url":"https://api.github.com/repos/guardian/grid/issues/events{/number}","events_url":"https://api.github.com/repos/guardian/grid/events","assignees_url":"https://api.github.com/repos/guardian/grid/assignees{/user}","branches_url":"https://api.github.com/repos/guardian/grid/branches{/branch}","tags_url":"https://api.github.com/repos/guardian/grid/tags","blobs_url":"https://api.github.com/repos/guardian/grid/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/guardian/grid/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/guardian/grid/git/refs{/sha}","trees_url":"https://api.github.com/repos/guardian/grid/git/trees{/sha}","statuses_url":"https://api.github.com/repos/guardian/grid/statuses/{sha}","languages_url":"https://api.github.com/repos/guardian/grid/languages","stargazers_url":"https://api.github.com/repos/guardian/grid/stargazers","contributors_url":"https://api.github.com/repos/guardian/grid/contributors","subscribers_url":"https://api.github.com/repos/guardian/grid/subscribers","subscription_url":"https://api.github.com/repos/guardian/grid/subscription","commits_url":"https://api.github.com/repos/guardian/grid/commits{/sha}","git_commits_url":"https://api.github.com/repos/guardian/grid/git/commits{/sha}","comments_url":"https://api.github.com/repos/guardian/grid/comments{/number}","issue_comment_url":"https://api.github.com/repos/guardian/grid/issues/comments{/number}","contents_url":"https://api.github.com/repos/guardian/grid/contents/{+path}","compare_url":"https://api.github.com/repos/guardian/grid/compare/{base}...{head}","merges_url":"https://api.github.com/repos/guardian/grid/merges","archive_url":"https://api.github.com/repos/guardian/grid/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/guardian/grid/downloads","issues_url":"https://api.github.com/repos/guardian/grid/issues{/number}","pulls_url":"https://api.github.com/repos/guardian/grid/pulls{/number}","milestones_url":"https://api.github.com/repos/guardian/grid/milestones{/number}","notifications_url":"https://api.github.com/repos/guardian/grid/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/guardian/grid/labels{/name}","releases_url":"https://api.github.com/repos/guardian/grid/releases{/id}","created_at":"2013-08-22T13:49:31Z","updated_at":"2015-09-24T18:29:46Z","pushed_at":"2015-09-25T09:46:35Z","git_url":"git://github.com/guardian/grid.git","ssh_url":"git@github.com:guardian/grid.git","clone_url":"https://github.com/guardian/grid.git","svn_url":"https://github.com/guardian/grid","homepage":"","size":23632,"stargazers_count":925,"watchers_count":925,"language":"Scala","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":44,"mirror_url":null,"open_issues_count":57,"forks":44,"open_issues":57,"watchers":925,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/guardian/grid/pulls/1281"},"html":{"href":"https://github.com/guardian/grid/pull/1281"},"issue":{"href":"https://api.github.com/repos/guardian/grid/issues/1281"},"comments":{"href":"https://api.github.com/repos/guardian/grid/issues/1281/comments"},"review_comments":{"href":"https://api.github.com/repos/guardian/grid/pulls/1281/comments"},"review_comment":{"href":"https://api.github.com/repos/guardian/grid/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/guardian/grid/pulls/1281/commits"},"statuses":{"href":"https://api.github.com/repos/guardian/grid/statuses/a83bf2b7e2ff81697d4993a1d8a340bf3ab9eb99"}}}}