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
This commit is contained in:
Eugen Rochko 2018-02-21 03:40:12 +01:00 committed by GitHub
parent a4fd4ad1d5
commit a7171af0a3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 73 additions and 34 deletions

View file

@ -40,6 +40,7 @@ gem 'omniauth', '~> 1.2'
gem 'doorkeeper', '~> 4.2' gem 'doorkeeper', '~> 4.2'
gem 'fast_blank', '~> 1.0' gem 'fast_blank', '~> 1.0'
gem 'fastimage'
gem 'goldfinger', '~> 2.1' gem 'goldfinger', '~> 2.1'
gem 'hiredis', '~> 0.6' gem 'hiredis', '~> 0.6'
gem 'redis-namespace', '~> 1.5' gem 'redis-namespace', '~> 1.5'

View file

@ -185,6 +185,7 @@ GEM
faraday (0.14.0) faraday (0.14.0)
multipart-post (>= 1.2, < 3) multipart-post (>= 1.2, < 3)
fast_blank (1.0.0) fast_blank (1.0.0)
fastimage (2.1.1)
ffi (1.9.18) ffi (1.9.18)
fog-core (1.45.0) fog-core (1.45.0)
builder builder
@ -641,6 +642,7 @@ DEPENDENCIES
fabrication (~> 2.18) fabrication (~> 2.18)
faker (~> 1.7) faker (~> 1.7)
fast_blank (~> 1.0) fast_blank (~> 1.0)
fastimage
fog-core (~> 1.45) fog-core (~> 1.45)
fog-local (~> 0.4) fog-local (~> 0.4)
fog-openstack (~> 0.1) fog-openstack (~> 0.1)

View file

@ -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

View file

@ -7,15 +7,9 @@ module AccountAvatar
class_methods do class_methods do
def avatar_styles(file) def avatar_styles(file)
styles = {} styles = { original: { geometry: '120x120#', file_geometry_parser: FastGeometryParser } }
geometry = Paperclip::Geometry.from_file(file) styles[:static] = { format: 'png', convert_options: '-coalesce', file_geometry_parser: FastGeometryParser } if file.content_type == 'image/gif'
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 styles
rescue Paperclip::Errors::NotIdentifiedByImageMagickError
{}
end end
private :avatar_styles private :avatar_styles
@ -23,7 +17,7 @@ module AccountAvatar
included do included do
# Avatar upload # 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_content_type :avatar, content_type: IMAGE_MIME_TYPES
validates_attachment_size :avatar, less_than: 2.megabytes validates_attachment_size :avatar, less_than: 2.megabytes
end end

View file

@ -7,15 +7,9 @@ module AccountHeader
class_methods do class_methods do
def header_styles(file) def header_styles(file)
styles = {} styles = { original: { geometry: '700x335#', file_geometry_parser: FastGeometryParser } }
geometry = Paperclip::Geometry.from_file(file) styles[:static] = { format: 'png', convert_options: '-coalesce', file_geometry_parser: FastGeometryParser } if file.content_type == 'image/gif'
styles[:original] = '700x335#' unless geometry.width == 700 && geometry.height == 335
styles[:static] = { format: 'png', convert_options: '-coalesce' } if file.content_type == 'image/gif'
styles styles
rescue Paperclip::Errors::NotIdentifiedByImageMagickError
{}
end end
private :header_styles private :header_styles
@ -23,7 +17,7 @@ module AccountHeader
included do included do
# Header upload # 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_content_type :header, content_type: IMAGE_MIME_TYPES
validates_attachment_size :header, less_than: 2.megabytes validates_attachment_size :header, less_than: 2.megabytes
end end

View file

@ -32,7 +32,18 @@ class MediaAttachment < ApplicationRecord
IMAGE_MIME_TYPES = ['image/jpeg', 'image/png', 'image/gif'].freeze IMAGE_MIME_TYPES = ['image/jpeg', 'image/png', 'image/gif'].freeze
VIDEO_MIME_TYPES = ['video/webm', 'video/mp4'].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 = { VIDEO_STYLES = {
small: { small: {
convert_options: { convert_options: {
@ -167,16 +178,16 @@ class MediaAttachment < ApplicationRecord
end end
def image_geometry(file) 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, width: width,
height: geo.height.to_i, height: height,
size: "#{geo.width.to_i}x#{geo.height.to_i}", size: "#{width}x#{height}",
aspect: geo.width.to_f / geo.height.to_f, aspect: width.to_f / height.to_f,
} }
rescue Paperclip::Errors::NotIdentifiedByImageMagickError
{}
end end
def video_metadata(file) def video_metadata(file)

View file

@ -33,7 +33,7 @@ class PreviewCard < ApplicationRecord
has_and_belongs_to_many :statuses 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 Attachmentable
include Remotable include Remotable
@ -58,10 +58,11 @@ class PreviewCard < ApplicationRecord
return if file.nil? return if file.nil?
geo = Paperclip::Geometry.from_file(file) width, height = FastImage.size(file.path)
self.width = geo.width.to_i
self.height = geo.height.to_i return nil if width.nil?
rescue Paperclip::Errors::NotIdentifiedByImageMagickError
nil self.width = width
self.height = height
end end
end end

View file

@ -34,8 +34,8 @@ class SiteUpload < ApplicationRecord
return if tempfile.nil? return if tempfile.nil?
geometry = Paperclip::Geometry.from_file(tempfile) width, height = FastImage.size(tempfile.path)
self.meta = { width: geometry.width.to_i, height: geometry.height.to_i } self.meta = { width: width, height: height }
end end
def clear_cache def clear_cache

View file

@ -7,6 +7,7 @@ require 'rails/all'
Bundler.require(*Rails.groups) Bundler.require(*Rails.groups)
require_relative '../app/lib/exceptions' require_relative '../app/lib/exceptions'
require_relative '../lib/paperclip/lazy_thumbnail'
require_relative '../lib/paperclip/gif_transcoder' require_relative '../lib/paperclip/gif_transcoder'
require_relative '../lib/paperclip/video_transcoder' require_relative '../lib/paperclip/video_transcoder'
require_relative '../lib/mastodon/snowflake' require_relative '../lib/mastodon/snowflake'

View file

@ -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