From c2e6202da615639e9687a6dfb4c8afb6048fd42b Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Tue, 30 Nov 2021 03:52:42 -0600
Subject: [PATCH] Fix #4920: Wrong color for certain samples.

The problem was that we were stripping color profiles from thumbnails,
but we weren't setting `export_profile: "srgb"` to convert images to
sRGB first. This resulted in wrong colors for images with non-sRGB color
profiles, such as Adobe RGB.

The fix is to convert images to sRGB when possible, while leaving CMYK
and greyscale images alone. We leave CMYK images alone because we can't
convert CMYK to sRGB without losing color. We leave greyscale images
alone if they don't have a color profile, that way they stay as
one-channel greyscale (or two-channel greyscale, in case of alpha)
instead of being converted to three-channel sRGB. However, if a
greyscale image has a color profile, then we have to convert to sRGB,
otherwise the colors would be wrong when we strip the profile.

We also have to set the import profile, otherwise images with broken
embedded color profiles won't have a fallback profile and may get
incorrect colors. In this case we also have to be careful, because we
can't specify an sRGB fallback for greyscale or CMYK images.
---
 app/logical/media_file/image.rb | 45 +++++++++++++++++++++++----------
 test/unit/media_file_test.rb    |  4 +--
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/app/logical/media_file/image.rb b/app/logical/media_file/image.rb
index eb90a2e1a..5e137f7e2 100644
--- a/app/logical/media_file/image.rb
+++ b/app/logical/media_file/image.rb
@@ -6,10 +6,6 @@ class MediaFile::Image < MediaFile
   # http://jcupitt.github.io/libvips/API/current/VipsForeignSave.html#vips-jpegsave
   JPEG_OPTIONS = { Q: 90, background: 255, strip: true, interlace: true, optimize_coding: true }
 
-  # http://jcupitt.github.io/libvips/API/current/libvips-resample.html#vips-thumbnail
-  THUMBNAIL_OPTIONS = { size: :down, linear: false }
-  CROP_OPTIONS = { crop: :attention, linear: false }
-
   def dimensions
     image.size
   rescue Vips::Error
@@ -51,22 +47,37 @@ class MediaFile::Image < MediaFile
     image.interpretation
   end
 
-  # @see https://github.com/jcupitt/libvips/wiki/HOWTO----Image-shrinking
-  # @see http://jcupitt.github.io/libvips/API/current/Using-vipsthumbnail.md.html
-  def preview(width, height)
+  def resize(max_width, max_height, format: :jpeg, **options)
+    # @see https://www.libvips.org/API/current/Using-vipsthumbnail.md.html
+    # @see https://www.libvips.org/API/current/libvips-resample.html#vips-thumbnail
+    if colorspace == :srgb
+      resized_image = preview_frame.image.thumbnail_image(max_width, height: max_height, import_profile: "srgb", export_profile: "srgb", **options)
+    elsif colorspace == :cmyk
+      # Leave CMYK as CMYK for better color accuracy than sRGB.
+      resized_image = preview_frame.image.thumbnail_image(max_width, height: max_height, import_profile: "cmyk", export_profile: "cmyk", intent: :relative, **options)
+    elsif colorspace == :"b-w" && has_embedded_profile?
+      # Convert greyscale to sRGB so that the color profile is properly applied before we strip it.
+      resized_image = preview_frame.image.thumbnail_image(max_width, height: max_height, export_profile: "srgb", **options)
+    elsif colorspace == :"b-w"
+      # Otherwise, leave greyscale without a profile as greyscale because
+      # converting it to sRGB would change it from 1 channel to 3 channels.
+      resized_image = preview_frame.image.thumbnail_image(max_width, height: max_height, **options)
+    else
+      raise NotImplementedError
+    end
+
     output_file = Tempfile.new(["image-preview", ".jpg"])
-    resized_image = preview_frame.image.thumbnail_image(width, height: height, **THUMBNAIL_OPTIONS)
     resized_image.jpegsave(output_file.path, **JPEG_OPTIONS)
 
     MediaFile::Image.new(output_file)
   end
 
-  def crop(width, height)
-    output_file = Tempfile.new(["image-crop", ".jpg"])
-    resized_image = preview_frame.image.thumbnail_image(width, height: height, **CROP_OPTIONS)
-    resized_image.jpegsave(output_file.path, **JPEG_OPTIONS)
+  def preview(max_width, max_height)
+    resize(max_width, max_height, size: :down)
+  end
 
-    MediaFile::Image.new(output_file)
+  def crop(max_width, max_height)
+    resize(max_width, max_height, crop: :attention)
   end
 
   def preview_frame
@@ -89,6 +100,14 @@ class MediaFile::Image < MediaFile
     file_ext == :png && is_animated?
   end
 
+  # Return true if the image has an embedded ICC color profile.
+  def has_embedded_profile?
+    image.icc_import(embedded: true)
+    true
+  rescue Vips::Error
+    false
+  end
+
   # @return [Vips::Image] the Vips image object for the file
   def image
     Vips::Image.new_from_file(file.path, fail: true).autorot
diff --git a/test/unit/media_file_test.rb b/test/unit/media_file_test.rb
index cd0f427c2..c17cdf911 100644
--- a/test/unit/media_file_test.rb
+++ b/test/unit/media_file_test.rb
@@ -361,8 +361,8 @@ class MediaFileTest < ActiveSupport::TestCase
       assert_equal(:cmyk, @image.colorspace)
       assert_equal([197, 256], @image.dimensions)
 
-      assert_equal(3, @preview.channels)
-      assert_equal(:srgb, @preview.colorspace)
+      assert_equal(4, @preview.channels)
+      assert_equal(:cmyk, @preview.colorspace)
       assert_equal([115, 150], @preview.dimensions)
     end
   end