diff --git a/build.sbt b/build.sbt index 48aa92e10f..24662a89c4 100644 --- a/build.sbt +++ b/build.sbt @@ -126,6 +126,7 @@ lazy val commonLib = project("common-lib").settings( // declare explicit dependency on desired version of aws sdk v2 bedrock runtime "software.amazon.awssdk" % "bedrockruntime" % awsSdkV2Version, "software.amazon.awssdk" % "s3vectors" % awsSdkV2Version, + "com.adobe.xmp" % "xmpcore" % "6.1.11", ws, "org.testcontainers" % "testcontainers-elasticsearch" % "2.0.2" % Test, ), diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala index 6209187f12..44a1c6086b 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala @@ -1,26 +1,27 @@ package com.gu.mediaservice.lib.imaging import app.photofox.vipsffm.enums.{VipsIntent, VipsInterpretation} -import app.photofox.vipsffm.{VImage, VipsHelper, VipsOption} +import app.photofox.vipsffm.jextract.VipsRaw +import app.photofox.vipsffm.{VBlob, VImage, VipsHelper, VipsOption} +import com.adobe.internal.xmp.options.SerializeOptions +import com.adobe.internal.xmp.{XMPConst, XMPMetaFactory} import com.gu.mediaservice.lib.BrowserViewableImage -import com.gu.mediaservice.lib.Files._ import com.gu.mediaservice.lib.imaging.ImageOperations.thumbMimeType -import com.gu.mediaservice.lib.imaging.im4jwrapper.{ExifTool, ImageMagick} +import com.gu.mediaservice.lib.imaging.im4jwrapper.ImageMagick import com.gu.mediaservice.lib.logging.{GridLogging, LogMarker, Stopwatch, addLogMarkers} import com.gu.mediaservice.model._ import org.im4java.core.IMOperation import java.io._ import java.lang.foreign.Arena +import java.nio.charset.StandardCharsets import scala.concurrent.{ExecutionContext, Future} -import scala.sys.process._ case class ExportResult(id: String, masterCrop: Asset, othersizings: List[Asset]) class UnsupportedCropOutputTypeException extends Exception class ImageOperations(playPath: String) extends GridLogging { - import ExifTool._ import ImageMagick._ private def profilePath(fileName: String): String = s"$playPath/$fileName" @@ -38,97 +39,108 @@ class ImageOperations(playPath: String) extends GridLogging { "Greyscale" -> profilePath("grayscale.icc") ) - private def tagFilter(metadata: ImageMetadata) = { - Map[String, Option[String]]( - "Copyright" -> metadata.copyright, - "Credit" -> metadata.credit, - "OriginalTransmissionReference" -> metadata.suppliersReference - ).collect { case (key, Some(value)) => (key, value) } - } + def cropImageVips( + sourceFile: File, + bounds: Bounds, + metadata: ImageMetadata, + orientationMetadata: Option[OrientationMetadata] + )(implicit logMarker: LogMarker, arena: Arena): VImage = { + // Read source image + val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) + + // Orient + val rotated = orientationMetadata.map(_.orientationCorrection()).map { angle => + image.rotate(angle) + }.getOrElse { + image + } - 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 - } + val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) + + // If we saw and ICC profile than we will need to transform + val needsICCTransform = VipsHelper.image_get_typeof(arena, image.getUnsafeStructAddress, "icc-profile-data") != 0 + val correctedForICCProfile = if (needsICCTransform) { + cropped.iccTransform("srgb", + VipsOption.Enum("intent", VipsIntent.INTENT_PERCEPTUAL), // Helps with CMYK; see https://github.com/libvips/libvips/issues/1110 + ) + } else { + // LAB gets corrupted by a needless icc_transform + cropped + } + + // Apply crop metadata + // https://developers.google.com/search/docs/appearance/structured-data/image-license-metadata#iptc-photo-metadata + makeXmpBlog(metadata).foreach { xmpBlob => + logger.info("Tagging master crop with XMP metadata: " + new String(xmpBlob)) + VipsHelper.image_set_blob_copy(arena, correctedForICCProfile.getUnsafeStructAddress, "xmp-data", VBlob.newFromBytes(arena, xmpBlob).getUnsafeDataAddress, xmpBlob.length) } + + correctedForICCProfile } - 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) + private def makeXmpBlog(metadata: ImageMetadata): Option[Array[Byte]] = { + val mappings: Seq[(String, String, String)] = Seq(metadata.byline.map { creator => + (XMPConst.NS_DC, "creator", creator) + }, + metadata.credit.map { credit => + (XMPConst.NS_PHOTOSHOP, "Credit", credit) + }, + metadata.copyright.map { copyright => + (XMPConst.NS_DC, "rights", copyright) + }, + metadata.suppliersReference.map { suppliersReference => + (XMPConst.NS_PHOTOSHOP, "TransmissionReference", suppliersReference) + }).flatten + + mappings.headOption.map { _ => + val xmpMeta = XMPMetaFactory.create() + mappings.foreach { mapping => + xmpMeta.setProperty(mapping._1, mapping._2, mapping._3) + } + + val serializeOptions = new SerializeOptions() + serializeOptions.setUseCompactFormat(true) + serializeOptions.setUseCanonicalFormat(false) + val xmpXml = XMPMetaFactory.serializeToString(xmpMeta, serializeOptions) + xmpXml.getBytes(StandardCharsets.UTF_8) } - yield outputFile } - // Updates metadata on existing file - def appendMetadata(sourceFile: File, metadata: ImageMetadata): Future[File] = { - runExiftoolCmd( - setTags(tagSource(sourceFile))(tagFilter(metadata)) - ).map(_ => sourceFile) + def createCrops(sourceImage: VImage, dimensionList: List[Dimensions], imageId: String, bounds: Bounds, cropType: MimeType, tempDir: File, cropQuality: Int + )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): Future[Seq[(File, String, Dimensions)]] = { + Stopwatch(s"Resizing crops for $imageId") { + logger.info("Starting resizes") + val resizes = dimensionList.map { dimensions => + val outputFile = File.createTempFile(s"resize-", s"${cropType.fileExtension}", tempDir) // TODO function for this + + resizeImageVips(sourceImage, dimensions, cropQuality, outputFile, cropType).map { f => + def outputFilename(imageId: String, bounds: Bounds, outputWidth: Int, fileType: MimeType, isMaster: Boolean = false, instance: Instance): String = { // TODO push back to Crops + val masterString: String = if (isMaster) "master/" else "" + instance.id + "/" + s"$imageId/${Crop.getCropId(bounds)}/$masterString$outputWidth${fileType.fileExtension}" + } + + val filename = outputFilename(imageId, bounds, dimensions.width, cropType, instance = instance) + (f, filename, dimensions) + } + } + logger.info("Done resizes") + Future.sequence(resizes) + } } - 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)) + def resizeImageVips( + sourceImage: VImage, + dimensions: Dimensions, + quality: Int = 100, + outputFile: File, + fileType: MimeType + )(implicit logMarker: LogMarker, arena: Arena): Future[File] = { + Future { + val scale = dimensions.width.toDouble / sourceImage.getWidth.toDouble + val resized = sourceImage.resize(scale) + + saveImageToFile(resized, fileType, quality, outputFile, quantise = true, keep = Some(VipsRaw.VIPS_FOREIGN_KEEP_XMP)) } - yield outputFile } private def orient(op: IMOperation, orientationMetadata: Option[OrientationMetadata]): IMOperation = { @@ -139,27 +151,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" @@ -219,18 +210,19 @@ class ImageOperations(playPath: String) extends GridLogging { } } - def saveImageToFile(image: VImage, mimeType: MimeType, qual: Double, outputFile: File, quantise: Boolean = false): File = { - logger.info(s"Saving image as $mimeType to file: " + outputFile.getAbsolutePath) + def saveImageToFile(image: VImage, mimeType: MimeType, quality: Int, outputFile: File, quantise: Boolean = false, keep: Option[Int] = None): File = { + val k = keep.getOrElse(VipsRaw.VIPS_FOREIGN_KEEP_NONE) mimeType match { case Jpeg => image.jpegsave(outputFile.getAbsolutePath, - VipsOption.Int("Q", qual.toInt), + 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) + VipsOption.Boolean("strip", true), + VipsOption.Int("keep", k) ) outputFile @@ -239,16 +231,17 @@ class ImageOperations(playPath: String) extends GridLogging { if (quantise) { image.pngsave(outputFile.getAbsolutePath, VipsOption.Boolean("palette", true), - VipsOption.Int("Q", qual.toInt), + VipsOption.Int("Q", quality), VipsOption.Int("effort", 1), //VipsOption.Int("compression", 6), - VipsOption.Int("bitdepth", 8), - VipsOption.Boolean("strip", true) + VipsOption.Boolean("strip", true), + VipsOption.Int("keep", k) ) } else { image.pngsave(outputFile.getAbsolutePath, //VipsOption.Int("compression", 6), - VipsOption.Boolean("strip", true) + VipsOption.Boolean("strip", true), + VipsOption.Int("keep", k) ) } outputFile @@ -258,38 +251,6 @@ class ImageOperations(playPath: String) extends GridLogging { throw new UnsupportedCropOutputTypeException } } - - // 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) - } - } - - @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) - } - } - } object ImageOperations extends GridLogging { @@ -304,7 +265,7 @@ object ImageOperations extends GridLogging { var colourModel: Option[String] = None var colourModelInformation: Map[String, String] = Map.empty - val arena = Arena.ofConfined + implicit val arena: Arena = Arena.ofConfined try { val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) @@ -316,8 +277,9 @@ object ImageOperations extends GridLogging { )) maybeExifOrientationWhichTransformsImage = Seq(orientation).flatten.find(_.transformsImage()) + val interpretationRawValue = VipsHelper.image_get_interpretation(image.getUnsafeStructAddress) // TODO better way to go straight from int to enum? - val maybeInterpretation = VipsInterpretation.values().toSeq.find(_.getRawValue == VipsHelper.image_get_interpretation(image.getUnsafeStructAddress)) + val maybeInterpretation = VipsInterpretation.values().toSeq.find(_.getRawValue == interpretationRawValue) colourModel = maybeInterpretation match { case Some(VipsInterpretation.INTERPRETATION_B_W) => Some("Greyscale") case Some(VipsInterpretation.INTERPRETATION_CMYK) => Some("CMYK") @@ -329,11 +291,12 @@ object ImageOperations extends GridLogging { } colourModelInformation = Map { - "hasAlpha" -> image.hasAlpha.toString + "hasAlpha" -> hasAlpha(image).toString } } catch { case e: Exception => logger.error("Error during getImageInformation", e) + arena.close() throw e } arena.close() @@ -345,4 +308,19 @@ object ImageOperations extends GridLogging { } } + def hasAlpha(image: VImage)(implicit arena: Arena): Boolean = image.hasAlpha + + 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 + } } diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/ExifTool.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/ExifTool.scala deleted file mode 100644 index 88ea75dbdd..0000000000 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/ExifTool.scala +++ /dev/null @@ -1,36 +0,0 @@ -package com.gu.mediaservice.lib.imaging.im4jwrapper - -import java.util.concurrent.Executors - -import java.io.File -import scala.concurrent.{Future, ExecutionContext} -import org.im4java.core.{ETOperation, ExiftoolCmd} - - -object ExifTool { - private implicit val ctx: ExecutionContext = - ExecutionContext.fromExecutor(Executors.newFixedThreadPool(Config.imagingThreadPoolSize)) - - def tagSource(source: File): ETOperation = { - val op = new ETOperation() - op.addImage(source.getAbsolutePath) - op - } - - def setTags(ops: ETOperation)(tags: Map[String, String]): ETOperation = { - tags.foldLeft(ops) { case (ops, (key, value)) => - ops.setTags(s"$key=$value") - } - } - - def overwriteOriginal(ops: ETOperation): ETOperation = { - ops.overwrite_original() - ops - } - - def runExiftoolCmd(ops: ETOperation): Future[Unit] = { - // Set overwrite original to ensure temporary file deletion - overwriteOriginal(ops) - Future((new ExiftoolCmd).run(ops)) - } -} diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/ImageMagick.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/ImageMagick.scala index 302eb189f0..72a164730d 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/imaging/im4jwrapper/ImageMagick.scala +++ b/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 { diff --git a/common-lib/src/test/resources/lab8-with-alpha.tif b/common-lib/src/test/resources/lab8-with-alpha.tif new file mode 100644 index 0000000000..f63cc44b30 Binary files /dev/null and b/common-lib/src/test/resources/lab8-with-alpha.tif differ diff --git a/common-lib/src/test/resources/schaik.com_pngsuite/README.md b/common-lib/src/test/resources/schaik.com_pngsuite/README.md new file mode 100644 index 0000000000..3854ba52a7 --- /dev/null +++ b/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 +``` diff --git a/common-lib/src/test/resources/schaik.com_pngsuite/basn3p08.png b/common-lib/src/test/resources/schaik.com_pngsuite/basn3p08.png new file mode 100644 index 0000000000..0ddad07e5f Binary files /dev/null and b/common-lib/src/test/resources/schaik.com_pngsuite/basn3p08.png differ diff --git a/common-lib/src/test/resources/schaik.com_pngsuite/tbbn0g04.png b/common-lib/src/test/resources/schaik.com_pngsuite/tbbn0g04.png new file mode 100644 index 0000000000..39a7050d27 Binary files /dev/null and b/common-lib/src/test/resources/schaik.com_pngsuite/tbbn0g04.png differ diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/imaging/ImageOperationsTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/imaging/ImageOperationsTest.scala index acd94928ca..63cad689b6 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/imaging/ImageOperationsTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/imaging/ImageOperationsTest.scala @@ -1,21 +1,18 @@ package com.gu.mediaservice.lib.imaging +import app.photofox.vipsffm.jextract.VipsRaw import app.photofox.vipsffm.{VImage, Vips} -import app.photofox.vipsffm.Vips import com.gu.mediaservice.lib.BrowserViewableImage import com.gu.mediaservice.lib.logging.{LogMarker, MarkerMap} -import com.gu.mediaservice.model.{Bounds, Dimensions, ImageMetadata, Instance, Jpeg, Png, Tiff} -import org.scalatest.time.{Millis, Span} -import com.gu.mediaservice.model.{Dimensions, Instance, Tiff} +import com.gu.mediaservice.model._ import org.scalatest.concurrent.ScalaFutures import org.scalatest.funspec.AnyFunSpec import org.scalatest.matchers.should.Matchers import org.scalatest.time.{Millis, Span} import java.io.File +import java.lang.foreign.Arena import scala.concurrent.ExecutionContext.Implicits.global -import scala.concurrent.{Await, Future} -import scala.concurrent.duration.{Duration, SECONDS} // This test is disabled for now as it doesn't run on our CI environment, because GraphicsMagick is not present... class ImageOperationsTest extends AnyFunSpec with Matchers with ScalaFutures { @@ -25,6 +22,12 @@ class ImageOperationsTest extends AnyFunSpec with Matchers with ScalaFutures { implicit override val patienceConfig: PatienceConfig = PatienceConfig(timeout = Span(1000, Millis), interval = Span(25, Millis)) implicit val logMarker: LogMarker = MarkerMap() + private val metadata = ImageMetadata( + credit = Some("Tony McCrae"), + copyright = Some("Eel Pie Consulting Ltd"), + suppliersReference = Some("eelpie-123") + ) + describe("thumbnail") { it("should write thumbnail to output file") { val image = fileAt("IMG_4403.jpg") @@ -87,6 +90,118 @@ class ImageOperationsTest extends AnyFunSpec with Matchers with ScalaFutures { } } + describe("resize") { + it("should output resized image to file in chosen format") { + implicit val arena: Arena = Arena.ofShared() + val fullSizedImage = VImage.newFromFile(arena, fileAt("IMG_4403.jpg").getAbsolutePath) + val imageOperations = new ImageOperations("") + + val outputFile = new File("/Users/tony/Desktop/out5.jpg") + + val eventuallyResized = imageOperations.resizeImageVips(fullSizedImage, Dimensions(1000, 800), 95, outputFile, Jpeg) + + whenReady(eventuallyResized) { resized => + arena.close() + resized.isFile should be(true) + } + } + + it("render LAB colour spaces correctly in sRGB") { + implicit val arena: Arena = Arena.ofShared + val imageOperations = new ImageOperations("") + + val fullSizedImage = VImage.newFromFile(arena, fileAt("halfdome_LAB.tif").getAbsolutePath) + val outputFile = new File("/Users/tony/Desktop/out6.jpg") + + val eventuallyResized = imageOperations.resizeImageVips(fullSizedImage, Dimensions(800, 600), 95, outputFile, Jpeg) + + whenReady(eventuallyResized) { resized => + arena.close() + resized.isFile should be(true) + } + } + + it("render LAB colour spaces correctly as PNG") { + implicit val arena: Arena = Arena.ofShared + val imageOperations = new ImageOperations("") + + val fullSizedImage = VImage.newFromFile(arena, fileAt("halfdome_LAB.tif").getAbsolutePath) + val outputFile = new File("/Users/tony/Desktop/out7.png") + + val eventuallyResized = imageOperations.resizeImageVips(fullSizedImage, Dimensions(800, 600), 95, outputFile, Png) + + whenReady(eventuallyResized) { resized => + arena.close() + resized.isFile should be(true) + } + } + + it("render LAB 16 bit colour spaces correctly") { + implicit val arena: Arena = Arena.ofShared + val imageOperations = new ImageOperations("") + + val fullSizedImage = VImage.newFromFile(arena, fileAt("halfdome_LAB16.tif").getAbsolutePath) + val outputFile = new File("/Users/tony/Desktop/out8.jpg") + + val eventuallyResized = imageOperations.resizeImageVips(fullSizedImage, Dimensions(800, 600), 95, outputFile, Jpeg) + + whenReady(eventuallyResized) { resized => + arena.close() + resized.isFile should be(true) + } + } + + it("render PNG with alpha correctly") { + implicit val arena: Arena = Arena.ofShared + val imageOperations = new ImageOperations("") + + val image = fileAt("with-alpha.png") + val fullSizedImage = VImage.newFromFile(arena, image.getAbsolutePath) + val outputFile = new File("/Users/tony/Desktop/resized-png-with-alpha.png") + + val eventuallyResized = imageOperations.resizeImageVips(fullSizedImage, Dimensions(800, 600), 95, outputFile, Png) + + whenReady(eventuallyResized) { resized => + arena.close() + resized.isFile should be(true) + } + } + + it("render LAB TIFF with alpha correctly") { + implicit val arena: Arena = Arena.ofShared + val imageOperations = new ImageOperations("") + + val image = fileAt("lab8-with-alpha.tif") + val fullSizedImage = VImage.newFromFile(arena, image.getAbsolutePath) + val outputFile = new File("/Users/tony/Desktop/out13.jpg") + + val eventuallyResized = imageOperations.resizeImageVips(fullSizedImage, Dimensions(800, 600), 95, outputFile, Jpeg) + + whenReady(eventuallyResized) { resized => + arena.close() + resized.isFile should be(true) + } + } + } + + describe("alpha") { + it("should return false for RGB for a Jpeg with no alpha") { + implicit val arena: Arena = Arena.ofShared + val image = VImage.newFromFile(arena, fileAt("rgb-wo-profile.jpg").getAbsolutePath) + val hasAlpha = ImageOperations.hasAlpha(image) + arena.close() + hasAlpha should be(false) + } + + it("should return true for PNG with alpha") { + implicit val arena: Arena = Arena.ofShared + val image = VImage.newFromFile(arena, fileAt("with-alpha.png").getAbsolutePath) + val hasAlpha = ImageOperations.hasAlpha(image) + arena.close() + hasAlpha should be(true) + } + } + describe("identifyColourModel") { it("should return RGB for a JPG image with RGB image data and no embedded profile") { val image = fileAt("rgb-wo-profile.jpg") @@ -218,7 +333,91 @@ class ImageOperationsTest extends AnyFunSpec with Matchers with ScalaFutures { } } - // TODO: test cropImage and its conversions + describe("graphic detection") { + it("should return not graphic for true colour jpeg") { + val arena = Arena.ofConfined + val image = VImage.newFromFile(arena, fileAt("exif-orientated-no-rotation.jpg").getAbsolutePath) + ImageOperations.isGraphicVips(image)(arena) should be(false) + arena.close() + } + + it("should return is graphic for depth 2 tiff") { + val arena = Arena.ofConfined + val image = VImage.newFromFile(arena, fileAt("flower.tif").getAbsolutePath) + ImageOperations.isGraphicVips(image)(arena) should be(true) + arena.close() + } + + it("should return is graphic for depth 4 png with alpha") { + val arena = Arena.ofConfined + val image = VImage.newFromFile(arena, fileAt("schaik.com_pngsuite/tbbn0g04.png").getAbsolutePath) + ImageOperations.isGraphicVips(image)(arena) should be(true) + arena.close() + } + + it("should return is graphic for depth 8 indexed png") { + val arena = Arena.ofConfined + val image = VImage.newFromFile(arena, fileAt("schaik.com_pngsuite/basn3p08.png").getAbsolutePath) + ImageOperations.isGraphicVips(image)(arena) should be(true) + arena.close() + } + + } + + describe("cropping") { + val operations = new ImageOperations("") + + it("should create unscaled master crop to resize from full sized images") { + implicit val arena: Arena = Arena.ofConfined + //val fullsizedImage = fileAt("Lab 16bpc (7d0b7c7b8e890d7e5d369093aa437bd833e20f71).tiff") + val fullsizedImage = fileAt("IMG_4403.jpg") + val metadata = ImageMetadata() + + val masterCrop = operations.cropImageVips(fullsizedImage, Bounds(100, 100, 2000, 2400), metadata, None) + + val outputFile = new File("/Users/tony/Desktop/master.jpg") + operations.saveImageToFile(masterCrop, Jpeg, 95, outputFile, keep = Some(VipsRaw.VIPS_FOREIGN_KEEP_XMP)) + arena.close() + } + + it("should create unscaled master crop from CMYK full sized image") { + implicit val arena: Arena = Arena.ofConfined + val fullsizedImage = fileAt("CMYK-with-profile.jpg") + val metadata = ImageMetadata() + + val masterCrop = operations.cropImageVips(fullsizedImage, Bounds(100, 100, 2000, 2400), metadata, None) + + val outputFile = new File("/Users/tony/Desktop/master-from-cmyk.jpg") + operations.saveImageToFile(masterCrop, Jpeg, 95, outputFile, keep = Some(VipsRaw.VIPS_FOREIGN_KEEP_XMP)) + + arena.close() + } + + it("should create files foreach crop size") { + implicit val arena: Arena = Arena.ofShared() + val fullsizedImage = fileAt("CMYK-with-profile.jpg") + val metadata = ImageMetadata() + + val masterCrop = operations.cropImageVips(fullsizedImage, Bounds(100, 100, 3000, 2000), metadata, None) + val landscapeCropSizingWidths = Seq( + Dimensions(140, 100), + Dimensions(320, 200), + Dimensions(800, 600), + Dimensions(1000, 1200), + Dimensions(2000, 3000), + ) + implicit val i: Instance = Instance("id") + + val crops = operations.createCrops(masterCrop, landscapeCropSizingWidths.toList, "test-image-id", + Bounds(0, 0, 1000, 1200), + Jpeg, + new File("/Users/tony/tmp/crops"), + 75 + ) + + arena.close() + } + } def fileAt(resourcePath: String): File = { new File(getClass.getResource(s"/$resourcePath").toURI) diff --git a/container-images/jdk-vips/Dockerfile b/container-images/jdk-vips/Dockerfile index 090b1bd538..c5dde5a3f4 100644 --- a/container-images/jdk-vips/Dockerfile +++ b/container-images/jdk-vips/Dockerfile @@ -39,8 +39,6 @@ RUN rm /tmp/vips-8.18.2.tar.xz RUN rm -r /tmp/vips-8.18.2/ RUN apt -y --no-install-suggests install \ - pngquant \ - libimage-exiftool-perl \ libjemalloc-dev \ graphicsmagick \ graphicsmagick-imagemagick-compat diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index d73599a422..631d2ac172 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -1,13 +1,15 @@ package lib -import java.io.File -import com.gu.mediaservice.lib.metadata.FileMetadataHelper +import app.photofox.vipsffm.VImage +import app.photofox.vipsffm.jextract.VipsRaw import com.gu.mediaservice.lib.Files import com.gu.mediaservice.lib.aws.{S3, S3Bucket} import com.gu.mediaservice.lib.imaging.{ExportResult, ImageOperations} import com.gu.mediaservice.lib.logging.{GridLogging, LogMarker, Stopwatch} import com.gu.mediaservice.model._ +import java.io.File +import java.lang.foreign.Arena import scala.concurrent.{ExecutionContext, Future} import scala.util.Try @@ -15,77 +17,46 @@ case object InvalidImage extends Exception("Invalid image cannot be cropped") case object MissingMimeType extends Exception("Missing mimeType from source API") case object InvalidCropRequest extends Exception("Crop request invalid for image dimensions") -case class MasterCrop(sizing: Future[Asset], file: File, dimensions: Dimensions, aspectRatio: Float) +case class MasterCrop(image: VImage, dimensions: Dimensions, aspectRatio: Float) class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOperations, imageBucket: S3Bucket, s3: S3)(implicit ec: ExecutionContext) extends GridLogging { import Files._ - private val cropQuality = 75d - private val masterCropQuality = 95d + private val cropQuality = 75 + private val jpegMasterCropQuality = 95 // For PNGs, Magick considers "quality" parameter as effort spent on compression - 1 meaning none, 100 meaning max. // We don't overly care about output crop file sizes here, but prefer a fast output, so turn it right down. - private val pngCropQuality = 1d + private val pngMasterCropQuality = 1 // No effort spend compressing the PNG master - def outputFilename(source: SourceImage, bounds: Bounds, outputWidth: Int, fileType: MimeType, isMaster: Boolean = false)(implicit instance: Instance): String = { + def outputFilename(imageId: String, bounds: Bounds, outputWidth: Int, fileType: MimeType, isMaster: Boolean = false)(implicit instance: Instance): String = { val masterString: String = if (isMaster) "master/" else "" - instance.id + "/" + s"${source.id}/${Crop.getCropId(bounds)}/$masterString$outputWidth${fileType.fileExtension}" + instance.id + "/" + s"$imageId/${Crop.getCropId(bounds)}/$masterString$outputWidth${fileType.fileExtension}" } private def createMasterCrop( apiImage: SourceImage, sourceFile: File, crop: Crop, - mediaType: MimeType, - colourModel: Option[String], - orientationMetadata: Option[OrientationMetadata], - )(implicit logMarker: LogMarker, instance: Instance): Future[MasterCrop] = { + metadata: ImageMetadata, + orientationMetadata: Option[OrientationMetadata] + )(implicit logMarker: LogMarker, arena: Arena): MasterCrop = { - Stopwatch.async(s"creating master crop for ${apiImage.id}") { + Stopwatch(s"creating master crop for ${apiImage.id}") { val source = crop.specification - val metadata = apiImage.metadata - val iccColourSpace = FileMetadataHelper.normalisedIccColourSpace(apiImage.fileMetadata) - // pngs are always lossless, so quality only means effort spent compressing them. We don't - // care too much about filesize of master crops, so skip expensive compression to get faster cropping - val quality = if (mediaType == Png) pngCropQuality else masterCropQuality - - for { - strip <- imageOperations.cropImage( - sourceFile, apiImage.source.mimeType, source.bounds, quality, config.tempDir, - iccColourSpace, colourModel, mediaType, isTransformedFromSource = false, - orientationMetadata = orientationMetadata - ) - file: File <- imageOperations.appendMetadata(strip, metadata) - dimensions = Dimensions(source.bounds.width, source.bounds.height) - filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType, isMaster = true) - sizing = store.storeCropSizing(file, filename, mediaType, crop, dimensions) - dirtyAspect = source.bounds.width.toFloat / source.bounds.height - aspect = crop.specification.aspectRatio.flatMap(AspectRatio.clean).getOrElse(dirtyAspect) - } - yield MasterCrop(sizing, file, dimensions, aspect) - } - } - - private def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, cropType: MimeType)(implicit logMarker: LogMarker, instance: Instance): Future[List[Asset]] = { - val quality = if (cropType == Png) pngCropQuality else cropQuality - - Stopwatch.async(s"creating crops for ${apiImage.id}") { - Future.sequence(dimensionList.map { dimensions => - val cropLogMarker = logMarker ++ Map("crop-dimensions" -> s"${dimensions.width}x${dimensions.height}") - for { - file <- imageOperations.resizeImage(sourceFile, - apiImage.source.mimeType, - dimensions, - quality, - config.tempDir, - cropType)(cropLogMarker) - optimisedFile = imageOperations.optimiseImage(file, cropType)(cropLogMarker) - filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType) - sizing <- store.storeCropSizing(optimisedFile, filename, cropType, crop, dimensions)(cropLogMarker) - _ <- delete(file) - _ <- delete(optimisedFile) - } - yield sizing - }) + logger.info(logMarker, s"creating master crop for ${apiImage.id}") + val masterImage = imageOperations.cropImageVips( + sourceFile, + source.bounds, + metadata, + orientationMetadata = orientationMetadata + ) + + //file: File <- imageOperations.appendMetadata(strip, metadata) + val dimensions = Dimensions(source.bounds.width, source.bounds.height) + val dirtyAspect = source.bounds.width.toFloat / source.bounds.height + val aspect = crop.specification.aspectRatio.flatMap(AspectRatio.clean).getOrElse(dirtyAspect) + + MasterCrop(masterImage, dimensions, aspect) } } @@ -107,48 +78,98 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera } def makeExport(apiImage: SourceImage, crop: Crop)(implicit logMarker: LogMarker, instance: Instance): Future[ExportResult] = { - val source = crop.specification + val source = crop.specification val mimeType = apiImage.source.mimeType.getOrElse(throw MissingMimeType) val secureFile = apiImage.source.file - val colourType = apiImage.fileMetadata.colourModelInformation.getOrElse("colorType", "") - val hasAlpha = apiImage.fileMetadata.colourModelInformation.get("hasAlpha").flatMap(a => Try(a.toBoolean).toOption).getOrElse(true) - val cropType = Crops.cropType(mimeType, colourType, hasAlpha) val key = imageBucket.keyFromS3URL(secureFile) val secureUrl = s3.signUrlTony(imageBucket, key) - Stopwatch.async(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { - for { - sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) - colourModelAndInformation <- ImageOperations.getImageInformation(sourceFile) - colourModel = colourModelAndInformation._3 - masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, colourModel, apiImage.source.orientationMetadata) + //val eventualResult = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { + tempFileFromURL(secureUrl, "cropSource", "", config.tempDir).flatMap { sourceFile => + logger.info("Starting vips operations") + implicit val arena: Arena = Arena.ofShared() + val masterCrop = createMasterCrop(apiImage, sourceFile, crop, apiImage.metadata, apiImage.source.orientationMetadata) - outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions + val isGraphic = ImageOperations.isGraphicVips(masterCrop.image) + val hasAlpha = ImageOperations.hasAlpha(masterCrop.image) + val cropType = Crops.cropType(mimeType, isGraphic = isGraphic, hasAlpha = hasAlpha) - sizes <- createCrops(masterCrop.file, outputDims, apiImage, crop, cropType) - masterSize <- masterCrop.sizing - _ <- Future.sequence(List(masterCrop.file, sourceFile).map(delete)) + // pngs are always lossless, so quality only means effort spent compressing them. We don't + // care too much about filesize of master crops, so skip expensive compression to get faster cropping + val masterQuality = if (mimeType == Png) pngMasterCropQuality else jpegMasterCropQuality + + // High quality rendering with minimal compression which will be used as the CDN resizer origin + logger.info("Requesting master file save") + val eventualMasterSaved = Future { + val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this + imageOperations.saveImageToFile(masterCrop.image, cropType, masterQuality, masterCropFile, keep = Some(VipsRaw.VIPS_FOREIGN_KEEP_XMP)) + masterCropFile + } + + // Static crops; higher compression + logger.info("Requesting resize file saves") + val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions + val eventualResizes = imageOperations.createCrops(masterCrop.image, outputDims, apiImage.id, crop.specification.bounds, cropType, config.tempDir, cropQuality) + + // Store assets after master and resize file generation completes; to avoid sending a partial set on failure + // Delete the stored files and return the export result + eventualMasterSaved.flatMap { masterCropFile => + eventualResizes.flatMap { resizes => + // All vips operations have completed; we can close the arena + arena.close() + logger.info("Finished vips operations") + + val eventuallyStoredMasterAsset = store.storeCropSizing(masterCropFile, outputFilename(apiImage.id, source.bounds, masterCrop.dimensions.width, cropType, isMaster = true), + cropType, crop, masterCrop.dimensions).map { masterAsset => + logger.info("Master Crop stored") + delete(masterCropFile) + masterAsset + } + + val eventualStoredResizeAssets = Future.sequence(resizes.map { resize: (File, String, Dimensions) => + val file = resize._1 + val filename = resize._2 + val dimensions = resize._3 + logger.info(s"Storing crop for: $file, $filename, $cropType") + for { + resizedAsset: Asset <- store.storeCropSizing(file, filename, cropType, crop, dimensions) + _ <- delete(file) + } + yield resizedAsset + }) + + for { + masterAsset: Asset <- eventuallyStoredMasterAsset + resizedAssets: Seq[Asset] <- eventualStoredResizeAssets + } yield { + logger.info("Store assets completed") + delete(sourceFile) + ExportResult(apiImage.id, masterAsset, resizedAssets.toList) + } + } } - yield ExportResult(apiImage.id, masterSize, sizes) } } } -object Crops { +object Crops extends GridLogging { /** * The aim here is to decide whether the crops should be JPEG or PNGs depending on a predicted quality/size trade-off. * - If the image has transparency then it should always be a PNG as the transparency is not available in JPEG * - If the image is not true colour then we assume it is a graphic that should be retained as a PNG */ - def cropType(mediaType: MimeType, colourType: String, hasAlpha: Boolean): MimeType = { - val isGraphic = !colourType.matches("True[ ]?Color.*") + def cropType(mediaType: MimeType, isGraphic: Boolean, hasAlpha: Boolean): MimeType = { val outputAsPng = hasAlpha || isGraphic - mediaType match { - case Png | Tiff if outputAsPng => Png + val decision = mediaType match { + case Png if outputAsPng => Png + case Tiff if outputAsPng => Png case _ => Jpeg } + + logger.info(s"Choose crop type for $mediaType, $isGraphic, $hasAlpha: " + decision) + decision } } diff --git a/cropper/test/lib/CropsTest.scala b/cropper/test/lib/CropsTest.scala index 83fdeb8590..cf66d76899 100644 --- a/cropper/test/lib/CropsTest.scala +++ b/cropper/test/lib/CropsTest.scala @@ -15,36 +15,36 @@ class CropsTest extends AnyFunSpec with Matchers with MockitoSugar { private implicit val instance: Instance = Instance(id = "an-instance") it("should return JPEG when the input type is a JPEG") { - Crops.cropType(Jpeg, "True Color", hasAlpha = false) shouldBe Jpeg - Crops.cropType(Jpeg, "Monkey", hasAlpha = false) shouldBe Jpeg + Crops.cropType(Jpeg, isGraphic = false, hasAlpha = false) shouldBe Jpeg + Crops.cropType(Jpeg, isGraphic = true, hasAlpha = false) shouldBe Jpeg } it("should return PNG when the input type is PNG and it has alpha") { - Crops.cropType(Png, "Monkey", hasAlpha = true) shouldBe Png + Crops.cropType(Png, isGraphic = true, hasAlpha = true) shouldBe Png } it("should return PNG when the input type is PNG and it has alpha even if it is True Color") { - Crops.cropType(Png, "True Color", hasAlpha = true) shouldBe Png + Crops.cropType(Png, isGraphic = false, hasAlpha = true) shouldBe Png } it("should return PNG when the input type is PNG and it is NOT true color (a graphic)") { - Crops.cropType(Png, "Monkey", hasAlpha = false) shouldBe Png + Crops.cropType(Png, isGraphic = true, hasAlpha = false) shouldBe Png } it("should return JPEG when the input type is PNG and it is true color") { - Crops.cropType(Png, "True Color", hasAlpha = false) shouldBe Jpeg + Crops.cropType(Png, isGraphic = false, hasAlpha = false) shouldBe Jpeg } it("should return PNG when the input type is TIFF and it has alpha") { - Crops.cropType(Tiff, "Monkey", hasAlpha = true) shouldBe Png + Crops.cropType(Tiff, isGraphic = false, hasAlpha = true) shouldBe Png } it("should return PNG when the input type is TIFF and it doesn't have alpha or is true color") { - Crops.cropType(Tiff, "Monkey", hasAlpha = false) shouldBe Png + Crops.cropType(Tiff, isGraphic = true, hasAlpha = false) shouldBe Png } it("should return JPEG when the input type is TIFF and it doesn't have alpha and it is true color") { - Crops.cropType(Tiff, "TrueColor", hasAlpha = false) shouldBe Jpeg + Crops.cropType(Tiff, isGraphic = false, hasAlpha = false) shouldBe Jpeg } private val config = { @@ -65,22 +65,22 @@ class CropsTest extends AnyFunSpec with Matchers with MockitoSugar { it("should should construct a correct address for a master jpg") { val outputFilename = new Crops(config, store, imageOperations, imageBucket, s3) - .outputFilename(source, bounds, outputWidth, Jpeg, isMaster = true) + .outputFilename(source.id, bounds, outputWidth, Jpeg, isMaster = true) outputFilename shouldBe "an-instance/test/10_20_30_40/master/1234.jpg" } it("should should construct a correct address for a non-master jpg") { val outputFilename = new Crops(config, store, imageOperations, imageBucket, s3) - .outputFilename(source, bounds, outputWidth, Jpeg) + .outputFilename(source.id, bounds, outputWidth, Jpeg) outputFilename shouldBe "an-instance/test/10_20_30_40/1234.jpg" } it("should should construct a correct address for a non-master tiff") { val outputFilename = new Crops(config, store, imageOperations, imageBucket, s3) - .outputFilename(source, bounds, outputWidth, Tiff) + .outputFilename(source.id, bounds, outputWidth, Tiff) outputFilename shouldBe "an-instance/test/10_20_30_40/1234.tiff" } it("should should construct a correct address for a non-master png") { val outputFilename = new Crops(config, store, imageOperations, imageBucket, s3) - .outputFilename(source, bounds, outputWidth, Png) + .outputFilename(source.id, bounds, outputWidth, Png) outputFilename shouldBe "an-instance/test/10_20_30_40/1234.png" } }