From 9c642a6ae0285578556e35ff7412d33060426d08 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 13:28:59 +0100 Subject: [PATCH 01/56] Clean up; createMasterCrop instance is implicit. --- cropper/app/lib/Crops.scala | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index c48f4d64bc..76281e0872 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -31,15 +31,14 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera instance.id + "/" + s"${source.id}/${Crop.getCropId(bounds)}/$masterString$outputWidth${fileType.fileExtension}" } - def createMasterCrop( + private def createMasterCrop( apiImage: SourceImage, sourceFile: File, crop: Crop, mediaType: MimeType, colourModel: Option[String], - instance: Instance, - orientationMetadata: Option[OrientationMetadata], - )(implicit logMarker: LogMarker): Future[MasterCrop] = { + orientationMetadata: Option[OrientationMetadata] + )(implicit logMarker: LogMarker, instance: Instance): Future[MasterCrop] = { Stopwatch.async(s"creating master crop for ${apiImage.id}") { val source = crop.specification @@ -123,7 +122,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) colourModelAndInformation <- ImageOperations.getImageInformation(sourceFile) colourModel = colourModelAndInformation._3 - masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, colourModel, instance, apiImage.source.orientationMetadata) + masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, colourModel, apiImage.source.orientationMetadata) outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions From 728f6bd21dad773e61720e59c4b88f2658f6dc6e Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Wed, 21 May 2025 22:01:04 +0100 Subject: [PATCH 02/56] Spike; create master crop file via vips. --- .../lib/imaging/ImageOperations.scala | 46 +++++++++++++++++++ cropper/app/lib/Crops.scala | 2 +- 2 files changed, 47 insertions(+), 1 deletion(-) 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 dd29855405..b896cce200 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 @@ -108,6 +108,52 @@ class ImageOperations(playPath: String) extends GridLogging { yield outputFile } + def cropImageVips( + 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] = { + val outputFile = File.createTempFile(s"crop-", s"${fileType.fileExtension}", tempDir) // TODO function for this + + Future { + val arena = Arena.ofConfined + + // Read source image + val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) + // Orient + val rotated = orientationMetadata.map(_.orientationCorrection()).map { angle => + image.rotate(angle) + }.getOrElse { + image + } + // TODO correct colour + // TODO strip meta data + // Output colour profile + val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) + // TODO depth adjust + + cropped.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) + ) + + arena.close() + outputFile + } + } + // Updates metadata on existing file def appendMetadata(sourceFile: File, metadata: ImageMetadata): Future[File] = { runExiftoolCmd( diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 76281e0872..d34fdc1a14 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -49,7 +49,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val quality = if (mediaType == Png) pngCropQuality else masterCropQuality for { - strip <- imageOperations.cropImage( + strip <- imageOperations.cropImageVips( sourceFile, apiImage.source.mimeType, source.bounds, quality, config.tempDir, iccColourSpace, colourModel, mediaType, isTransformedFromSource = false, orientationMetadata = orientationMetadata From 2ae0f9692dc57c43a00002ed531ed1f46312fb0f Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 12:04:40 +0100 Subject: [PATCH 03/56] Naive resize crops to jpegs only step. Explicitly calculate the scaling based on the change in width. instance moves to implicit for consistency. --- .../lib/imaging/ImageOperations.scala | 35 +++++++++++++++++++ .../app/controllers/CropperController.scala | 2 +- cropper/app/lib/Crops.scala | 13 ++++--- 3 files changed, 44 insertions(+), 6 deletions(-) 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 b896cce200..0372d18308 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 @@ -161,6 +161,41 @@ class ImageOperations(playPath: String) extends GridLogging { ).map(_ => sourceFile) } + def resizeImageVips( + sourceFile: File, + sourceMimeType: Option[MimeType], + dimensions: Dimensions, + qual: Double = 100d, + tempDir: File, + fileType: MimeType, + sourceDimensions: Dimensions + )(implicit logMarker: LogMarker): Future[File] = { + + Future { + val arena = Arena.ofConfined + + val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this + + val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) + + val scale = dimensions.width.toDouble / sourceDimensions.width.toDouble + val resized = image.resize(scale) + + resized.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", false) + ) + + arena.close() + outputFile + } + } + def resizeImage( sourceFile: File, sourceMimeType: Option[MimeType], diff --git a/cropper/app/controllers/CropperController.scala b/cropper/app/controllers/CropperController.scala index 0600ab4742..60fe2c6aac 100644 --- a/cropper/app/controllers/CropperController.scala +++ b/cropper/app/controllers/CropperController.scala @@ -181,7 +181,7 @@ class CropperController(auth: Authentication, crops: Crops, store: CropStore, no specification = cropSpec ) markersWithCropDetails = logMarker ++ Map("imageId" -> apiImage.id, "cropId" -> Crop.getCropId(cropSpec.bounds)) - ExportResult(id, masterSizing, sizings) <- crops.makeExport(apiImage, crop, instance)(markersWithCropDetails) + ExportResult(id, masterSizing, sizings) <- crops.makeExport(apiImage, crop)(markersWithCropDetails, instance) finalCrop = Crop.createFromCrop(crop, masterSizing, sizings) } yield (id, finalCrop) } diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index d34fdc1a14..bb9c347b37 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -65,19 +65,22 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera } } - def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, cropType: MimeType, instance: Instance)(implicit logMarker: LogMarker): Future[List[Asset]] = { + def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, cropType: MimeType, masterCrop: MasterCrop + )(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, + file <- imageOperations.resizeImageVips(sourceFile, apiImage.source.mimeType, dimensions, quality, config.tempDir, - cropType)(cropLogMarker) + cropType, + masterCrop.dimensions + )(cropLogMarker) optimisedFile = imageOperations.optimiseImage(file, cropType)(cropLogMarker) filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType, instance = instance) sizing <- store.storeCropSizing(optimisedFile, filename, cropType, crop, dimensions)(cropLogMarker) @@ -106,7 +109,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera positiveCoords && strictlyPositiveSize && withinBounds } - def makeExport(apiImage: SourceImage, crop: Crop, instance: Instance)(implicit logMarker: LogMarker): Future[ExportResult] = { + def makeExport(apiImage: SourceImage, crop: Crop)(implicit logMarker: LogMarker, instance: Instance): Future[ExportResult] = { val source = crop.specification val mimeType = apiImage.source.mimeType.getOrElse(throw MissingMimeType) val secureFile = apiImage.source.file @@ -126,7 +129,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions - sizes <- createCrops(masterCrop.file, outputDims, apiImage, crop, cropType, instance) + sizes <- createCrops(masterCrop.file, outputDims, apiImage, crop, cropType, masterCrop) masterSize <- masterCrop.sizing _ <- Future.sequence(List(masterCrop.file, sourceFile).map(delete)) From 4e71bb1ec20da5be9b90cc609fffaf83ab05d6b1 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 13:13:46 +0100 Subject: [PATCH 04/56] createCrops and resizeImageVips in same Future and arena to allow sharing of source image. --- .../lib/imaging/ImageOperations.scala | 36 ++++++++----------- cropper/app/lib/Crops.scala | 24 ++++++------- 2 files changed, 27 insertions(+), 33 deletions(-) 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 0372d18308..429e8c629a 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 @@ -169,31 +169,25 @@ class ImageOperations(playPath: String) extends GridLogging { tempDir: File, fileType: MimeType, sourceDimensions: Dimensions - )(implicit logMarker: LogMarker): Future[File] = { + )(implicit logMarker: LogMarker, arena: Arena): File = { + val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this - Future { - val arena = Arena.ofConfined + val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) - val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this - - val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) + val scale = dimensions.width.toDouble / sourceDimensions.width.toDouble + val resized = image.resize(scale) - val scale = dimensions.width.toDouble / sourceDimensions.width.toDouble - val resized = image.resize(scale) + resized.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", false) + ) - resized.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", false) - ) - - arena.close() - outputFile - } + outputFile } def resizeImage( diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index bb9c347b37..ca364d01b4 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -8,6 +8,7 @@ import com.gu.mediaservice.lib.imaging.{ExportResult, ImageOperations} import com.gu.mediaservice.lib.logging.{GridLogging, LogMarker, Stopwatch} import com.gu.mediaservice.model._ +import java.lang.foreign.Arena import scala.concurrent.{ExecutionContext, Future} import scala.util.Try @@ -65,30 +66,29 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera } } - def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, cropType: MimeType, masterCrop: MasterCrop + private def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, cropType: MimeType, masterCrop: MasterCrop )(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 => + implicit val arena: Arena = Arena.ofConfined() + + val eventualAssets = Future.sequence(dimensionList.map { dimensions => val cropLogMarker = logMarker ++ Map("crop-dimensions" -> s"${dimensions.width}x${dimensions.height}") + val file = imageOperations.resizeImageVips(sourceFile, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) + val optimisedFile = imageOperations.optimiseImage(file, cropType) + val filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType, instance = instance) + for { - file <- imageOperations.resizeImageVips(sourceFile, - apiImage.source.mimeType, - dimensions, - quality, - config.tempDir, - cropType, - masterCrop.dimensions - )(cropLogMarker) - optimisedFile = imageOperations.optimiseImage(file, cropType)(cropLogMarker) - filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType, instance = instance) sizing <- store.storeCropSizing(optimisedFile, filename, cropType, crop, dimensions)(cropLogMarker) _ <- delete(file) _ <- delete(optimisedFile) } yield sizing }) + + arena.close() + eventualAssets } } From 0a355a5fecc195f7fde6e6cc24512229152e6d7b Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 13:15:47 +0100 Subject: [PATCH 05/56] Try to use 1 load of the source image across all resizes; do the resizes mutate the source? --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 6 ++---- cropper/app/lib/Crops.scala | 6 +++++- 2 files changed, 7 insertions(+), 5 deletions(-) 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 429e8c629a..8750422442 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 @@ -162,7 +162,7 @@ class ImageOperations(playPath: String) extends GridLogging { } def resizeImageVips( - sourceFile: File, + sourceImage: VImage, sourceMimeType: Option[MimeType], dimensions: Dimensions, qual: Double = 100d, @@ -172,10 +172,8 @@ class ImageOperations(playPath: String) extends GridLogging { )(implicit logMarker: LogMarker, arena: Arena): File = { val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this - val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) - val scale = dimensions.width.toDouble / sourceDimensions.width.toDouble - val resized = image.resize(scale) + val resized = sourceImage.resize(scale) resized.jpegsave(outputFile.getAbsolutePath, VipsOption.Int("Q", qual.toInt), diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index ca364d01b4..fccd89ae16 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -1,5 +1,7 @@ package lib +import app.photofox.vipsffm.VImage + import java.io.File import com.gu.mediaservice.lib.metadata.FileMetadataHelper import com.gu.mediaservice.lib.Files @@ -73,9 +75,11 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera Stopwatch.async(s"creating crops for ${apiImage.id}") { implicit val arena: Arena = Arena.ofConfined() + val sourceImage = VImage.newFromFile(arena, sourceFile.getAbsolutePath) + val eventualAssets = Future.sequence(dimensionList.map { dimensions => val cropLogMarker = logMarker ++ Map("crop-dimensions" -> s"${dimensions.width}x${dimensions.height}") - val file = imageOperations.resizeImageVips(sourceFile, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) + val file = imageOperations.resizeImageVips(sourceImage, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) val optimisedFile = imageOperations.optimiseImage(file, cropType) val filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType, instance = instance) From 53d7b22c0db5b06faf21856e89aab2fec5abd2c7 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 16:02:14 +0100 Subject: [PATCH 06/56] createCrops takes the masterCrop as a VImage rather than a file. --- cropper/app/lib/Crops.scala | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index fccd89ae16..64bd18ecbf 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -68,18 +68,14 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera } } - private def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, cropType: MimeType, masterCrop: MasterCrop - )(implicit logMarker: LogMarker, instance: Instance): Future[List[Asset]] = { + private def createCrops(sourceImage: VImage, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, cropType: MimeType, masterCrop: MasterCrop + )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): Future[List[Asset]] = { val quality = if (cropType == Png) pngCropQuality else cropQuality - Stopwatch.async(s"creating crops for ${apiImage.id}") { - implicit val arena: Arena = Arena.ofConfined() - - val sourceImage = VImage.newFromFile(arena, sourceFile.getAbsolutePath) - + Stopwatch(s"creating crops for ${apiImage.id}") { val eventualAssets = Future.sequence(dimensionList.map { dimensions => val cropLogMarker = logMarker ++ Map("crop-dimensions" -> s"${dimensions.width}x${dimensions.height}") - val file = imageOperations.resizeImageVips(sourceImage, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) + val file = imageOperations.resizeImageVips(sourceImage, apiImage.source.mimeType, dimensions, quality, config.tempDir, cropType, masterCrop.dimensions) val optimisedFile = imageOperations.optimiseImage(file, cropType) val filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType, instance = instance) @@ -91,7 +87,6 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera yield sizing }) - arena.close() eventualAssets } } @@ -124,7 +119,9 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val key = imageBucket.keyFromS3URL(secureFile) val secureUrl = s3.signUrlTony(imageBucket, key) - Stopwatch.async(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { + implicit val arena: Arena = Arena.ofConfined() + + val result = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) colourModelAndInformation <- ImageOperations.getImageInformation(sourceFile) @@ -133,13 +130,17 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions - sizes <- createCrops(masterCrop.file, outputDims, apiImage, crop, cropType, masterCrop) + masterCropImage = VImage.newFromFile(arena, masterCrop.file.getAbsolutePath) + sizes <- createCrops(masterCropImage, outputDims, apiImage, crop, cropType, masterCrop) masterSize <- masterCrop.sizing _ <- Future.sequence(List(masterCrop.file, sourceFile).map(delete)) } yield ExportResult(apiImage.id, masterSize, sizes) } + + arena.close() + result } } From 81991828e5c3ad45b2388202e429b6452f521efc Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 16:21:04 +0100 Subject: [PATCH 07/56] createMasterCrop moving to same arena. --- .../lib/imaging/ImageOperations.scala | 55 +++++++++---------- cropper/app/lib/Crops.scala | 17 +++--- 2 files changed, 34 insertions(+), 38 deletions(-) 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 8750422442..06d7b77c75 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 @@ -119,41 +119,36 @@ class ImageOperations(playPath: String) extends GridLogging { fileType: MimeType, isTransformedFromSource: Boolean, orientationMetadata: Option[OrientationMetadata] - )(implicit logMarker: LogMarker): Future[File] = { + )(implicit logMarker: LogMarker, arena: Arena): File = { val outputFile = File.createTempFile(s"crop-", s"${fileType.fileExtension}", tempDir) // TODO function for this + // Read source image + val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) + // Orient + val rotated = orientationMetadata.map(_.orientationCorrection()).map { angle => + image.rotate(angle) + }.getOrElse { + image + } + // TODO correct colour + // TODO strip meta data + // Output colour profile + val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) + // TODO depth adjust - Future { - val arena = Arena.ofConfined - - // Read source image - val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) - // Orient - val rotated = orientationMetadata.map(_.orientationCorrection()).map { angle => - image.rotate(angle) - }.getOrElse { - image - } - // TODO correct colour - // TODO strip meta data - // Output colour profile - val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) - // TODO depth adjust - - cropped.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) - ) + cropped.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) + ) - arena.close() - outputFile - } + outputFile } + // Updates metadata on existing file def appendMetadata(sourceFile: File, metadata: ImageMetadata): Future[File] = { runExiftoolCmd( diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 64bd18ecbf..389025563c 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -41,9 +41,9 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera mediaType: MimeType, colourModel: Option[String], orientationMetadata: Option[OrientationMetadata] - )(implicit logMarker: LogMarker, instance: Instance): Future[MasterCrop] = { + )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): Future[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) @@ -51,12 +51,13 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // care too much about filesize of master crops, so skip expensive compression to get faster cropping val quality = if (mediaType == Png) pngCropQuality else masterCropQuality + val strip = imageOperations.cropImageVips( + sourceFile, apiImage.source.mimeType, source.bounds, masterCropQuality, config.tempDir, + iccColourSpace, colourModel, mediaType, isTransformedFromSource = false, + orientationMetadata = orientationMetadata + ) + for { - strip <- imageOperations.cropImageVips( - 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, instance = instance) @@ -120,10 +121,10 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val secureUrl = s3.signUrlTony(imageBucket, key) implicit val arena: Arena = Arena.ofConfined() + val sourceFile = File.createTempFile("cropSource", "", config.tempDir) // TODO function for this val result = Stopwatch(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) From 514af808cd8cd083fe0d6d67d8f706653bca5f6d Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 16:35:57 +0100 Subject: [PATCH 08/56] Remove colourModel check from createMasterCrop. --- .../gu/mediaservice/lib/imaging/ImageOperations.scala | 1 - cropper/app/lib/Crops.scala | 9 +++------ 2 files changed, 3 insertions(+), 7 deletions(-) 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 06d7b77c75..69bf5ff29d 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 @@ -115,7 +115,6 @@ class ImageOperations(playPath: String) extends GridLogging { qual: Double = 100d, tempDir: File, iccColourSpace: Option[String], - colourModel: Option[String], fileType: MimeType, isTransformedFromSource: Boolean, orientationMetadata: Option[OrientationMetadata] diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 389025563c..b7a4b5181d 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -39,7 +39,6 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera sourceFile: File, crop: Crop, mediaType: MimeType, - colourModel: Option[String], orientationMetadata: Option[OrientationMetadata] )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): Future[MasterCrop] = { @@ -53,7 +52,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val strip = imageOperations.cropImageVips( sourceFile, apiImage.source.mimeType, source.bounds, masterCropQuality, config.tempDir, - iccColourSpace, colourModel, mediaType, isTransformedFromSource = false, + iccColourSpace, mediaType, isTransformedFromSource = false, orientationMetadata = orientationMetadata ) @@ -121,13 +120,11 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val secureUrl = s3.signUrlTony(imageBucket, key) implicit val arena: Arena = Arena.ofConfined() - val sourceFile = File.createTempFile("cropSource", "", config.tempDir) // TODO function for this val result = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { for { - colourModelAndInformation <- ImageOperations.getImageInformation(sourceFile) - colourModel = colourModelAndInformation._3 - masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, colourModel, apiImage.source.orientationMetadata) + sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) + masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions From d766d05237b583fd8ea19fd97a1f0f8d07de1319 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 17:46:55 +0100 Subject: [PATCH 09/56] Logging. Show when we reload the master crop from disk. --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 1 + cropper/app/lib/Crops.scala | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) 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 69bf5ff29d..176c22debc 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 @@ -134,6 +134,7 @@ class ImageOperations(playPath: String) extends GridLogging { val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust + logger.info("Saving master crop tmp file to: " + outputFile.getAbsolutePath) cropped.jpegsave(outputFile.getAbsolutePath, VipsOption.Int("Q", qual.toInt), //VipsOption.Boolean("optimize-scans", true), diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index b7a4b5181d..36f2576695 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -128,7 +128,11 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions - masterCropImage = VImage.newFromFile(arena, masterCrop.file.getAbsolutePath) + masterCropImage = { + logger.info("Reloading master crop image from: " + masterCrop.file.getAbsolutePath) + VImage.newFromFile(arena, masterCrop.file.getAbsolutePath) + } + sizes <- createCrops(masterCropImage, outputDims, apiImage, crop, cropType, masterCrop) masterSize <- masterCrop.sizing From ab4e6f10c61a01aafa8ad587184400f8a683a58d Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 20:50:50 +0100 Subject: [PATCH 10/56] Correct arena close timing. --- cropper/app/lib/Crops.scala | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 36f2576695..6c4741a8f0 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -121,7 +121,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera implicit val arena: Arena = Arena.ofConfined() - val result = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { + Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) @@ -139,10 +139,11 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera _ <- Future.sequence(List(masterCrop.file, sourceFile).map(delete)) } yield ExportResult(apiImage.id, masterSize, sizes) - } - arena.close() - result + }.map { result => + arena.close() + result + } } } From 7746c9ccabfd631a7e18913ff93457c748f6a4cf Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 21:21:16 +0100 Subject: [PATCH 11/56] createMasterCrop no Futures. --- cropper/app/lib/Crops.scala | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 6c4741a8f0..2f4ca26a7f 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -40,11 +40,10 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera crop: Crop, mediaType: MimeType, orientationMetadata: Option[OrientationMetadata] - )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): Future[MasterCrop] = { + )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): MasterCrop = { 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 @@ -56,15 +55,15 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera orientationMetadata = orientationMetadata ) - for { - file: File <- imageOperations.appendMetadata(strip, metadata) - dimensions = Dimensions(source.bounds.width, source.bounds.height) - filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType, isMaster = true, instance = instance) - 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) + val file = strip + + val dimensions = Dimensions(source.bounds.width, source.bounds.height) + val filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType, isMaster = true, instance = instance) + val sizing = store.storeCropSizing(file, filename, mediaType, crop, dimensions) + val dirtyAspect = source.bounds.width.toFloat / source.bounds.height + val aspect = crop.specification.aspectRatio.flatMap(AspectRatio.clean).getOrElse(dirtyAspect) + + MasterCrop(sizing, file, dimensions, aspect) } } @@ -124,7 +123,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) - masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) + masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions From 0e0231cd5ffc430fb1addf0b8f9a822d12875925 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 22:06:21 +0100 Subject: [PATCH 12/56] arena scope? --- cropper/app/lib/Crops.scala | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 2f4ca26a7f..96b501ac59 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -118,9 +118,9 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val key = imageBucket.keyFromS3URL(secureFile) val secureUrl = s3.signUrlTony(imageBucket, key) - implicit val arena: Arena = Arena.ofConfined() - Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { + val eventualResult = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { + implicit val arena: Arena = Arena.ofConfined() for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) @@ -137,12 +137,12 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera _ <- Future.sequence(List(masterCrop.file, sourceFile).map(delete)) } - yield ExportResult(apiImage.id, masterSize, sizes) - - }.map { result => - arena.close() - result + yield { + arena.close() + ExportResult(apiImage.id, masterSize, sizes) + } } + eventualResult } } From 07278eb847ec290596969685b5f10c0b4a21d997 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 22:17:33 +0100 Subject: [PATCH 13/56] arena scope? --- cropper/app/lib/Crops.scala | 40 +++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 96b501ac59..2b84c1c43b 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -118,31 +118,37 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val key = imageBucket.keyFromS3URL(secureFile) val secureUrl = s3.signUrlTony(imageBucket, key) - - val eventualResult = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { + //val eventualResult = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { + val eventualSourceFile: Future[File] = tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) + val x = eventualSourceFile.flatMap { sourceFile => implicit val arena: Arena = Arena.ofConfined() - for { - sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) - masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) - - outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions - - masterCropImage = { + val masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) + val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions + val masterCropImage = { logger.info("Reloading master crop image from: " + masterCrop.file.getAbsolutePath) VImage.newFromFile(arena, masterCrop.file.getAbsolutePath) - } + } - sizes <- createCrops(masterCropImage, outputDims, apiImage, crop, cropType, masterCrop) - masterSize <- masterCrop.sizing + val eventualSizes: Future[List[Asset]] = createCrops(masterCropImage, outputDims, apiImage, crop, cropType, masterCrop) + val eventualMasterSize: Future[Asset] = masterCrop.sizing - _ <- Future.sequence(List(masterCrop.file, sourceFile).map(delete)) + // _ <- Future.sequence(List(masterCrop.file, sourceFile).map(delete)) + + val z: Future[ExportResult] = eventualSizes.flatMap { sizes => + val a = eventualMasterSize.flatMap { masterSize => + val z: Future[ExportResult] = Future.sequence(List(masterCrop.file, sourceFile).map(delete)).map { _ => + ExportResult(apiImage.id, masterSize, sizes) + } + z + } + a } - yield { + z.onComplete( _ => arena.close() - ExportResult(apiImage.id, masterSize, sizes) - } + ) + z } - eventualResult + x } } From 456ddfeda0f39fc7371156ee7df7d8943a575a1a Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 22:24:28 +0100 Subject: [PATCH 14/56] arena scope? --- cropper/app/lib/Crops.scala | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 2b84c1c43b..6f117cdf18 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -128,6 +128,8 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera logger.info("Reloading master crop image from: " + masterCrop.file.getAbsolutePath) VImage.newFromFile(arena, masterCrop.file.getAbsolutePath) } + // All vips operationa have completed; we can close the arena + arena.close() val eventualSizes: Future[List[Asset]] = createCrops(masterCropImage, outputDims, apiImage, crop, cropType, masterCrop) val eventualMasterSize: Future[Asset] = masterCrop.sizing @@ -143,9 +145,6 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera } a } - z.onComplete( _ => - arena.close() - ) z } x From 188719923db2f4159cb4d190853ea149e28a4927 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 22:42:47 +0100 Subject: [PATCH 15/56] arena scope? --- cropper/app/lib/Crops.scala | 44 +++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 6f117cdf18..09d43557e4 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -8,6 +8,7 @@ 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.lib.resource.FutureResources import com.gu.mediaservice.model._ import java.lang.foreign.Arena @@ -72,21 +73,27 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val quality = if (cropType == Png) pngCropQuality else cropQuality Stopwatch(s"creating crops for ${apiImage.id}") { - val eventualAssets = Future.sequence(dimensionList.map { dimensions => - val cropLogMarker = logMarker ++ Map("crop-dimensions" -> s"${dimensions.width}x${dimensions.height}") - val file = imageOperations.resizeImageVips(sourceImage, apiImage.source.mimeType, dimensions, quality, config.tempDir, cropType, masterCrop.dimensions) + val resizes = dimensionList.map { dimensions => + val file = imageOperations.resizeImageVips(sourceImage, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) val optimisedFile = imageOperations.optimiseImage(file, cropType) val filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType, instance = instance) + (file, optimisedFile, filename, dimensions) + } + logger.info("Done resizes") + val eventualStoredAssets = resizes.map { resize => + val file = resize._1 + val optimisedFile = resize._2 + val filename = resize._3 + val dimensions = resize._4 for { - sizing <- store.storeCropSizing(optimisedFile, filename, cropType, crop, dimensions)(cropLogMarker) + sizing <- store.storeCropSizing(optimisedFile, filename, cropType, crop, dimensions) _ <- delete(file) _ <- delete(optimisedFile) } yield sizing - }) - - eventualAssets + } + Future.sequence(eventualStoredAssets) } } @@ -120,34 +127,29 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera //val eventualResult = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { val eventualSourceFile: Future[File] = tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) - val x = eventualSourceFile.flatMap { sourceFile => + eventualSourceFile.flatMap { sourceFile => implicit val arena: Arena = Arena.ofConfined() val masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions val masterCropImage = { - logger.info("Reloading master crop image from: " + masterCrop.file.getAbsolutePath) - VImage.newFromFile(arena, masterCrop.file.getAbsolutePath) + logger.info("Reloading master crop image from: " + masterCrop.file.getAbsolutePath) + VImage.newFromFile(arena, masterCrop.file.getAbsolutePath) } - // All vips operationa have completed; we can close the arena - arena.close() val eventualSizes: Future[List[Asset]] = createCrops(masterCropImage, outputDims, apiImage, crop, cropType, masterCrop) - val eventualMasterSize: Future[Asset] = masterCrop.sizing - // _ <- Future.sequence(List(masterCrop.file, sourceFile).map(delete)) + // All vips operationa have completed; we can close the arena + arena.close() - val z: Future[ExportResult] = eventualSizes.flatMap { sizes => - val a = eventualMasterSize.flatMap { masterSize => - val z: Future[ExportResult] = Future.sequence(List(masterCrop.file, sourceFile).map(delete)).map { _ => + // Map out the eventual store futures + eventualSizes.flatMap { sizes => + masterCrop.sizing.flatMap { masterSize => + Future.sequence(List(masterCrop.file, sourceFile).map(delete)).map { _ => ExportResult(apiImage.id, masterSize, sizes) } - z } - a } - z } - x } } From e36e004341a87e308ca2b01e916f7cc6804783f3 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 23 May 2025 23:09:04 +0100 Subject: [PATCH 16/56] Spike; no reload of master crop image. --- .../mediaservice/lib/imaging/ImageOperations.scala | 4 ++-- cropper/app/lib/Crops.scala | 13 ++++++------- 2 files changed, 8 insertions(+), 9 deletions(-) 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 176c22debc..00138db202 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 @@ -118,7 +118,7 @@ class ImageOperations(playPath: String) extends GridLogging { fileType: MimeType, isTransformedFromSource: Boolean, orientationMetadata: Option[OrientationMetadata] - )(implicit logMarker: LogMarker, arena: Arena): File = { + )(implicit logMarker: LogMarker, arena: Arena): (File, VImage) = { val outputFile = File.createTempFile(s"crop-", s"${fileType.fileExtension}", tempDir) // TODO function for this // Read source image val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) @@ -145,7 +145,7 @@ class ImageOperations(playPath: String) extends GridLogging { VipsOption.Boolean("strip", true) ) - outputFile + (outputFile, cropped) } diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 09d43557e4..f155c366da 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -19,7 +19,7 @@ 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(sizing: Future[Asset], image: VImage, file: File, dimensions: Dimensions, aspectRatio: Float) class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOperations, imageBucket: S3Bucket, s3: S3)(implicit ec: ExecutionContext) extends GridLogging { import Files._ @@ -50,13 +50,15 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // care too much about filesize of master crops, so skip expensive compression to get faster cropping val quality = if (mediaType == Png) pngCropQuality else masterCropQuality + logger.info(logMarker, s"creating master crop for ${apiImage.id}") val strip = imageOperations.cropImageVips( sourceFile, apiImage.source.mimeType, source.bounds, masterCropQuality, config.tempDir, iccColourSpace, mediaType, isTransformedFromSource = false, orientationMetadata = orientationMetadata ) - val file = strip + val file = strip._1 + val image = strip._2 val dimensions = Dimensions(source.bounds.width, source.bounds.height) val filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType, isMaster = true, instance = instance) @@ -64,7 +66,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val dirtyAspect = source.bounds.width.toFloat / source.bounds.height val aspect = crop.specification.aspectRatio.flatMap(AspectRatio.clean).getOrElse(dirtyAspect) - MasterCrop(sizing, file, dimensions, aspect) + MasterCrop(sizing, image, file, dimensions, aspect) } } @@ -131,10 +133,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera implicit val arena: Arena = Arena.ofConfined() val masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions - val masterCropImage = { - logger.info("Reloading master crop image from: " + masterCrop.file.getAbsolutePath) - VImage.newFromFile(arena, masterCrop.file.getAbsolutePath) - } + val masterCropImage = masterCrop.image val eventualSizes: Future[List[Asset]] = createCrops(masterCropImage, outputDims, apiImage, crop, cropType, masterCrop) From ad676720ddbce228b03e54c2090cb90001c69835 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 11:09:51 +0100 Subject: [PATCH 17/56] Marking TODO; why local file for master crop. --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 00138db202..110e415d55 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 @@ -119,7 +119,6 @@ class ImageOperations(playPath: String) extends GridLogging { isTransformedFromSource: Boolean, orientationMetadata: Option[OrientationMetadata] )(implicit logMarker: LogMarker, arena: Arena): (File, VImage) = { - val outputFile = File.createTempFile(s"crop-", s"${fileType.fileExtension}", tempDir) // TODO function for this // Read source image val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) // Orient @@ -134,6 +133,8 @@ class ImageOperations(playPath: String) extends GridLogging { val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust + // TODO separate this local file create from the vips master image create + val outputFile = File.createTempFile(s"crop-", s"${fileType.fileExtension}", tempDir) // TODO function for this logger.info("Saving master crop tmp file to: " + outputFile.getAbsolutePath) cropped.jpegsave(outputFile.getAbsolutePath, VipsOption.Int("Q", qual.toInt), From d268d164c2b1e5fccf4dbe989cd00723b237a5e2 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 11:18:08 +0100 Subject: [PATCH 18/56] Log local resize file location. --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 110e415d55..f540625760 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 @@ -166,11 +166,12 @@ class ImageOperations(playPath: String) extends GridLogging { fileType: MimeType, sourceDimensions: Dimensions )(implicit logMarker: LogMarker, arena: Arena): File = { - val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this val scale = dimensions.width.toDouble / sourceDimensions.width.toDouble val resized = sourceImage.resize(scale) + val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this + logger.info("Saving resized crop as JPEG tmp file to: " + outputFile.getAbsolutePath) resized.jpegsave(outputFile.getAbsolutePath, VipsOption.Int("Q", qual.toInt), //VipsOption.Boolean("optimize-scans", true), From 935f38571796d22677b2ab388ebf246b22e4feb7 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 11:30:18 +0100 Subject: [PATCH 19/56] PNG specific optimiseImage steps can move straight into the resizeImageVips VIPS save. --- .../lib/imaging/ImageOperations.scala | 36 +++++++++++++------ cropper/app/lib/Crops.scala | 12 +++---- 2 files changed, 31 insertions(+), 17 deletions(-) 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 f540625760..5bb32bbee4 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 @@ -172,17 +172,33 @@ class ImageOperations(playPath: String) extends GridLogging { val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this logger.info("Saving resized crop as JPEG tmp file to: " + outputFile.getAbsolutePath) - resized.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", false) - ) - outputFile + fileType match { + case Jpeg => + resized.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", false) + ) + outputFile + + case Png => + // val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" + // Seq("pngquant","-s8", "--quality", "1-85", fileName, "--output", optimisedImageName).! + resized.pngsave(outputFile.getAbsolutePath, + VipsOption.Int("Q", qual.toInt), + VipsOption.Boolean("strip", false) + ) + outputFile + + case _ => + logger.error(s"Cropping to $fileType is not supported.") + throw new UnsupportedCropOutputTypeException + } } def resizeImage( diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index f155c366da..aab99e091d 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -77,21 +77,19 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera Stopwatch(s"creating crops for ${apiImage.id}") { val resizes = dimensionList.map { dimensions => val file = imageOperations.resizeImageVips(sourceImage, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) - val optimisedFile = imageOperations.optimiseImage(file, cropType) val filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType, instance = instance) - (file, optimisedFile, filename, dimensions) + (file, filename, dimensions) } logger.info("Done resizes") val eventualStoredAssets = resizes.map { resize => val file = resize._1 - val optimisedFile = resize._2 - val filename = resize._3 - val dimensions = resize._4 + val filename = resize._2 + val dimensions = resize._3 for { - sizing <- store.storeCropSizing(optimisedFile, filename, cropType, crop, dimensions) + sizing <- store.storeCropSizing(file, filename, cropType, crop, dimensions) + _ <- delete(file) _ <- delete(file) - _ <- delete(optimisedFile) } yield sizing } From 7c06a26d9093fb4fc6a5aaaef8d579b03ec40e96 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 11:40:00 +0100 Subject: [PATCH 20/56] Logging. --- cropper/app/lib/Crops.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index aab99e091d..578bc0cbc9 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -86,6 +86,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val file = resize._1 val filename = resize._2 val dimensions = resize._3 + logger.info(s"Storing crop for: $file, $filename, $cropType") for { sizing <- store.storeCropSizing(file, filename, cropType, crop, dimensions) _ <- delete(file) From f94a8fe719a4652c267fef389794df59ed9d468b Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 12:04:56 +0100 Subject: [PATCH 21/56] TODO Strip true is needed to stop exif auto correction until we can strip metadata with vips. --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 5bb32bbee4..ba58d54a78 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 @@ -182,7 +182,7 @@ class ImageOperations(playPath: String) extends GridLogging { //VipsOption.Boolean("interlace", true), //VipsOption.Boolean("trellis-quant", true), // VipsOption.Int("quant-table", 3), - VipsOption.Boolean("strip", false) + VipsOption.Boolean("strip", true) ) outputFile @@ -191,7 +191,7 @@ class ImageOperations(playPath: String) extends GridLogging { // Seq("pngquant","-s8", "--quality", "1-85", fileName, "--output", optimisedImageName).! resized.pngsave(outputFile.getAbsolutePath, VipsOption.Int("Q", qual.toInt), - VipsOption.Boolean("strip", false) + VipsOption.Boolean("strip", true) ) outputFile From 0980f972e7307c017c08adf7ed352fcd66bffaba Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 12:05:10 +0100 Subject: [PATCH 22/56] Revert "Revert "Master image converted to sRBG colour space."" This reverts commit e010b37249c84289d2f1f6cf9ed36abf002eed53. --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 ba58d54a78..ae55789a45 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 @@ -133,10 +133,14 @@ class ImageOperations(playPath: String) extends GridLogging { val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust + val corrected = cropped.colourspace(VipsInterpretation.INTERPRETATION_sRGB) + + val master = corrected + // TODO separate this local file create from the vips master image create val outputFile = File.createTempFile(s"crop-", s"${fileType.fileExtension}", tempDir) // TODO function for this logger.info("Saving master crop tmp file to: " + outputFile.getAbsolutePath) - cropped.jpegsave(outputFile.getAbsolutePath, + master.jpegsave(outputFile.getAbsolutePath, VipsOption.Int("Q", qual.toInt), //VipsOption.Boolean("optimize-scans", true), //VipsOption.Boolean("optimize-coding", true), @@ -146,7 +150,7 @@ class ImageOperations(playPath: String) extends GridLogging { VipsOption.Boolean("strip", true) ) - (outputFile, cropped) + (outputFile, master) } From d352701ac6c5106af26cff0f51e589b71422cb74 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 12:17:27 +0100 Subject: [PATCH 23/56] Log crop type decision. --- cropper/app/lib/Crops.scala | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 578bc0cbc9..d3fcf19e5c 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -151,7 +151,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera } } -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 @@ -161,9 +161,12 @@ object Crops { val isGraphic = !colourType.matches("True[ ]?Color.*") val outputAsPng = hasAlpha || isGraphic - mediaType match { + val decision = mediaType match { case Png | Tiff if outputAsPng => Png case _ => Jpeg } + + logger.info(s"Choose crop type for $mediaType, $colourType, $hasAlpha: " + decision) + decision } } From 6f4c6c993f28fdb5ff0dae3b349c6887689a3a9d Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 12:26:51 +0100 Subject: [PATCH 24/56] Colour correct not effective if metadata stripped. Bake it in? --- .../scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ae55789a45..ab6c5f1819 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 @@ -133,7 +133,7 @@ class ImageOperations(playPath: String) extends GridLogging { val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust - val corrected = cropped.colourspace(VipsInterpretation.INTERPRETATION_sRGB) + val corrected = cropped.iccTransform("srgb") val master = corrected From df585b79194fed3abed61dcb5ff64e7609e369c9 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 19:10:52 +0100 Subject: [PATCH 25/56] Refactor; master crop image file write pushes up out of the create master image. --- .../lib/imaging/ImageOperations.scala | 22 ++----------- cropper/app/lib/Crops.scala | 33 +++++++++++++------ 2 files changed, 25 insertions(+), 30 deletions(-) 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 ab6c5f1819..9d28df307e 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 @@ -112,13 +112,11 @@ class ImageOperations(playPath: String) extends GridLogging { sourceFile: File, sourceMimeType: Option[MimeType], bounds: Bounds, - qual: Double = 100d, - tempDir: File, iccColourSpace: Option[String], fileType: MimeType, isTransformedFromSource: Boolean, orientationMetadata: Option[OrientationMetadata] - )(implicit logMarker: LogMarker, arena: Arena): (File, VImage) = { + )(implicit logMarker: LogMarker, arena: Arena): VImage = { // Read source image val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) // Orient @@ -127,30 +125,14 @@ class ImageOperations(playPath: String) extends GridLogging { }.getOrElse { image } - // TODO correct colour // TODO strip meta data // Output colour profile val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust val corrected = cropped.iccTransform("srgb") - val master = corrected - - // TODO separate this local file create from the vips master image create - val outputFile = File.createTempFile(s"crop-", s"${fileType.fileExtension}", tempDir) // TODO function for this - logger.info("Saving master crop tmp file to: " + outputFile.getAbsolutePath) - master.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, master) + master } diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index d3fcf19e5c..41732ee6c2 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -1,6 +1,6 @@ package lib -import app.photofox.vipsffm.VImage +import app.photofox.vipsffm.{VImage, VipsOption} import java.io.File import com.gu.mediaservice.lib.metadata.FileMetadataHelper @@ -50,15 +50,28 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // care too much about filesize of master crops, so skip expensive compression to get faster cropping val quality = if (mediaType == Png) pngCropQuality else masterCropQuality - logger.info(logMarker, s"creating master crop for ${apiImage.id}") - val strip = imageOperations.cropImageVips( - sourceFile, apiImage.source.mimeType, source.bounds, masterCropQuality, config.tempDir, - iccColourSpace, mediaType, isTransformedFromSource = false, - orientationMetadata = orientationMetadata - ) - - val file = strip._1 - val image = strip._2 + logger.info(logMarker, s"creating master crop for ${apiImage.id}") + val masterImage = imageOperations.cropImageVips( + sourceFile, apiImage.source.mimeType, source.bounds, + iccColourSpace, mediaType, isTransformedFromSource = false, + orientationMetadata = orientationMetadata + ) + + // TODO separate this local file create from the vips master image create + val outputFile = File.createTempFile(s"crop-", s"${mediaType.fileExtension}", config.tempDir) // TODO function for this + logger.info("Saving master crop tmp file to: " + outputFile.getAbsolutePath) + masterImage.jpegsave(outputFile.getAbsolutePath, + VipsOption.Int("Q", masterCropQuality.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) + ) + + val file = outputFile + val image = masterImage val dimensions = Dimensions(source.bounds.width, source.bounds.height) val filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType, isMaster = true, instance = instance) From 7b3c5e4042447c2238c4441a5aed3e91cb6d416b Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 19:12:47 +0100 Subject: [PATCH 26/56] Master crop is saved to requested crop type format. --- cropper/app/lib/Crops.scala | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 41732ee6c2..68389e7a12 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -6,7 +6,7 @@ import java.io.File import com.gu.mediaservice.lib.metadata.FileMetadataHelper 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.imaging.{ExportResult, ImageOperations, UnsupportedCropOutputTypeException} import com.gu.mediaservice.lib.logging.{GridLogging, LogMarker, Stopwatch} import com.gu.mediaservice.lib.resource.FutureResources import com.gu.mediaservice.model._ @@ -60,15 +60,32 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // TODO separate this local file create from the vips master image create val outputFile = File.createTempFile(s"crop-", s"${mediaType.fileExtension}", config.tempDir) // TODO function for this logger.info("Saving master crop tmp file to: " + outputFile.getAbsolutePath) - masterImage.jpegsave(outputFile.getAbsolutePath, - VipsOption.Int("Q", masterCropQuality.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) - ) + mediaType match { + case Jpeg => + masterImage.jpegsave(outputFile.getAbsolutePath, + VipsOption.Int("Q", masterCropQuality.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 => + // val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" + // Seq("pngquant","-s8", "--quality", "1-85", fileName, "--output", optimisedImageName).! + masterImage.pngsave(outputFile.getAbsolutePath, + VipsOption.Int("Q", masterCropQuality.toInt), + VipsOption.Boolean("strip", true) + ) + outputFile + + case _ => + logger.error(s"Cropping to $mediaType is not supported.") + throw new UnsupportedCropOutputTypeException + } val file = outputFile val image = masterImage From b2d5acf58b758b4ebb2cb5968b4b7b7f97f73ea0 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 19:20:16 +0100 Subject: [PATCH 27/56] Refactor; deduplicate save Vimage to file. --- .../lib/imaging/ImageOperations.scala | 59 ++++++++++--------- cropper/app/lib/Crops.scala | 28 +-------- 2 files changed, 32 insertions(+), 55 deletions(-) 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 9d28df307e..0cacd6981a 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 @@ -157,34 +157,7 @@ class ImageOperations(playPath: String) extends GridLogging { val resized = sourceImage.resize(scale) val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this - logger.info("Saving resized crop as JPEG tmp file to: " + outputFile.getAbsolutePath) - - fileType match { - case Jpeg => - resized.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 => - // val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" - // Seq("pngquant","-s8", "--quality", "1-85", fileName, "--output", optimisedImageName).! - resized.pngsave(outputFile.getAbsolutePath, - VipsOption.Int("Q", qual.toInt), - VipsOption.Boolean("strip", true) - ) - outputFile - - case _ => - logger.error(s"Cropping to $fileType is not supported.") - throw new UnsupportedCropOutputTypeException - } + saveImageToFile(resized, fileType, qual, outputFile) } def resizeImage( @@ -297,6 +270,36 @@ class ImageOperations(playPath: String) extends GridLogging { } } + def saveImageToFile(image: VImage, mimeType: MimeType, qual: Double, outputFile: File): 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 => + // val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" + // Seq("pngquant","-s8", "--quality", "1-85", fileName, "--output", optimisedImageName).! + image.pngsave(outputFile.getAbsolutePath, + VipsOption.Int("Q", qual.toInt), + 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 // to blah-0.something and contains the composite layer (which is what we want). // Other layers are then saved as blah-1.something etc. diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 68389e7a12..d6baa67fa4 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -59,33 +59,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // TODO separate this local file create from the vips master image create val outputFile = File.createTempFile(s"crop-", s"${mediaType.fileExtension}", config.tempDir) // TODO function for this - logger.info("Saving master crop tmp file to: " + outputFile.getAbsolutePath) - mediaType match { - case Jpeg => - masterImage.jpegsave(outputFile.getAbsolutePath, - VipsOption.Int("Q", masterCropQuality.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 => - // val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" - // Seq("pngquant","-s8", "--quality", "1-85", fileName, "--output", optimisedImageName).! - masterImage.pngsave(outputFile.getAbsolutePath, - VipsOption.Int("Q", masterCropQuality.toInt), - VipsOption.Boolean("strip", true) - ) - outputFile - - case _ => - logger.error(s"Cropping to $mediaType is not supported.") - throw new UnsupportedCropOutputTypeException - } + imageOperations.saveImageToFile(masterImage, mediaType, masterCropQuality, outputFile) val file = outputFile val image = masterImage From 15612017c2c36df75885bbd95e905dd7acf152f7 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 20:21:20 +0100 Subject: [PATCH 28/56] Refactor. S3 store of master crop pushes up. MasterCrop is a simpler opject with no Futures in it. Grouping the vips images operations in 1 tight block with no eventual side effects. --- cropper/app/lib/Crops.scala | 48 ++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index d6baa67fa4..d258d14202 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -19,7 +19,7 @@ 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], image: VImage, file: File, dimensions: Dimensions, aspectRatio: Float) +case class MasterCrop(image: VImage, file: File, dimensions: Dimensions, aspectRatio: Float) class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOperations, imageBucket: S3Bucket, s3: S3)(implicit ec: ExecutionContext) extends GridLogging { import Files._ @@ -50,27 +50,26 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // care too much about filesize of master crops, so skip expensive compression to get faster cropping val quality = if (mediaType == Png) pngCropQuality else masterCropQuality - logger.info(logMarker, s"creating master crop for ${apiImage.id}") - val masterImage = imageOperations.cropImageVips( - sourceFile, apiImage.source.mimeType, source.bounds, - iccColourSpace, mediaType, isTransformedFromSource = false, - orientationMetadata = orientationMetadata - ) + logger.info(logMarker, s"creating master crop for ${apiImage.id}") + val masterImage = imageOperations.cropImageVips( + sourceFile, apiImage.source.mimeType, source.bounds, + iccColourSpace, mediaType, isTransformedFromSource = false, + orientationMetadata = orientationMetadata + ) - // TODO separate this local file create from the vips master image create - val outputFile = File.createTempFile(s"crop-", s"${mediaType.fileExtension}", config.tempDir) // TODO function for this - imageOperations.saveImageToFile(masterImage, mediaType, masterCropQuality, outputFile) + // TODO separate this local file create from the vips master image create + val outputFile = File.createTempFile(s"crop-", s"${mediaType.fileExtension}", config.tempDir) // TODO function for this + imageOperations.saveImageToFile(masterImage, mediaType, masterCropQuality, outputFile) - val file = outputFile - val image = masterImage + val file = outputFile + val image = masterImage + //file: File <- imageOperations.appendMetadata(strip, metadata) val dimensions = Dimensions(source.bounds.width, source.bounds.height) - val filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType, isMaster = true, instance = instance) - val sizing = store.storeCropSizing(file, filename, mediaType, crop, dimensions) val dirtyAspect = source.bounds.width.toFloat / source.bounds.height val aspect = crop.specification.aspectRatio.flatMap(AspectRatio.clean).getOrElse(dirtyAspect) - MasterCrop(sizing, image, file, dimensions, aspect) + MasterCrop(image, file, dimensions, aspect) } } @@ -120,7 +119,7 @@ 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", "") @@ -131,21 +130,20 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val secureUrl = s3.signUrlTony(imageBucket, key) //val eventualResult = Stopwatch(s"making crop assets for ${apiImage.id} ${Crop.getCropId(source.bounds)}") { - val eventualSourceFile: Future[File] = tempFileFromURL(secureUrl, "cropSource", "", config.tempDir) - eventualSourceFile.flatMap { sourceFile => + tempFileFromURL(secureUrl, "cropSource", "", config.tempDir).flatMap { sourceFile => + logger.info("Starting vips operations") implicit val arena: Arena = Arena.ofConfined() val masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions - val masterCropImage = masterCrop.image - - val eventualSizes: Future[List[Asset]] = createCrops(masterCropImage, outputDims, apiImage, crop, cropType, masterCrop) - - // All vips operationa have completed; we can close the arena + val eventualSizesAssets: Future[List[Asset]] = createCrops(masterCrop.image, outputDims, apiImage, crop, cropType, masterCrop) + // All vips operations have completed; we can close the arena arena.close() + logger.info("Finished vips operations") // Map out the eventual store futures - eventualSizes.flatMap { sizes => - masterCrop.sizing.flatMap { masterSize => + val eventualMasterCropAsset = store.storeCropSizing(masterCrop.file, outputFilename(apiImage, source.bounds, masterCrop.dimensions.width, cropType, isMaster = true, instance = instance), cropType, crop, masterCrop.dimensions) + eventualMasterCropAsset.flatMap { masterSize => + eventualSizesAssets.flatMap { sizes => Future.sequence(List(masterCrop.file, sourceFile).map(delete)).map { _ => ExportResult(apiImage.id, masterSize, sizes) } From a490435ab914fdba26ea9008dd0e12cc8d8c3f8e Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 20:25:27 +0100 Subject: [PATCH 29/56] Refactor. File create for mastercrop pushes up to be next to the s3 store. --- cropper/app/lib/Crops.scala | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index d258d14202..513a05ffc9 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -19,7 +19,7 @@ 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(image: VImage, 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._ @@ -57,19 +57,12 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera orientationMetadata = orientationMetadata ) - // TODO separate this local file create from the vips master image create - val outputFile = File.createTempFile(s"crop-", s"${mediaType.fileExtension}", config.tempDir) // TODO function for this - imageOperations.saveImageToFile(masterImage, mediaType, masterCropQuality, outputFile) - - val file = outputFile - val image = masterImage - //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(image, file, dimensions, aspect) + MasterCrop(masterImage, dimensions, aspect) } } @@ -141,10 +134,14 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera logger.info("Finished vips operations") // Map out the eventual store futures - val eventualMasterCropAsset = store.storeCropSizing(masterCrop.file, outputFilename(apiImage, source.bounds, masterCrop.dimensions.width, cropType, isMaster = true, instance = instance), cropType, crop, masterCrop.dimensions) + + val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this + imageOperations.saveImageToFile(masterCrop.image, cropType, masterCropQuality, masterCropFile) + + val eventualMasterCropAsset = store.storeCropSizing(masterCropFile, outputFilename(apiImage, source.bounds, masterCrop.dimensions.width, cropType, isMaster = true, instance = instance), cropType, crop, masterCrop.dimensions) eventualMasterCropAsset.flatMap { masterSize => eventualSizesAssets.flatMap { sizes => - Future.sequence(List(masterCrop.file, sourceFile).map(delete)).map { _ => + Future.sequence(List(masterCropFile, sourceFile).map(delete)).map { _ => ExportResult(apiImage.id, masterSize, sizes) } } From 083fd0a8ad1e455e27f8592cf1df0d191726fe98 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 20:40:35 +0100 Subject: [PATCH 30/56] Master file create order; before arena close. --- cropper/app/lib/Crops.scala | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 513a05ffc9..a0ce4566fb 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -127,6 +127,9 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera logger.info("Starting vips operations") implicit val arena: Arena = Arena.ofConfined() val masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) + val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this + imageOperations.saveImageToFile(masterCrop.image, cropType, masterCropQuality, masterCropFile) + val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions val eventualSizesAssets: Future[List[Asset]] = createCrops(masterCrop.image, outputDims, apiImage, crop, cropType, masterCrop) // All vips operations have completed; we can close the arena @@ -134,10 +137,6 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera logger.info("Finished vips operations") // Map out the eventual store futures - - val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this - imageOperations.saveImageToFile(masterCrop.image, cropType, masterCropQuality, masterCropFile) - val eventualMasterCropAsset = store.storeCropSizing(masterCropFile, outputFilename(apiImage, source.bounds, masterCrop.dimensions.width, cropType, isMaster = true, instance = instance), cropType, crop, masterCrop.dimensions) eventualMasterCropAsset.flatMap { masterSize => eventualSizesAssets.flatMap { sizes => From 91da54658400b1822ded8fd994519bca9f41d13f Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 24 May 2025 20:50:33 +0100 Subject: [PATCH 31/56] Comment; master crop if different from the the full dimensions crop; it's higher quality for CDN resizer origin. --- cropper/app/lib/Crops.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index a0ce4566fb..c74397abc7 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -127,6 +127,8 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera logger.info("Starting vips operations") implicit val arena: Arena = Arena.ofConfined() val masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) + + // High quality rendering with minimal compression which will be used as the CDN resizer origin val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this imageOperations.saveImageToFile(masterCrop.image, cropType, masterCropQuality, masterCropFile) From e571e52cc893474ddcba0507043bc76667f8e8e7 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 12:11:29 +0100 Subject: [PATCH 32/56] Refactor; split creation of crop files from publishing to S3. --- cropper/app/lib/Crops.scala | 47 +++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index c74397abc7..cfacff00c2 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -67,9 +67,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera } private def createCrops(sourceImage: VImage, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, cropType: MimeType, masterCrop: MasterCrop - )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): Future[List[Asset]] = { - val quality = if (cropType == Png) pngCropQuality else cropQuality - + )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): Seq[(File, String, Dimensions)] = { Stopwatch(s"creating crops for ${apiImage.id}") { val resizes = dimensionList.map { dimensions => val file = imageOperations.resizeImageVips(sourceImage, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) @@ -77,20 +75,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera (file, filename, dimensions) } logger.info("Done resizes") - - val eventualStoredAssets = resizes.map { resize => - val file = resize._1 - val filename = resize._2 - val dimensions = resize._3 - logger.info(s"Storing crop for: $file, $filename, $cropType") - for { - sizing <- store.storeCropSizing(file, filename, cropType, crop, dimensions) - _ <- delete(file) - _ <- delete(file) - } - yield sizing - } - Future.sequence(eventualStoredAssets) + resizes } } @@ -132,18 +117,34 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this imageOperations.saveImageToFile(masterCrop.image, cropType, masterCropQuality, masterCropFile) + // Static crops; higher compression val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions - val eventualSizesAssets: Future[List[Asset]] = createCrops(masterCrop.image, outputDims, apiImage, crop, cropType, masterCrop) + val resizes = createCrops(masterCrop.image, outputDims, apiImage, crop, cropType, masterCrop) + // All vips operations have completed; we can close the arena arena.close() logger.info("Finished vips operations") - // Map out the eventual store futures - val eventualMasterCropAsset = store.storeCropSizing(masterCropFile, outputFilename(apiImage, source.bounds, masterCrop.dimensions.width, cropType, isMaster = true, instance = instance), cropType, crop, masterCrop.dimensions) - eventualMasterCropAsset.flatMap { masterSize => - eventualSizesAssets.flatMap { sizes => + // Store assets + val eventualStoredMasterCropAsset = store.storeCropSizing(masterCropFile, outputFilename(apiImage, source.bounds, masterCrop.dimensions.width, cropType, isMaster = true, instance = instance), cropType, crop, masterCrop.dimensions) + val eventualStoredCropAssets = Future.sequence { + resizes.map { resize => + val file = resize._1 + val filename = resize._2 + val dimensions = resize._3 + logger.info(s"Storing crop for: $file, $filename, $cropType") + for { + sizing <- store.storeCropSizing(file, filename, cropType, crop, dimensions) + _ <- delete(file) + } + yield sizing + } + } + + eventualStoredMasterCropAsset.flatMap { masterSize => + eventualStoredCropAssets.flatMap { sizes => Future.sequence(List(masterCropFile, sourceFile).map(delete)).map { _ => - ExportResult(apiImage.id, masterSize, sizes) + ExportResult(apiImage.id, masterSize, sizes.toList) } } } From cc1c031c192a7881b82cd64f94c8f33543807502 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 12:24:45 +0100 Subject: [PATCH 33/56] Palettise PNG crops. --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 0cacd6981a..5e584196bc 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 @@ -286,10 +286,12 @@ class ImageOperations(playPath: String) extends GridLogging { outputFile case Png => - // val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" - // Seq("pngquant","-s8", "--quality", "1-85", fileName, "--output", optimisedImageName).! - image.pngsave(outputFile.getAbsolutePath, + // We are allowed to quantise PNG crops but not the master + image.pngsave(outputFile.getAbsolutePath, + VipsOption.Boolean("palette", true), VipsOption.Int("Q", qual.toInt), + VipsOption.Int("effort", 1), + VipsOption.Int("bitdepth", 8), VipsOption.Boolean("strip", true) ) outputFile From 2df15d32a0284cdf712757d79bc200f86fdc8ae7 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 12:44:28 +0100 Subject: [PATCH 34/56] Do not quantise master crop; only the resizes. --- .../lib/imaging/ImageOperations.scala | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) 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 5e584196bc..45d81387d7 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 @@ -157,7 +157,7 @@ class ImageOperations(playPath: String) extends GridLogging { val resized = sourceImage.resize(scale) val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this - saveImageToFile(resized, fileType, qual, outputFile) + saveImageToFile(resized, fileType, qual, outputFile, quantise = true) } def resizeImage( @@ -270,7 +270,7 @@ class ImageOperations(playPath: String) extends GridLogging { } } - def saveImageToFile(image: VImage, mimeType: MimeType, qual: Double, outputFile: File): File = { + 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 => @@ -287,13 +287,20 @@ class ImageOperations(playPath: String) extends GridLogging { case Png => // We are allowed to quantise PNG crops but not the master - image.pngsave(outputFile.getAbsolutePath, - VipsOption.Boolean("palette", true), - VipsOption.Int("Q", qual.toInt), - VipsOption.Int("effort", 1), - VipsOption.Int("bitdepth", 8), - VipsOption.Boolean("strip", true) - ) + if (quantise) { + image.pngsave(outputFile.getAbsolutePath, + VipsOption.Boolean("palette", true), + VipsOption.Int("Q", qual.toInt), + VipsOption.Int("effort", 1), + VipsOption.Int("bitdepth", 8), + VipsOption.Boolean("strip", true) + ) + } else { + image.pngsave(outputFile.getAbsolutePath, + VipsOption.Int("Q", qual.toInt), + VipsOption.Boolean("strip", true) + ) + } outputFile case _ => From 51399f1df0228f255b1fa1a78ed841dcb65f11de Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 12:49:42 +0100 Subject: [PATCH 35/56] isGraphic was sendint all TIFFs to PNG. TODO restore this decision. --- cropper/app/lib/Crops.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index cfacff00c2..ae2c03e4ae 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -159,8 +159,8 @@ object Crops extends GridLogging { * - 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.*") - val outputAsPng = hasAlpha || isGraphic + //val isGraphic = !colourType.matches("True[ ]?Color.*") + val outputAsPng = hasAlpha // || isGraphic val decision = mediaType match { case Png | Tiff if outputAsPng => Png From f0e188cd85ed287ce174d73c8c61274f576b449f Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 12:50:18 +0100 Subject: [PATCH 36/56] isGraphic was sendint all TIFFs to PNG. TODO restore this decision. --- cropper/app/lib/Crops.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index ae2c03e4ae..29263c36b8 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -159,7 +159,8 @@ object Crops extends GridLogging { * - 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.*") + // TODO how to get source image colour depth from vips? + // val isGraphic = !colourType.matches("True[ ]?Color.*") val outputAsPng = hasAlpha // || isGraphic val decision = mediaType match { From 8b23bed43007e4405fe624fb8078141244be49e7 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 18:22:38 +0100 Subject: [PATCH 37/56] Correct CMYK renders too light in crops. --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 45d81387d7..cf4d893208 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 @@ -130,7 +130,11 @@ class ImageOperations(playPath: String) extends GridLogging { val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust - val corrected = cropped.iccTransform("srgb") + // Helps with CMYK; see https://github.com/libvips/libvips/issues/1110 + val corrected = cropped.iccTransform("srgb", + VipsOption.Enum("intent",VipsIntent.INTENT_PERCEPTUAL) + ) + val master = corrected master } From ad5a4202eaf8d14df96708b7945ec2399cb0c55c Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 21:30:14 +0100 Subject: [PATCH 38/56] Match depth between icc_transform and pngsave for visble saves. TODO jpeg? --- .../lib/imaging/ImageOperations.scala | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) 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 cf4d893208..10ca8fabca 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 @@ -130,9 +130,19 @@ class ImageOperations(playPath: String) extends GridLogging { val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust + // Bit used in save must match used in transform + val interpretation = VipsHelper.image_get_interpretation(image.getUnsafeStructAddress) + val is16bit = interpretation == VipsInterpretation.INTERPRETATION_GREY16.getRawValue || interpretation == VipsInterpretation.INTERPRETATION_LABS.getRawValue + val depth = if (is16bit) { + 16 + } else { + 8 + } + // Helps with CMYK; see https://github.com/libvips/libvips/issues/1110 val corrected = cropped.iccTransform("srgb", - VipsOption.Enum("intent",VipsIntent.INTENT_PERCEPTUAL) + VipsOption.Enum("intent",VipsIntent.INTENT_PERCEPTUAL), + VipsOption.Int("depth", depth) ) val master = corrected @@ -290,18 +300,28 @@ class ImageOperations(playPath: String) extends GridLogging { outputFile case Png => + // Bit used in save must match used in transform + val interpretation = VipsHelper.image_get_interpretation(image.getUnsafeStructAddress) + val is16bit = interpretation == VipsInterpretation.INTERPRETATION_GREY16.getRawValue || interpretation == VipsInterpretation.INTERPRETATION_LABS.getRawValue + val depth = if (is16bit) { + 16 + } else { + 8 + } + // 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("bitdepth", 8), + VipsOption.Int("bitdepth", depth), VipsOption.Boolean("strip", true) ) } else { image.pngsave(outputFile.getAbsolutePath, VipsOption.Int("Q", qual.toInt), + VipsOption.Int("bitdepth", depth), VipsOption.Boolean("strip", true) ) } From a91323c636dc6993d8967428b26525f17e0b1c94 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 21:54:37 +0100 Subject: [PATCH 39/56] Extract getDepthFor function to infer the depth of a VImage. --- .../lib/imaging/ImageOperations.scala | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) 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 10ca8fabca..f9dfa6c6e1 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 @@ -130,19 +130,10 @@ class ImageOperations(playPath: String) extends GridLogging { val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust - // Bit used in save must match used in transform - val interpretation = VipsHelper.image_get_interpretation(image.getUnsafeStructAddress) - val is16bit = interpretation == VipsInterpretation.INTERPRETATION_GREY16.getRawValue || interpretation == VipsInterpretation.INTERPRETATION_LABS.getRawValue - val depth = if (is16bit) { - 16 - } else { - 8 - } - // Helps with CMYK; see https://github.com/libvips/libvips/issues/1110 val corrected = cropped.iccTransform("srgb", VipsOption.Enum("intent",VipsIntent.INTENT_PERCEPTUAL), - VipsOption.Int("depth", depth) + VipsOption.Int("depth", getDepthFor(image)) ) val master = corrected @@ -301,13 +292,7 @@ class ImageOperations(playPath: String) extends GridLogging { case Png => // Bit used in save must match used in transform - val interpretation = VipsHelper.image_get_interpretation(image.getUnsafeStructAddress) - val is16bit = interpretation == VipsInterpretation.INTERPRETATION_GREY16.getRawValue || interpretation == VipsInterpretation.INTERPRETATION_LABS.getRawValue - val depth = if (is16bit) { - 16 - } else { - 8 - } + val depth: Int = getDepthFor(image) // We are allowed to quantise PNG crops but not the master if (quantise) { @@ -333,6 +318,20 @@ class ImageOperations(playPath: String) extends GridLogging { } } + private def getDepthFor(image: VImage) = { + val maybeInterpretation = VipsInterpretation.values().toSeq.find(_.getRawValue == VipsHelper.image_get_interpretation(image.getUnsafeStructAddress)) + val depth = maybeInterpretation.map { interpretation => + val is16bit = interpretation == VipsInterpretation.INTERPRETATION_GREY16 || interpretation == VipsInterpretation.INTERPRETATION_LABS + if (is16bit) { + 16 + } else { + 8 + } + }.getOrElse(8) + logger.info(s"Depth for interpretation $maybeInterpretation is $depth") + depth + } + // 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. From 074a7d6553b562f1e4e5d271fcda3ea1499008a9 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 22:18:56 +0100 Subject: [PATCH 40/56] INTERPRETATION_RGB16 is 16 bit. --- .../gu/mediaservice/lib/imaging/ImageOperations.scala | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) 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 f9dfa6c6e1..c1f2dac3f6 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 @@ -319,10 +319,15 @@ class ImageOperations(playPath: String) extends GridLogging { } private def getDepthFor(image: VImage) = { + val sixteenBitInterpretations = Set( + VipsInterpretation.INTERPRETATION_GREY16, + VipsInterpretation.INTERPRETATION_LABS, + VipsInterpretation.INTERPRETATION_RGB16 + ) + val maybeInterpretation = VipsInterpretation.values().toSeq.find(_.getRawValue == VipsHelper.image_get_interpretation(image.getUnsafeStructAddress)) val depth = maybeInterpretation.map { interpretation => - val is16bit = interpretation == VipsInterpretation.INTERPRETATION_GREY16 || interpretation == VipsInterpretation.INTERPRETATION_LABS - if (is16bit) { + if (sixteenBitInterpretations.contains(interpretation)) { 16 } else { 8 From d6a5141f601519f369b58db8364991ad5b36a1fe Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 25 May 2025 21:56:23 +0100 Subject: [PATCH 41/56] palette and bitdepth should be mutually exclusive. --- .../scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala | 1 - 1 file changed, 1 deletion(-) 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 c1f2dac3f6..dc8bc14521 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 @@ -300,7 +300,6 @@ class ImageOperations(playPath: String) extends GridLogging { VipsOption.Boolean("palette", true), VipsOption.Int("Q", qual.toInt), VipsOption.Int("effort", 1), - VipsOption.Int("bitdepth", depth), VipsOption.Boolean("strip", true) ) } else { From 4d21018d97ee448f7f004facc4dff2ad73053aa3 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Mon, 26 May 2025 11:01:37 +0100 Subject: [PATCH 42/56] Unused parameters. --- .../mediaservice/lib/imaging/ImageOperations.scala | 4 ---- cropper/app/lib/Crops.scala | 12 ++++++------ 2 files changed, 6 insertions(+), 10 deletions(-) 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 dc8bc14521..15f7882c14 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 @@ -110,11 +110,7 @@ class ImageOperations(playPath: String) extends GridLogging { def cropImageVips( sourceFile: File, - sourceMimeType: Option[MimeType], bounds: Bounds, - iccColourSpace: Option[String], - fileType: MimeType, - isTransformedFromSource: Boolean, orientationMetadata: Option[OrientationMetadata] )(implicit logMarker: LogMarker, arena: Arena): VImage = { // Read source image diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 29263c36b8..a15a8ff57a 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -50,12 +50,12 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // care too much about filesize of master crops, so skip expensive compression to get faster cropping val quality = if (mediaType == Png) pngCropQuality else masterCropQuality - logger.info(logMarker, s"creating master crop for ${apiImage.id}") - val masterImage = imageOperations.cropImageVips( - sourceFile, apiImage.source.mimeType, source.bounds, - iccColourSpace, mediaType, isTransformedFromSource = false, - orientationMetadata = orientationMetadata - ) + logger.info(logMarker, s"creating master crop for ${apiImage.id}") + val masterImage = imageOperations.cropImageVips( + sourceFile, + source.bounds, + orientationMetadata = orientationMetadata + ) //file: File <- imageOperations.appendMetadata(strip, metadata) val dimensions = Dimensions(source.bounds.width, source.bounds.height) From ac9c4efd739d968185623638d1b49a4b52ea4a2b Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Mon, 26 May 2025 11:04:21 +0100 Subject: [PATCH 43/56] Unused parameters. --- .../scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala | 1 - cropper/app/lib/Crops.scala | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) 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 15f7882c14..ff7cb30e9e 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 @@ -146,7 +146,6 @@ class ImageOperations(playPath: String) extends GridLogging { def resizeImageVips( sourceImage: VImage, - sourceMimeType: Option[MimeType], dimensions: Dimensions, qual: Double = 100d, tempDir: File, diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index a15a8ff57a..25c980bb35 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -70,7 +70,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): Seq[(File, String, Dimensions)] = { Stopwatch(s"creating crops for ${apiImage.id}") { val resizes = dimensionList.map { dimensions => - val file = imageOperations.resizeImageVips(sourceImage, apiImage.source.mimeType, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) + val file = imageOperations.resizeImageVips(sourceImage, dimensions, cropQuality, config.tempDir, cropType, masterCrop.dimensions) val filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, cropType, instance = instance) (file, filename, dimensions) } From a6ad060816c4e6d4e26bc381d73eaf1f69312e22 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Mon, 26 May 2025 11:37:08 +0100 Subject: [PATCH 44/56] Bypass icc_transform for LAB images. --- .../lib/imaging/ImageOperations.scala | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) 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 ff7cb30e9e..712fe73bc0 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 @@ -115,6 +115,8 @@ class ImageOperations(playPath: String) extends GridLogging { )(implicit logMarker: LogMarker, arena: Arena): VImage = { // Read source image val image = VImage.newFromFile(arena, sourceFile.getAbsolutePath) + val maybeInterpretation = VipsInterpretation.values().toSeq.find(_.getRawValue == VipsHelper.image_get_interpretation(image.getUnsafeStructAddress)) + // Orient val rotated = orientationMetadata.map(_.orientationCorrection()).map { angle => image.rotate(angle) @@ -126,12 +128,22 @@ class ImageOperations(playPath: String) extends GridLogging { val cropped = rotated.extractArea(bounds.x, bounds.y, bounds.width, bounds.height) // TODO depth adjust - // Helps with CMYK; see https://github.com/libvips/libvips/issues/1110 - val corrected = cropped.iccTransform("srgb", - VipsOption.Enum("intent",VipsIntent.INTENT_PERCEPTUAL), - VipsOption.Int("depth", getDepthFor(image)) + val labInterpretations = Set ( + VipsInterpretation.INTERPRETATION_LAB, + VipsInterpretation.INTERPRETATION_LABS ) + val isLab = maybeInterpretation.exists(interpretation => labInterpretations.contains(interpretation)) + val corrected = if (!isLab) { + cropped.iccTransform("srgb", + VipsOption.Enum("intent",VipsIntent.INTENT_PERCEPTUAL), // Helps with CMYK; see https://github.com/libvips/libvips/issues/1110 + VipsOption.Int("depth", getDepthFor(image)) + ) + } else { + // LAB gets corrupted by icc_transform something about with no profile? + cropped + } + val master = corrected master } From 3af181530cb4cb1971c757e7d0450937652f8ba5 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Mon, 26 May 2025 18:21:20 +0100 Subject: [PATCH 45/56] Remove non used non vips functions. --- .../lib/imaging/ImageOperations.scala | 148 +----------------- 1 file changed, 6 insertions(+), 142 deletions(-) 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 712fe73bc0..740336add7 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,22 +1,17 @@ package com.gu.mediaservice.lib.imaging import app.photofox.vipsffm.enums.{VipsIntent, VipsInterpretation} - -import java.io._ -import org.im4java.core.IMOperation -import com.gu.mediaservice.lib.Files._ -import com.gu.mediaservice.lib.{BrowserViewableImage, StorableThumbImage} -import com.gu.mediaservice.lib.imaging.ImageOperations.{optimisedMimeType, thumbMimeType} -import com.gu.mediaservice.lib.imaging.im4jwrapper.ImageMagick.{addDestImage, addImage, format, runIdentifyCmd} +import app.photofox.vipsffm.{VImage, VipsHelper, VipsOption} +import com.gu.mediaservice.lib.BrowserViewableImage +import com.gu.mediaservice.lib.imaging.ImageOperations.thumbMimeType import com.gu.mediaservice.lib.imaging.im4jwrapper.{ExifTool, ImageMagick} import com.gu.mediaservice.lib.logging.{GridLogging, LogMarker, Stopwatch, addLogMarkers} import com.gu.mediaservice.model._ +import org.im4java.core.IMOperation -import scala.concurrent.{ExecutionContext, Future} -import scala.sys.process._ -import app.photofox.vipsffm.{VImage, Vips, VipsHelper, VipsOption} - +import java.io._ import java.lang.foreign.Arena +import scala.concurrent.{ExecutionContext, Future} case class ExportResult(id: String, masterCrop: Asset, othersizings: List[Asset]) @@ -49,65 +44,6 @@ class ImageOperations(playPath: String) extends GridLogging { ).collect { case (key, Some(value)) => (key, value) } } - private def applyOutputProfile(base: IMOperation, optimised: Boolean = false) = profile(base)(rgbProfileLocation(optimised)) - - // Optionally apply transforms to the base operation if the colour space - // in the ICC profile doesn't match the colour model of the image data - private def correctColour(base: IMOperation)(iccColourSpace: Option[String], colourModel: Option[String], isTransformedFromSource: Boolean)(implicit logMarker: LogMarker): IMOperation = { - (iccColourSpace, colourModel, isTransformedFromSource) match { - // If matching, all is well, just pass through - case (icc, model, _) if icc == model => base - // If no colour model detected, we can't do anything anyway so just hope all is well - case (_, None, _) => base - // Do not correct colour if file has already been transformed (ie. source file was TIFF) as correctColour has already been run - case (_, _, true) => base - // If mismatching, strip any (incorrect) ICC profile and inject a profile matching the model - // Note: Strip both ICC and ICM (Windows variant?) to be safe - case (icc, Some(model), _) => - profileLocations.get(model) match { - // If this is a supported model, strip profile from base and add profile for model - case Some(location) => profile(stripProfile(base)("icm,icc"))(location) - // Do not attempt to correct colour if we don't support that colour model - case None => - logger.warn( - logMarker, - s"Wanted to update colour model where iccColourSpace=$icc and colourModel=$model but we don't have a profile file for that model" - ) - base - } - } - } - - def 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) - } - yield outputFile - } - def cropImageVips( sourceFile: File, bounds: Bounds, @@ -172,25 +108,6 @@ class ImageOperations(playPath: String) extends GridLogging { saveImageToFile(resized, fileType, qual, outputFile, quantise = true) } - def resizeImage( - sourceFile: File, - sourceMimeType: Option[MimeType], - dimensions: Dimensions, - qual: Double = 100d, - tempDir: File, - fileType: MimeType - )(implicit logMarker: LogMarker): Future[File] = Stopwatch.async("magick resize image") { - for { - outputFile <- createTempFile(s"resize-", s".${fileType.fileExtension}", tempDir) - resizeSource = addImage(sourceFile) - qualified = quality(resizeSource)(qual) - resized = scale(qualified)(dimensions) - addOutput = addDestImage(resized)(outputFile) - _ <- runConvertCmd(addOutput, useImageMagick = sourceMimeType.contains(Tiff)) - } - yield outputFile - } - private def orient(op: IMOperation, orientationMetadata: Option[OrientationMetadata]): IMOperation = { logger.info("Correcting for orientation: " + orientationMetadata) orientationMetadata.map(_.orientationCorrection()) match { @@ -199,27 +116,6 @@ class ImageOperations(playPath: String) extends GridLogging { } } - def optimiseImage(resizedFile: File, mediaType: MimeType)(implicit logMarker: LogMarker): File = mediaType match { - case Png => - val fileName: String = resizedFile.getAbsolutePath - - val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" - Stopwatch("pngquant") { - Seq("pngquant", "-s10", "--quality", "1-85", fileName, "--output", optimisedImageName).! - } - - new File(optimisedImageName) - case Jpeg => resizedFile - - // This should never happen as we only ever crop as PNG or JPEG. See `Crops.cropType` and `CropsTest` - // TODO We should create a `CroppingMimeType` to enforce this at the type level. - // However we'd need to change the `Asset` model as source image and crop use this model - // and a source can legally be a `Tiff`. It's not a small change... - case Tiff => - logger.error("Attempting to optimize a Tiff crop. Cropping as Tiff is not supported.") - throw new UnsupportedCropOutputTypeException - } - val interlacedHow = "Line" val backgroundColour = "#333333" @@ -342,38 +238,6 @@ class ImageOperations(playPath: String) extends GridLogging { logger.info(s"Depth for interpretation $maybeInterpretation is $depth") depth } - - // 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 { From 9667f1c19f6b0d2deed7784f42841dae9886912b Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Mon, 26 May 2025 18:22:06 +0100 Subject: [PATCH 46/56] Remove non used runConvert command. --- .../lib/imaging/im4jwrapper/ImageMagick.scala | 8 -------- 1 file changed, 8 deletions(-) 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 { From 11e84179e667c7cf14470fee6b66d9a2dc20b595 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 29 Jun 2025 20:22:22 +0100 Subject: [PATCH 47/56] Remove unused iccColourSpace val. --- cropper/app/lib/Crops.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 25c980bb35..f79756048c 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -45,7 +45,6 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera Stopwatch(s"creating master crop for ${apiImage.id}") { val source = crop.specification - 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 @@ -164,7 +163,8 @@ object Crops extends GridLogging { val outputAsPng = hasAlpha // || isGraphic val decision = mediaType match { - case Png | Tiff if outputAsPng => Png + case Png if outputAsPng => Png + case Tiff if outputAsPng => Png case _ => Jpeg } From 654408d9b44ede979e1f0e15adcb87034c070a02 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 29 Jun 2025 20:26:38 +0100 Subject: [PATCH 48/56] Reapply png master quality. --- cropper/app/lib/Crops.scala | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index f79756048c..923eec1b8a 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -28,6 +28,7 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera private val masterCropQuality = 95d // 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. + // TODO confirm this for vips private val pngCropQuality = 1d def outputFilename(source: SourceImage, bounds: Bounds, outputWidth: Int, fileType: MimeType, isMaster: Boolean = false, instance: Instance): String = { @@ -45,10 +46,6 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera Stopwatch(s"creating master crop for ${apiImage.id}") { val source = crop.specification - // 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 - logger.info(logMarker, s"creating master crop for ${apiImage.id}") val masterImage = imageOperations.cropImageVips( sourceFile, @@ -114,7 +111,12 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // High quality rendering with minimal compression which will be used as the CDN resizer origin val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this - imageOperations.saveImageToFile(masterCrop.image, cropType, masterCropQuality, masterCropFile) + + // 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 (mimeType == Png) pngCropQuality else masterCropQuality + + imageOperations.saveImageToFile(masterCrop.image, cropType, quality, masterCropFile) // Static crops; higher compression val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions From 086f80a1eff36e8e91d34ad86018ecb6ab47b9ef Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Mon, 24 Nov 2025 08:53:34 +0000 Subject: [PATCH 49/56] CropType decision can be deferred until after the master crop has been loaded. So that hasAlpha and colour depth can possibly come from the actual master crop image. --- cropper/app/lib/Crops.scala | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 923eec1b8a..1abcd9a9d1 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -40,9 +40,8 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera apiImage: SourceImage, sourceFile: File, crop: Crop, - mediaType: MimeType, orientationMetadata: Option[OrientationMetadata] - )(implicit logMarker: LogMarker, instance: Instance, arena: Arena): MasterCrop = { + )(implicit logMarker: LogMarker, arena: Arena): MasterCrop = { Stopwatch(s"creating master crop for ${apiImage.id}") { val source = crop.specification @@ -96,9 +95,6 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera 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) @@ -107,7 +103,11 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera tempFileFromURL(secureUrl, "cropSource", "", config.tempDir).flatMap { sourceFile => logger.info("Starting vips operations") implicit val arena: Arena = Arena.ofConfined() - val masterCrop = createMasterCrop(apiImage, sourceFile, crop, cropType, apiImage.source.orientationMetadata) + val masterCrop = createMasterCrop(apiImage, sourceFile, crop, apiImage.source.orientationMetadata) + + 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) // High quality rendering with minimal compression which will be used as the CDN resizer origin val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this From 754cfd9dac71b2966673a2306abc42980fa63d4b Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Mon, 29 Dec 2025 17:25:57 +0000 Subject: [PATCH 50/56] Refactor. Push the isGraphic? decision up out of cropType so that is can be made at a more implementation specific level. --- cropper/app/lib/Crops.scala | 13 +++++++------ cropper/test/lib/CropsTest.scala | 18 +++++++++--------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 1abcd9a9d1..bfb80029fe 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -107,7 +107,10 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera 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) + + // TODO how to get source image colour depth from vips? + // val isGraphic = !colourType.matches("True[ ]?Color.*") + val cropType = Crops.cropType(mimeType, isGraphic = false, hasAlpha = hasAlpha) // High quality rendering with minimal compression which will be used as the CDN resizer origin val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this @@ -159,10 +162,8 @@ object Crops extends GridLogging { * - 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 = { - // TODO how to get source image colour depth from vips? - // val isGraphic = !colourType.matches("True[ ]?Color.*") - val outputAsPng = hasAlpha // || isGraphic + def cropType(mediaType: MimeType, isGraphic: Boolean, hasAlpha: Boolean): MimeType = { + val outputAsPng = hasAlpha || isGraphic val decision = mediaType match { case Png if outputAsPng => Png @@ -170,7 +171,7 @@ object Crops extends GridLogging { case _ => Jpeg } - logger.info(s"Choose crop type for $mediaType, $colourType, $hasAlpha: " + decision) + 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 77f6c4ace7..3eb0a8bd35 100644 --- a/cropper/test/lib/CropsTest.scala +++ b/cropper/test/lib/CropsTest.scala @@ -14,36 +14,36 @@ class CropsTest extends AnyFunSpec with Matchers with MockitoSugar { private val 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 = { From 0db6cbfb2b48d90c0aae81b9adbb3f888ed4f65c Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Mon, 29 Dec 2025 18:18:43 +0000 Subject: [PATCH 51/56] Spike. vips based implementation of isGraphic? is likely to involve the number of bands in the image. image_get_typeof palette as an indication that the image had a palette is another good signal for isGraphic. format and bands are likely to be required to infer colour depth. --- .../lib/imaging/ImageOperations.scala | 13 +++++++ .../resources/schaik.com_pngsuite/README.md | 25 +++++++++++++ .../schaik.com_pngsuite/basn3p08.png | Bin 0 -> 1286 bytes .../schaik.com_pngsuite/tbbn0g04.png | Bin 0 -> 429 bytes .../lib/imaging/ImageOperationsTest.scala | 34 +++++++++++++++++- cropper/app/lib/Crops.scala | 7 ++-- 6 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 common-lib/src/test/resources/schaik.com_pngsuite/README.md create mode 100644 common-lib/src/test/resources/schaik.com_pngsuite/basn3p08.png create mode 100644 common-lib/src/test/resources/schaik.com_pngsuite/tbbn0g04.png 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 740336add7..2eae417fa0 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 @@ -293,4 +293,17 @@ object ImageOperations extends GridLogging { } } + def isGraphicVips(image: VImage)(implicit arena: Arena): Boolean = { + val numberOfBands = VipsHelper.image_get_bands(image.getUnsafeStructAddress) + logger.info("Number of bands: " + numberOfBands) + // Indexed plus alpha would be 2 bands + + val format = VipsHelper.image_get_format(image.getUnsafeStructAddress) + logger.info("Format: " + format) + + val paletteType = VipsHelper.image_get_typeof(arena, image.getUnsafeStructAddress, "palette") + logger.info("Palette type: " + paletteType) + + paletteType > 0 || numberOfBands < 3 + } } 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 0000000000000000000000000000000000000000..0ddad07e5f5de86a61bf43d8b89e138d7f1ea8ad GIT binary patch literal 1286 zcmWks4NMkw7=KO>3}y1F=z>r=LEEca@LnZf*~BA0ROZ5~$d#4)lIa>n+tGAx#s~NCh;!h=Q?&7t5~~Lk>mL*|1x+L`Ivp6mn?kJ^kK$c6a~%|NrOteLPR)ru^*4 zh?oc>ipx)wHw!xP<||VqG2mh2yNQ1IZKr30Xr(I7PBXU z(o_;ftk^?X5O*qmM)+A^aUR*s!>r3PlcI=3mc_D63M-aHQVj83+hHDKi)~wG8A%eb zMRFVzAa0kL4aW(lxy(-(A3@r7{KRdCcI^9^ zBwSWlMY4uMp(p@%T`0C7K$rnonuoRmLY9xP#5bTx(RKJi zCYfG^*s^*-{Ro^e4Kgw{Dlmv)JpjQ5bKwF@CLI|%59=nhA>e#HJh5GNjRLr(&Jco- zL=roU($L^w70~)&xPh+uFV&-K!K0w3W-9Hy@g@HWXL3ML4|%5ULen8 zlT@|D2@;VI*iada4446_ptp_#Sul*Cx7Y6>PlSiq8TyN-q=y}cXZX&@20)p=ihIP{CfA$Z*4W@ zEr$zzJ<~t_(0G1&!T9U{_Iz>DPFy@Dgq4(i*+1}U*ZiG1m)32#<4sRFJ5!AP>yZ}L zzdq~5lB&|C6*=qgy?nj!is#SN$*Gwo6M?qJgKJtd1_tvb_xS3qJuRh&eH)f1f0J@< z)4uq(Z_li{H~+s=+3L-xjU7X)ZvEUC-CLA+t^7d7$jI)tsK~;c&XdnBOC5Txp}1)@ zusm-vWl2G`{dnN}K>pDE=;yi{rVB@Y3B-HrtGOePxNz#}j}FD`z3|nlwikX+ZZ56= zAY&lKTUG9w&2^VN89D}o5=^>%~&6ID5>`0fk%7K&zZcY&F#znJTW|E zB5&cFwM~ly35&;HdZq5*>W9rM<&V4Wr%mNt{B`@CxYpR8I;^7!?Tc=wu6t&Du2Ec< zKIiz-xXN3dttqGahAW20Qo6MzgSk7e_{!(?CKyw`*?j|NY=7wG%F`JaWZ(VOm8CUr z$$no_)1uMK%W6)nN!IrL6EB>puUj#i+1xg}<&qUPnY6#j4&NW^MpZNn{t+Cq+^l@L JEAySt{s-!kV`~5a literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..39a7050d27af28f3bcb7bfc6b071cae91f8c091b GIT binary patch literal 429 zcmV;e0aE^nP)eGf1AoYPn=%bN?oV&q3&xZP#{- zd#wGeV8okcyH@o)cboHb1rrf^jB^?sG2T2?@G1n)CJc@hyhCf)&E8%^0~1oF;v>7S z;6sBJiji@r0Yq=mmR;)Ojr>$W46R-Sau7HO7xZFtI1{|4z%-nLbW=B z3o0EAC)h$;8WIR1rohQ#w444G0#1m$Dj66xet|L#R>4C36e7xMM0T)(aSAa2XfV)D zcEU#my$(kCaRx~QL^LgQ_;=)3gHxFOXBZJd{maEJLqY3H<>rDuET^1Yth7z0>+1gi X Try(a.toBoolean).toOption).getOrElse(true) - - // TODO how to get source image colour depth from vips? - // val isGraphic = !colourType.matches("True[ ]?Color.*") - val cropType = Crops.cropType(mimeType, isGraphic = false, hasAlpha = hasAlpha) + val cropType = Crops.cropType(mimeType, isGraphic = isGraphic, hasAlpha = hasAlpha) // High quality rendering with minimal compression which will be used as the CDN resizer origin val masterCropFile = File.createTempFile(s"crop-", s"${cropType.fileExtension}", config.tempDir) // TODO function for this From 48a545021d55048efd2894e39f833bb2bfd5bae4 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sun, 29 Jun 2025 15:52:31 +0100 Subject: [PATCH 52/56] VIPS enabled image loader and cropper can probably use a smaller heap; leaving more RAM for off heap VIPS operations. Cropped 20% heap. --- build.sbt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.sbt b/build.sbt index fb03b0de98..fefbfca64e 100644 --- a/build.sbt +++ b/build.sbt @@ -292,6 +292,6 @@ def playImageLoaderProject(projectName: String, port: Int, path: Option[String] "-Dpidfile.path=/dev/null", s"-Dconfig.file=/opt/docker/conf/application.conf", s"-Dlogger.file=/opt/docker/conf/logback.xml", - "-XX:+PrintCommandLineFlags" + "-XX:+PrintCommandLineFlags", "-XX:MaxRAMPercentage=20" ))) } From 0994c8f275122748843842560cda47cd2f795f18 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Fri, 16 Jan 2026 20:37:44 +0000 Subject: [PATCH 53/56] png Q option is not needed if we are not actually quantising the image. --- .../scala/com/gu/mediaservice/lib/imaging/ImageOperations.scala | 1 - 1 file changed, 1 deletion(-) 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 2eae417fa0..411deecfab 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 @@ -207,7 +207,6 @@ class ImageOperations(playPath: String) extends GridLogging { ) } else { image.pngsave(outputFile.getAbsolutePath, - VipsOption.Int("Q", qual.toInt), VipsOption.Int("bitdepth", depth), VipsOption.Boolean("strip", true) ) From 1a1a582419edc8f338067e91a9b54c5020169600 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 17 Jan 2026 21:00:03 +0000 Subject: [PATCH 54/56] Crop quality values can be int. --- .../gu/mediaservice/lib/imaging/ImageOperations.scala | 10 +++++----- cropper/app/lib/Crops.scala | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) 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 411deecfab..21334e3c62 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 @@ -95,7 +95,7 @@ class ImageOperations(playPath: String) extends GridLogging { def resizeImageVips( sourceImage: VImage, dimensions: Dimensions, - qual: Double = 100d, + quality: Int = 100, tempDir: File, fileType: MimeType, sourceDimensions: Dimensions @@ -105,7 +105,7 @@ class ImageOperations(playPath: String) extends GridLogging { val resized = sourceImage.resize(scale) val outputFile = File.createTempFile(s"resize-", s"${fileType.fileExtension}", tempDir) // TODO function for this - saveImageToFile(resized, fileType, qual, outputFile, quantise = true) + saveImageToFile(resized, fileType, quality, outputFile, quantise = true) } private def orient(op: IMOperation, orientationMetadata: Option[OrientationMetadata]): IMOperation = { @@ -178,12 +178,12 @@ class ImageOperations(playPath: String) extends GridLogging { } } - def saveImageToFile(image: VImage, mimeType: MimeType, qual: Double, outputFile: File, quantise: Boolean = false): File = { + def saveImageToFile(image: VImage, mimeType: MimeType, quality: Int, outputFile: File, quantise: Boolean = false): File = { logger.info(s"Saving image as $mimeType to file: " + outputFile.getAbsolutePath) mimeType match { case Jpeg => image.jpegsave(outputFile.getAbsolutePath, - VipsOption.Int("Q", qual.toInt), + VipsOption.Int("Q", quality), //VipsOption.Boolean("optimize-scans", true), //VipsOption.Boolean("optimize-coding", true), //VipsOption.Boolean("interlace", true), @@ -201,7 +201,7 @@ 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.Boolean("strip", true) ) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index 901cb14df5..f28157e314 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -24,12 +24,12 @@ 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 masterCropQuality = 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. // TODO confirm this for vips - private val pngCropQuality = 1d + private val pngCropQuality = 1 def outputFilename(source: SourceImage, bounds: Bounds, outputWidth: Int, fileType: MimeType, isMaster: Boolean = false, instance: Instance): String = { val masterString: String = if (isMaster) "master/" else "" From ce4886d47b22df6102c246ea9a9cb7f1b83b45a3 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 17 Jan 2026 21:23:02 +0000 Subject: [PATCH 55/56] Test to exercise ImageOperations resize. --- .../lib/imaging/ImageOperationsTest.scala | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) 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 3341ae5995..6df4235fed 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,11 +1,11 @@ package com.gu.mediaservice.lib.imaging import app.photofox.vipsffm.{VImage, Vips} -import com.gu.mediaservice.lib.BrowserViewableImage import com.gu.mediaservice.lib.logging.{LogMarker, MarkerMap} import java.io.File -import com.gu.mediaservice.model.{Dimensions, Instance, Jpeg, MimeType} +import com.gu.mediaservice.model.{Dimensions, Jpeg} +import org.apache.commons.io.FileUtils import org.scalatest.time.{Millis, Span} import org.scalatest.concurrent.ScalaFutures import org.scalatest.funspec.AnyFunSpec @@ -22,6 +22,19 @@ 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() + describe("resize") { + it ("should output resized image to file in chosen format") { + implicit val arena: Arena = Arena.ofConfined + val fullSizedJpegImage = VImage.newFromFile(arena, fileAt("IMG_4403.jpg").getAbsolutePath) + val imageOperations = new ImageOperations("") + + val resized = imageOperations.resizeImageVips(fullSizedJpegImage, Dimensions(140, 100), 85, FileUtils.getTempDirectory, Jpeg, Dimensions(fullSizedJpegImage.getWidth, fullSizedJpegImage.getHeight)) + + arena.close() + resized.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") From 8454725959e88f269cacf695f1e12009cce4dd07 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 17 Jan 2026 21:06:10 +0000 Subject: [PATCH 56/56] Clarify master crop quality values. --- cropper/app/lib/Crops.scala | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index f28157e314..8b8331db3e 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -25,11 +25,10 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera import Files._ private val cropQuality = 75 - private val masterCropQuality = 95 + 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. - // TODO confirm this for vips - private val pngCropQuality = 1 + private val pngMasterCropQuality = 1 // No effort spend compressing the PNG master def outputFilename(source: SourceImage, bounds: Bounds, outputWidth: Int, fileType: MimeType, isMaster: Boolean = false, instance: Instance): String = { val masterString: String = if (isMaster) "master/" else "" @@ -114,9 +113,9 @@ class Crops(config: CropperConfig, store: CropStore, imageOperations: ImageOpera // 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 (mimeType == Png) pngCropQuality else masterCropQuality + val masterQuality = if (mimeType == Png) pngMasterCropQuality else jpegMasterCropQuality - imageOperations.saveImageToFile(masterCrop.image, cropType, quality, masterCropFile) + imageOperations.saveImageToFile(masterCrop.image, cropType, masterQuality, masterCropFile) // Static crops; higher compression val outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions