Fix avatar and header issues by using custom geometry detector (#6515)

* Fix avatar and header issues by using custom geometry detector

Revert a part of #6508. The file passed to dynamic styles method
was not actually a file, but an instance of Paperclip::Attachment,
which broke all styles by always returning {} from the method.

One problem with GIF avatars was that Paperclip::GeometryDetector
reported wrong dimensions for them, e.g. 120x120 GIF avatar would
for some reason be detected as 120x53. By writing our own geometry
parser, we can use FastImage, which also happens to be faster than
ImageMagick, to detect image dimensions, which are also correct.

Unfortunately, this PR does not implement skipping a `convert`
entirely if the dimensions are already correct, as I found no easy
way to write that behaviour into Paperclip without rewriting the
Paperclip::Thumbnail class.

* Only invoke convert if dimension or format needs to be changed

diff --git a/Gemfile b/Gemfile
index ef744064b21bc260a5d19dbf38fb7737854b0636..ad1598af31af7a9bd4b38adff7745cc3dd488834 100644 (file)
--- a/Gemfile
+++ b/Gemfile
@@ -40,6 +40,7 @@ gem 'omniauth', '~> 1.2'
 
 gem 'doorkeeper', '~> 4.2'
 gem 'fast_blank', '~> 1.0'
+gem 'fastimage'
 gem 'goldfinger', '~> 2.1'
 gem 'hiredis', '~> 0.6'
 gem 'redis-namespace', '~> 1.5'

diff --git a/Gemfile.lock b/Gemfile.lock
index 8e4edb8e12035042359d7e38cf2eefa94a7c7d57..920262ededb7dbfbfc95da98a9a2a0d7195ec5cc 100644 (file)
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -185,6 +185,7 @@ GEM
     faraday (0.14.0)
       multipart-post (>= 1.2, < 3)
     fast_blank (1.0.0)
+    fastimage (2.1.1)
     ffi (1.9.18)
     fog-core (1.45.0)
       builder
@@ -641,6 +642,7 @@ DEPENDENCIES
   fabrication (~> 2.18)
   faker (~> 1.7)
   fast_blank (~> 1.0)
+  fastimage
   fog-core (~> 1.45)
   fog-local (~> 0.4)
   fog-openstack (~> 0.1)

diff --git a/app/lib/fast_geometry_parser.rb b/app/lib/fast_geometry_parser.rb
new file mode 100644 (file)
index 0000000..5209c2b
--- /dev/null
+++ b/app/lib/fast_geometry_parser.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class FastGeometryParser
+  def self.from_file(file)
+    width, height = FastImage.size(file.path)
+
+    raise Paperclip::Errors::NotIdentifiedByImageMagickError if width.nil?
+
+    Paperclip::Geometry.new(width, height)
+  end
+end

diff --git a/app/models/concerns/account_avatar.rb b/app/models/concerns/account_avatar.rb
index 53d0d876f59e913b8b4e9705830c5b81dd96b250..619644c9a4f2b241cc42dd6d2ad534b73a8d8638 100644 (file)
--- a/app/models/concerns/account_avatar.rb
+++ b/app/models/concerns/account_avatar.rb
@@ -7,15 +7,9 @@ module AccountAvatar
 
   class_methods do
     def avatar_styles(file)
-      styles   = {}
-      geometry = Paperclip::Geometry.from_file(file)
-
-      styles[:original] = '120x120#' if geometry.width != geometry.height || geometry.width > 120 || geometry.height > 120
-      styles[:static]   = { format: 'png', convert_options: '-coalesce' } if file.content_type == 'image/gif'
-
+      styles = { original: { geometry: '120x120#', file_geometry_parser: FastGeometryParser } }
+      styles[:static] = { format: 'png', convert_options: '-coalesce', file_geometry_parser: FastGeometryParser } if file.content_type == 'image/gif'
       styles
-    rescue Paperclip::Errors::NotIdentifiedByImageMagickError
-      {}
     end
 
     private :avatar_styles
@@ -23,7 +17,7 @@ module AccountAvatar
 
   included do
     # Avatar upload
-    has_attached_file :avatar, styles: ->(f) { avatar_styles(f) }, convert_options: { all: '-strip' }
+    has_attached_file :avatar, styles: ->(f) { avatar_styles(f) }, convert_options: { all: '-strip' }, processors: [:lazy_thumbnail]
     validates_attachment_content_type :avatar, content_type: IMAGE_MIME_TYPES
     validates_attachment_size :avatar, less_than: 2.megabytes
   end

diff --git a/app/models/concerns/account_header.rb b/app/models/concerns/account_header.rb
index 991473d8c0f7073a925a9a9f479a4765343c74f6..5ed8a9c832109a9547caf5c71af35095fa6d6ce9 100644 (file)
--- a/app/models/concerns/account_header.rb
+++ b/app/models/concerns/account_header.rb
@@ -7,15 +7,9 @@ module AccountHeader
 
   class_methods do
     def header_styles(file)
-      styles   = {}
-      geometry = Paperclip::Geometry.from_file(file)
-
-      styles[:original] = '700x335#' unless geometry.width == 700 && geometry.height == 335
-      styles[:static]   = { format: 'png', convert_options: '-coalesce' } if file.content_type == 'image/gif'
-
+      styles = { original: { geometry: '700x335#', file_geometry_parser: FastGeometryParser } }
+      styles[:static] = { format: 'png', convert_options: '-coalesce', file_geometry_parser: FastGeometryParser } if file.content_type == 'image/gif'
       styles
-    rescue Paperclip::Errors::NotIdentifiedByImageMagickError
-      {}
     end
 
     private :header_styles
@@ -23,7 +17,7 @@ module AccountHeader
 
   included do
     # Header upload
-    has_attached_file :header, styles: ->(f) { header_styles(f) }, convert_options: { all: '-strip' }
+    has_attached_file :header, styles: ->(f) { header_styles(f) }, convert_options: { all: '-strip' }, processors: [:lazy_thumbnail]
     validates_attachment_content_type :header, content_type: IMAGE_MIME_TYPES
     validates_attachment_size :header, less_than: 2.megabytes
   end

diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb
index b6e5916cbed4e74b6577b08665464d95925cc99a..38f88e9f7eb0fc6c35fc819e1e063ca8234b2caf 100644 (file)
--- a/app/models/media_attachment.rb
+++ b/app/models/media_attachment.rb
@@ -32,7 +32,18 @@ class MediaAttachment < ApplicationRecord
   IMAGE_MIME_TYPES = ['image/jpeg', 'image/png', 'image/gif'].freeze
   VIDEO_MIME_TYPES = ['video/webm', 'video/mp4'].freeze
 
-  IMAGE_STYLES = { original: '1280x1280>', small: '400x400>' }.freeze
+  IMAGE_STYLES = {
+    original: {
+      geometry: '1280x1280>',
+      file_geometry_parser: FastGeometryParser,
+    },
+
+    small: {
+      geometry: '400x400>',
+      file_geometry_parser: FastGeometryParser,
+    },
+  }.freeze
+
   VIDEO_STYLES = {
     small: {
       convert_options: {
@@ -167,16 +178,16 @@ class MediaAttachment < ApplicationRecord
   end
 
   def image_geometry(file)
-    geo = Paperclip::Geometry.from_file file
+    width, height = FastImage.size(file.path)
+
+    return {} if width.nil?
 
     {
-      width:  geo.width.to_i,
-      height: geo.height.to_i,
-      size: "#{geo.width.to_i}x#{geo.height.to_i}",
-      aspect: geo.width.to_f / geo.height.to_f,
+      width:  width,
+      height: height,
+      size: "#{width}x#{height}",
+      aspect: width.to_f / height.to_f,
     }
-  rescue Paperclip::Errors::NotIdentifiedByImageMagickError
-    {}
   end
 
   def video_metadata(file)

diff --git a/app/models/preview_card.rb b/app/models/preview_card.rb
index 716b8224361f8fb80a94f4d136eb19b914e53932..86eecdfe5b0ebc41dcf91d49027d44cc2b4296a4 100644 (file)
--- a/app/models/preview_card.rb
+++ b/app/models/preview_card.rb
@@ -33,7 +33,7 @@ class PreviewCard < ApplicationRecord
 
   has_and_belongs_to_many :statuses
 
-  has_attached_file :image, styles: { original: '400x400>' }, convert_options: { all: '-quality 80 -strip' }
+  has_attached_file :image, styles: { original: { geometry: '400x400>', file_geometry_parser: FastGeometryParser } }, convert_options: { all: '-quality 80 -strip' }
 
   include Attachmentable
   include Remotable
@@ -58,10 +58,11 @@ class PreviewCard < ApplicationRecord
 
     return if file.nil?
 
-    geo         = Paperclip::Geometry.from_file(file)
-    self.width  = geo.width.to_i
-    self.height = geo.height.to_i
-  rescue Paperclip::Errors::NotIdentifiedByImageMagickError
-    nil
+    width, height = FastImage.size(file.path)
+
+    return nil if width.nil?
+
+    self.width  = width
+    self.height = height
   end
 end

diff --git a/app/models/site_upload.rb b/app/models/site_upload.rb
index 8ffdc831313ad93cd61bbacdabee8697ca3a6693..641128adfc81d0428eabaee4682e4b185d00d519 100644 (file)
--- a/app/models/site_upload.rb
+++ b/app/models/site_upload.rb
@@ -34,8 +34,8 @@ class SiteUpload < ApplicationRecord
 
     return if tempfile.nil?
 
-    geometry  = Paperclip::Geometry.from_file(tempfile)
-    self.meta = { width: geometry.width.to_i, height: geometry.height.to_i }
+    width, height = FastImage.size(tempfile.path)
+    self.meta = { width: width, height: height }
   end
 
   def clear_cache

diff --git a/config/application.rb b/config/application.rb
index 33981791e8d4b94e4dc9e12b03d029da91a993c9..cd180782c3cb4a75c32ed5f1f55750c99b80006e 100644 (file)
--- a/config/application.rb
+++ b/config/application.rb
@@ -7,6 +7,7 @@ require 'rails/all'
 Bundler.require(*Rails.groups)
 
 require_relative '../app/lib/exceptions'
+require_relative '../lib/paperclip/lazy_thumbnail'
 require_relative '../lib/paperclip/gif_transcoder'
 require_relative '../lib/paperclip/video_transcoder'
 require_relative '../lib/mastodon/snowflake'

diff --git a/lib/paperclip/lazy_thumbnail.rb b/lib/paperclip/lazy_thumbnail.rb
new file mode 100644 (file)
index 0000000..594f0ce
--- /dev/null
+++ b/lib/paperclip/lazy_thumbnail.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module Paperclip
+  class LazyThumbnail < Paperclip::Thumbnail
+    def make
+      return @file unless needs_convert?
+      Paperclip::Thumbnail.make(file, options, attachment)
+    end
+
+    private
+
+    def needs_convert?
+      needs_different_geometry? || needs_different_format?
+    end
+
+    def needs_different_geometry?
+      !@target_geometry.nil? && @current_geometry.width != @target_geometry.width && @current_geometry.height != @target_geometry.height
+    end
+
+    def needs_different_format?
+      @format.present? && @current_format != @format
+    end
+  end
+end