From a7171af0a34f612d05667f1a5c35a4ca834da082 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 21 Feb 2018 03:40:12 +0100 Subject: [PATCH] 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 --- Gemfile | 1 + Gemfile.lock | 2 ++ app/lib/fast_geometry_parser.rb | 11 +++++++++++ app/models/concerns/account_avatar.rb | 12 +++--------- app/models/concerns/account_header.rb | 12 +++--------- app/models/media_attachment.rb | 27 +++++++++++++++++++-------- app/models/preview_card.rb | 13 +++++++------ app/models/site_upload.rb | 4 ++-- config/application.rb | 1 + lib/paperclip/lazy_thumbnail.rb | 24 ++++++++++++++++++++++++ 10 files changed, 73 insertions(+), 34 deletions(-) create mode 100644 app/lib/fast_geometry_parser.rb create mode 100644 lib/paperclip/lazy_thumbnail.rb diff --git a/Gemfile b/Gemfile index ef744064b2..ad1598af31 100644 --- 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 8e4edb8e12..920262eded 100644 --- 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 index 0000000000..5209c2bc59 --- /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 53d0d876f5..619644c9a4 100644 --- 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 991473d8c0..5ed8a9c832 100644 --- 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 b6e5916cbe..38f88e9f7e 100644 --- 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 716b822436..86eecdfe5b 100644 --- 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 8ffdc83131..641128adfc 100644 --- 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 33981791e8..cd180782c3 100644 --- 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 index 0000000000..594f0ce39c --- /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