diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 33a6704cd4b..35fa225e230 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -168,7 +168,15 @@ class UploadCreator pixels > MIN_PIXELS_TO_CONVERT_TO_JPEG end + MIN_CONVERT_TO_JPEG_BYTES_SAVED = 75_000 + MIN_CONVERT_TO_JPEG_SAVING_RATIO = 0.70 + def convert_to_jpeg! + + if filesize < MIN_CONVERT_TO_JPEG_BYTES_SAVED + return + end + jpeg_tempfile = Tempfile.new(["image", ".jpg"]) from = @file.path @@ -186,8 +194,12 @@ class UploadCreator execute_convert(from, to, true) end - # keep the JPEG if it's at least 15% smaller - if File.size(jpeg_tempfile.path) < filesize * 0.85 + new_size = File.size(jpeg_tempfile.path) + + keep_jpeg = new_size < filesize * MIN_CONVERT_TO_JPEG_SAVING_RATIO + keep_jpeg &&= (filesize - new_size) > MIN_CONVERT_TO_JPEG_BYTES_SAVED + + if keep_jpeg @file = jpeg_tempfile extract_image_info! else diff --git a/spec/fixtures/images/should_be_jpeg.png b/spec/fixtures/images/should_be_jpeg.png new file mode 100644 index 00000000000..b3723f3950e Binary files /dev/null and b/spec/fixtures/images/should_be_jpeg.png differ diff --git a/spec/lib/upload_creator_spec.rb b/spec/lib/upload_creator_spec.rb index 43cd261766c..04ac72f00b1 100644 --- a/spec/lib/upload_creator_spec.rb +++ b/spec/lib/upload_creator_spec.rb @@ -99,13 +99,37 @@ RSpec.describe UploadCreator do end describe 'converting to jpeg' do - let(:filename) { "logo.png" } + let(:filename) { "should_be_jpeg.png" } let(:file) { file_from_fixtures(filename) } + let(:small_filename) { "logo.png" } + let(:small_file) { file_from_fixtures(small_filename) } + before do SiteSetting.png_to_jpg_quality = 1 end + it 'should not store file as jpeg if it does not meet absolute byte saving requirements' do + + # logo.png is 2297 bytes, converting to jpeg saves 30% but does not meet + # the absolute savings required of 25_000 bytes, if you save less than that + # skip this + + expect do + UploadCreator.new(small_file, small_filename, + pasted: true, + force_optimize: true + ).create_for(user.id) + end.to change { Upload.count }.by(1) + + upload = Upload.last + + expect(upload.extension).to eq('png') + expect(File.extname(upload.url)).to eq('.png') + expect(upload.original_filename).to eq('logo.png') + + end + it 'should store the upload with the right extension' do expect do UploadCreator.new(file, filename, @@ -118,7 +142,7 @@ RSpec.describe UploadCreator do expect(upload.extension).to eq('jpeg') expect(File.extname(upload.url)).to eq('.jpeg') - expect(upload.original_filename).to eq('logo.jpg') + expect(upload.original_filename).to eq('should_be_jpeg.jpg') end end end