From 9924fb8910e772ae61f3576a0546d35f43eab543 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Thu, 21 Apr 2016 15:28:48 +0100 Subject: [PATCH 01/17] crop images with alpha as pngs --- .../lib/imaging/ImageOperations.scala | 10 +++--- cropper/app/lib/Crops.scala | 31 ++++++++++++++----- 2 files changed, 29 insertions(+), 12 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 40f48d4dd8..3c26af7d5b 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 @@ -48,7 +48,7 @@ object ImageOperations { val source = addImage(sourceFile) val formatter = format(source)("%[JPEG-Colorspace-Name]") - + for { output <- runIdentifyCmd(formatter) colourModel = output.headOption @@ -78,9 +78,9 @@ object ImageOperations { } def cropImage(sourceFile: File, bounds: Bounds, qual: Double = 100d, tempDir: File, - iccColourSpace: Option[String], colourModel: Option[String]): Future[File] = { + iccColourSpace: Option[String], colourModel: Option[String], fileType: String): Future[File] = { for { - outputFile <- createTempFile(s"crop-", ".jpg", tempDir) + outputFile <- createTempFile(s"crop-", s".${fileType}", tempDir) cropSource = addImage(sourceFile) qualified = quality(cropSource)(qual) corrected = correctColour(qualified)(iccColourSpace, colourModel) @@ -101,9 +101,9 @@ object ImageOperations { ).map(_ => sourceFile) } - def resizeImage(sourceFile: File, dimensions: Dimensions, qual: Double = 100d, tempDir: File): Future[File] = { + def resizeImage(sourceFile: File, dimensions: Dimensions, qual: Double = 100d, tempDir: File, fileType: String): Future[File] = { for { - outputFile <- createTempFile(s"resize-", ".jpg", tempDir) + outputFile <- createTempFile(s"resize-", s".${fileType}", tempDir) resizeSource = addImage(sourceFile) qualified = quality(resizeSource)(qual) resized = scale(qualified)(dimensions) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index d39947cb0c..f78e7d6fe1 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -21,33 +21,37 @@ object Crops { import scala.concurrent.ExecutionContext.Implicits.global import Files._ - def outputFilename(source: SourceImage, bounds: Bounds, outputWidth: Int, isMaster: Boolean = false): String = { - s"${source.id}/${Crop.getCropId(bounds)}/${if(isMaster) "master/" else ""}$outputWidth.jpg" + def outputFilename(source: SourceImage, bounds: Bounds, outputWidth: Int, fileType: String, isMaster: Boolean = false): String = { + s"${source.id}/${Crop.getCropId(bounds)}/${if(isMaster) "master/" else ""}$outputWidth.${fileType}" } def createMasterCrop(apiImage: SourceImage, sourceFile: File, crop: Crop, mediaType: String, colourModel: Option[String]): Future[MasterCrop] = { val source = crop.specification val metadata = apiImage.metadata val iccColourSpace = FileMetadataHelper.normalisedIccColourSpace(apiImage.fileMetadata) + val fileType = getFileExtension(mediaType) for { - strip <- ImageOperations.cropImage(sourceFile, source.bounds, 100d, Config.tempDir, iccColourSpace, colourModel) + strip <- ImageOperations.cropImage(sourceFile, source.bounds, 100d, Config.tempDir, iccColourSpace, colourModel, fileType) file <- ImageOperations.appendMetadata(strip, metadata) dimensions = Dimensions(source.bounds.width, source.bounds.height) - filename = outputFilename(apiImage, source.bounds, dimensions.width, true) + filename = outputFilename(apiImage, source.bounds, dimensions.width, fileType, true) sizing = CropStore.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) } def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, mediaType: String): Future[List[Asset]] = { + val fileType = getFileExtension(mediaType) + Future.sequence[Asset, List](dimensionList.map { dimensions => for { - file <- ImageOperations.resizeImage(sourceFile, dimensions, 75d, Config.tempDir) - filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width) + file <- ImageOperations.resizeImage(sourceFile, dimensions, 75d, Config.tempDir, fileType) + filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, fileType) sizing <- CropStore.storeCropSizing(file, filename, mediaType, crop, dimensions) _ <- delete(file) } @@ -75,7 +79,13 @@ object Crops { val source = crop.specification val mediaType = apiImage.source.mimeType.getOrElse(throw MissingMimeType) val secureUrl = apiImage.source.secureUrl.getOrElse(throw MissingSecureSourceUrl) - val cropType = "image/jpeg" + val colourType = apiImage.fileMetadata.colourModelInformation.get("colorType").getOrElse("") + + val cropType = if (mediaType == "image/png" && colourType != "True Color") + "image/png" + else + "image/jpeg" + for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", Config.tempDir) @@ -92,4 +102,11 @@ object Crops { yield ExportResult(apiImage.id, masterSize, sizes) } + + private def getFileExtension(mediaType: String): String = + if (mediaType == "image/png") + "png" + else + "jpg" + } From 27cfa30c38d74d705f45d4a4eb100f36d2d63281 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Thu, 21 Apr 2016 15:42:29 +0100 Subject: [PATCH 02/17] enable cropping of images of type true color with alpha --- media-api/app/lib/ImageExtras.scala | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/media-api/app/lib/ImageExtras.scala b/media-api/app/lib/ImageExtras.scala index 3fc3d0031d..082356aa04 100644 --- a/media-api/app/lib/ImageExtras.scala +++ b/media-api/app/lib/ImageExtras.scala @@ -19,10 +19,16 @@ object ImageExtras { def hasCredit(meta: ImageMetadata) = optToBool(meta.credit) def hasDescription(meta: ImageMetadata) = optToBool(meta.description) - def isInvalidPng(image: Image) = - image.source.mimeType == Some("image/png") && - image.fileMetadata.colourModelInformation.get("colorType").getOrElse("") != "True Color" + def isInvalidPng(image: Image) = + image.source.mimeType match { + case Some("image/png") => { + val colourType = image.fileMetadata.colourModelInformation.get("colorType").getOrElse("") + colourType != "True Color" && colourType != "True Color with Alpha" + } + case _ => false + } + def validityMap(image: Image): Map[String, Boolean] = Map( "missing_credit" -> !hasCredit(image.metadata), "missing_description" -> !hasDescription(image.metadata), From ec6df22e571ecb2694c61fa65e06f3908e0e705d Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Fri, 22 Apr 2016 10:38:54 +0100 Subject: [PATCH 03/17] adjust depth to 8 bits when cropping --- .../lib/imaging/ImageOperations.scala | 19 ++++++++++--------- .../lib/imaging/im4jwrapper/ImageMagick.scala | 1 + 2 files changed, 11 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 3c26af7d5b..37e637b3c5 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 @@ -81,15 +81,16 @@ object ImageOperations { iccColourSpace: Option[String], colourModel: Option[String], fileType: String): Future[File] = { for { outputFile <- createTempFile(s"crop-", s".${fileType}", tempDir) - cropSource = addImage(sourceFile) - qualified = quality(cropSource)(qual) - corrected = correctColour(qualified)(iccColourSpace, colourModel) - converted = applyOutputProfile(corrected) - stripped = stripMeta(converted) - profiled = applyOutputProfile(stripped) - cropped = crop(profiled)(bounds) - addOutput = addDestImage(cropped)(outputFile) - _ <- runConvertCmd(addOutput) + cropSource = addImage(sourceFile) + qualified = quality(cropSource)(qual) + corrected = correctColour(qualified)(iccColourSpace, colourModel) + converted = applyOutputProfile(corrected) + stripped = stripMeta(converted) + profiled = applyOutputProfile(stripped) + cropped = crop(profiled)(bounds) + depthAdjusted = depth(cropped)(8) + addOutput = addDestImage(depthAdjusted)(outputFile) + _ <- runConvertCmd(addOutput) } yield outputFile } 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 ba5348260c..c88c77e214 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 @@ -30,6 +30,7 @@ object ImageMagick { def resize(op: IMOperation)(maxSize: Int): IMOperation = op <| (_.resize(maxSize, maxSize)) def scale(op: IMOperation)(dimensions: Dimensions): IMOperation = op <| (_.scale(dimensions.width, dimensions.height)) def format(op: IMOperation)(definition: String): IMOperation = op <| (_.format(definition)) + def depth(op: IMOperation)(depth: Int): IMOperation = op <| (_.depth(depth)) val useGraphicsMagick = true def runConvertCmd(op: IMOperation): Future[Unit] = Future((new ConvertCmd(useGraphicsMagick)).run(op)) From 9b23c168d39e4ba240367daf9e5b3f91194d334a Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Tue, 26 Apr 2016 15:33:31 +0100 Subject: [PATCH 04/17] reduce size of png crops with pngquant --- .../lib/imaging/ImageOperations.scala | 19 +++++++++++++++++++ cropper/app/lib/Crops.scala | 11 +++++++---- 2 files changed, 26 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 37e637b3c5..82dcb6984c 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 @@ -10,6 +10,7 @@ import com.gu.mediaservice.model.{Asset, Bounds, Dimensions, ImageMetadata} import play.api.libs.concurrent.Execution.Implicits._ import scala.concurrent.Future +import scala.sys.process._ case class ExportResult(id: String, masterCrop: Asset, othersizings: List[Asset]) @@ -114,6 +115,24 @@ object ImageOperations { yield outputFile } + def optimiseImage(resizedFile: File, mediaType: String): Future[File] = { + + if (mediaType == "image/png") { + val fileName: String = resizedFile.getAbsolutePath() + val split = fileName.split('.') + + val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" + Seq("pngquant", "--quality", "45-90", fileName, "--output", optimisedImageName).! + + val file = new File(optimisedImageName) + + Future(file) + } + + else + Future(resizedFile) + } + val thumbUnsharpRadius = 0.5d val thumbUnsharpSigma = 0.5d val thumbUnsharpAmount = 0.8d diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index f78e7d6fe1..afddbed093 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -46,14 +46,17 @@ object Crops { } def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, mediaType: String): Future[List[Asset]] = { + val fileType = getFileExtension(mediaType) Future.sequence[Asset, List](dimensionList.map { dimensions => for { - file <- ImageOperations.resizeImage(sourceFile, dimensions, 75d, Config.tempDir, fileType) - filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, fileType) - sizing <- CropStore.storeCropSizing(file, filename, mediaType, crop, dimensions) - _ <- delete(file) + file <- ImageOperations.resizeImage(sourceFile, dimensions, 75d, Config.tempDir, fileType) + optimisedFile <- ImageOperations.optimiseImage(file, mediaType) + filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, fileType) + sizing <- CropStore.storeCropSizing(optimisedFile, filename, mediaType, crop, dimensions) + _ <- delete(file) + _ <- delete(optimisedFile) } yield sizing }) From 5147f9b5d6e0967be9ba4e843ec7dc794ac840a3 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Thu, 28 Apr 2016 09:39:12 +0100 Subject: [PATCH 05/17] ensure that the correct extension is saved in downloads --- .../main/scala/com/gu/mediaservice/lib/aws/S3.scala | 11 +++++++++-- kahuna/public/js/services/image/downloads.js | 1 + 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/aws/S3.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/aws/S3.scala index 298a87bbbc..66778de7f6 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/aws/S3.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/aws/S3.scala @@ -37,9 +37,16 @@ class S3(credentials: AWSCredentials) { } private def getContentDispositionFilename(image: Image, charset: CharSet): String = { + + val extension = image.source.mimeType match { + case Some("image/jpeg") => "jpg" + case Some("image/png") => "png" + case _ => throw new Exception("Unsupported mime type") + } + val baseFilename: String = image.uploadInfo.filename match { - case Some(f) => s"${removeExtension(f)} (${image.id}).jpg" - case _ => s"${image.id}.jpg" + case Some(f) => s"${removeExtension(f)} (${image.id}).${extension}" + case _ => s"${image.id}.${extension}" } charset match { diff --git a/kahuna/public/js/services/image/downloads.js b/kahuna/public/js/services/image/downloads.js index 5f68b1dee8..00f992afd8 100644 --- a/kahuna/public/js/services/image/downloads.js +++ b/kahuna/public/js/services/image/downloads.js @@ -16,6 +16,7 @@ imageDownloadsService.factory('imageDownloadsService', ['imgops', '$http', funct function imageName(imageData) { const filename = imageData.uploadInfo.filename; const imageId = imageData.id; + const extension = imageData.source.mimeType === 'image/jpeg' ? 'jpg' : 'png'; if (filename) { const basename = stripExtension(filename); From 43310fde0b7dbd1fa1d3ec781359144eed801ad7 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Thu, 28 Apr 2016 15:18:41 +0100 Subject: [PATCH 06/17] update png error messages --- cropper/app/lib/Crops.scala | 2 +- kahuna/public/js/edits/image-editor.html | 2 +- media-api/app/lib/ImageExtras.scala | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index afddbed093..edc6ac8ad8 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -46,7 +46,7 @@ object Crops { } def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, mediaType: String): Future[List[Asset]] = { - + val fileType = getFileExtension(mediaType) Future.sequence[Asset, List](dimensionList.map { dimensions => diff --git a/kahuna/public/js/edits/image-editor.html b/kahuna/public/js/edits/image-editor.html index db1e4bb05e..4a2245aa0b 100644 --- a/kahuna/public/js/edits/image-editor.html +++ b/kahuna/public/js/edits/image-editor.html @@ -70,7 +70,7 @@ help
- PNG: Pngs with transparency not supported! + PNG: This type currently not supported.
diff --git a/media-api/app/lib/ImageExtras.scala b/media-api/app/lib/ImageExtras.scala index 082356aa04..12b6bb2a3c 100644 --- a/media-api/app/lib/ImageExtras.scala +++ b/media-api/app/lib/ImageExtras.scala @@ -11,7 +11,7 @@ object ImageExtras { val validityDescription = Map( "missing_credit" -> "Missing credit information", "missing_description" -> "Missing description", - "is_invalid_png" -> "PNG images with transparency cannot be used" + "is_invalid_png" -> "PNG images with this type cannot be used" ) private def optToBool[T](o: Option[T]): Boolean = @@ -28,7 +28,7 @@ object ImageExtras { } case _ => false } - + def validityMap(image: Image): Map[String, Boolean] = Map( "missing_credit" -> !hasCredit(image.metadata), "missing_description" -> !hasDescription(image.metadata), From 33ecf8f60b3bb201d9af10100b585e9c8cb69457 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Wed, 4 May 2016 11:37:52 +0100 Subject: [PATCH 07/17] add a sealed trait for mime types for cropping --- .../lib/imaging/ImageOperations.scala | 40 +++++++++++++------ cropper/app/lib/Crops.scala | 36 ++++++----------- 2 files changed, 41 insertions(+), 35 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 82dcb6984c..67d0090fbe 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,23 +115,23 @@ object ImageOperations { yield outputFile } - def optimiseImage(resizedFile: File, mediaType: String): Future[File] = { + def optimiseImage(resizedFile: File, mediaType: MimeType): File = - if (mediaType == "image/png") { - val fileName: String = resizedFile.getAbsolutePath() - val split = fileName.split('.') + mediaType.name match { + case "image/png" => { + val fileName: String = resizedFile.getAbsolutePath() + val split = fileName.split('.') - val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" - Seq("pngquant", "--quality", "45-90", fileName, "--output", optimisedImageName).! + val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" + Seq("pngquant", "--quality", "45-90", fileName, "--output", optimisedImageName).! - val file = new File(optimisedImageName) + val file = new File(optimisedImageName) - Future(file) - } + file - else - Future(resizedFile) - } + } + case "image/jpeg" => resizedFile + } val thumbUnsharpRadius = 0.5d val thumbUnsharpSigma = 0.5d @@ -151,4 +151,20 @@ object ImageOperations { _ <- runConvertCmd(addOutput) } yield outputFile } + + sealed trait MimeType { + def name: String + def extension: String + } + + case object Png extends MimeType { + val name = "image/png" + val extension = "png" + } + + case object Jpeg extends MimeType { + val name = "image/jpeg" + val extension = "jpg" + } + } diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index edc6ac8ad8..ad527b0c1f 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -2,6 +2,7 @@ package lib import java.io.File +import com.gu.mediaservice.lib.imaging.ImageOperations.MimeType import com.gu.mediaservice.lib.metadata.FileMetadataHelper import scala.concurrent.Future @@ -25,19 +26,18 @@ object Crops { s"${source.id}/${Crop.getCropId(bounds)}/${if(isMaster) "master/" else ""}$outputWidth.${fileType}" } - def createMasterCrop(apiImage: SourceImage, sourceFile: File, crop: Crop, mediaType: String, colourModel: Option[String]): Future[MasterCrop] = { + def createMasterCrop(apiImage: SourceImage, sourceFile: File, crop: Crop, mediaType: MimeType, colourModel: Option[String]): Future[MasterCrop] = { val source = crop.specification val metadata = apiImage.metadata val iccColourSpace = FileMetadataHelper.normalisedIccColourSpace(apiImage.fileMetadata) - val fileType = getFileExtension(mediaType) for { - strip <- ImageOperations.cropImage(sourceFile, source.bounds, 100d, Config.tempDir, iccColourSpace, colourModel, fileType) + strip <- ImageOperations.cropImage(sourceFile, source.bounds, 100d, Config.tempDir, iccColourSpace, colourModel, mediaType.extension) file <- ImageOperations.appendMetadata(strip, metadata) dimensions = Dimensions(source.bounds.width, source.bounds.height) - filename = outputFilename(apiImage, source.bounds, dimensions.width, fileType, true) - sizing = CropStore.storeCropSizing(file, filename, mediaType, crop, dimensions) + filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType.extension, true) + sizing = CropStore.storeCropSizing(file, filename, mediaType.name, crop, dimensions) dirtyAspect = source.bounds.width.toFloat / source.bounds.height aspect = crop.specification.aspectRatio.flatMap(AspectRatio.clean(_)).getOrElse(dirtyAspect) @@ -45,16 +45,15 @@ object Crops { yield MasterCrop(sizing, file, dimensions, aspect) } - def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, mediaType: String): Future[List[Asset]] = { - - val fileType = getFileExtension(mediaType) + def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, + mediaType: MimeType): Future[List[Asset]] = { Future.sequence[Asset, List](dimensionList.map { dimensions => for { - file <- ImageOperations.resizeImage(sourceFile, dimensions, 75d, Config.tempDir, fileType) - optimisedFile <- ImageOperations.optimiseImage(file, mediaType) - filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, fileType) - sizing <- CropStore.storeCropSizing(optimisedFile, filename, mediaType, crop, dimensions) + file <- ImageOperations.resizeImage(sourceFile, dimensions, 75d, Config.tempDir, mediaType.extension) + optimisedFile = ImageOperations.optimiseImage(file, mediaType) + filename = outputFilename(apiImage, crop.specification.bounds, dimensions.width, mediaType.extension) + sizing <- CropStore.storeCropSizing(optimisedFile, filename, mediaType.extension, crop, dimensions) _ <- delete(file) _ <- delete(optimisedFile) } @@ -85,10 +84,9 @@ object Crops { val colourType = apiImage.fileMetadata.colourModelInformation.get("colorType").getOrElse("") val cropType = if (mediaType == "image/png" && colourType != "True Color") - "image/png" + ImageOperations.Png else - "image/jpeg" - + ImageOperations.Jpeg for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", Config.tempDir) @@ -104,12 +102,4 @@ object Crops { } yield ExportResult(apiImage.id, masterSize, sizes) } - - - private def getFileExtension(mediaType: String): String = - if (mediaType == "image/png") - "png" - else - "jpg" - } From 5081fbe955f2202ec5e7a11613b0e6e161d52861 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Wed, 4 May 2016 14:13:10 +0100 Subject: [PATCH 08/17] better error messages --- kahuna/public/js/edits/image-editor.html | 8 ++++++-- kahuna/public/js/edits/image-editor.js | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/kahuna/public/js/edits/image-editor.html b/kahuna/public/js/edits/image-editor.html index 4a2245aa0b..6cb44aac3f 100644 --- a/kahuna/public/js/edits/image-editor.html +++ b/kahuna/public/js/edits/image-editor.html @@ -67,10 +67,14 @@ Invalid - help + help
- PNG: This type currently not supported. + PNG: This type currently not supported +
diff --git a/kahuna/public/js/edits/image-editor.js b/kahuna/public/js/edits/image-editor.js index cf009a7c9b..5617743199 100644 --- a/kahuna/public/js/edits/image-editor.js +++ b/kahuna/public/js/edits/image-editor.js @@ -48,6 +48,7 @@ imageEditor.controller('ImageEditorCtrl', [ ctrl.status = ctrl.image.data.valid ? 'ready' : 'invalid'; ctrl.usageRights = imageService(ctrl.image).usageRights; ctrl.invalidReasons = ctrl.image.data.invalidReasons; + ctrl.invalidPngHelp = 'Transparent pngs of type Palette are not supported. To crop this image, convert it to True Color and upload it again'; //TODO put collections in their own directive ctrl.addCollection = false; From 32a30ab12166e2f40b4e391de36d0b4a7f664331 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Thu, 5 May 2016 15:43:19 +0100 Subject: [PATCH 09/17] ensure that the extension for downloads is recorded correctly --- kahuna/public/js/services/image/downloads.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kahuna/public/js/services/image/downloads.js b/kahuna/public/js/services/image/downloads.js index 00f992afd8..63265d54d8 100644 --- a/kahuna/public/js/services/image/downloads.js +++ b/kahuna/public/js/services/image/downloads.js @@ -20,9 +20,9 @@ imageDownloadsService.factory('imageDownloadsService', ['imgops', '$http', funct if (filename) { const basename = stripExtension(filename); - return `${basename} (${imageId}).jpg`; + return `${basename} (${imageId}).` + extension; } else { - return `${imageId}.jpg`; + return `${imageId}` + extension; } } From e8092cc56205c83cce79607f56c8b6a5e9d597f7 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Thu, 5 May 2016 15:43:33 +0100 Subject: [PATCH 10/17] better error messaging --- kahuna/public/js/edits/image-editor.html | 2 +- kahuna/public/js/edits/image-editor.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/kahuna/public/js/edits/image-editor.html b/kahuna/public/js/edits/image-editor.html index 6cb44aac3f..498b8a2a91 100644 --- a/kahuna/public/js/edits/image-editor.html +++ b/kahuna/public/js/edits/image-editor.html @@ -72,7 +72,7 @@
PNG: This type currently not supported
diff --git a/kahuna/public/js/edits/image-editor.js b/kahuna/public/js/edits/image-editor.js index 5617743199..1181efe889 100644 --- a/kahuna/public/js/edits/image-editor.js +++ b/kahuna/public/js/edits/image-editor.js @@ -48,7 +48,8 @@ imageEditor.controller('ImageEditorCtrl', [ ctrl.status = ctrl.image.data.valid ? 'ready' : 'invalid'; ctrl.usageRights = imageService(ctrl.image).usageRights; ctrl.invalidReasons = ctrl.image.data.invalidReasons; - ctrl.invalidPngHelp = 'Transparent pngs of type Palette are not supported. To crop this image, convert it to True Color and upload it again'; + ctrl.invalidPngHelp = 'Transparent pngs of type Palette are not supported. ' + + 'To crop this image, convert it to True Color and upload it again'; //TODO put collections in their own directive ctrl.addCollection = false; From 1ec47ce89f085e8ce380569c793e7f1ea990fd9c Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Wed, 11 May 2016 15:18:55 +0100 Subject: [PATCH 11/17] adjust quality parameters --- .../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 67d0090fbe..e3347e0f89 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 @@ -123,7 +123,7 @@ object ImageOperations { val split = fileName.split('.') val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" - Seq("pngquant", "--quality", "45-90", fileName, "--output", optimisedImageName).! + Seq("pngquant", "--quality", "1-85", fileName, "--output", optimisedImageName).! val file = new File(optimisedImageName) From cecd6fb480e6daab4972bbddd75a429bd91fc215 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Thu, 12 May 2016 09:18:51 +0100 Subject: [PATCH 12/17] spinning when waiting for crop to finish --- kahuna/public/js/components/gr-icon/gr-icon.js | 5 +++-- kahuna/public/js/crop/view.html | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/kahuna/public/js/components/gr-icon/gr-icon.js b/kahuna/public/js/components/gr-icon/gr-icon.js index 31efca6ede..188f8554f0 100644 --- a/kahuna/public/js/components/gr-icon/gr-icon.js +++ b/kahuna/public/js/components/gr-icon/gr-icon.js @@ -28,11 +28,12 @@ icon.directive('grIconLabel', [function () { return { restrict: 'E', scope: { - grIcon: '@' + grIcon: '@', + grLoading: '@' }, transclude: 'replace', template: ` - {{grIcon}} + {{grIcon}} ` }; }]); diff --git a/kahuna/public/js/crop/view.html b/kahuna/public/js/crop/view.html index 9569b2c0e4..014f2ffffa 100644 --- a/kahuna/public/js/crop/view.html +++ b/kahuna/public/js/crop/view.html @@ -100,7 +100,7 @@ ng:click="ctrl.callCrop()" ng:disabled="ctrl.cropping" gr-tooltip="Crop [enter]"> - Cropping… + Cropping… From 0a02ac5a82259169505b16d730f4ab00fc195f9b Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Thu, 12 May 2016 14:24:43 +0100 Subject: [PATCH 13/17] update cropper readme --- cropper/README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/cropper/README.md b/cropper/README.md index e69de29bb2..942c6fec41 100644 --- a/cropper/README.md +++ b/cropper/README.md @@ -0,0 +1,11 @@ +# cropper + +## Requirements + +You need to install +* pngquant + +Pngquant should be available from /usr/bin/ + +If installing pngquant with brew, create a symlink from /usr/local/bin/pngquant to /urs/bin/pngquant. + From a0281bf3acf6b04e642aff76a36dc6b4d420d4da Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Mon, 16 May 2016 08:53:41 +0100 Subject: [PATCH 14/17] allow for cropping all kinds of pngs --- kahuna/public/js/edits/image-editor.html | 9 +-------- kahuna/public/js/edits/image-editor.js | 2 -- media-api/app/lib/ImageExtras.scala | 15 ++------------- 3 files changed, 3 insertions(+), 23 deletions(-) diff --git a/kahuna/public/js/edits/image-editor.html b/kahuna/public/js/edits/image-editor.html index 498b8a2a91..de6adb2b05 100644 --- a/kahuna/public/js/edits/image-editor.html +++ b/kahuna/public/js/edits/image-editor.html @@ -67,15 +67,8 @@ Invalid - help + help -
- PNG: This type currently not supported - -
"Missing credit information", - "missing_description" -> "Missing description", - "is_invalid_png" -> "PNG images with this type cannot be used" + "missing_description" -> "Missing description" ) private def optToBool[T](o: Option[T]): Boolean = @@ -20,19 +19,9 @@ object ImageExtras { def hasCredit(meta: ImageMetadata) = optToBool(meta.credit) def hasDescription(meta: ImageMetadata) = optToBool(meta.description) - def isInvalidPng(image: Image) = - image.source.mimeType match { - case Some("image/png") => { - val colourType = image.fileMetadata.colourModelInformation.get("colorType").getOrElse("") - colourType != "True Color" && colourType != "True Color with Alpha" - } - case _ => false - } - def validityMap(image: Image): Map[String, Boolean] = Map( "missing_credit" -> !hasCredit(image.metadata), - "missing_description" -> !hasDescription(image.metadata), - "is_invalid_png" -> isInvalidPng(image) + "missing_description" -> !hasDescription(image.metadata) ) def invalidReasons(validityMap: Map[String, Boolean]) = validityMap From 64fbacdc0799353f002728ab66933ed573e92d5d Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Tue, 17 May 2016 15:49:02 +0100 Subject: [PATCH 15/17] add double quotes to directive --- kahuna/public/js/crop/view.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kahuna/public/js/crop/view.html b/kahuna/public/js/crop/view.html index 014f2ffffa..74186507a0 100644 --- a/kahuna/public/js/crop/view.html +++ b/kahuna/public/js/crop/view.html @@ -100,7 +100,7 @@ ng:click="ctrl.callCrop()" ng:disabled="ctrl.cropping" gr-tooltip="Crop [enter]"> - Cropping… + Cropping… From c9a688d4fdd7e001938531c00de44142eca95bc2 Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Tue, 24 May 2016 14:54:01 +0100 Subject: [PATCH 16/17] pngquant png24 masters --- cropper/app/lib/Crops.scala | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/cropper/app/lib/Crops.scala b/cropper/app/lib/Crops.scala index ad527b0c1f..a1b347ce75 100644 --- a/cropper/app/lib/Crops.scala +++ b/cropper/app/lib/Crops.scala @@ -10,6 +10,7 @@ import scala.concurrent.Future import com.gu.mediaservice.model._ import com.gu.mediaservice.lib.Files import com.gu.mediaservice.lib.imaging.{ImageOperations, ExportResult} +import scala.sys.process._ case object InvalidImage extends Exception("Invalid image cannot be cropped") case object MissingMimeType extends Exception("Missing mimeType from source API") @@ -26,14 +27,28 @@ object Crops { s"${source.id}/${Crop.getCropId(bounds)}/${if(isMaster) "master/" else ""}$outputWidth.${fileType}" } - def createMasterCrop(apiImage: SourceImage, sourceFile: File, crop: Crop, mediaType: MimeType, colourModel: Option[String]): Future[MasterCrop] = { + def createMasterCrop(apiImage: SourceImage, sourceFile: File, crop: Crop, mediaType: MimeType, colourModel: Option[String], + colourType: String): Future[MasterCrop] = { + val source = crop.specification val metadata = apiImage.metadata val iccColourSpace = FileMetadataHelper.normalisedIccColourSpace(apiImage.fileMetadata) for { strip <- ImageOperations.cropImage(sourceFile, source.bounds, 100d, Config.tempDir, iccColourSpace, colourModel, mediaType.extension) - file <- ImageOperations.appendMetadata(strip, metadata) + file: File <- ImageOperations.appendMetadata(strip, metadata) + + + //Before apps and frontend can handle PNG24s we need to pngquant PNG24 master crops + optimisedFile = if (colourType == "True Color with Alpha") { + + val fileName = file.getAbsolutePath() + + + val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" + Seq("pngquant", "--quality", "1-85", fileName, "--output", optimisedImageName).! + new File(optimisedImageName) + } else file dimensions = Dimensions(source.bounds.width, source.bounds.height) filename = outputFilename(apiImage, source.bounds, dimensions.width, mediaType.extension, true) @@ -42,7 +57,7 @@ object Crops { aspect = crop.specification.aspectRatio.flatMap(AspectRatio.clean(_)).getOrElse(dirtyAspect) } - yield MasterCrop(sizing, file, dimensions, aspect) + yield MasterCrop(sizing, optimisedFile, dimensions, aspect) } def createCrops(sourceFile: File, dimensionList: List[Dimensions], apiImage: SourceImage, crop: Crop, @@ -91,7 +106,7 @@ object Crops { for { sourceFile <- tempFileFromURL(secureUrl, "cropSource", "", Config.tempDir) colourModel <- ImageOperations.identifyColourModel(sourceFile, mediaType) - masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, colourModel) + masterCrop <- createMasterCrop(apiImage, sourceFile, crop, cropType, colourModel, colourType) outputDims = dimensionsFromConfig(source.bounds, masterCrop.aspectRatio) :+ masterCrop.dimensions From fe51949bec2f4b2e31440cf1130af6ac7152afcc Mon Sep 17 00:00:00 2001 From: Reetta Vaahtoranta Date: Tue, 24 May 2016 14:58:22 +0100 Subject: [PATCH 17/17] remove redundant lines from pngquanting function --- .../com/gu/mediaservice/lib/imaging/ImageOperations.scala | 5 +---- 1 file changed, 1 insertion(+), 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 e3347e0f89..438be10465 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 @@ -120,14 +120,11 @@ object ImageOperations { mediaType.name match { case "image/png" => { val fileName: String = resizedFile.getAbsolutePath() - val split = fileName.split('.') val optimisedImageName: String = fileName.split('.')(0) + "optimised.png" Seq("pngquant", "--quality", "1-85", fileName, "--output", optimisedImageName).! - val file = new File(optimisedImageName) - - file + new File(optimisedImageName) } case "image/jpeg" => resizedFile