Correct colour space before cropping

cropper/app/lib/Files.scala → common-lib/src/main/scala/com/gu/mediaservice/lib/Files.scala

@@ -1,19 +1,20 @@
-package lib
+package com.gu.mediaservice.lib
 
-import java.io.{FileOutputStream, File}
-import java.nio.channels.Channels
+import java.io.{File, FileOutputStream}
 import java.net.URL
-import scala.concurrent.{Future, ExecutionContext}
+import java.nio.channels.Channels
 import java.util.concurrent.Executors
 
+import scala.concurrent.{ExecutionContext, Future}
+
 
 object Files {
 
   private implicit val ctx = ExecutionContext.fromExecutor(Executors.newCachedThreadPool)
 
-  def createTempFile(prefix: String, suffix: String): Future[File] =
+  def createTempFile(prefix: String, suffix: String, tempDir: File): Future[File] =
     Future {
-      File.createTempFile(prefix, suffix, Config.tempDir)
+      File.createTempFile(prefix, suffix, tempDir)
     }
 
   def transferFromURL(from: URL, to: File): Future[Unit] =
@@ -23,9 +24,9 @@ object Files {
       output.getChannel.transferFrom(channel, 0, java.lang.Long.MAX_VALUE)
     }
 
-  def tempFileFromURL(from: URL, prefix: String, suffix: String): Future[File] =
+  def tempFileFromURL(from: URL, prefix: String, suffix: String, tempDir: File): Future[File] =
     for {
-      tempFile <- createTempFile(prefix, suffix)
+      tempFile <- createTempFile(prefix, suffix, tempDir: File)
       _ <- transferFromURL(from, tempFile)
     }
     yield tempFile

common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala

@@ -0,0 +1,100 @@
+package com.gu.mediaservice.lib.imaging
+
+import java.io._
+
+import org.im4java.core.IMOperation
+
+import com.gu.mediaservice.lib.Files._
+import com.gu.mediaservice.lib.imaging.im4jwrapper.{ExifTool, ImageMagick}
+import com.gu.mediaservice.model.{Asset, Bounds, Dimensions, ImageMetadata}
+import play.api.libs.concurrent.Execution.Implicits._
+
+import scala.concurrent.Future
+
+
+case class ExportResult(id: String, masterCrop: Asset, othersizings: List[Asset])
+
+object ImageOperations {
+  import ExifTool._
+  import ImageMagick._
+
+  private def profilePath(fileName: String): String = s"${play.api.Play.current.path}/$fileName"
+
+  private def profileLocation(colourModel: String): String = colourModel match {
+    case "RGB"       => profilePath("srgb.icc")
+    case "CMYK"      => profilePath("cmyk.icc")
+    case "GRAYSCALE" => profilePath("grayscale.icc")
+    case model       => throw new Exception(s"Profile for invalid colour model requested: $model")
+  }
+
+  private def tagFilter(metadata: ImageMetadata) = {
+    Map[String, Option[String]](
+      "Copyright" -> metadata.copyright,
+      "CopyrightNotice" -> metadata.copyrightNotice,
+      "Credit" -> metadata.credit,
+      "OriginalTransmissionReference" -> metadata.suppliersReference
+    ).collect { case (key, Some(value)) => (key, value) }
+  }
+
+  private def applyOutputProfile(base: IMOperation) = profile(base)(profileLocation("RGB"))
+
+  def identifyColourModel(sourceFile: File, mimeType: String): Future[Option[String]] = {
+    val source    = addImage(sourceFile)
+    // TODO: use mimeType to lookup other properties once we support other formats
+    val formatter = format(source)("%[JPEG-Colorspace-Name]")
+    for {
+      output      <- runIdentifyCmd(formatter)
+      colourModel  = output.headOption
+    } yield colourModel
+  }
+
+  // Optionally apply transforms to the base operation if the colour space
+  // in the ICC profile doesn't match the colour model of the image data
+  private def correctColour(base: IMOperation)(iccColourSpace: Option[String], colourModel: Option[String]) = {
+    (iccColourSpace, colourModel) match {
+      // If matching, all is well, just pass through
+      case (icc, model) if icc == model => base
+      // If no colour model detected, we can't do anything anyway so just hope all is well
+      case (_,   None) => base
+      // If mismatching, strip any (incorrect) ICC profile and inject a profile matching the model
+      // Note: ICC identified as "icm" here
+      case (_,   Some(model)) => profile(stripProfile(base)("icm"))(profileLocation(model))
+    }
+  }
+
+  def cropImage(sourceFile: File, bounds: Bounds, qual: Double = 100d, tempDir: File,
+                iccColourSpace: Option[String], colourModel: Option[String]): Future[File] = {
+    for {
+      outputFile <- createTempFile(s"crop-", ".jpg", tempDir)
+      cropSource  = addImage(sourceFile)
+      qualified   = quality(cropSource)(qual)
+      corrected   = correctColour(qualified)(iccColourSpace, colourModel)
+      converted   = applyOutputProfile(corrected)
+      stripped    = stripMeta(converted)
+      profiled    = applyOutputProfile(stripped)
+      cropped     = crop(profiled)(bounds)
+      addOutput   = addDestImage(cropped)(outputFile)
+      _          <- runConvertCmd(addOutput)
+    }
+    yield outputFile
+  }
+
+  // Updates metadata on existing file
+  def appendMetadata(sourceFile: File, metadata: ImageMetadata): Future[File] = {
+    runExiftoolCmd(
+      setTags(tagSource(sourceFile))(tagFilter(metadata))
+      ).map(_ => sourceFile)
+  }
+
+  def resizeImage(sourceFile: File, dimensions: Dimensions, qual: Double = 100d, tempDir: File): Future[File] = {
+    for {
+      outputFile  <- createTempFile(s"resize-", ".jpg", tempDir)
+      resizeSource = addImage(sourceFile)
+      qualified    = quality(resizeSource)(qual)
+      resized      = scale(qualified)(dimensions)
+      addOutput    = addDestImage(resized)(outputFile)
+      _           <- runConvertCmd(addOutput)
+    }
+    yield outputFile
+  }
+}

common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/Config.scala

@@ -0,0 +1,5 @@
+package com.gu.mediaservice.lib.imaging.im4jwrapper
+
+object Config {
+  val imagingThreadPoolSize = 4
+}

cropper/app/lib/imaging/im4jwrapper/ExifTool.scala → common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/ExifTool.scala

@@ -1,7 +1,6 @@
-package lib.imaging.im4jwrapper
+package com.gu.mediaservice.lib.imaging.im4jwrapper
 
 import java.util.concurrent.Executors
-import lib.Config
 
 import java.io.File
 import scala.concurrent.{Future, ExecutionContext}

cropper/app/lib/imaging/im4jwrapper/ImageMagick.scala → common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/ImageMagick.scala

@@ -1,11 +1,14 @@
-package lib.imaging.im4jwrapper
+package com.gu.mediaservice.lib.imaging.im4jwrapper
 
 import java.util.concurrent.Executors
-import lib.Config
 
 import java.io.File
+import org.im4java.process.ArrayListOutputConsumer
+
+import scala.collection.JavaConverters._
+
 import scala.concurrent.{Future, ExecutionContext}
-import org.im4java.core.{IMOperation, ConvertCmd}
+import org.im4java.core.{IdentifyCmd, IMOperation, ConvertCmd}
 import scalaz.syntax.id._
 
 import com.gu.mediaservice.model.{Dimensions, Bounds}
@@ -18,9 +21,20 @@ object ImageMagick {
   def addImage(source: File) = (new IMOperation()) <| { op => { op.addImage(source.getAbsolutePath) }}
   def quality(op: IMOperation)(qual: Double) = op <| (_.quality(qual))
   def stripMeta(op: IMOperation) = op <| (_.strip())
+  def stripProfile(op: IMOperation)(profile: String) = op <| (_.p_profile(profile))
   def addDestImage(op: IMOperation)(dest: File) = op <| (_.addImage(dest.getAbsolutePath))
   def crop(op: IMOperation)(b: Bounds): IMOperation = op <| (_.crop(b.width, b.height, b.x, b.y))
   def profile(op: IMOperation)(profileFileLocation: String): IMOperation = op <| (_.profile(profileFileLocation))
+  def thumbnail(op: IMOperation)(width: Int): IMOperation = op <| (_.thumbnail(width))
   def scale(op: IMOperation)(dimensions: Dimensions): IMOperation = op <| (_.scale(dimensions.width, dimensions.height))
+  def format(op: IMOperation)(definition: String): IMOperation = op <| (_.format(definition))
+
   def runConvertCmd(op: IMOperation): Future[Unit] = Future((new ConvertCmd).run(op))
+  def runIdentifyCmd(op: IMOperation): Future[List[String]] = Future {
+    val cmd = new IdentifyCmd()
+    val output = new ArrayListOutputConsumer()
+    cmd.setOutputConsumer(output)
+    cmd.run(op)
+    output.getOutput.asScala.toList
+  }
 }

common-lib/src/main/scala/com/gu/mediaservice/lib/metadata/FileMetadataHelper.scala

@@ -0,0 +1,14 @@
+package com.gu.mediaservice.lib.metadata
+
+import com.gu.mediaservice.model.FileMetadata
+
+object FileMetadataHelper {
+
+  def normalisedIccColourSpace(fileMetadata: FileMetadata): Option[String] = {
+    fileMetadata.icc.get("Color space") map {
+      case "GRAY" => "GRAYSCALE"
+      case other  => other
+    }
+  }
+
+}

common-lib/src/main/scala/com/gu/mediaservice/model/SourceImage.scala

@@ -4,13 +4,14 @@ import play.api.libs.json._
 import play.api.libs.functional.syntax._
 
 
-case class SourceImage(id: String, source: Asset, valid: Boolean, metadata: ImageMetadata)
+case class SourceImage(id: String, source: Asset, valid: Boolean, metadata: ImageMetadata, fileMetadata: FileMetadata)
 object SourceImage {
   implicit val sourceImageReads: Reads[SourceImage] =
     ((__ \ "data" \ "id").read[String] ~
       (__ \ "data" \ "source").read[Asset] ~
       (__ \ "data" \ "valid").read[Boolean] ~
-      (__ \ "data" \ "metadata").read[ImageMetadata]
+      (__ \ "data" \ "metadata").read[ImageMetadata] ~
+      (__ \ "data" \ "fileMetadata" \ "data").read[FileMetadata]
       )(SourceImage.apply _)
 }
 

cropper/app/controllers/Application.scala

@@ -16,12 +16,11 @@ import com.gu.mediaservice.lib.auth
 import com.gu.mediaservice.lib.auth._
 import com.gu.mediaservice.lib.argo.ArgoHelpers
 import com.gu.mediaservice.lib.argo.model.{Action, Link}
+import com.gu.mediaservice.lib.imaging.ExportResult
 import com.gu.mediaservice.model.{Crop, SourceImage, CropSource, Bounds}
 
 import org.joda.time.DateTime
 
-import lib.imaging.ExportResult
-
 import lib._
 
 
@@ -124,10 +123,14 @@ object Application extends Controller with ArgoHelpers {
     }
   }
 
-  def fetchSourceFromApi(uri: String): Future[SourceImage] =
-    for (resp <- WS.url(uri).withHeaders("X-Gu-Media-Key" -> mediaApiKey).get)
+  def fetchSourceFromApi(uri: String): Future[SourceImage] = {
+    val imageRequest = WS.url(uri).
+      withHeaders("X-Gu-Media-Key" -> mediaApiKey).
+      withQueryString("include" -> "fileMetadata")
+    for (resp <- imageRequest.get)
     yield {
       if (resp.status != 200) Logger.warn(s"HTTP status ${resp.status} ${resp.statusText} from $uri")
       resp.json.as[SourceImage]
     }
+  }
 }

cropper/app/lib/Config.scala

@@ -36,8 +36,6 @@ object Config extends CommonPlayAppProperties with CommonPlayAppConfig {
 
   val tempDir: File = new File(properties.getOrElse("crop.output.tmp.dir", "/tmp"))
 
-  val imagingThreadPoolSize = 4
-
   val landscapeCropSizingWidths = List(2000, 1000, 500, 140)
   val portraitCropSizingHeights = List(2000, 1000, 500)
 }

cropper/app/lib/Crops.scala

@@ -1,10 +1,14 @@
 package lib
 
+import java.io.File
+
+import com.gu.mediaservice.lib.metadata.FileMetadataHelper
+
 import scala.concurrent.Future
-import lib.imaging.{ExportOperations, ExportResult}
+
 import com.gu.mediaservice.model.{Asset, Dimensions, SourceImage, Crop, Bounds, CropSource}
-import java.net.{URI, URL}
-import java.io.File
+import com.gu.mediaservice.lib.Files
+import com.gu.mediaservice.lib.imaging.{ImageOperations, ExportResult}
 
 case object InvalidImage extends Exception("Invalid image cannot be cropped")
 case object MissingSecureSourceUrl extends Exception("Missing secureUrl from source API")
@@ -20,13 +24,14 @@ object Crops {
     s"${source.id}/${Crop.getCropId(bounds)}/${if(isMaster) "master/" else ""}$outputWidth.jpg"
   }
 
-  def createMasterCrop(apiImage: SourceImage, sourceFile: File, crop: Crop, mediaType: String): Future[MasterCrop] = {
+  def createMasterCrop(apiImage: SourceImage, sourceFile: File, crop: Crop, mediaType: String, colourModel: Option[String]): Future[MasterCrop] = {
     val source   = crop.specification
     val metadata = apiImage.metadata
+    val iccColourSpace = FileMetadataHelper.normalisedIccColourSpace(apiImage.fileMetadata)
 
     for {
-      strip <- ExportOperations.cropImage(sourceFile, source.bounds, 100d)
-      file  <- ExportOperations.appendMetadata(strip, metadata)
+      strip <- ImageOperations.cropImage(sourceFile, source.bounds, 100d, Config.tempDir, iccColourSpace, colourModel)
+      file  <- ImageOperations.appendMetadata(strip, metadata)
 
       dimensions = Dimensions(source.bounds.width, source.bounds.height)
       filename   = outputFilename(apiImage, source.bounds, dimensions.width, true)
@@ -40,7 +45,7 @@ object Crops {
     Future.sequence[Asset, List](dimensionList.map { dimensions =>
       val filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width)
       for {
-        file    <- ExportOperations.resizeImage(sourceFile, dimensions, 75d)
+        file    <- ImageOperations.resizeImage(sourceFile, dimensions, 75d, Config.tempDir)
         sizing  <- CropStore.storeCropSizing(file, filename, mediaType, crop, dimensions)
         _       <- delete(file)
       }
@@ -75,8 +80,9 @@ object Crops {
     if(isInvalidCrop(apiImage.source, source)) throw InvalidCropRequest
 
     for {
-      sourceFile <- tempFileFromURL(secureUrl, "cropSource", "")
-      masterCrop <- createMasterCrop(apiImage, sourceFile, crop, mediaType)
+      sourceFile  <- tempFileFromURL(secureUrl, "cropSource", "", Config.tempDir)
+      colourModel <- ImageOperations.identifyColourModel(sourceFile, mediaType)
+      masterCrop  <- createMasterCrop(apiImage, sourceFile, crop, mediaType, colourModel)
 
       outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions
 

project/Build.scala

@@ -26,7 +26,7 @@ object Build extends Build {
   val lib = project("common-lib")
     .libraryDependencies(loggingDeps ++ awsDeps ++ elasticsearchDeps ++
       playDeps ++ playWsDeps ++ scalazDeps ++ commonsIODeps ++ akkaAgentDeps ++
-      pandaDeps)
+      pandaDeps ++ imagingDeps)
     .testDependencies(scalaCheckDeps ++ scalaTestDeps)
 
   val thrall = playProject("thrall")