From 9ab1fb7dfc349293e433d4d3e78518d7d2ba1b06 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 28 Aug 2018 12:48:43 +1000 Subject: [PATCH] FEATURE: correctly store width and height on uploads Previously we used width and height for thumbnails, new code ensures 1. We auto correct width and height 2. We added extra columns for thumbnail_width and height, this is determined by actual upload and no longer passed in as a side effect 3. Optimized Image now stores filesize which can be used for analysis, decisions Also - fixes Android image manifest as a side effect - fixes issue where a thumbnail generated that is smaller than the upload is no longer used --- .../discourse/lib/utilities.js.es6 | 5 +- app/jobs/onceoff/clear_width_and_height.rb | 15 ++++++ app/models/optimized_image.rb | 28 ++++++++++ app/models/upload.rb | 51 +++++++++++++++++-- app/serializers/upload_serializer.rb | 2 + ...053514_add_filesize_to_optimized_images.rb | 7 +++ lib/cooked_post_processor.rb | 12 ++++- lib/upload_creator.rb | 3 +- spec/models/optimized_image_spec.rb | 41 ++++++++++----- spec/models/upload_spec.rb | 16 ++++++ test/javascripts/lib/utilities-test.js.es6 | 4 +- 11 files changed, 159 insertions(+), 25 deletions(-) create mode 100644 app/jobs/onceoff/clear_width_and_height.rb create mode 100644 db/migrate/20180827053514_add_filesize_to_optimized_images.rb diff --git a/app/assets/javascripts/discourse/lib/utilities.js.es6 b/app/assets/javascripts/discourse/lib/utilities.js.es6 index 1829e36dfe9..bfb76fbbc34 100644 --- a/app/assets/javascripts/discourse/lib/utilities.js.es6 +++ b/app/assets/javascripts/discourse/lib/utilities.js.es6 @@ -441,8 +441,9 @@ export function uploadLocation(url) { export function getUploadMarkdown(upload) { if (isAnImage(upload.original_filename)) { const name = imageNameFromFileName(upload.original_filename); - return `![${name}|${upload.width}x${upload.height}](${upload.short_url || - upload.url})`; + return `![${name}|${upload.thumbnail_width}x${ + upload.thumbnail_height + }](${upload.short_url || upload.url})`; } else if ( !Discourse.SiteSettings.prevent_anons_from_downloading_files && /\.(mov|mp4|webm|ogv|mp3|ogg|wav|m4a)$/i.test(upload.original_filename) diff --git a/app/jobs/onceoff/clear_width_and_height.rb b/app/jobs/onceoff/clear_width_and_height.rb new file mode 100644 index 00000000000..226a863023a --- /dev/null +++ b/app/jobs/onceoff/clear_width_and_height.rb @@ -0,0 +1,15 @@ +module Jobs + class ClearWidthAndHeight < Jobs::Onceoff + def execute_onceoff(args) + # we have to clear all old uploads cause + # we could have old versions of height / width + # this column used to store thumbnail size instead of + # actual size + DB.exec(<<~SQL) + UPDATE uploads + SET width = null, height = null + WHERE width IS NOT NULL OR height IS NOT NULL + SQL + end + end +end diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index a344d4f5fc5..55e4080a022 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -81,6 +81,7 @@ class OptimizedImage < ActiveRecord::Base end if resized + thumbnail = OptimizedImage.create!( upload_id: upload.id, sha1: Upload.generate_digest(temp_path), @@ -88,6 +89,7 @@ class OptimizedImage < ActiveRecord::Base width: width, height: height, url: "", + filesize: File.size(temp_path) ) # store the optimized image and update its url File.open(temp_path) do |file| @@ -123,6 +125,32 @@ class OptimizedImage < ActiveRecord::Base !(url =~ /^(https?:)?\/\//) end + def calculate_filesize + path = + if local? + Discourse.store.path_for(self) + else + Discourse.store.download(self).path + end + File.size(path) + end + + def filesize + if size = read_attribute(:filesize) + size + else + # we may have a bad optimized image so just skip for now + # and do not break here + size = calculate_filesize rescue nil + + write_attribute(:filesize, size) + if !new_record? + update_columns(filesize: size) + end + size + end + end + def self.safe_path?(path) # this matches instructions which call #to_s path = path.to_s diff --git a/app/models/upload.rb b/app/models/upload.rb index 276681105f9..25d7ecc4722 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -24,7 +24,7 @@ class Upload < ActiveRecord::Base validates_with ::Validators::UploadValidator - def thumbnail(width = self.width, height = self.height) + def thumbnail(width = self.thumbnail_width, height = self.thumbnail_height) optimized_images.find_by(width: width, height: height) end @@ -41,10 +41,6 @@ class Upload < ActiveRecord::Base } if get_optimized_image(width, height, opts) - # TODO: this code is not right, we may have multiple - # thumbs - self.width = width - self.height = height save(validate: false) end end @@ -103,6 +99,51 @@ class Upload < ActiveRecord::Base "upload://#{Base62.encode(sha1.hex)}.#{extension}" end + def local? + !(url =~ /^(https?:)?\/\//) + end + + def fix_dimensions! + return if !FileHelper.is_image?("image.#{extension}") + + path = + if local? + Discourse.store.path_for(self) + else + Discourse.store.download(self).path + end + + self.width, self.height = size = FastImage.new(path).size + self.thumbnail_width, self.thumbnail_height = ImageSizer.resize(*size) + nil + end + + # on demand image size calculation, this allows us to null out image sizes + # and still handle as needed + def get_dimension(key) + if v = read_attribute(key) + return v + end + fix_dimensions! + read_attribute(key) + end + + def width + get_dimension(:width) + end + + def height + get_dimension(:height) + end + + def thumbnail_width + get_dimension(:thumbnail_width) + end + + def thumbnail_height + get_dimension(:thumbnail_height) + end + def self.sha1_from_short_url(url) if url =~ /(upload:\/\/)?([a-zA-Z0-9]+)(\..*)?/ sha1 = Base62.decode($2).to_s(16) diff --git a/app/serializers/upload_serializer.rb b/app/serializers/upload_serializer.rb index a02fbef9b09..9e2098b8155 100644 --- a/app/serializers/upload_serializer.rb +++ b/app/serializers/upload_serializer.rb @@ -5,6 +5,8 @@ class UploadSerializer < ApplicationSerializer :filesize, :width, :height, + :thumbnail_width, + :thumbnail_height, :extension, :short_url, :retain_hours diff --git a/db/migrate/20180827053514_add_filesize_to_optimized_images.rb b/db/migrate/20180827053514_add_filesize_to_optimized_images.rb new file mode 100644 index 00000000000..8840b57afe5 --- /dev/null +++ b/db/migrate/20180827053514_add_filesize_to_optimized_images.rb @@ -0,0 +1,7 @@ +class AddFilesizeToOptimizedImages < ActiveRecord::Migration[5.2] + def change + add_column :optimized_images, :filesize, :integer + add_column :uploads, :thumbnail_width, :integer + add_column :uploads, :thumbnail_height, :integer + end +end diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 72423cf43a7..8daef42e89e 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -327,7 +327,17 @@ class CookedPostProcessor # replace the image by its thumbnail w, h = img["width"].to_i, img["height"].to_i - img["src"] = upload.thumbnail(w, h).url if upload && upload.has_thumbnail?(w, h) + + if upload + thumbnail = upload.thumbnail(w, h) + + img["src"] = + if thumbnail && thumbnail.filesize.to_i < upload.filesize + upload.thumbnail(w, h).url + else + upload.url + end + end # then, some overlay informations meta = create_node("div", "meta") diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index ea365b18b3f..25d2eef45d1 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -106,7 +106,8 @@ class UploadCreator @upload.extension = image_type || File.extname(@filename)[1..10] if is_image - @upload.width, @upload.height = ImageSizer.resize(*@image_info.size) + @upload.thumbnail_width, @upload.thumbnail_height = ImageSizer.resize(*@image_info.size) + @upload.width, @upload.height = @image_info.size end @upload.for_private_message = true if @opts[:for_private_message] diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb index 0b7fbd63869..9d7db74d4cd 100644 --- a/spec/models/optimized_image_spec.rb +++ b/spec/models/optimized_image_spec.rb @@ -54,22 +54,35 @@ describe OptimizedImage do end it 'should work correctly' do - tmp_path = "/tmp/resized.png" - begin - OptimizedImage.resize( - "#{Rails.root}/spec/fixtures/images/logo.png", - tmp_path, - 5, - 5 - ) + file = File.open("#{Rails.root}/spec/fixtures/images/resized.png") + upload = UploadCreator.new(file, "test.bin").create_for(-1) - expect(File.read(tmp_path)).to eq( - File.read("#{Rails.root}/spec/fixtures/images/resized.png") - ) - ensure - File.delete(tmp_path) if File.exists?(tmp_path) - end + expect(upload.filesize).to eq(199) + + expect(upload.width).to eq(5) + expect(upload.height).to eq(5) + + upload.create_thumbnail!(10, 10) + thumb = upload.thumbnail(10, 10) + + expect(thumb.width).to eq(10) + expect(thumb.height).to eq(10) + + # very image magic specific so fudge here + expect(thumb.filesize).to be > 200 + + # this size is based off original upload + # it is the size we render, by default, in the post + expect(upload.thumbnail_width).to eq(5) + expect(upload.thumbnail_height).to eq(5) + + # lets ensure we can rebuild the filesize + thumb.update_columns(filesize: nil) + thumb = OptimizedImage.find(thumb.id) + + # attempts to auto correct + expect(thumb.filesize).to be > 200 end describe 'when an svg with a href is masked as a png' do diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 1ee24f9b953..a52af4a0bd5 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -46,6 +46,22 @@ describe Upload do end + it "can reconstruct dimensions on demand" do + upload = UploadCreator.new(huge_image, "image.png").create_for(user_id) + + upload.update_columns(width: nil, height: nil, thumbnail_width: nil, thumbnail_height: nil) + + upload = Upload.find(upload.id) + + expect(upload.width).to eq(64250) + expect(upload.height).to eq(64250) + + upload.update_columns(width: nil, height: nil, thumbnail_width: nil, thumbnail_height: nil) + + expect(upload.thumbnail_width).to eq(500) + expect(upload.thumbnail_height).to eq(500) + end + it "extracts file extension" do created_upload = UploadCreator.new(image, image_filename).create_for(user_id) expect(created_upload.extension).to eq("png") diff --git a/test/javascripts/lib/utilities-test.js.es6 b/test/javascripts/lib/utilities-test.js.es6 index 82fbfe6bba6..ba2448b35c0 100644 --- a/test/javascripts/lib/utilities-test.js.es6 +++ b/test/javascripts/lib/utilities-test.js.es6 @@ -161,8 +161,8 @@ var testUploadMarkdown = function(filename) { return getUploadMarkdown({ original_filename: filename, filesize: 42, - width: 100, - height: 200, + thumbnail_width: 100, + thumbnail_height: 200, url: "/uploads/123/abcdef.ext" }); };