diff --git a/build.sbt b/build.sbt index adec9825e9..48aa92e10f 100644 --- a/build.sbt +++ b/build.sbt @@ -107,6 +107,7 @@ lazy val commonLib = project("common-lib").settings( "com.gu" %% "thrift-serializer" % "5.0.2", "org.scalaz" %% "scalaz-core" % "7.3.8", "org.im4java" % "im4java" % "1.4.0", + "app.photofox.vips-ffm" % "vips-ffm-core" % "1.9.6", "com.gu" % "kinesis-logback-appender" % "1.4.4", "net.logstash.logback" % "logstash-logback-encoder" % "5.0", logback, // play-logback; needed when running the scripts @@ -255,7 +256,7 @@ def playProject(projectName: String, port: Int, path: Option[String] = None): Pr .enablePlugins(PlayScala, BuildInfoPlugin, DockerPlugin) .dependsOn(restLib) .settings(commonSettings ++ buildInfo ++ Seq( - dockerBaseImage := "eclipse-temurin:11", + dockerBaseImage := "eclipse-temurin:25", dockerExposedPorts := Seq(port), playDefaultPort := port, @@ -278,7 +279,7 @@ def playImageLoaderProject(projectName: String, port: Int, path: Option[String] .enablePlugins(PlayScala, BuildInfoPlugin, DockerPlugin) .dependsOn(restLib) .settings(commonSettings ++ buildInfo ++ Seq( - dockerBaseImage := "eu.gcr.io/grid-301122/jdk-vips:25-8.18", + dockerBaseImage := "eu.gcr.io/grid-301122/jdk-vips:25-8.18.2", dockerExposedPorts := Seq(port), dockerCommands ++= Seq( Cmd("ENV", "LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so") @@ -298,6 +299,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" ))) } diff --git a/cloudbuild.yaml b/cloudbuild.yaml index 596bb770d9..afff352688 100644 --- a/cloudbuild.yaml +++ b/cloudbuild.yaml @@ -14,9 +14,10 @@ steps: dir: 'kahuna' args: [ 'run', 'dist' ] - - name: 'gcr.io/$PROJECT_ID/scala-sbt:1.6.2-jdk-11' + - name: 'gcr.io/$PROJECT_ID/scala-sbt:1.11.7-jdk-25' args: ['docker:publishLocal'] - + env: + - 'DOCKER_API_VERSION=1.41' - name: 'gcr.io/cloud-builders/docker' args: ['tag', 'auth:0.1', 'eu.gcr.io/$PROJECT_ID/auth:$BRANCH_NAME'] - name: 'gcr.io/cloud-builders/docker' 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 d07fe814fa..6209187f12 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,15 +1,17 @@ package com.gu.mediaservice.lib.imaging -import java.io._ -import org.im4java.core.IMOperation +import app.photofox.vipsffm.enums.{VipsIntent, VipsInterpretation} +import app.photofox.vipsffm.{VImage, VipsHelper, VipsOption} +import com.gu.mediaservice.lib.BrowserViewableImage 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 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 java.io._ +import java.lang.foreign.Arena import scala.concurrent.{ExecutionContext, Future} import scala.sys.process._ @@ -158,77 +160,103 @@ class ImageOperations(playPath: String) extends GridLogging { throw new UnsupportedCropOutputTypeException } - val thumbUnsharpRadius = 0.5d - val thumbUnsharpSigma = 0.5d - val thumbUnsharpAmount = 0.8d val interlacedHow = "Line" val backgroundColour = "#333333" /** - * Given a source file containing an image (the 'browser viewable' file), - * construct a thumbnail file in the provided temp directory, and return - * the file with metadata about it. - * @param browserViewableImage - * @param width Desired with of thumbnail - * @param qual Desired quality of thumbnail - * @param outputFile 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(browserViewableImage: BrowserViewableImage, - width: Int, - qual: Double = 100d, - outputFile: File, - iccColourSpace: Option[String], - colourModel: Option[String], - orientationMetadata: Option[OrientationMetadata] - )(implicit logMarker: LogMarker): Future[(File, MimeType)] = { - val stopwatch = Stopwatch.start + * Given a source file containing an image (the 'browser viewable' file), + * construct a thumbnail file in the provided temp directory, and return + * the file with metadata about it. + * + * @param browserViewableImage + * @param width Desired with of thumbnail + * @param qual Desired quality of thumbnail + * @param outputFile Location to create thumbnail file + * @param orientationMetadata OrientationMetadata for rotation correction + * @return The file created and the mimetype of the content of that file and it's dimensions, in a future. + */ + def createThumbnailVips(browserViewableImage: BrowserViewableImage, + width: Int, + qual: Double = 100d, + outputFile: File, + orientationMetadata: Option[OrientationMetadata] + )(implicit logMarker: LogMarker): Future[(File, MimeType, Option[Dimensions])] = { + Future { + val stopwatch = Stopwatch.start + val arena = Arena.ofConfined + + try { + val thumbnail = VImage.thumbnail(arena, browserViewableImage.file.getAbsolutePath, width, + VipsOption.Boolean("auto-rotate", false), + VipsOption.Enum("intent", VipsIntent.INTENT_PERCEPTUAL), + VipsOption.String("export-profile", "srgb") + ) + val rotated = orientationMetadata.map(_.orientationCorrection()).map { angle => + logger.info("Rotating thumbnail: " + angle) + thumbnail.rotate(angle) + }.getOrElse { + thumbnail + } + logger.info("Created thumbnail: " + rotated.getWidth + "x" + rotated.getHeight) + saveImageToFile(rotated, Jpeg, qual.toInt, outputFile) - val cropSource = addImage(browserViewableImage.file) - val orientated = orient(cropSource, orientationMetadata) - val thumbnailed = thumbnail(orientated)(width) - val corrected = correctColour(thumbnailed)(iccColourSpace, colourModel, browserViewableImage.isTransformedFromSource) - val converted = applyOutputProfile(corrected, optimised = true) - val stripped = stripMeta(converted) - val profiled = applyOutputProfile(stripped, optimised = true) - val withBackground = setBackgroundColour(profiled)(backgroundColour) - val flattened = flatten(withBackground) - val unsharpened = unsharp(flattened)(thumbUnsharpRadius, thumbUnsharpSigma, thumbUnsharpAmount) - val qualified = quality(unsharpened)(qual) - val interlaced = interlace(qualified)(interlacedHow) - val addOutput = {file:File => addDestImage(interlaced)(file)} - for { - _ <- runConvertCmd(addOutput(outputFile), useImageMagick = browserViewableImage.mimeType == Tiff) - _ = logger.info(addLogMarkers(stopwatch.elapsed), "Finished creating thumbnail") - } yield (outputFile, thumbMimeType) + val thumbDimensions = Some(Dimensions(rotated.getWidth, rotated.getHeight)) + arena.close() + + logger.info(addLogMarkers(stopwatch.elapsed), "Finished creating thumbnail") + (outputFile, thumbMimeType, thumbDimensions) + + } catch { + case e: Throwable => + arena.close() + throw e + } + + }.recoverWith { + case e: Throwable => + logger.error("Error creating thumbnail", e) + Future.failed(e) + } } - /** - * 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)(implicit logMarker: LogMarker): Future[(File, MimeType)] = { - val stopwatch = Stopwatch.start - for { - // png suffix is used by imagemagick to infer the required type - outputFile <- createTempFile(s"transformed-", optimisedMimeType.fileExtension, tempDir) - transformSource = addImage(sourceFile) - converted = applyOutputProfile(transformSource, optimised = true) - stripped = stripMeta(converted) - profiled = applyOutputProfile(stripped, optimised = true) - depthAdjusted = depth(profiled)(8) - addOutput = addDestImage(depthAdjusted)(outputFile) - _ <- runConvertCmd(addOutput, useImageMagick = sourceMimeType.contains(Tiff)) - _ <- checkForOutputFileChange(outputFile) - _ = logger.info(addLogMarkers(stopwatch.elapsed), "Finished creating browser-viewable image") - } yield (outputFile, optimisedMimeType) + 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) + mimeType match { + case Jpeg => + image.jpegsave(outputFile.getAbsolutePath, + VipsOption.Int("Q", qual.toInt), + //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 => + // We are allowed to quantise PNG crops but not the master + if (quantise) { + image.pngsave(outputFile.getAbsolutePath, + VipsOption.Boolean("palette", true), + VipsOption.Int("Q", qual.toInt), + VipsOption.Int("effort", 1), + //VipsOption.Int("compression", 6), + VipsOption.Int("bitdepth", 8), + VipsOption.Boolean("strip", true) + ) + } else { + image.pngsave(outputFile.getAbsolutePath, + //VipsOption.Int("compression", 6), + VipsOption.Boolean("strip", true) + ) + } + outputFile + + case _ => + logger.error(s"Save to $mimeType is not supported.") + throw new UnsupportedCropOutputTypeException + } } // When a layered tiff is unpacked, the temp file (blah.something) is moved @@ -264,65 +292,57 @@ class ImageOperations(playPath: String) extends GridLogging { } -object ImageOperations { +object ImageOperations extends GridLogging { val thumbMimeType = Jpeg val optimisedMimeType = Png - def identifyColourModel(sourceFile: File, mimeType: MimeType)(implicit ec: ExecutionContext, logMarker: LogMarker): Future[Option[String]] = { - // TODO: use mimeType to lookup other properties once we support other formats - mimeType match { - case Jpeg => - val source = addImage(sourceFile) - val formatter = format(source)("%[JPEG-Colorspace-Name]") - - for { - output <- runIdentifyCmd(formatter, false) - colourModel = output.headOption - } yield colourModel match { - case Some("GRAYSCALE") => Some("Greyscale") - case Some("CMYK") => Some("CMYK") - case _ => Some("RGB") - } - case Tiff => - val op = new IMOperation() - val formatter = format(op)("%[colorspace]") - val withSource = addDestImage(formatter)(sourceFile) - - for { - output <- runIdentifyCmd(withSource, true) - colourModel = output.headOption - } yield colourModel match { - case Some("sRGB") => Some("RGB") - case Some("Gray") => Some("Greyscale") - case Some("CIELab") => Some("LAB") - // IM returns doubles for TIFFs with transparency… - case Some("sRGBsRGB") => Some("RGB") - case Some("GrayGray") => Some("Greyscale") - case Some("CIELabCIELab") => Some("LAB") - case Some("CMYKCMYK") => Some("CMYK") - // …and triples for TIFFs with transparency and alpha channel(s). I think. - case Some("sRGBsRGBsRGB") => Some("RGB") - case Some("GrayGrayGray") => Some("Greyscale") - case Some("CIELabCIELabCIELab") => Some("LAB") - case Some("CMYKCMYKCMYK") => Some("CMYK") - case _ => colourModel + def getImageInformation(sourceFile: File)(implicit ec: ExecutionContext, logMarker: LogMarker): Future[(Option[Dimensions], Option[OrientationMetadata], Option[String], Map[String, String])] = { + val stopwatch = Stopwatch.start + Future { + var dimensions: Option[Dimensions] = None + var maybeExifOrientationWhichTransformsImage: Option[OrientationMetadata] = None + var colourModel: Option[String] = None + var colourModelInformation: Map[String, String] = Map.empty + + val arena = Arena.ofConfined + try { + val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) + + dimensions = Some(Dimensions(width = image.getWidth, height = image.getHeight)) + + val exifOrientation = VipsHelper.image_get_orientation(image.getUnsafeStructAddress) + val orientation = Some(OrientationMetadata( + exifOrientation = Some(exifOrientation) + )) + maybeExifOrientationWhichTransformsImage = Seq(orientation).flatten.find(_.transformsImage()) + + // TODO better way to go straight from int to enum? + val maybeInterpretation = VipsInterpretation.values().toSeq.find(_.getRawValue == VipsHelper.image_get_interpretation(image.getUnsafeStructAddress)) + colourModel = maybeInterpretation match { + case Some(VipsInterpretation.INTERPRETATION_B_W) => Some("Greyscale") + case Some(VipsInterpretation.INTERPRETATION_CMYK) => Some("CMYK") + case Some(VipsInterpretation.INTERPRETATION_LAB) => Some("LAB") + case Some(VipsInterpretation.INTERPRETATION_LABS) => Some("LAB") + case Some(VipsInterpretation.INTERPRETATION_RGB16) => Some("RGB") + case Some(VipsInterpretation.INTERPRETATION_sRGB) => Some("RGB") + case _ => None } - case Png => - val op = new IMOperation() - val formatter = format(op)("%[colorspace]") - val withSource = addDestImage(formatter)(sourceFile) - - for { - output <- runIdentifyCmd(withSource, true) - colourModel = output.headOption - } yield colourModel match { - case Some("sRGB") => Some("RGB") - case Some("Gray") => Some("Greyscale") - case _ => Some("RGB") + + colourModelInformation = Map { + "hasAlpha" -> image.hasAlpha.toString } - case _ => - // assume that the colour model is RGB for other image types - Future.successful(Some("RGB")) + } catch { + case e: Exception => + logger.error("Error during getImageInformation", e) + throw e + } + arena.close() + + (dimensions, maybeExifOrientationWhichTransformsImage, colourModel, colourModelInformation) + }.map { result => + logger.info(addLogMarkers(stopwatch.elapsed), "Finished getImageInformation") + result } } + } diff --git a/common-lib/src/test/resources/CMYK-with-profile.jpg b/common-lib/src/test/resources/CMYK-with-profile.jpg new file mode 100644 index 0000000000..6c78dff467 Binary files /dev/null and b/common-lib/src/test/resources/CMYK-with-profile.jpg differ diff --git a/common-lib/src/test/resources/IMG_4403.JPG b/common-lib/src/test/resources/IMG_4403.JPG new file mode 100755 index 0000000000..da8ed3230d Binary files /dev/null and b/common-lib/src/test/resources/IMG_4403.JPG differ diff --git a/rest-lib/src/test/resources/cmyk.jpg b/common-lib/src/test/resources/cmyk.jpg similarity index 100% rename from rest-lib/src/test/resources/cmyk.jpg rename to common-lib/src/test/resources/cmyk.jpg diff --git a/common-lib/src/test/resources/cs-black-000.png b/common-lib/src/test/resources/cs-black-000.png new file mode 100644 index 0000000000..01f409774a Binary files /dev/null and b/common-lib/src/test/resources/cs-black-000.png differ diff --git a/common-lib/src/test/resources/exif-orientated-no-rotation.jpg b/common-lib/src/test/resources/exif-orientated-no-rotation.jpg new file mode 100755 index 0000000000..0f37a7c3db Binary files /dev/null and b/common-lib/src/test/resources/exif-orientated-no-rotation.jpg differ diff --git a/common-lib/src/test/resources/exif-orientated.jpg b/common-lib/src/test/resources/exif-orientated.jpg new file mode 100755 index 0000000000..d7e4506717 Binary files /dev/null and b/common-lib/src/test/resources/exif-orientated.jpg differ diff --git a/common-lib/src/test/resources/flower.tif b/common-lib/src/test/resources/flower.tif new file mode 100644 index 0000000000..2278aff182 Binary files /dev/null and b/common-lib/src/test/resources/flower.tif differ diff --git a/rest-lib/src/test/resources/grayscale-with-profile.jpg b/common-lib/src/test/resources/grayscale-with-profile.jpg similarity index 100% rename from rest-lib/src/test/resources/grayscale-with-profile.jpg rename to common-lib/src/test/resources/grayscale-with-profile.jpg diff --git a/rest-lib/src/test/resources/grayscale-wo-profile.jpg b/common-lib/src/test/resources/grayscale-wo-profile.jpg similarity index 100% rename from rest-lib/src/test/resources/grayscale-wo-profile.jpg rename to common-lib/src/test/resources/grayscale-wo-profile.jpg diff --git a/common-lib/src/test/resources/halfdome_LAB.tif b/common-lib/src/test/resources/halfdome_LAB.tif new file mode 100644 index 0000000000..564562e5ea Binary files /dev/null and b/common-lib/src/test/resources/halfdome_LAB.tif differ diff --git a/common-lib/src/test/resources/halfdome_LAB16.tif b/common-lib/src/test/resources/halfdome_LAB16.tif new file mode 100644 index 0000000000..0b54c644b3 Binary files /dev/null and b/common-lib/src/test/resources/halfdome_LAB16.tif differ diff --git a/rest-lib/src/test/resources/rgb-with-cmyk-profile.jpg b/common-lib/src/test/resources/rgb-with-cmyk-profile.jpg similarity index 100% rename from rest-lib/src/test/resources/rgb-with-cmyk-profile.jpg rename to common-lib/src/test/resources/rgb-with-cmyk-profile.jpg diff --git a/rest-lib/src/test/resources/rgb-with-rgb-profile.jpg b/common-lib/src/test/resources/rgb-with-rgb-profile.jpg similarity index 100% rename from rest-lib/src/test/resources/rgb-with-rgb-profile.jpg rename to common-lib/src/test/resources/rgb-with-rgb-profile.jpg diff --git a/rest-lib/src/test/resources/rgb-wo-profile.jpg b/common-lib/src/test/resources/rgb-wo-profile.jpg similarity index 100% rename from rest-lib/src/test/resources/rgb-wo-profile.jpg rename to common-lib/src/test/resources/rgb-wo-profile.jpg diff --git a/common-lib/src/test/resources/schaik.com_pngsuite/basi2c16.png b/common-lib/src/test/resources/schaik.com_pngsuite/basi2c16.png new file mode 100644 index 0000000000..cd7e50f914 Binary files /dev/null and b/common-lib/src/test/resources/schaik.com_pngsuite/basi2c16.png differ diff --git a/common-lib/src/test/resources/schaik.com_pngsuite/basn0g08.png b/common-lib/src/test/resources/schaik.com_pngsuite/basn0g08.png new file mode 100644 index 0000000000..23c82379a2 Binary files /dev/null and b/common-lib/src/test/resources/schaik.com_pngsuite/basn0g08.png differ diff --git a/common-lib/src/test/resources/with-alpha.png b/common-lib/src/test/resources/with-alpha.png new file mode 100644 index 0000000000..2e765a1ba2 Binary files /dev/null and b/common-lib/src/test/resources/with-alpha.png differ diff --git a/common-lib/src/test/resources/with-alpha.tif b/common-lib/src/test/resources/with-alpha.tif new file mode 100644 index 0000000000..a4efd9cc37 Binary files /dev/null and b/common-lib/src/test/resources/with-alpha.tif 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 8f78918f2d..acd94928ca 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,62 +1,219 @@ package com.gu.mediaservice.lib.imaging +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 java.io.File -import com.gu.mediaservice.model.Jpeg +import com.gu.mediaservice.model.{Bounds, Dimensions, ImageMetadata, Instance, Jpeg, Png, Tiff} import org.scalatest.time.{Millis, Span} -import org.scalatest.Ignore +import com.gu.mediaservice.model.{Dimensions, Instance, Tiff} 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 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... -@Ignore class ImageOperationsTest extends AnyFunSpec with Matchers with ScalaFutures { + Vips.init() + implicit override val patienceConfig: PatienceConfig = PatienceConfig(timeout = Span(1000, Millis), interval = Span(25, Millis)) implicit val logMarker: LogMarker = MarkerMap() + describe("thumbnail") { + it("should write thumbnail to output file") { + val image = fileAt("IMG_4403.jpg") + + val outputFile = new File("/Users/tony/Desktop/thumbnail.jpg") + val browserViewableImageImage = BrowserViewableImage("TODO", image, Tiff, Map.empty, false, Instance("TODO")) + + val eventualThumbnail = new ImageOperations("").createThumbnailVips(browserViewableImageImage, 240, 95, outputFile, None) + whenReady(eventualThumbnail) { r => + r._1.isFile should be(true) + } + } + + it("render LAB colour spaces correctly in sRGB") { + val image = fileAt("halfdome_LAB.tif") + + val outputFile = new File("/Users/tony/Desktop/out2.jpg") + val browserViewableImageImage = BrowserViewableImage("TODO", image, Tiff, Map.empty, false, Instance("TODO")) + + val eventualThumbnail = new ImageOperations("").createThumbnailVips(browserViewableImageImage, 1000, 95, outputFile, None) + whenReady(eventualThumbnail) { r => + r._1.isFile should be(true) + } + } + + it("render LAB 16 bits colour spaces correctly in 8 bit sRGB") { + val image = fileAt("halfdome_LAB16.tif") + + val outputFile = new File("/Users/tony/Desktop/out3.jpg") + val browserViewableImageImage = BrowserViewableImage("TODO", image, Tiff, Map.empty, false, Instance("TODO")) + + val eventualThumbnail = new ImageOperations("").createThumbnailVips(browserViewableImageImage, 1000, 95, outputFile, None) + whenReady(eventualThumbnail) { r => + r._1.isFile should be(true) + } + } + + it("render PNG with alpha correctly") { + val image = fileAt("with-alpha.png") + + val outputFile = new File("/Users/tony/Desktop/thumbnail-png-with-alpha.jpg") + val browserViewableImageImage = BrowserViewableImage("TODO", image, Tiff, Map.empty, false, Instance("TODO")) + + val eventualThumbnail = new ImageOperations("").createThumbnailVips(browserViewableImageImage, 1000, 95, outputFile, None) + whenReady(eventualThumbnail) { r => + r._1.isFile should be(true) + } + } + + it("render TIF with alpha correctly") { + val image = fileAt("with-alpha.tif") + + val outputFile = new File("/Users/tony/Desktop/thumbnail-tif-with-alpha.jpg") + val browserViewableImageImage = BrowserViewableImage("TODO", image, Tiff, Map.empty, false, Instance("TODO")) + + val eventualThumbnail = new ImageOperations("").createThumbnailVips(browserViewableImageImage, 1000, 95, outputFile, None) + whenReady(eventualThumbnail) { r => + r._1.isFile 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") - val colourModelFuture = ImageOperations.identifyColourModel(image, Jpeg) + val colourModelFuture = ImageOperations.getImageInformation(image) whenReady(colourModelFuture) { colourModel => - colourModel should be (Some("RGB")) + colourModel._3 should be(Some("RGB")) } } it("should return RGB for a JPG image with RGB image data and an RGB embedded profile") { val image = fileAt("rgb-with-rgb-profile.jpg") - val colourModelFuture = ImageOperations.identifyColourModel(image, Jpeg) + val colourModelFuture = ImageOperations.getImageInformation(image) whenReady(colourModelFuture) { colourModel => - colourModel should be (Some("RGB")) + colourModel._3 should be(Some("RGB")) + } + } + + it("should return RGB for a PNG image with RGB image data and an embedded profile") { + val image = fileAt("cs-black-000.png") + val colourModelFuture = ImageOperations.getImageInformation(image) + whenReady(colourModelFuture) { colourModel => + colourModel._3 should be(Some("RGB")) } } it("should return RGB for a JPG image with RGB image data and an incorrect CMYK embedded profile") { val image = fileAt("rgb-with-cmyk-profile.jpg") - val colourModelFuture = ImageOperations.identifyColourModel(image, Jpeg) + val colourModelFuture = ImageOperations.getImageInformation(image) whenReady(colourModelFuture) { colourModel => - colourModel should be (Some("RGB")) + colourModel._3 should be(Some("RGB")) } } it("should return CMYK for a JPG image with CMYK image data") { val image = fileAt("cmyk.jpg") - val colourModelFuture = ImageOperations.identifyColourModel(image, Jpeg) + val colourModelFuture = ImageOperations.getImageInformation(image) whenReady(colourModelFuture) { colourModel => - colourModel should be (Some("CMYK")) + colourModel._3 should be(Some("CMYK")) } } it("should return Greyscale for a JPG image with greyscale image data and no embedded profile") { val image = fileAt("grayscale-wo-profile.jpg") - val colourModelFuture = ImageOperations.identifyColourModel(image, Jpeg) + val colourModelFuture = ImageOperations.getImageInformation(image) + whenReady(colourModelFuture) { colourModel => + colourModel._3 should be(Some("Greyscale")) + } + } + + it("should return RGB for a PNG image with 16 bit RGB image data") { + val image = fileAt("schaik.com_pngsuite/basi2c16.png") + val colourModelFuture = ImageOperations.getImageInformation(image) + whenReady(colourModelFuture) { colourModel => + colourModel._3 should be(Some("RGB")) + } + } + + it("should return LAB for a TIFF image with LAB16 image data") { + val image = fileAt("halfdome_LAB16.tif") + val colourModelFuture = ImageOperations.getImageInformation(image) whenReady(colourModelFuture) { colourModel => - colourModel should be (Some("Greyscale")) + colourModel._3 should be(Some("LAB")) + } + } + + it("should return CMYK for a TIFF image with CMYK image data") { + val image = fileAt("CMYK-with-profile.jpg") + val colourModelFuture = ImageOperations.getImageInformation(image) + whenReady(colourModelFuture) { colourModel => + colourModel._3 should be(Some("CMYK")) + } + } + } + + describe("dimensions") { + it("should return dimensions of horizontal image") { + val inputFile = fileAt("exif-orientated-no-rotation.jpg") + val dimsFuture = ImageOperations.getImageInformation(inputFile) + whenReady(dimsFuture) { dims => + dims._1.get shouldBe new Dimensions(3456, 2304) + } + } + + it("should return uncorrected dimensions for exif oriented images") { + val inputFile = fileAt("exif-orientated.jpg") + val dimsFuture = ImageOperations.getImageInformation(inputFile) + whenReady(dimsFuture) { dims => + dims._1.get shouldBe new Dimensions(3456, 2304) + } + } + + it("should read the correct dimensions for a tiff image") { + val inputFile = fileAt("flower.tif") + val dimsFuture = ImageOperations.getImageInformation(inputFile) + whenReady(dimsFuture) { dimOpt => + dimOpt._1 should be(Symbol("defined")) + dimOpt._1.get.width should be(73) + dimOpt._1.get.height should be(43) + } + } + + it("should read the correct dimensions for a png image") { + val inputFile = fileAt("schaik.com_pngsuite/basn0g08.png") + val dimsFuture = ImageOperations.getImageInformation(inputFile) + whenReady(dimsFuture) { dimOpt => + dimOpt._1 should be(Symbol("defined")) + dimOpt._1.get.width should be(32) + dimOpt._1.get.height should be(32) + } + } + } + + describe("orientation") { + it("should capture exif orientation tag from JPG images") { + val image = fileAt("exif-orientated.jpg") + val orientationFuture = ImageOperations.getImageInformation(image) + whenReady(orientationFuture) { orientationOpt => + orientationOpt._2 should be(defined) + orientationOpt._2.get.exifOrientation should be(Some(6)) + } + } + + it("should ignore 0 degree exif orientation tag as it has no material effect") { + val image = fileAt("exif-orientated-no-rotation.jpg") + val orientationFuture = ImageOperations.getImageInformation(image) + whenReady(orientationFuture) { orientationOpt => + orientationOpt._2 should be(None) } } } diff --git a/container-images/jdk-vips/Dockerfile b/container-images/jdk-vips/Dockerfile index 4cf774b95b..090b1bd538 100644 --- a/container-images/jdk-vips/Dockerfile +++ b/container-images/jdk-vips/Dockerfile @@ -25,18 +25,18 @@ RUN apt -y --no-install-suggests install \ libheif-dev WORKDIR /tmp -RUN wget https://github.com/libvips/libvips/releases/download/v8.18.0/vips-8.18.0.tar.xz -RUN tar xf vips-8.18.0.tar.xz -WORKDIR /tmp/vips-8.18.0 +RUN wget https://github.com/libvips/libvips/releases/download/v8.18.2/vips-8.18.2.tar.xz +RUN tar xf vips-8.18.2.tar.xz +WORKDIR /tmp/vips-8.18.2 RUN meson setup build -WORKDIR /tmp/vips-8.18.0/build +WORKDIR /tmp/vips-8.18.2/build RUN meson compile RUN meson test RUN meson install RUN ldconfig -RUN rm /tmp/vips-8.18.0.tar.xz -RUN rm -r /tmp/vips-8.18.0/ +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 \ diff --git a/container-images/jdk-vips/cloudbuild.yaml b/container-images/jdk-vips/cloudbuild.yaml index 1fe3754e36..18bd224954 100644 --- a/container-images/jdk-vips/cloudbuild.yaml +++ b/container-images/jdk-vips/cloudbuild.yaml @@ -1,6 +1,6 @@ steps: - name: 'gcr.io/cloud-builders/docker' - args: ['build', '-t', 'eu.gcr.io/$PROJECT_ID/jdk-vips:25-8.18', '.'] + args: ['build', '-t', 'eu.gcr.io/$PROJECT_ID/jdk-vips:25-8.18.2', '.'] - name: 'gcr.io/cloud-builders/docker' - args: ['push', 'eu.gcr.io/$PROJECT_ID/jdk-vips:25-8.18'] + args: ['push', 'eu.gcr.io/$PROJECT_ID/jdk-vips:25-8.18.2'] diff --git a/cropper/app/CropperComponents.scala b/cropper/app/CropperComponents.scala index d570859ade..0bd730b360 100644 --- a/cropper/app/CropperComponents.scala +++ b/cropper/app/CropperComponents.scala @@ -1,3 +1,4 @@ +import app.photofox.vipsffm.{Vips, VipsHelper} import com.gu.mediaservice.GridClient import com.gu.mediaservice.lib.aws.S3 import com.gu.mediaservice.lib.imaging.ImageOperations @@ -12,7 +13,11 @@ class CropperComponents(context: Context) extends GridComponents(context, new Cr final override val buildInfo = utils.buildinfo.BuildInfo val store = new CropStore(config) - val imageOperations = new ImageOperations(context.environment.rootPath.getAbsolutePath) + val imageOperations = { + Vips.init() + VipsHelper.cache_set_max(0) + new ImageOperations(context.environment.rootPath.getAbsolutePath) + } val s3 = new S3(config) val crops = new Crops(config, store, imageOperations, config.imageBucket, s3) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index d7b3a7eda7..d73599a422 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -120,7 +120,8 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera Stopwatch.async(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) - colourModel <- ImageOperations.identifyColourModel(sourceFile, mimeType) + colourModelAndInformation <- ImageOperations.getImageInformation(sourceFile) + colourModel = colourModelAndInformation._3 masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, colourModel, apiImage.source.orientationMetadata) outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions diff --git a/image-loader/app/ImageLoaderComponents.scala b/image-loader/app/ImageLoaderComponents.scala index 6f8b255498..4f3528b5cb 100644 --- a/image-loader/app/ImageLoaderComponents.scala +++ b/image-loader/app/ImageLoaderComponents.scala @@ -1,12 +1,13 @@ +import app.photofox.vipsffm.{Vips, VipsHelper} import com.gu.mediaservice.GridClient -import com.gu.mediaservice.lib.aws.{Bedrock, Embedder, S3Vectors, SimpleSqsMessageConsumer} -import com.gu.mediaservice.lib.aws.{Bedrock, S3, S3Vectors, SimpleSqsMessageConsumer, Embedder} +import com.gu.mediaservice.lib.aws._ import com.gu.mediaservice.lib.imaging.ImageOperations import com.gu.mediaservice.lib.logging.GridLogging import com.gu.mediaservice.lib.play.GridComponents import controllers.{ImageLoaderController, ImageLoaderManagement, UploadStatusController} import lib._ import lib.storage.{ImageLoaderStore, QuarantineStore} +import model.upload.OptimiseWithPngQuant import model.{Projector, QuarantineUploader, Uploader} import play.api.ApplicationLoader.Context import router.Routes @@ -25,7 +26,11 @@ class ImageLoaderComponents(context: Context) extends GridComponents(context, ne val store = new ImageLoaderStore(config) val maybeIngestQueue = config.maybeIngestSqsQueueUrl.map(queueUrl => new SimpleSqsMessageConsumer(queueUrl, config)) val uploadStatusTable = new UploadStatusTable(config) - val imageOperations = new ImageOperations(context.environment.rootPath.getAbsolutePath) + val imageOperations = { + Vips.init() + VipsHelper.cache_set_max(0) + new ImageOperations(context.environment.rootPath.getAbsolutePath) + } val notifications = new Notifications(config) val downloader = new Downloader()(ec,wsClient) @@ -35,9 +40,10 @@ class ImageLoaderComponents(context: Context) extends GridComponents(context, ne new Embedder(new Bedrock(config), new SimpleSqsMessageConsumer(queueUrl, config)) } - val uploader = new Uploader(store, config, imageOperations, notifications, maybeEmbedder, imageProcessor, gridClient, auth) + val optimiseOps = new OptimiseWithPngQuant(imageOperations) + val uploader = new Uploader(store, config, imageOperations, notifications, maybeEmbedder, imageProcessor, gridClient, auth, optimiseOps) val s3 = new S3(config) - val projector = Projector(config, imageOperations, imageProcessor, auth, maybeEmbedder, s3) + val projector = Projector(config, imageOperations, imageProcessor, auth, maybeEmbedder, s3, optimiseOps) val quarantineUploader: Option[QuarantineUploader] = config.maybeQuarantineBucket.map(_ => new QuarantineUploader(new QuarantineStore(config), config) ) diff --git a/image-loader/app/lib/ImageLoaderConfig.scala b/image-loader/app/lib/ImageLoaderConfig.scala index 6e8bfde3a6..1ad67fe2fe 100644 --- a/image-loader/app/lib/ImageLoaderConfig.scala +++ b/image-loader/app/lib/ImageLoaderConfig.scala @@ -32,8 +32,7 @@ class ImageLoaderConfig(resources: GridConfigResources) extends CommonConfig(res val apiUri: Instance => String = services.apiBaseUri val kahunaUri: Instance => String = services.kahunaBaseUri - val transcodedMimeTypes: List[MimeType] = getStringSet("transcoded.mime.types").toList.map(MimeType(_)) - val supportedMimeTypes: List[MimeType] = List(Jpeg, Png) ::: transcodedMimeTypes + val supportedMimeTypes: List[MimeType] = List(Jpeg, Png) val uploadStatusTable: String = string("dynamo.table.upload.status") val uploadStatusExpiry: FiniteDuration = configuration.get[FiniteDuration]("uploadStatus.recordExpiry") diff --git a/image-loader/app/lib/imaging/FileMetadataReader.scala b/image-loader/app/lib/imaging/FileMetadataReader.scala index 40b9ad413e..32b092f9b6 100644 --- a/image-loader/app/lib/imaging/FileMetadataReader.scala +++ b/image-loader/app/lib/imaging/FileMetadataReader.scala @@ -13,7 +13,7 @@ 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.logging.{GridLogging, LogMarker} +import com.gu.mediaservice.lib.logging.{GridLogging, LogMarker, Stopwatch, addLogMarkers} import com.gu.mediaservice.lib.metadata.ImageMetadataConverter import com.gu.mediaservice.model._ import model.upload.UploadRequest @@ -53,7 +53,7 @@ object FileMetadataReader extends GridLogging { private implicit val ctx: ExecutionContext = ExecutionContext.fromExecutor(Executors.newCachedThreadPool) - def fromIPTCHeaders(image: File, imageId:String): Future[FileMetadata] = + def fromIPTCHeaders(image: File, imageId:String)(implicit logMarker: LogMarker): Future[FileMetadata] = for { metadata <- readMetadata(image) } @@ -183,67 +183,17 @@ object FileMetadataReader extends GridLogging { private def dateToUTCString(date: DateTime): String = ISODateTimeFormat.dateTime.print(date.withZone(DateTimeZone.UTC)) - - def orientation(image: File): Future[Option[OrientationMetadata]] = { - for { - metadata <- readMetadata(image) - } yield { - - for { - exifDirectory <- Option(metadata.getFirstDirectoryOfType(classOf[ExifIFD0Directory])) - exifOrientation <- Option(exifDirectory.getInteger(ExifDirectoryBase.TAG_ORIENTATION)) - orientation = OrientationMetadata(exifOrientation = Some(exifOrientation)) - orientationWhichTransformsImage <- Seq(orientation).find(_.transformsImage()) - } yield { - orientationWhichTransformsImage - } - } - } - - def dimensions(image: File, mimeType: Option[MimeType]): Future[Option[Dimensions]] = - for { - metadata <- readMetadata(image) - } - yield { - - mimeType match { - - case Some(Jpeg) => for { - jpegDir <- Option(metadata.getFirstDirectoryOfType(classOf[JpegDirectory])) - - } yield Dimensions(jpegDir.getImageWidth, jpegDir.getImageHeight) - - case Some(Png) => for { - pngDir <- Option(metadata.getFirstDirectoryOfType(classOf[PngDirectory])) - - } yield { - val width = pngDir.getInt(PngDirectory.TAG_IMAGE_WIDTH) - val height = pngDir.getInt(PngDirectory.TAG_IMAGE_HEIGHT) - Dimensions(width, height) - } - - case Some(Tiff) => for { - exifDir <- Option(metadata.getFirstDirectoryOfType(classOf[ExifIFD0Directory])) - - } yield { - val width = exifDir.getInt(ExifDirectoryBase.TAG_IMAGE_WIDTH) - val height = exifDir.getInt(ExifDirectoryBase.TAG_IMAGE_HEIGHT) - Dimensions(width, height) - } - - case _ => None - - } - } - def getColorModelInformation(image: File, metadata: Metadata, mimeType: MimeType)(implicit logMarker: LogMarker): Future[Map[String, String]] = { - + val stopWatch = Stopwatch.start val source = addImage(image) val formatter = format(source)("%r") - runIdentifyCmd(formatter, useImageMagick = false).map{ imageType => getColourInformation(metadata, imageType.headOption, mimeType) } - .recover { case _ => getColourInformation(metadata, None, mimeType) } + runIdentifyCmd(formatter, useImageMagick = false).map { imageType => getColourInformation(metadata, imageType.headOption, mimeType) } + .recover { case _ => getColourInformation(metadata, None, mimeType) }.map { result => + logger.info(addLogMarkers(stopWatch.elapsed), "Finished getColorModelInformation") + result + } } // bits per sample might be a useful value, eg. "1", "8"; or it might be annoying like "1 bits/component/pixel", "8 8 8 bits/component/pixel" @@ -293,8 +243,14 @@ object FileMetadataReader extends GridLogging { private def nonEmptyTrimmed(nullableStr: String): Option[String] = Option(nullableStr) map (_.trim) filter (_.nonEmpty) - private def readMetadata(file: File): Future[Metadata] = Future { - ImageMetadataReader.readMetadata(file) + private def readMetadata(file: File)(implicit logMarker: LogMarker): Future[Metadata] = { + val stopwatch = Stopwatch.start + Future { + ImageMetadataReader.readMetadata(file) + }.map { result => + logger.info(addLogMarkers(stopwatch.elapsed),"Finished readMetadata") + result + } } // Helper to flatten maps of options diff --git a/image-loader/app/model/Projector.scala b/image-loader/app/model/Projector.scala index 82bdf3ad09..bc99dcd8a3 100644 --- a/image-loader/app/model/Projector.scala +++ b/image-loader/app/model/Projector.scala @@ -1,7 +1,9 @@ package model +import _root_.play.api.libs.ws.WSRequest import com.amazonaws.services.s3.model.{ObjectMetadata, S3Object => AwsS3Object} import com.gu.mediaservice.lib.ImageIngestOperations.{fileKeyFromId, optimisedPngKeyFromId} +import com.gu.mediaservice.lib._ import com.gu.mediaservice.lib.auth.Authentication import com.gu.mediaservice.lib.aws.{Embedder, EmbedderMessage, S3, S3Bucket} import com.gu.mediaservice.lib.cleanup.ImageProcessor @@ -9,15 +11,13 @@ import com.gu.mediaservice.lib.config.InstanceForRequest import com.gu.mediaservice.lib.imaging.ImageOperations import com.gu.mediaservice.lib.logging.{GridLogging, LogMarker, Stopwatch} import com.gu.mediaservice.lib.net.URI -import com.gu.mediaservice.lib._ import com.gu.mediaservice.model.{Image, Instance, MimeType, UploadInfo} import com.gu.mediaservice.{GridClient, ImageDataMerger} import lib.imaging.{MimeTypeDetection, NoSuchImageExistsInS3} import lib.{DigestedFile, ImageLoaderConfig} -import model.upload.UploadRequest +import model.upload.{OptimiseOps, UploadRequest} import org.apache.commons.io.IOUtils import org.joda.time.{DateTime, DateTimeZone} -import _root_.play.api.libs.ws.WSRequest import java.io.{File, FileOutputStream} import scala.concurrent.duration.Duration @@ -28,8 +28,8 @@ object Projector { import Uploader.toImageUploadOpsCfg - def apply(config: ImageLoaderConfig, imageOps: ImageOperations, processor: ImageProcessor, auth: Authentication, maybeEmbedder: Option[Embedder], s3: S3)(implicit ec: ExecutionContext): Projector - = new Projector(toImageUploadOpsCfg(config), s3, imageOps, processor, auth, maybeEmbedder) + def apply(config: ImageLoaderConfig, imageOps: ImageOperations, processor: ImageProcessor, auth: Authentication, maybeEmbedder: Option[Embedder], s3: S3, optimiseOps: OptimiseOps)(implicit ec: ExecutionContext): Projector + = new Projector(toImageUploadOpsCfg(config), s3, imageOps, processor, auth, maybeEmbedder, optimiseOps) } case class S3FileExtractedMetadata( @@ -86,9 +86,10 @@ class Projector(config: ImageUploadOpsCfg, imageOps: ImageOperations, processor: ImageProcessor, auth: Authentication, - maybeEmbedder: Option[Embedder]) extends GridLogging with InstanceForRequest { + maybeEmbedder: Option[Embedder], + optimiseOps: OptimiseOps) extends GridLogging with InstanceForRequest { - private val imageUploadProjectionOps = new ImageUploadProjectionOps(config, imageOps, processor, s3, maybeEmbedder) + private val imageUploadProjectionOps = new ImageUploadProjectionOps(config, imageOps, processor, s3, maybeEmbedder, optimiseOps) def projectS3ImageById(imageId: String, tempFile: File, gridClient: GridClient, onBehalfOfFn: WSRequest => WSRequest) (implicit ec: ExecutionContext, logMarker: LogMarker, instance: Instance): Future[Option[Image]] = { @@ -163,6 +164,7 @@ class ImageUploadProjectionOps(config: ImageUploadOpsCfg, processor: ImageProcessor, s3: S3, maybeEmbedder: Option[Embedder], + optimiseOps: OptimiseOps ) extends GridLogging { import Uploader.fromUploadRequestShared @@ -180,7 +182,7 @@ class ImageUploadProjectionOps(config: ImageUploadOpsCfg, tryFetchOptimisedFile = fetchOptimisedFile ) - fromUploadRequestShared(uploadRequest, dependenciesWithProjectionsOnly, processor) + fromUploadRequestShared(uploadRequest, dependenciesWithProjectionsOnly, processor, optimiseOps) } private def projectOriginalFileAsS3Model(storableOriginalImage: StorableOriginalImage) = diff --git a/image-loader/app/model/Uploader.scala b/image-loader/app/model/Uploader.scala index 7b22de8b41..90a9724427 100644 --- a/image-loader/app/model/Uploader.scala +++ b/image-loader/app/model/Uploader.scala @@ -3,19 +3,16 @@ package model import _root_.play.api.libs.json.Json import _root_.play.api.libs.ws.WSRequest import com.gu.mediaservice.lib.Files.createTempFile -import com.gu.mediaservice.lib.ImageIngestOperations.fileKeyFromId import com.gu.mediaservice.lib._ import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.auth.Authentication -import com.gu.mediaservice.lib.aws.{Embedder, EmbedderMessage, S3Object, UpdateMessage} -import com.gu.mediaservice.lib.{BrowserViewableImage, ImageStorageProps, StorableOptimisedImage, StorableOriginalImage, StorableThumbImage} -import com.gu.mediaservice.lib.aws.{Embedder, S3Bucket, S3Object, S3Vectors, UpdateMessage} +import com.gu.mediaservice.lib.aws._ import com.gu.mediaservice.lib.cleanup.ImageProcessor import com.gu.mediaservice.lib.formatting._ import com.gu.mediaservice.lib.imaging.ImageOperations import com.gu.mediaservice.lib.imaging.ImageOperations.{optimisedMimeType, thumbMimeType} import com.gu.mediaservice.lib.logging._ -import com.gu.mediaservice.lib.metadata.{FileMetadataHelper, ImageMetadataConverter} +import com.gu.mediaservice.lib.metadata.ImageMetadataConverter import com.gu.mediaservice.lib.net.URI import com.gu.mediaservice.model._ import com.gu.mediaservice.syntax.MessageSubjects @@ -24,7 +21,7 @@ import lib.imaging.{FileMetadataReader, MimeTypeDetection} import lib.storage.ImageLoaderStore import lib.{DigestedFile, ImageLoaderConfig, Notifications} import model.Uploader.{fromUploadRequestShared, toImageUploadOpsCfg} -import model.upload.{OptimiseOps, OptimiseWithPngQuant, UploadRequest} +import model.upload.{OptimiseOps, UploadRequest} import org.joda.time.DateTime import java.io.File @@ -67,7 +64,6 @@ case class ImageUploadOpsCfg( tempDir: File, thumbWidth: Int, thumbQuality: Double, - transcodedMimeTypes: List[MimeType], originalFileBucket: S3Bucket, thumbBucket: S3Bucket, ) @@ -94,13 +90,12 @@ object Uploader extends GridLogging { config.tempDir, config.thumbWidth, config.thumbQuality, - config.transcodedMimeTypes, config.imageBucket, config.thumbnailBucket, ) } - def fromUploadRequestShared(uploadRequest: UploadRequest, deps: ImageUploadOpsDependencies, processor: ImageProcessor) + def fromUploadRequestShared(uploadRequest: UploadRequest, deps: ImageUploadOpsDependencies, processor: ImageProcessor, optimiseOps: OptimiseOps) (implicit ec: ExecutionContext, logMarker: LogMarker): Future[Image] = { import deps._ @@ -116,22 +111,20 @@ object Uploader extends GridLogging { storeOrProjectOriginalFile, storeOrProjectThumbFile, storeOrProjectOptimisedImage, - OptimiseWithPngQuant, uploadRequest, deps, - fileMetadata, - processor)(ec, addLogMarkers(fileMetadata.toLogMarker)) + processor, + optimiseOps)(ec, addLogMarkers(fileMetadata.toLogMarker)) }) } private[model] def uploadAndStoreImage(storeOrProjectOriginalFile: StorableOriginalImage => Future[S3Object], storeOrProjectThumbFile: StorableThumbImage => Future[S3Object], storeOrProjectOptimisedFile: StorableOptimisedImage => Future[S3Object], - optimiseOps: OptimiseOps, uploadRequest: UploadRequest, deps: ImageUploadOpsDependencies, - fileMetadata: FileMetadata, - processor: ImageProcessor) + processor: ImageProcessor, + optimiseOps: OptimiseOps) (implicit ec: ExecutionContext, logMarker: LogMarker) = { val originalMimeType = uploadRequest.mimeType .orElse(MimeTypeDetection.guessMimeType(uploadRequest.tempFile).toOption) @@ -143,10 +136,6 @@ object Uploader extends GridLogging { val tempDirForRequest: File = Files.createTempDirectory(deps.config.tempDir.toPath, "upload").toFile - val colourModelFuture = ImageOperations.identifyColourModel(uploadRequest.tempFile, originalMimeType) - val sourceDimensionsFuture = FileMetadataReader.dimensions(uploadRequest.tempFile, Some(originalMimeType)) - val sourceOrientationMetadataFuture = FileMetadataReader.orientation(uploadRequest.tempFile) - val storableOriginalImage = StorableOriginalImage( uploadRequest.imageId, uploadRequest.tempFile, @@ -156,27 +145,37 @@ object Uploader extends GridLogging { uploadRequest.instance ) val sourceStoreFuture = storeOrProjectOriginalFile(storableOriginalImage) - val eventualBrowserViewableImage = createBrowserViewableFileFuture(uploadRequest, tempDirForRequest, deps) + val eventualBrowserViewableImage = createBrowserViewableFileFuture(uploadRequest) val eventualImage = for { browserViewableImage <- eventualBrowserViewableImage s3Source <- sourceStoreFuture mergedUploadRequest = patchUploadRequestWithS3Metadata(uploadRequest, s3Source) - optimisedFileMetadata <- FileMetadataReader.fromIPTCHeadersWithColorInfo(browserViewableImage) - sourceDimensions <- sourceDimensionsFuture - sourceOrientationMetadata <- sourceOrientationMetadataFuture - thumbViewableImage <- createThumbFuture(optimisedFileMetadata, colourModelFuture, browserViewableImage, deps, tempDirForRequest, uploadRequest.instance, orientationMetadata = sourceOrientationMetadata) + imageInformation <- ImageOperations.getImageInformation(uploadRequest.tempFile) + sourceDimensions = imageInformation._1 + sourceOrientationMetadata = imageInformation._2 + colourModel = imageInformation._3 + colourModelInformation = imageInformation._4 + fileMetadata <- toFileMetadata(uploadRequest.tempFile, uploadRequest.imageId, uploadRequest.mimeType) + thumbViewableImageAndDimensions <- createThumbFuture(browserViewableImage, deps, tempDirForRequest, uploadRequest.instance, orientationMetadata = sourceOrientationMetadata) + thumbViewableImage = thumbViewableImageAndDimensions._1 + maybeThumbDimensions = thumbViewableImageAndDimensions._2 + thumbDimensions <- { + maybeThumbDimensions.map { dimensions => + Future.successful(Some(dimensions)) + }.getOrElse { + ImageOperations.getImageInformation(thumbViewableImage.file).map(_._1) + } + } s3Thumb <- storeOrProjectThumbFile(thumbViewableImage) maybeStorableOptimisedImage <- getStorableOptimisedImage( - tempDirForRequest, optimiseOps, browserViewableImage, optimisedFileMetadata, deps.tryFetchOptimisedFile, uploadRequest.instance) + tempDirForRequest, browserViewableImage, deps.tryFetchOptimisedFile, optimiseOps, uploadRequest.instance) s3PngOption <- maybeStorableOptimisedImage match { case Some(storableOptimisedImage) => storeOrProjectOptimisedFile(storableOptimisedImage).map(a=>Some(a)) case None => Future.successful(None) } - thumbDimensions <- FileMetadataReader.dimensions(thumbViewableImage.file, Some(thumbViewableImage.mimeType)) - colourModel <- colourModelFuture } yield { - val fullFileMetadata = fileMetadata.copy(colourModel = colourModel) + val fullFileMetadata = fileMetadata.copy(colourModel = colourModel).copy(colourModelInformation = colourModelInformation) val metadata = ImageMetadataConverter.fromFileMetadata(fullFileMetadata, s3Source.metadata.objectMetadata.lastModified) val sourceAsset = Asset.fromS3Object(s3Source, sourceDimensions, sourceOrientationMetadata) @@ -193,7 +192,7 @@ object Uploader extends GridLogging { ) val processedImage = processor(baseImage) - logger.info(logMarker, s"Ending image ops") + logger.info(addLogMarkers(fileMetadata.toLogMarker), s"Ending image ops") // FIXME: dirty hack to sync the originalUsageRights and originalMetadata as well processedImage.copy( originalMetadata = processedImage.metadata, @@ -209,13 +208,12 @@ object Uploader extends GridLogging { private def getStorableOptimisedImage( tempDir: File, - optimiseOps: OptimiseOps, browserViewableImage: BrowserViewableImage, - optimisedFileMetadata: FileMetadata, tryFetchOptimisedFile: (String, File, Instance) => Future[Option[(File, MimeType)]], + optimiseOps: OptimiseOps, instance: Instance )(implicit ec: ExecutionContext, logMarker: LogMarker): Future[Option[StorableOptimisedImage]] = { - if (optimiseOps.shouldOptimise(Some(browserViewableImage.mimeType), optimisedFileMetadata)) { + if (optimiseOps.shouldOptimise(Some(browserViewableImage.mimeType))) { for { tempFile <- createTempFile("optimisedpng-", optimisedMimeType.fileExtension, tempDir) maybeDownloadedOptimisedFile <- tryFetchOptimisedFile(browserViewableImage.id, tempFile, instance) @@ -249,34 +247,32 @@ object Uploader extends GridLogging { baseMeta.view.mapValues(URI.encode).toMap } - private def toFileMetadata(f: File, imageId: String, mimeType: Option[MimeType])(implicit logMarker: LogMarker): Future[FileMetadata] = { - mimeType match { - case Some(Png | Tiff | Jpeg) => FileMetadataReader.fromIPTCHeadersWithColorInfo(f, imageId, mimeType.get) + private def toFileMetadata(f: File, imageId: String, mimeType: Option[MimeType])(implicit ec: ExecutionContext, logMarker: LogMarker): Future[FileMetadata] = { + val stopwatch = Stopwatch.start + (mimeType match { + //case Some(Png | Tiff | Jpeg) => FileMetadataReader.fromIPTCHeadersWithColorInfo(f, imageId, mimeType.get) case _ => FileMetadataReader.fromIPTCHeaders(f, imageId) + }).map { result => + logger.info(addLogMarkers(stopwatch.elapsed), "Finished toFileMetadata") + result } } - private def createThumbFuture(fileMetadata: FileMetadata, - colourModelFuture: Future[Option[String]], - browserViewableImage: BrowserViewableImage, + private def createThumbFuture(browserViewableImage: BrowserViewableImage, deps: ImageUploadOpsDependencies, tempDir: File, instance: Instance, orientationMetadata: Option[OrientationMetadata] - )(implicit ec: ExecutionContext, logMarker: LogMarker) = { + )(implicit ec: ExecutionContext, logMarker: LogMarker): Future[(StorableThumbImage, Option[Dimensions])] = { import deps._ def generateThumbnail(tempFile: File) = { for { - colourModel <- colourModelFuture - iccColourSpace = FileMetadataHelper.normalisedIccColourSpace(fileMetadata) - thumbData <- imageOps.createThumbnail( + thumbData <- imageOps.createThumbnailVips( browserViewableImage, config.thumbWidth, config.thumbQuality, tempFile, - iccColourSpace, - colourModel, orientationMetadata, ) } yield thumbData @@ -285,34 +281,23 @@ object Uploader extends GridLogging { for { tempFile <- createTempFile(s"thumb-", thumbMimeType.fileExtension, tempDir) maybeThumbFile <- deps.tryFetchThumbFile(browserViewableImage.id, tempFile, instance) - (thumb, thumbMimeType) <- { + (thumb, thumbMimeType, thumbDimensions) <- { maybeThumbFile match { - case Some(thumbData) => Future.successful(thumbData) + case Some(thumbData) => Future.successful((thumbData._1, thumbData._2, None)) case None => generateThumbnail(tempFile) } } - } yield browserViewableImage - .copy(file = thumb, mimeType = thumbMimeType) - .asStorableThumbImage + } yield { + (browserViewableImage + .copy(file = thumb, mimeType = thumbMimeType) + .asStorableThumbImage, thumbDimensions) + } } private def createBrowserViewableFileFuture( - uploadRequest: UploadRequest, - tempDir: File, - deps: ImageUploadOpsDependencies + uploadRequest: UploadRequest )(implicit ec: ExecutionContext, logMarker: LogMarker): Future[BrowserViewableImage] = { - import deps._ uploadRequest.mimeType match { - case Some(mime) if config.transcodedMimeTypes.contains(mime) => - for { - (file, mimeType) <- imageOps.transformImage(uploadRequest.tempFile, uploadRequest.mimeType, tempDir) - } yield BrowserViewableImage( - uploadRequest.imageId, - file = file, - mimeType = mimeType, - isTransformedFromSource = true, - instance = uploadRequest.instance - ) case Some(mimeType) => Future.successful( BrowserViewableImage( @@ -344,7 +329,8 @@ class Uploader( val maybeEmbedder: Option[Embedder], imageProcessor: ImageProcessor, gridClient: GridClient, - auth: Authentication + auth: Authentication, + optimiseOps: OptimiseOps )( implicit val ec: ExecutionContext ) extends MessageSubjects with ArgoHelpers { @@ -375,7 +361,7 @@ class Uploader( val sideEffectDependencies = ImageUploadOpsDependencies(toImageUploadOpsCfg(config), imageOps, storeSource, storeThumbnail, storeOptimisedImage) Stopwatch.async("finalImage") { - val finalImage = fromUploadRequestShared(uploadRequest, sideEffectDependencies, imageProcessor) + val finalImage = fromUploadRequestShared(uploadRequest, sideEffectDependencies, imageProcessor, optimiseOps) uploadRequest.identifiers.foreach{ case (ImageStorageProps.derivativeOfMediaIdsIdentifierKey, commaSeparatedMediaIdsToAddUsagesTo) => commaSeparatedMediaIdsToAddUsagesTo.split(",").map(_.trim).foreach( diff --git a/image-loader/app/model/upload/OptimiseOps.scala b/image-loader/app/model/upload/OptimiseOps.scala index 1b753dd38d..9705728131 100644 --- a/image-loader/app/model/upload/OptimiseOps.scala +++ b/image-loader/app/model/upload/OptimiseOps.scala @@ -1,21 +1,24 @@ package model.upload +import app.photofox.vipsffm.enums.VipsIntent +import app.photofox.vipsffm.{VImage, VipsHelper, VipsOption} import com.gu.mediaservice.lib.ImageWrapper +import com.gu.mediaservice.lib.imaging.ImageOperations import com.gu.mediaservice.lib.logging.{LogMarker, MarkerMap, Stopwatch} -import com.gu.mediaservice.model.{FileMetadata, MimeType, Png, Tiff} +import com.gu.mediaservice.model.{MimeType, Png} import java.io.File +import java.lang.foreign.Arena import scala.concurrent.{ExecutionContext, Future} -import scala.sys.process._ trait OptimiseOps { def toOptimisedFile(file: File, imageWrapper: ImageWrapper, tempDir: File) (implicit ec: ExecutionContext, logMarker: LogMarker): Future[(File, MimeType)] - def shouldOptimise(mimeType: Option[MimeType], fileMetadata: FileMetadata): Boolean + def shouldOptimise(mimeType: Option[MimeType]): Boolean def optimiseMimeType: MimeType } -object OptimiseWithPngQuant extends OptimiseOps { +class OptimiseWithPngQuant(imageOperations: ImageOperations) extends OptimiseOps { override def optimiseMimeType: MimeType = Png @@ -23,33 +26,35 @@ object OptimiseWithPngQuant extends OptimiseOps { (implicit ec: ExecutionContext, logMarker: LogMarker): Future[(File, MimeType)] = Future { val marker = MarkerMap( - "fileName" -> file.getName() + "fileName" -> file.getName ) - Stopwatch("pngquant") { - val result = Seq("pngquant", "-s10", "--quality", "1-85", file.getAbsolutePath, - "--force", "--output", optimisedFile.getAbsolutePath - ).! - if (result > 0) - throw new Exception(s"pngquant failed to convert to optimised png file (rc = $result)") - }(marker) + // Given a source file on any valid upload type, return a file of the optimised type + Stopwatch("toOptimisedFile") { + try { + val arena = Arena.ofConfined + + val image = VImage.newFromFile(arena, file.getAbsolutePath) + + // 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) { + image.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 + image + } - if (optimisedFile.exists()) { - (optimisedFile, optimiseMimeType) - } else { - throw new Exception(s"Attempted to optimise PNG file ${optimisedFile.getPath}") - } + imageOperations.saveImageToFile(correctedForICCProfile: VImage, optimiseMimeType, 85, optimisedFile, quantise = true) + (optimisedFile, optimiseMimeType) + } catch { + case _: Exception => + throw new Exception(s"Failed to optimise PNG file ${file.getAbsolutePath}") + } + }(marker) } - def shouldOptimise(mimeType: Option[MimeType], fileMetadata: FileMetadata): Boolean = - mimeType match { - case Some(Png) => - fileMetadata.colourModelInformation.get("colorType") match { - case Some("True Color") => true - case Some("True Color with Alpha") => true - case _ => false - } - case Some(Tiff) => true // TODO This should be done better, it could be better optimised into a jpeg if there is no transparency. - case _ => false - } + def shouldOptimise(mimeType: Option[MimeType]): Boolean = false } diff --git a/image-loader/test/scala/lib/imaging/FileMetadataReaderTest.scala b/image-loader/test/scala/lib/imaging/FileMetadataReaderTest.scala index d9a570e224..de0434db75 100644 --- a/image-loader/test/scala/lib/imaging/FileMetadataReaderTest.scala +++ b/image-loader/test/scala/lib/imaging/FileMetadataReaderTest.scala @@ -23,63 +23,6 @@ class FileMetadataReaderTest 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() - it("should read the correct dimensions for a JPG image") { - val image = fileAt("getty.jpg") - val dimsFuture = FileMetadataReader.dimensions(image, Some(Jpeg)) - whenReady(dimsFuture) { dimOpt => - dimOpt should be(Symbol("defined")) - dimOpt.get.width should be(100) - dimOpt.get.height should be(60) - } - } - - it("should capture exif orientation tag in JPG images") { - val image = fileAt("exif-orientated.jpg") - val orientationFuture = FileMetadataReader.orientation(image) - whenReady(orientationFuture) { orientationOpt => - orientationOpt should be(Symbol("defined")) - orientationOpt.get.exifOrientation should be(Some(6)) - } - } - - it("should ignore 0 degree exif orientation tag as it has no material effect") { - val image = fileAt("exif-orientated-no-rotation.jpg") - val orientationFuture = FileMetadataReader.orientation(image) - whenReady(orientationFuture) { orientationOpt => - orientationOpt should be(None) - } - } - - it("should use uncorrected width and height as dimensions for exif 90 rotations") { - val image = fileAt("exif-orientated.jpg") - val dimsFuture = FileMetadataReader.dimensions(image, Some(Jpeg)) - whenReady(dimsFuture) { dimOpt => - dimOpt should be(Symbol("defined")) - dimOpt.get.width should be(3456) - dimOpt.get.height should be(2304) - } - } - - it("should read the correct dimensions for a tiff image") { - val image = fileAt("flower.tif") - val dimsFuture = FileMetadataReader.dimensions(image, Some(Tiff)) - whenReady(dimsFuture) { dimOpt => - dimOpt should be(Symbol("defined")) - dimOpt.get.width should be(73) - dimOpt.get.height should be(43) - } - } - - it("should read the correct dimensions for a png image") { - val image = fileAt("schaik.com_pngsuite/basn0g08.png") - val dimsFuture = FileMetadataReader.dimensions(image, Some(Png)) - whenReady(dimsFuture) { dimOpt => - dimOpt should be(Symbol("defined")) - dimOpt.get.width should be(32) - dimOpt.get.height should be(32) - } - } - it("should read the correct metadata for Getty JPG images") { val image = fileAt("getty.jpg") val metadataFuture = FileMetadataReader.fromIPTCHeaders(image, "dummy") diff --git a/image-loader/test/scala/model/ImageUploadTest.scala b/image-loader/test/scala/model/ImageUploadTest.scala index 0d894a90fe..57ab31443f 100644 --- a/image-loader/test/scala/model/ImageUploadTest.scala +++ b/image-loader/test/scala/model/ImageUploadTest.scala @@ -34,7 +34,7 @@ class ImageUploadTest extends AsyncFunSuite with Matchers with MockitoSugar { private implicit val logMarker: MockLogMarker = new MockLogMarker() // For mime type info, see https://github.com/guardian/grid/pull/2568 val tempDir = new File("/tmp") - val mockConfig: ImageUploadOpsCfg = ImageUploadOpsCfg(tempDir, 256, 85d, List(Tiff), S3Bucket("img-bucket", S3.AmazonAwsS3Endpoint, usesPathStyleURLs = false, mockS3Client), S3Bucket("thumb-bucket", S3.AmazonAwsS3Endpoint, usesPathStyleURLs = false, mockS3Client)) + val mockConfig: ImageUploadOpsCfg = ImageUploadOpsCfg(tempDir, 256, 85d, S3Bucket("img-bucket", S3.AmazonAwsS3Endpoint, usesPathStyleURLs = false, mockS3Client), S3Bucket("thumb-bucket", S3.AmazonAwsS3Endpoint, usesPathStyleURLs = false, mockS3Client)) /** * @todo: I flailed about until I found a path that worked, but @@ -85,11 +85,10 @@ class ImageUploadTest extends AsyncFunSuite with Matchers with MockitoSugar { storeOrProjectOriginalFile = mockDependencies.storeOrProjectOriginalFile, storeOrProjectThumbFile = mockDependencies.storeOrProjectThumbFile, storeOrProjectOptimisedFile = mockDependencies.storeOrProjectOptimisedImage, - optimiseOps = OptimiseWithPngQuant, uploadRequest = uploadRequest, deps = mockDependencies, - fileMetadata = FileMetadata(), processor = ImageProcessor.identity, + new OptimiseWithPngQuant(imageOps) ) // Assertions; Failure will auto-fail diff --git a/image-loader/test/scala/model/ProjectorTest.scala b/image-loader/test/scala/model/ProjectorTest.scala index e7eceb3fb0..ac07cbe707 100644 --- a/image-loader/test/scala/model/ProjectorTest.scala +++ b/image-loader/test/scala/model/ProjectorTest.scala @@ -16,6 +16,7 @@ import com.gu.mediaservice.lib.logging.{LogMarker, MarkerMap} import com.gu.mediaservice.model.{Instance, _} import com.gu.mediaservice.model.leases.LeasesByMedia import lib.DigestedFile +import model.upload.OptimiseWithPngQuant import org.joda.time.{DateTime, DateTimeZone} import org.mockito.ArgumentMatchers.any import org.mockito.Mockito.{times, verify, when} @@ -44,13 +45,13 @@ class ProjectorTest extends AnyFreeSpec with Matchers with ScalaFutures with Moc private val imageOperations = new ImageOperations(ctxPath) private val mockS3Client = mock[AmazonS3] - private val config = ImageUploadOpsCfg(new File("/tmp"), 256, 85d, Nil, S3Bucket("img-bucket", S3.AmazonAwsS3Endpoint, usesPathStyleURLs = false, mockS3Client), S3Bucket("thumb-bucket", S3.AmazonAwsS3Endpoint, usesPathStyleURLs = false, mockS3Client)) + private val config = ImageUploadOpsCfg(new File("/tmp"), 256, 85d, S3Bucket("img-bucket", S3.AmazonAwsS3Endpoint, usesPathStyleURLs = false, mockS3Client), S3Bucket("thumb-bucket", S3.AmazonAwsS3Endpoint, usesPathStyleURLs = false, mockS3Client)) private val maybeEmbedder = None private val s3 = mock[S3] private val auth = mock[Authentication] - private val projector = new Projector(config, s3, imageOperations, ImageProcessor.identity, auth, maybeEmbedder) + private val projector = new Projector(config, s3, imageOperations, ImageProcessor.identity, auth, maybeEmbedder, new OptimiseWithPngQuant(imageOperations)) private implicit val instance: Instance = Instance("an-instance")