Handle tiffs correctly

common-lib/src/main/scala/com/gu/mediaservice/lib/ImageIngestOperations.scala

@@ -20,19 +20,43 @@ class ImageIngestOperations(imageBucket: String, thumbnailBucket: String, config
 
   import ImageIngestOperations.{fileKeyFromId, optimisedPngKeyFromId}
 
-  def storeOriginal(id: String, file: File, mimeType: Option[MimeType], meta: Map[String, String] = Map.empty)
-                   (implicit logMarker: LogMarker): Future[S3Object] =
-    storeImage(imageBucket, fileKeyFromId(id), file, mimeType, meta)
-
-  def storeThumbnail(id: String, file: File, mimeType: Option[MimeType])
-                    (implicit logMarker: LogMarker): Future[S3Object] =
-    storeImage(thumbnailBucket, fileKeyFromId(id), file, mimeType)
-
-  def storeOptimisedPng(id: String, file: File)
+  def store(storableImage: StorableImage)
+           (implicit logMarker: LogMarker): Future[S3Object] = storableImage match {
+    case s:StorableOriginalImage => storeOriginalImage(s)
+    case s:StorableThumbImage => storeThumbnailImage(s)
+    case s:StorableOptimisedImage => storeOptimisedImage(s)
+  }
+
+  private def storeOriginalImage(storableImage: StorableOriginalImage)
+                        (implicit logMarker: LogMarker): Future[S3Object] =
+    storeImage(imageBucket, fileKeyFromId(storableImage.id), storableImage.file, Some(storableImage.mimeType), storableImage.meta)
+
+  private def storeThumbnailImage(storableImage: StorableThumbImage)
+                         (implicit logMarker: LogMarker): Future[S3Object] =
+    storeImage(thumbnailBucket, fileKeyFromId(storableImage.id), storableImage.file, Some(storableImage.mimeType))
+
+  private def storeOptimisedImage(storableImage: StorableOptimisedImage)
                        (implicit logMarker: LogMarker): Future[S3Object] =
-    storeImage(imageBucket, optimisedPngKeyFromId(id), file, Some(Png))
-  
+    storeImage(imageBucket, optimisedPngKeyFromId(storableImage.id), storableImage.file, Some(storableImage.mimeType))
+
   def deleteOriginal(id: String): Future[Unit] = if(isVersionedS3) deleteVersionedImage(imageBucket, fileKeyFromId(id)) else deleteImage(imageBucket, fileKeyFromId(id))
   def deleteThumbnail(id: String): Future[Unit] = deleteImage(thumbnailBucket, fileKeyFromId(id))
   def deletePng(id: String): Future[Unit] = deleteImage(imageBucket, optimisedPngKeyFromId(id))
 }
+
+sealed trait ImageWrapper {
+  val id: String
+  val file: File
+  val mimeType: MimeType
+  val meta: Map[String, String]
+}
+sealed trait StorableImage extends ImageWrapper
+
+case class StorableThumbImage(id: String, file: File, mimeType: MimeType, meta: Map[String, String] = Map.empty) extends StorableImage
+case class StorableOriginalImage(id: String, file: File, mimeType: MimeType, meta: Map[String, String] = Map.empty) extends StorableImage
+case class StorableOptimisedImage(id: String, file: File, mimeType: MimeType, meta: Map[String, String] = Map.empty) extends StorableImage
+case class BrowserViewableImage(id: String, file: File, mimeType: MimeType, meta: Map[String, String] = Map.empty, mustUpload: Boolean = false) extends ImageWrapper {
+  def asStorableOptimisedImage = StorableOptimisedImage(id, file, mimeType, meta)
+  def asStorableThumbImage = StorableThumbImage(id, file, mimeType, meta)
+}
+

common-lib/src/main/scala/com/gu/mediaservice/lib/S3ImageStorage.scala

@@ -2,7 +2,7 @@ package com.gu.mediaservice.lib
 
 import java.io.File
 
-import com.gu.mediaservice.lib.aws.S3
+import com.gu.mediaservice.lib.aws.{S3, S3Ops}
 import com.gu.mediaservice.lib.config.CommonConfig
 import com.gu.mediaservice.lib.logging.LogMarker
 import com.gu.mediaservice.model.MimeType
@@ -15,9 +15,15 @@ import scala.concurrent.Future
 class S3ImageStorage(config: CommonConfig) extends S3(config) with ImageStorage {
   private val log = LoggerFactory.getLogger(getClass)
 
+  private val cacheSetting = Some(cacheForever)
   def storeImage(bucket: String, id: String, file: File, mimeType: Option[MimeType], meta: Map[String, String] = Map.empty)
-                (implicit logMarker: LogMarker) =
-    store(bucket, id, file, mimeType, meta, Some(cacheForever))
+                (implicit logMarker: LogMarker) = {
+    store(bucket, id, file, mimeType, meta, cacheSetting)
+      .map( _ =>
+        // TODO this is just giving back the stuff we passed in and should be factored out.
+        S3Ops.projectFileAsS3Object(bucket, id, file, mimeType, meta, cacheSetting)
+      )
+  }
 
   def deleteImage(bucket: String, id: String) = Future {
     client.deleteObject(bucket, id)

common-lib/src/main/scala/com/gu/mediaservice/lib/aws/S3.scala

@@ -116,7 +116,7 @@ class S3(config: CommonConfig) extends GridLogging {
   }
 
   def store(bucket: Bucket, id: Key, file: File, mimeType: Option[MimeType], meta: UserMetadata = Map.empty, cacheControl: Option[String] = None)
-           (implicit ex: ExecutionContext, logMarker: LogMarker): Future[S3Object] =
+           (implicit ex: ExecutionContext, logMarker: LogMarker): Future[Unit] =
     Future {
       val metadata = new ObjectMetadata
       mimeType.foreach(m => metadata.setContentType(m.name))
@@ -127,8 +127,6 @@ class S3(config: CommonConfig) extends GridLogging {
       Stopwatch(s"S3 client.putObject ($req)"){
         client.putObject(req)
       }
-
-      S3Ops.projectFileAsS3Object(bucket, id, file, mimeType, meta, cacheControl)
     }
 
   def list(bucket: Bucket, prefixDir: String)
@@ -199,9 +197,9 @@ object S3Ops {
     new URI("http", bucketUrl, s"/$key", null)
   }
 
-  def projectFileAsS3Object(bucket: String, key: String, file: File, mimeType: Option[MimeType], meta: Map[String, String] = Map.empty, cacheControl: Option[String] = None): S3Object = {
+  def projectFileAsS3Object(url: URI, file: File, mimeType: Option[MimeType], meta: Map[String, String], cacheControl: Option[String]): S3Object = {
     S3Object(
-      objectUrl(bucket, key),
+      url,
       file.length,
       S3Metadata(
         meta,
@@ -212,4 +210,8 @@ object S3Ops {
       )
     )
   }
+
+  def projectFileAsS3Object(bucket: String, key: String, file: File, mimeType: Option[MimeType], meta: Map[String, String] = Map.empty, cacheControl: Option[String] = None): S3Object = {
+    projectFileAsS3Object(objectUrl(bucket, key), file, mimeType, meta, cacheControl)
+  }
 }

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

@@ -4,6 +4,8 @@ import java.io._
 
 import org.im4java.core.IMOperation
 import com.gu.mediaservice.lib.Files._
+import com.gu.mediaservice.lib.StorableThumbImage
+import com.gu.mediaservice.lib.imaging.ImageOperations.{optimisedMimeType, thumbMimeType}
 import com.gu.mediaservice.lib.imaging.im4jwrapper.ImageMagick.{addImage, format, runIdentifyCmd}
 import com.gu.mediaservice.lib.imaging.im4jwrapper.{ExifTool, ImageMagick}
 import com.gu.mediaservice.lib.logging.GridLogging
@@ -114,31 +116,60 @@ class ImageOperations(playPath: String) extends GridLogging {
   val thumbUnsharpRadius = 0.5d
   val thumbUnsharpSigma = 0.5d
   val thumbUnsharpAmount = 0.8d
-  def createThumbnail(sourceFile: File, sourceMimeType: Option[MimeType], width: Int, qual: Double = 100d,
-                      tempDir: File, iccColourSpace: Option[String], colourModel: Option[String]): Future[File] = {
+
+  /**
+    * Given a source file containing a png (the 'browser viewable' file),
+    * construct a thumbnail file in the provided temp directory, and return
+    * the file with metadata about it.
+    * @param sourceFile File containing browser viewable (ie not too big or colourful) image
+    * @param sourceMimeType Mime time of browser viewable file
+    * @param width Desired with of thumbnail
+    * @param qual Desired quality of thumbnail
+    * @param tempDir Location to create thumbnail file
+    * @param iccColourSpace (Approximately) number of colours to use
+    * @param colourModel Colour model - eg RGB or CMYK
+    * @return The file created and the mimetype of the content of that file, in a future.
+    */
+  def createThumbnail(sourceFile: File,
+                      sourceMimeType: Option[MimeType],
+                      width: Int,
+                      qual: Double = 100d,
+                      tempDir: File,
+                      iccColourSpace: Option[String],
+                      colourModel: Option[String]): Future[(File, MimeType)] = {
+    val cropSource  = addImage(sourceFile)
+    val thumbnailed = thumbnail(cropSource)(width)
+    val corrected   = correctColour(thumbnailed)(iccColourSpace, colourModel)
+    val converted   = applyOutputProfile(corrected, optimised = true)
+    val stripped    = stripMeta(converted)
+    val profiled    = applyOutputProfile(stripped, optimised = true)
+    val unsharpened = unsharp(profiled)(thumbUnsharpRadius, thumbUnsharpSigma, thumbUnsharpAmount)
+    val qualified   = quality(unsharpened)(qual)
+    val addOutput   = {file:File => addDestImage(qualified)(file)}
     for {
-      outputFile <- createTempFile(s"thumb-", ".jpg", tempDir)
-      cropSource  = addImage(sourceFile)
-      thumbnailed = thumbnail(cropSource)(width)
-      corrected   = correctColour(thumbnailed)(iccColourSpace, colourModel)
-      converted   = applyOutputProfile(corrected, optimised = true)
-      stripped    = stripMeta(converted)
-      profiled    = applyOutputProfile(stripped, optimised = true)
-      unsharpened = unsharp(profiled)(thumbUnsharpRadius, thumbUnsharpSigma, thumbUnsharpAmount)
-      qualified   = quality(unsharpened)(qual)
-      addOutput   = addDestImage(qualified)(outputFile)
-      _          <- runConvertCmd(addOutput, useImageMagick = sourceMimeType.contains(Tiff))
-    } yield outputFile
+      outputFile <- createTempFile(s"thumb-", thumbMimeType.fileExtension, tempDir)
+      _          <- runConvertCmd(addOutput(outputFile), useImageMagick = sourceMimeType.contains(Tiff))
+    } yield (outputFile, thumbMimeType)
   }
 
-  def transformImage(sourceFile: File, sourceMimeType: Option[MimeType], tempDir: File): Future[File] = {
+  /**
+    * Given a source file containing a file which requires optimising to make it suitable for viewing in
+    * a browser, construct a new image file in the provided temp directory, and return
+    * * the file with metadata about it.
+    * @param sourceFile File containing browser viewable (ie not too big or colourful) image
+    * @param sourceMimeType Mime time of browser viewable file
+    * @param tempDir Location to create optimised file
+    * @return The file created and the mimetype of the content of that file, in a future.
+    */
+  def transformImage(sourceFile: File, sourceMimeType: Option[MimeType], tempDir: File): Future[(File, MimeType)] = {
     for {
-      outputFile      <- createTempFile(s"transformed-", ".png", tempDir)
+      // png suffix is used by imagemagick to infer the required type
+      outputFile      <- createTempFile(s"transformed-", optimisedMimeType.fileExtension, tempDir)
       transformSource = addImage(sourceFile)
       addOutput       = addDestImage(transformSource)(outputFile)
       _               <- runConvertCmd(addOutput, useImageMagick = sourceMimeType.contains(Tiff))
       _               <- checkForOutputFileChange(outputFile)
-    } yield outputFile
+    } yield (outputFile, optimisedMimeType)
   }
 
   // When a layered tiff is unpacked, the temp file (blah.something) is moved
@@ -175,6 +206,8 @@ class ImageOperations(playPath: String) extends GridLogging {
 }
 
 object ImageOperations {
+  val thumbMimeType = Jpeg
+  val optimisedMimeType = Png
   def identifyColourModel(sourceFile: File, mimeType: MimeType)(implicit ec: ExecutionContext): Future[Option[String]] = {
     // TODO: use mimeType to lookup other properties once we support other formats
 

docs/06-objects-of-interest/06.01-tiffs.md

@@ -0,0 +1,19 @@
+# TIFF files
+
+Tiff files have special handling for importing.
+
+Like all files, we upload the original, but create an (optional) optimised
+PNG, and (mandatory) thumbnail JPEG version for use in the UI.
+
+For TIFF files, these pngs derive from the first image file extracted by
+ImageMagick.  The `convert` function will extract a file of the
+specified type (PNG in our case), which, in a file named `xxx.tif` will be
+`xxx.png`.
+
+However, there is additional complexity with tiff files which contain layered
+content.  Specifically, the layer files will explode as `xxx-n.jpg` or
+`xxx-n.png` where `n` is a numeric index.
+
+Our code looks for this special case (see `ImageOperations.checkForOutputFileChange`)
+and simply moves the `-n` layer file to the expected location.
+

image-loader/app/ImageLoaderComponents.scala

@@ -3,7 +3,7 @@ import com.gu.mediaservice.lib.play.GridComponents
 import controllers.ImageLoaderController
 import lib._
 import lib.storage.ImageLoaderStore
-import model.{Uploader, Projector}
+import model.{Projector, Uploader}
 import play.api.ApplicationLoader.Context
 import router.Routes
 

image-loader/app/lib/imaging/FileMetadataReader.scala

@@ -12,9 +12,11 @@ import com.drew.metadata.jpeg.JpegDirectory
 import com.drew.metadata.png.PngDirectory
 import com.drew.metadata.xmp.XmpDirectory
 import com.drew.metadata.{Directory, Metadata}
+import com.gu.mediaservice.lib.{ImageWrapper, StorableImage}
 import com.gu.mediaservice.lib.imaging.im4jwrapper.ImageMagick._
 import com.gu.mediaservice.lib.metadata.ImageMetadataConverter
 import com.gu.mediaservice.model._
+import model.upload.UploadRequest
 import org.joda.time.{DateTime, DateTimeZone}
 import org.joda.time.format.ISODateTimeFormat
 import play.api.libs.json.JsValue
@@ -54,16 +56,19 @@ object FileMetadataReader {
     for {
       metadata <- readMetadata(image)
     }
-    yield getMetadataWithICPTCHeaders(metadata, imageId) // FIXME: JPEG, JFIF, Photoshop, GPS, File
+    yield getMetadataWithIPTCHeaders(metadata, imageId) // FIXME: JPEG, JFIF, Photoshop, GPS, File
 
-  def fromICPTCHeadersWithColorInfo(image: File, imageId:String, mimeType: MimeType): Future[FileMetadata] =
+  def fromIPTCHeadersWithColorInfo(image: ImageWrapper): Future[FileMetadata] =
+    fromIPTCHeadersWithColorInfo(image.file, image.id, image.mimeType)
+
+  def fromIPTCHeadersWithColorInfo(image: File, imageId:String, mimeType: MimeType): Future[FileMetadata] =
     for {
       metadata <- readMetadata(image)
       colourModelInformation <- getColorModelInformation(image, metadata, mimeType)
     }
-    yield getMetadataWithICPTCHeaders(metadata, imageId).copy(colourModelInformation = colourModelInformation)
+    yield getMetadataWithIPTCHeaders(metadata, imageId).copy(colourModelInformation = colourModelInformation)
 
-  private def getMetadataWithICPTCHeaders(metadata: Metadata, imageId:String): FileMetadata =
+  private def getMetadataWithIPTCHeaders(metadata: Metadata, imageId:String): FileMetadata =
     FileMetadata(
       iptc = exportDirectory(metadata, classOf[IptcDirectory]),
       exif = exportDirectory(metadata, classOf[ExifIFD0Directory]),
@@ -217,12 +222,12 @@ object FileMetadataReader {
           "paletteSize" -> Option(metaDir.getDescription(PngDirectory.TAG_PALETTE_SIZE)),
           "iccProfileName" -> Option(metaDir.getDescription(PngDirectory.TAG_ICC_PROFILE_NAME))
         ).flattenOptions
-      case _ => val metaDir = metadata.getFirstDirectoryOfType(classOf[ExifIFD0Directory])
+      case _ => val metaDir = Option(metadata.getFirstDirectoryOfType(classOf[ExifIFD0Directory]))
         Map(
           "hasAlpha" -> hasAlpha,
           "colorType" -> maybeImageType,
-          "photometricInterpretation" -> Option(metaDir.getDescription(ExifDirectoryBase.TAG_PHOTOMETRIC_INTERPRETATION)),
-          "bitsPerSample" -> Option(metaDir.getDescription(ExifDirectoryBase.TAG_BITS_PER_SAMPLE))
+          "photometricInterpretation" -> metaDir.map(_.getDescription(ExifDirectoryBase.TAG_PHOTOMETRIC_INTERPRETATION)),
+          "bitsPerSample" -> metaDir.map(_.getDescription(ExifDirectoryBase.TAG_BITS_PER_SAMPLE))
         ).flattenOptions
     }