From 43e52a7dc1d9e610792ff37755d26b1168ecf20d Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Fri, 16 Oct 2020 13:41:27 +0300 Subject: [PATCH] DEV: Remove gifsicle dependency (#10357) Dependency on gifsicle, allow_animated_avatars and allow_animated_thumbnails site settings were all removed. Animated GIF images are still allowed, but the generated optimized images are no longer animated for those (which were used for avatars and thumbnails). The added 'animated' is populated by extracting information using FastImage. This field was used to selectively reoptimize old animations. This process happens in the background. --- .github/workflows/ci.yml | 2 +- .../discourse/tests/helpers/site-settings.js | 1 - app/controllers/user_avatars_controller.rb | 2 +- app/jobs/regular/create_avatar_thumbnails.rb | 7 +--- app/jobs/scheduled/update_animated_uploads.rb | 23 ++++++++++++ app/models/optimized_image.rb | 35 ------------------- app/models/upload.rb | 8 +++-- config/locales/server.en.yml | 2 -- config/site_settings.yml | 4 --- .../20200707183007_add_animated_to_uploads.rb | 7 ++++ ...ete_allow_animated_avatars_site_setting.rb | 11 ++++++ ..._allow_animated_thumbnails_site_setting.rb | 11 ++++++ docs/DEVELOPER-ADVANCED.md | 2 +- docs/DEVELOPMENT-OSX-NATIVE.md | 2 +- lib/upload_creator.rb | 14 +++----- spec/jobs/update_animated_uploads_spec.rb | 18 ++++++++++ 16 files changed, 85 insertions(+), 64 deletions(-) create mode 100644 app/jobs/scheduled/update_animated_uploads.rb create mode 100644 db/migrate/20200707183007_add_animated_to_uploads.rb create mode 100644 db/migrate/20200714105026_delete_allow_animated_avatars_site_setting.rb create mode 100644 db/migrate/20200714105027_delete_allow_animated_thumbnails_site_setting.rb create mode 100644 spec/jobs/update_animated_uploads_spec.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3eb5f267b7..8e4c2b6ac1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -65,7 +65,7 @@ jobs: if: env.BUILD_TYPE != 'LINT' run: | sudo apt-get update - sudo apt-get -yqq install postgresql-client libpq-dev gifsicle jpegoptim optipng jhead + sudo apt-get -yqq install postgresql-client libpq-dev jpegoptim optipng jhead wget -qO- https://raw.githubusercontent.com/discourse/discourse_docker/master/image/base/install-pngquant | sudo sh - name: Update imagemagick diff --git a/app/assets/javascripts/discourse/tests/helpers/site-settings.js b/app/assets/javascripts/discourse/tests/helpers/site-settings.js index ba52f3f9b5..d84b53e4c7 100644 --- a/app/assets/javascripts/discourse/tests/helpers/site-settings.js +++ b/app/assets/javascripts/discourse/tests/helpers/site-settings.js @@ -63,7 +63,6 @@ const ORIGINAL_SETTINGS = { max_image_height: 500, allow_profile_backgrounds: true, allow_uploaded_avatars: true, - allow_animated_avatars: false, tl1_requires_read_posts: 30, enable_long_polling: true, polling_interval: 3000, diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb index be9118855f..583fc81e65 100644 --- a/app/controllers/user_avatars_controller.rb +++ b/app/controllers/user_avatars_controller.rb @@ -189,7 +189,7 @@ class UserAvatarsController < ApplicationController return if !upload return upload if upload.extension == "svg" - upload.get_optimized_image(size, size, allow_animation: SiteSetting.allow_animated_avatars) + upload.get_optimized_image(size, size) # TODO decide if we want to detach here end diff --git a/app/jobs/regular/create_avatar_thumbnails.rb b/app/jobs/regular/create_avatar_thumbnails.rb index 540c5803d5..51988a4764 100644 --- a/app/jobs/regular/create_avatar_thumbnails.rb +++ b/app/jobs/regular/create_avatar_thumbnails.rb @@ -14,12 +14,7 @@ module Jobs return unless upload = Upload.find_by(id: upload_id) Discourse.avatar_sizes.each do |size| - OptimizedImage.create_for( - upload, - size, - size, - allow_animation: SiteSetting.allow_animated_avatars - ) + OptimizedImage.create_for(upload, size, size) end end diff --git a/app/jobs/scheduled/update_animated_uploads.rb b/app/jobs/scheduled/update_animated_uploads.rb new file mode 100644 index 0000000000..2938911b75 --- /dev/null +++ b/app/jobs/scheduled/update_animated_uploads.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Jobs + class UpdateAnimatedUploads < ::Jobs::Scheduled + every 1.hour + + MAX_PROCESSED_GIF_IMAGES ||= 200 + + def execute(args) + Upload + .where("extension = 'gif' OR (extension IS NULL AND original_filename LIKE '%.gif')") + .where(animated: nil) + .limit(MAX_PROCESSED_GIF_IMAGES) + .each do |upload| + uri = Discourse.store.path_for(upload) || upload.url + upload.update!(animated: FastImage.animated?(uri)) + upload.optimized_images.destroy_all if upload.animated + end + + nil + end + end +end diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 589cc5f156..42a62b98f7 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -234,20 +234,6 @@ class OptimizedImage < ActiveRecord::Base }) end - def self.resize_instructions_animated(from, to, dimensions, opts = {}) - ensure_safe_paths!(from, to) - resize_method = opts[:scale_image] ? "scale" : "resize-fit" - - %W{ - gifsicle - --colors=#{opts[:colors] || 256} - --#{resize_method} #{dimensions} - --optimize=3 - --output #{to} - #{from} - } - end - def self.crop_instructions(from, to, dimensions, opts = {}) ensure_safe_paths!(from, to) @@ -270,19 +256,6 @@ class OptimizedImage < ActiveRecord::Base } end - def self.crop_instructions_animated(from, to, dimensions, opts = {}) - ensure_safe_paths!(from, to) - - %W{ - gifsicle - --crop 0,0+#{dimensions} - --colors=#{opts[:colors] || 256} - --optimize=3 - --output #{to} - #{from} - } - end - def self.downsize_instructions(from, to, dimensions, opts = {}) ensure_safe_paths!(from, to) @@ -302,10 +275,6 @@ class OptimizedImage < ActiveRecord::Base } end - def self.downsize_instructions_animated(from, to, dimensions, opts = {}) - resize_instructions_animated(from, to, dimensions, opts) - end - def self.resize(from, to, width, height, opts = {}) optimize("resize", from, to, "#{width}x#{height}", opts) end @@ -322,10 +291,6 @@ class OptimizedImage < ActiveRecord::Base def self.optimize(operation, from, to, dimensions, opts = {}) method_name = "#{operation}_instructions" - if !!opts[:allow_animation] && (from =~ /\.GIF$/i) - method_name += "_animated" - end - instructions = self.public_send(method_name.to_sym, from, to, dimensions, opts) convert_with(instructions, to, opts) end diff --git a/app/models/upload.rb b/app/models/upload.rb index 36fc6da0a2..27d8825fdd 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -78,7 +78,6 @@ class Upload < ActiveRecord::Base def create_thumbnail!(width, height, opts = nil) return unless SiteSetting.create_thumbnails? opts ||= {} - opts[:allow_animation] = SiteSetting.allow_animated_thumbnails if get_optimized_image(width, height, opts) save(validate: false) @@ -86,7 +85,9 @@ class Upload < ActiveRecord::Base end # this method attempts to correct old incorrect extensions - def get_optimized_image(width, height, opts) + def get_optimized_image(width, height, opts = nil) + opts ||= {} + if (!extension || extension.length == 0) fix_image_extension end @@ -461,11 +462,12 @@ end # secure :boolean default(FALSE), not null # access_control_post_id :bigint # original_sha1 :string -# verified :boolean # verification_status :integer default(1), not null +# animated :boolean # # Indexes # +# idx_uploads_on_verification_status (verification_status) # index_uploads_on_access_control_post_id (access_control_post_id) # index_uploads_on_etag (etag) # index_uploads_on_extension (lower((extension)::text)) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 2ef198f219..16455b5f21 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2000,8 +2000,6 @@ en: logout_redirect: "Location to redirect browser to after logout (eg: https://example.com/logout)" allow_uploaded_avatars: "Allow users to upload custom profile pictures." - allow_animated_avatars: "Allow users to use animated gif profile pictures. WARNING: run the avatars:refresh rake task after changing this setting." - allow_animated_thumbnails: "Generates animated thumbnails of animated gifs." default_avatars: "URLs to avatars that will be used by default for new users until they change them." automatically_download_gravatars: "Download Gravatars for users upon account creation or email change." digest_topics: "The maximum number of popular topics to display in the email summary." diff --git a/config/site_settings.yml b/config/site_settings.yml index 42722d958f..0483fed283 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1308,10 +1308,6 @@ files: allow_uploaded_avatars: client: true default: true - allow_animated_avatars: - client: true - default: false - allow_animated_thumbnails: true default_avatars: default: "" type: url_list diff --git a/db/migrate/20200707183007_add_animated_to_uploads.rb b/db/migrate/20200707183007_add_animated_to_uploads.rb new file mode 100644 index 0000000000..3dec32c836 --- /dev/null +++ b/db/migrate/20200707183007_add_animated_to_uploads.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddAnimatedToUploads < ActiveRecord::Migration[6.0] + def change + add_column :uploads, :animated, :boolean + end +end diff --git a/db/migrate/20200714105026_delete_allow_animated_avatars_site_setting.rb b/db/migrate/20200714105026_delete_allow_animated_avatars_site_setting.rb new file mode 100644 index 0000000000..b1ab2f56a1 --- /dev/null +++ b/db/migrate/20200714105026_delete_allow_animated_avatars_site_setting.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class DeleteAllowAnimatedAvatarsSiteSetting < ActiveRecord::Migration[6.0] + def up + execute "DELETE FROM site_settings WHERE name = 'allow_animated_avatars'" + end + + def down + raise ActiveRecord::IrreversibleMigration.new + end +end diff --git a/db/migrate/20200714105027_delete_allow_animated_thumbnails_site_setting.rb b/db/migrate/20200714105027_delete_allow_animated_thumbnails_site_setting.rb new file mode 100644 index 0000000000..e0e98f8289 --- /dev/null +++ b/db/migrate/20200714105027_delete_allow_animated_thumbnails_site_setting.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class DeleteAllowAnimatedThumbnailsSiteSetting < ActiveRecord::Migration[6.0] + def up + execute "DELETE FROM site_settings WHERE name = 'allow_animated_thumbnails'" + end + + def down + raise ActiveRecord::IrreversibleMigration.new + end +end diff --git a/docs/DEVELOPER-ADVANCED.md b/docs/DEVELOPER-ADVANCED.md index 59527a289a..95f0295f4e 100644 --- a/docs/DEVELOPER-ADVANCED.md +++ b/docs/DEVELOPER-ADVANCED.md @@ -12,7 +12,7 @@ To get your Ubuntu 16.04 or 18.04 LTS install up and running to develop Discours whoami > /tmp/username sudo add-apt-repository ppa:chris-lea/redis-server sudo apt-get -yqq update - sudo apt-get -yqq install software-properties-common vim curl expect debconf-utils git-core build-essential zlib1g-dev libssl-dev openssl libcurl4-openssl-dev libreadline6-dev libpcre3 libpcre3-dev imagemagick redis-server advancecomp gifsicle jhead jpegoptim libjpeg-turbo-progs optipng pngcrush pngquant gnupg2 + sudo apt-get -yqq install software-properties-common vim curl expect debconf-utils git-core build-essential zlib1g-dev libssl-dev openssl libcurl4-openssl-dev libreadline6-dev libpcre3 libpcre3-dev imagemagick redis-server advancecomp jhead jpegoptim libjpeg-turbo-progs optipng pngcrush pngquant gnupg2 # Ruby curl -sSL https://rvm.io/mpapis.asc | gpg2 --import - diff --git a/docs/DEVELOPMENT-OSX-NATIVE.md b/docs/DEVELOPMENT-OSX-NATIVE.md index 93721f51f3..f34cbcc922 100644 --- a/docs/DEVELOPMENT-OSX-NATIVE.md +++ b/docs/DEVELOPMENT-OSX-NATIVE.md @@ -223,7 +223,7 @@ In addition to ImageMagick we also need to install some other image related software: ```sh -brew install gifsicle jpegoptim optipng jhead +brew install jpegoptim optipng jhead npm install -g svgo ``` diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 69985a947f..bdfb26cdf1 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -126,6 +126,7 @@ class UploadCreator if is_image @upload.thumbnail_width, @upload.thumbnail_height = ImageSizer.resize(*@image_info.size) @upload.width, @upload.height = @image_info.size + @upload.animated = FastImage.animated?(@file) end add_metadata! @@ -279,7 +280,6 @@ class UploadCreator from, to, scale, - allow_animation: allow_animation, scale_image: true, raise_on_error: true ) @@ -351,17 +351,17 @@ class UploadCreator case @opts[:type] when "avatar" width = height = Discourse.avatar_sizes.max - OptimizedImage.resize(@file.path, @file.path, width, height, filename: filename_with_correct_ext, allow_animation: allow_animation) + OptimizedImage.resize(@file.path, @file.path, width, height, filename: filename_with_correct_ext) when "profile_background" max_width = 850 * max_pixel_ratio width, height = ImageSizer.resize(@image_info.size[0], @image_info.size[1], max_width: max_width, max_height: max_width) - OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\>", filename: filename_with_correct_ext, allow_animation: allow_animation) + OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\>", filename: filename_with_correct_ext) when "card_background" max_width = 590 * max_pixel_ratio width, height = ImageSizer.resize(@image_info.size[0], @image_info.size[1], max_width: max_width, max_height: max_width) - OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\>", filename: filename_with_correct_ext, allow_animation: allow_animation) + OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\>", filename: filename_with_correct_ext) when "custom_emoji" - OptimizedImage.downsize(@file.path, @file.path, "100x100\>", filename: filename_with_correct_ext, allow_animation: allow_animation) + OptimizedImage.downsize(@file.path, @file.path, "100x100\>", filename: filename_with_correct_ext) end extract_image_info! @@ -401,10 +401,6 @@ class UploadCreator @image_info.size&.reduce(:*).to_i end - def allow_animation - @allow_animation ||= @opts[:type] == "avatar" ? SiteSetting.allow_animated_avatars : SiteSetting.allow_animated_thumbnails - end - def svg_allowlist_xpath @@svg_allowlist_xpath ||= "//*[#{ALLOWED_SVG_ELEMENTS.map { |e| "name()!='#{e}'" }.join(" and ") }]" end diff --git a/spec/jobs/update_animated_uploads_spec.rb b/spec/jobs/update_animated_uploads_spec.rb new file mode 100644 index 0000000000..65e2e8e39e --- /dev/null +++ b/spec/jobs/update_animated_uploads_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Jobs::UpdateAnimatedUploads do + let!(:upload) { Fabricate(:upload) } + let!(:gif_upload) { Fabricate(:upload, extension: "gif") } + + it "affects only GIF uploads" do + url = Discourse.store.path_for(gif_upload) || gif_upload.url + FastImage.expects(:animated?).with(url).returns(true).once + + described_class.new.execute({}) + + expect(upload.reload.animated).to eq(nil) + expect(gif_upload.reload.animated).to eq(true) + end +end