Libvips cropping

build.sbt

@@ -292,6 +292,6 @@ def playImageLoaderProject(projectName: String, port: Int, path: Option[String]
         "-Dpidfile.path=/dev/null",
         s"-Dconfig.file=/opt/docker/conf/application.conf",
         s"-Dlogger.file=/opt/docker/conf/logback.xml",
-        "-XX:+PrintCommandLineFlags"
+        "-XX:+PrintCommandLineFlags", "-XX:MaxRAMPercentage=20"
   )))
 }

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

@@ -1,22 +1,17 @@
 package com.gu.mediaservice.lib.imaging
 
 import app.photofox.vipsffm.enums.{VipsIntent, VipsInterpretation}
-
-import java.io._
-import org.im4java.core.IMOperation
-import com.gu.mediaservice.lib.Files._
-import com.gu.mediaservice.lib.{BrowserViewableImage, StorableThumbImage}
-import com.gu.mediaservice.lib.imaging.ImageOperations.{optimisedMimeType, thumbMimeType}
-import com.gu.mediaservice.lib.imaging.im4jwrapper.ImageMagick.{addDestImage, addImage, format, runIdentifyCmd}
+import app.photofox.vipsffm.{VImage, VipsHelper, VipsOption}
+import com.gu.mediaservice.lib.BrowserViewableImage
+import com.gu.mediaservice.lib.imaging.ImageOperations.thumbMimeType
 import com.gu.mediaservice.lib.imaging.im4jwrapper.{ExifTool, ImageMagick}
 import com.gu.mediaservice.lib.logging.{GridLogging, LogMarker, Stopwatch, addLogMarkers}
 import com.gu.mediaservice.model._
+import org.im4java.core.IMOperation
 
-import scala.concurrent.{ExecutionContext, Future}
-import scala.sys.process._
-import app.photofox.vipsffm.{VImage, Vips, VipsHelper, VipsOption}
-
+import java.io._
 import java.lang.foreign.Arena
+import scala.concurrent.{ExecutionContext, Future}
 
 
 case class ExportResult(id: String, masterCrop: Asset, othersizings: List[Asset])
@@ -49,89 +44,68 @@ class ImageOperations(playPath: String) extends GridLogging {
     ).collect { case (key, Some(value)) => (key, value) }
   }
 
-  private def applyOutputProfile(base: IMOperation, optimised: Boolean = false) = profile(base)(rgbProfileLocation(optimised))
-
-  // 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], isTransformedFromSource: Boolean)(implicit logMarker: LogMarker): IMOperation = {
-    (iccColourSpace, colourModel, isTransformedFromSource) 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
-      // Do not correct colour if file has already been transformed (ie. source file was TIFF) as correctColour has already been run
-      case (_, _, true) => base
-      // If mismatching, strip any (incorrect) ICC profile and inject a profile matching the model
-      // Note: Strip both ICC and ICM (Windows variant?) to be safe
-      case (icc, Some(model), _) =>
-        profileLocations.get(model) match {
-          // If this is a supported model, strip profile from base and add profile for model
-          case Some(location) => profile(stripProfile(base)("icm,icc"))(location)
-          // Do not attempt to correct colour if we don't support that colour model
-          case None =>
-            logger.warn(
-              logMarker,
-              s"Wanted to update colour model where iccColourSpace=$icc and colourModel=$model but we don't have a profile file for that model"
-            )
-            base
-        }
+  def cropImageVips(
+                     sourceFile: File,
+                     bounds: Bounds,
+                     orientationMetadata: Option[OrientationMetadata]
+                   )(implicit logMarker: LogMarker, arena: Arena): VImage = {
+    // Read source image
+    val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath)
+    val maybeInterpretation = VipsInterpretation.values().toSeq.find(_.getRawValue == VipsHelper.image_get_interpretation(image.getUnsafeStructAddress))
+
+    // Orient
+    val rotated = orientationMetadata.map(_.orientationCorrection()).map { angle =>
+      image.rotate(angle)
+    }.getOrElse {
+      image
     }
-  }
-
-  def cropImage(
-    sourceFile: File,
-    sourceMimeType: Option[MimeType],
-    bounds: Bounds,
-    qual: Double = 100d,
-    tempDir: File,
-    iccColourSpace: Option[String],
-    colourModel: Option[String],
-    fileType: MimeType,
-    isTransformedFromSource: Boolean,
-    orientationMetadata: Option[OrientationMetadata]
-  )(implicit logMarker: LogMarker): Future[File] = Stopwatch.async("magick crop image") {
-    for {
-      outputFile <- createTempFile(s"crop-", s"${fileType.fileExtension}", tempDir)
-      cropSource    = addImage(sourceFile)
-      oriented      = orient(cropSource, orientationMetadata)
-      qualified     = quality(oriented)(qual)
-      corrected     = correctColour(qualified)(iccColourSpace, colourModel, isTransformedFromSource)
-      converted     = applyOutputProfile(corrected)
-      stripped      = stripMeta(converted)
-      profiled      = applyOutputProfile(stripped)
-      cropped       = crop(profiled)(bounds)
-      depthAdjusted = depth(cropped)(8)
-      addOutput     = addDestImage(depthAdjusted)(outputFile)
-      _             <- runConvertCmd(addOutput, useImageMagick = sourceMimeType.contains(Tiff))
-      _             <- checkForOutputFileChange(outputFile)
+    // TODO strip meta data
+    // Output colour profile
+    val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height)
+    // TODO depth adjust
+
+    val labInterpretations = Set (
+      VipsInterpretation.INTERPRETATION_LAB,
+      VipsInterpretation.INTERPRETATION_LABS
+    )
+
+    val isLab = maybeInterpretation.exists(interpretation => labInterpretations.contains(interpretation))
+    val corrected = if (!isLab) {
+      cropped.iccTransform("srgb",
+        VipsOption.Enum("intent",VipsIntent.INTENT_PERCEPTUAL),     // Helps with CMYK; see https://github.com/libvips/libvips/issues/1110
+        VipsOption.Int("depth", getDepthFor(image))
+      )
+    } else {
+      // LAB gets corrupted by icc_transform something about with no profile?
+      cropped
     }
-    yield outputFile
+
+    val master = corrected
+    master
   }
 
+
   // 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,
-    sourceMimeType: Option[MimeType],
-    dimensions: Dimensions,
-    qual: Double = 100d,
-    tempDir: File,
-    fileType: MimeType
-  )(implicit logMarker: LogMarker): Future[File] = Stopwatch.async("magick resize image") {
-    for {
-      outputFile  <- createTempFile(s"resize-", s".${fileType.fileExtension}", tempDir)
-      resizeSource = addImage(sourceFile)
-      qualified    = quality(resizeSource)(qual)
-      resized      = scale(qualified)(dimensions)
-      addOutput    = addDestImage(resized)(outputFile)
-      _           <- runConvertCmd(addOutput, useImageMagick = sourceMimeType.contains(Tiff))
-    }
-    yield outputFile
+  def resizeImageVips(
+                       sourceImage: VImage,
+                       dimensions: Dimensions,
+                       quality: Int = 100,
+                       tempDir: File,
+                       fileType: MimeType,
+                       sourceDimensions: Dimensions
+                     )(implicit logMarker: LogMarker, arena: Arena): File = {
+
+    val scale = dimensions.width.toDouble / sourceDimensions.width.toDouble
+    val resized = sourceImage.resize(scale)
+
+    val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this
+    saveImageToFile(resized, fileType, quality, outputFile, quantise = true)
   }
 
   private def orient(op: IMOperation, orientationMetadata: Option[OrientationMetadata]): IMOperation = {
@@ -142,27 +116,6 @@ class ImageOperations(playPath: String) extends GridLogging {
     }
   }
 
-  def optimiseImage(resizedFile: File, mediaType: MimeType)(implicit logMarker: LogMarker): File = mediaType match {
-    case Png =>
-      val fileName: String = resizedFile.getAbsolutePath
-
-      val optimisedImageName: String = fileName.split('.')(0) + "optimised.png"
-      Stopwatch("pngquant") {
-        Seq("pngquant", "-s10", "--quality", "1-85", fileName, "--output", optimisedImageName).!
-      }
-
-      new File(optimisedImageName)
-    case Jpeg => resizedFile
-
-    // This should never happen as we only ever crop as PNG or JPEG. See `Crops.cropType` and `CropsTest`
-    // TODO We should create a `CroppingMimeType` to enforce this at the type level.
-    //  However we'd need to change the `Asset` model as source image and crop use this model
-    //  and a source can legally be a `Tiff`. It's not a small change...
-    case Tiff =>
-      logger.error("Attempting to optimize a Tiff crop. Cropping as Tiff is not supported.")
-      throw new UnsupportedCropOutputTypeException
-  }
-
   val interlacedHow = "Line"
   val backgroundColour = "#333333"
 
@@ -225,37 +178,65 @@ class ImageOperations(playPath: String) extends GridLogging {
     }
   }
 
-  // When a layered tiff is unpacked, the temp file (blah.something) is moved
-  // to blah-0.something and contains the composite layer (which is what we want).
-  // Other layers are then saved as blah-1.something etc.
-  // As the file has been renamed, the file object still exists, but has the wrong name
-  // We will need to put it back where it is expected to be found, and clean up the other
-  // files.
-  private def checkForOutputFileChange(f: File): Future[Unit] = Future {
-    val fileBits = f.getAbsolutePath.split("\\.").toList
-    val mainPart = fileBits.dropRight(1).mkString(".")
-    val extension = fileBits.last
-
-    // f2 is the blah-0 name that gets created from a layered tiff.
-    val f2 = new File(List(s"$mainPart-0", extension).mkString("."))
-    if (f2.exists()) {
-      // f HAS been renamed to blah-0.  Rename it right back!
-      f2.renameTo(f)
-      // Tidy up any other files (blah-1,2,3 etc will be created for each subsequent layer)
-      cleanUpLayerFiles(mainPart, extension, 1)
+  def saveImageToFile(image: VImage, mimeType: MimeType, quality: Int, outputFile: File, quantise: Boolean = false): File = {
+    logger.info(s"Saving image as $mimeType to file: " + outputFile.getAbsolutePath)
+    mimeType match {
+      case Jpeg =>
+        image.jpegsave(outputFile.getAbsolutePath,
+          VipsOption.Int("Q", quality),
+          //VipsOption.Boolean("optimize-scans", true),
+          //VipsOption.Boolean("optimize-coding", true),
+          //VipsOption.Boolean("interlace", true),
+          //VipsOption.Boolean("trellis-quant", true),
+          // VipsOption.Int("quant-table", 3),
+          VipsOption.Boolean("strip", true)
+        )
+        outputFile
+
+      case Png =>
+        // Bit used in save must match used in transform
+        val depth: Int = getDepthFor(image)
+
+        // We are allowed to quantise PNG crops but not the master
+        if (quantise) {
+          image.pngsave(outputFile.getAbsolutePath,
+            VipsOption.Boolean("palette", true),
+            VipsOption.Int("Q", quality),
+            VipsOption.Int("effort", 1),
+            VipsOption.Boolean("strip", true)
+          )
+        } else {
+          image.pngsave(outputFile.getAbsolutePath,
+            VipsOption.Int("bitdepth", depth),
+            VipsOption.Boolean("strip", true)
+          )
+        }
+        outputFile
+
+      case _ =>
+        logger.error(s"Save to $mimeType is not supported.")
+        throw new UnsupportedCropOutputTypeException
     }
   }
 
-  @scala.annotation.tailrec
-  private def cleanUpLayerFiles(mainPart: String, extension: String, index: Int):Unit = {
-     val newFile = List(s"$mainPart-$index", extension).mkString(".")
-     val f3 = new File(newFile)
-     if (f3.exists()) {
-       f3.delete()
-       cleanUpLayerFiles(mainPart, extension, index+1)
-     }
+  private def getDepthFor(image: VImage) = {
+    val sixteenBitInterpretations = Set(
+      VipsInterpretation.INTERPRETATION_GREY16,
+      VipsInterpretation.INTERPRETATION_LABS,
+      VipsInterpretation.INTERPRETATION_RGB16
+    )
+
+    val maybeInterpretation = VipsInterpretation.values().toSeq.find(_.getRawValue == VipsHelper.image_get_interpretation(image.getUnsafeStructAddress))
+    val depth = maybeInterpretation.map { interpretation =>
+      if (sixteenBitInterpretations.contains(interpretation)) {
+        16
+      } else {
+        8
+      }
+    }.getOrElse(8)
+    logger.info(s"Depth for interpretation $maybeInterpretation is $depth")
+    depth
   }
-
 }
 
 object ImageOperations extends GridLogging {
@@ -311,4 +292,17 @@ object ImageOperations extends GridLogging {
     }
   }
 
+  def isGraphicVips(image: VImage)(implicit arena: Arena): Boolean = {
+    val numberOfBands = VipsHelper.image_get_bands(image.getUnsafeStructAddress)
+   logger.info("Number of bands: " + numberOfBands)
+    // Indexed plus alpha would be 2 bands
+
+    val format = VipsHelper.image_get_format(image.getUnsafeStructAddress)
+    logger.info("Format: " + format)
+
+    val paletteType = VipsHelper.image_get_typeof(arena, image.getUnsafeStructAddress, "palette")
+    logger.info("Palette type: " + paletteType)
+
+    paletteType > 0 || numberOfBands < 3
+  }
 }

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

@@ -87,14 +87,6 @@ object ImageMagick extends GridLogging {
     op
   }
 
-  def runConvertCmd(op: IMOperation, useImageMagick: Boolean)(implicit logMarker: LogMarker): Future[Unit] = {
-    Stopwatch.async(s"Using ${if(useImageMagick) "imagemagick" else "graphicsmagick"} for imaging conversion operation '$op'") {
-      Future {
-        new ConvertCmd(!useImageMagick).run(op)
-      }
-    }
-  }
-
   def runIdentifyCmd(op: IMOperation, useImageMagick: Boolean)(implicit logMarker: LogMarker): Future[List[String]] = {
     Stopwatch.async(s"Using ${if (useImageMagick) "imagemagick" else "graphicsmagick"} for imaging identification operation '$op'") {
       Future {

common-lib/src/test/resources/schaik.com_pngsuite/README.md

@@ -0,0 +1,25 @@
+Test PNG images
+===============
+
+These images are taken from http://www.schaik.com/pngsuite/pngsuite_bas_png.html
+
+     basn0g08 - 8 bit (256 level) grayscale 
+     basn2c08 - 3x8 bits rgb color 
+     basn3p08 - 8 bit (256 color) paletted 
+     basn6a08 - 3x8 bits rgb color + 8 bit alpha-channel 
+
+LICENCE
+-------
+
+At the time of downloading these images the licence file at http://www.schaik.com/pngsuite/PngSuite.LICENSE contained the following text:
+
+```
+PngSuite
+--------
+
+Permission to use, copy, modify and distribute these images for any
+purpose and without fee is hereby granted.
+
+
+(c) Willem van Schaik, 1996, 2011
+```