diff --git a/app/assets/javascripts/discourse/app/lib/lazy-load-images.js b/app/assets/javascripts/discourse/app/lib/lazy-load-images.js index 5b79ebe17dd..d905ec313f3 100644 --- a/app/assets/javascripts/discourse/app/lib/lazy-load-images.js +++ b/app/assets/javascripts/discourse/app/lib/lazy-load-images.js @@ -1,14 +1,3 @@ -// Min size in pixels for consideration for lazy loading -const MINIMUM_SIZE = 150; - -function forEachImage(post, callback) { - post.querySelectorAll("img").forEach((img) => { - if (img.width >= MINIMUM_SIZE && img.height >= MINIMUM_SIZE) { - callback(img); - } - }); -} - function isLoaded(img) { // In Safari, img.complete sometimes returns true even when the image is not loaded. // naturalHeight seems to be a more reliable check @@ -16,40 +5,48 @@ function isLoaded(img) { } export function nativeLazyLoading(api) { + api.decorateCookedElement( + (post) => + post.querySelectorAll("img").forEach((img) => (img.loading = "lazy")), + { + id: "discourse-lazy-load", + } + ); + api.decorateCookedElement( (post) => { const siteSettings = api.container.lookup("service:site-settings"); - forEachImage(post, (img) => { - img.loading = "lazy"; + post.querySelectorAll("img").forEach((img) => { + // Support for smallUpload should be maintained until Post::BAKED_VERSION is bumped higher than 2 + const { smallUpload, dominantColor } = img.dataset; - if (siteSettings.secure_media) { + if (siteSettings.secure_media && smallUpload) { // Secure media requests go through the app. In topics with many images, // this makes it very easy to hit rate limiters. Skipping the low-res // placeholders reduces the chance of this problem occuring. return; } - if (img.dataset.smallUpload) { - if (!isLoaded(img)) { - if (!img.onload) { - img.onload = () => { - img.style.removeProperty("background-image"); - img.style.removeProperty("background-size"); - }; - } + if ((smallUpload || dominantColor) && !isLoaded(img)) { + if (!img.onload) { + img.onload = () => { + img.style.removeProperty("background-image"); + img.style.removeProperty("background-size"); + img.style.removeProperty("background-color"); + }; + } - img.style.setProperty( - "background-image", - `url(${img.dataset.smallUpload})` - ); + if (smallUpload) { + img.style.setProperty("background-image", `url(${smallUpload})`); img.style.setProperty("background-size", "cover"); + } else { + img.style.setProperty("background-color", `#${dominantColor}`); } } }); }, { - onlyStream: true, id: "discourse-lazy-load-after-adopt", afterAdopt: true, } diff --git a/app/jobs/scheduled/periodical_updates.rb b/app/jobs/scheduled/periodical_updates.rb index 5c5da8bac6b..01649de0627 100644 --- a/app/jobs/scheduled/periodical_updates.rb +++ b/app/jobs/scheduled/periodical_updates.rb @@ -48,6 +48,8 @@ module Jobs Category.auto_bump_topic! + Upload.backfill_dominant_colors!(25) + nil end diff --git a/app/models/upload.rb b/app/models/upload.rb index 11fb98d0e15..08a7213d047 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -10,6 +10,7 @@ class Upload < ActiveRecord::Base SEEDED_ID_THRESHOLD = 0 URL_REGEX ||= /(\/original\/\dX[\/\.\w]*\/(\h+)[\.\w]*)/ MAX_IDENTIFY_SECONDS = 5 + DOMINANT_COLOR_COMMAND_TIMEOUT_SECONDS = 5 belongs_to :user belongs_to :access_control_post, class_name: 'Post' @@ -316,6 +317,71 @@ class Upload < ActiveRecord::Base get_dimension(:thumbnail_height) end + def dominant_color(calculate_if_missing: false) + val = read_attribute(:dominant_color) + if val.nil? && calculate_if_missing + calculate_dominant_color! + read_attribute(:dominant_color) + else + val + end + end + + def calculate_dominant_color!(local_path = nil) + color = nil + + if !FileHelper.is_supported_image?("image.#{extension}") || extension == "svg" + color = "" + end + + if color.nil? + local_path ||= + if local? + Discourse.store.path_for(self) + else + Discourse.store.download(self).path + end + + color = begin + data = Discourse::Utils.execute_command( + "nice", + "-n", + "10", + "convert", + local_path, + "-resize", + "1x1", + "-define", + "histogram:unique-colors=true", + "-format", + "%c", + "histogram:info:", + timeout: DOMINANT_COLOR_COMMAND_TIMEOUT_SECONDS + ) + + # Output format: + # 1: (110.873,116.226,93.8821) #6F745E srgb(43.4798%,45.5789%,36.8165%) + + color = data[/#([0-9A-F]{6})/, 1] + + raise "Calculated dominant color but unable to parse output:\n#{data}" if color.nil? + + color + rescue Discourse::Utils::CommandError => e + # Timeout or unable to parse image + # This can happen due to bad user input - ignore and save + # an empty string to prevent re-evaluation + "" + end + end + + if persisted? + self.update_column(:dominant_color, color) + else + self.dominant_color = color + end + end + def target_image_quality(local_path, test_quality) @file_quality ||= Discourse::Utils.execute_command("identify", "-format", "%Q", local_path, timeout: MAX_IDENTIFY_SECONDS).to_i rescue 0 @@ -522,6 +588,12 @@ class Upload < ActiveRecord::Base Upload.where(sha1: sha1s.uniq).pluck(:id) end + def self.backfill_dominant_colors!(count) + Upload.where(dominant_color: nil).order("id desc").first(count).each do |upload| + upload.calculate_dominant_color! + end + end + private def short_url_basename @@ -553,10 +625,11 @@ end # secure :boolean default(FALSE), not null # access_control_post_id :bigint # original_sha1 :string -# verification_status :integer default(1), not null # animated :boolean +# verification_status :integer default(1), not null # security_last_changed_at :datetime # security_last_changed_reason :string +# dominant_color :text # # Indexes # @@ -564,6 +637,7 @@ end # index_uploads_on_access_control_post_id (access_control_post_id) # index_uploads_on_etag (etag) # index_uploads_on_extension (lower((extension)::text)) +# index_uploads_on_id (id) WHERE (dominant_color IS NULL) # index_uploads_on_id_and_url (id,url) # index_uploads_on_original_sha1 (original_sha1) # index_uploads_on_sha1 (sha1) UNIQUE diff --git a/app/serializers/upload_serializer.rb b/app/serializers/upload_serializer.rb index 43b89526f91..a31a6d02100 100644 --- a/app/serializers/upload_serializer.rb +++ b/app/serializers/upload_serializer.rb @@ -13,7 +13,8 @@ class UploadSerializer < ApplicationSerializer :short_url, :short_path, :retain_hours, - :human_filesize + :human_filesize, + :dominant_color def url object.for_site_setting ? object.url : UrlHelper.cook_url(object.url, secure: SiteSetting.secure_media? && object.secure) diff --git a/db/migrate/20220915132547_add_dominant_color_to_uploads.rb b/db/migrate/20220915132547_add_dominant_color_to_uploads.rb new file mode 100644 index 00000000000..1b60a4b191b --- /dev/null +++ b/db/migrate/20220915132547_add_dominant_color_to_uploads.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddDominantColorToUploads < ActiveRecord::Migration[7.0] + def change + add_column :uploads, :dominant_color, :text, limit: 6, null: true + add_index :uploads, :id, where: "dominant_color IS NULL" + end +end diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 2f27ecdf466..f2a3717ab19 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -7,8 +7,6 @@ class CookedPostProcessor include CookedProcessorMixin LIGHTBOX_WRAPPER_CSS_CLASS = "lightbox-wrapper" - LOADING_SIZE = 10 - LOADING_COLORS = 32 GIF_SOURCES_REGEXP = /(giphy|tenor)\.com\// attr_reader :cooking_options, :doc @@ -32,7 +30,7 @@ class CookedPostProcessor @has_oneboxes = post.post_analyzer.found_oneboxes? @size_cache = {} - @disable_loading_image = !!opts[:disable_loading_image] + @disable_dominant_color = !!opts[:disable_dominant_color] @omit_nofollow = post.omit_nofollow? end @@ -193,10 +191,6 @@ class CookedPostProcessor end end - unless @disable_loading_image - upload.create_thumbnail!(LOADING_SIZE, LOADING_SIZE, format: 'png', colors: LOADING_COLORS) - end - return if upload.animated? if img.ancestors('.onebox, .onebox-body, .quote').blank? && !img.classes.include?("onebox") @@ -207,10 +201,6 @@ class CookedPostProcessor end end - def loading_image(upload) - upload.thumbnail(LOADING_SIZE, LOADING_SIZE) - end - def each_responsive_ratio SiteSetting .responsive_post_image_sizes @@ -223,7 +213,7 @@ class CookedPostProcessor def optimize_image!(img, upload, cropped: false) w, h = img["width"].to_i, img["height"].to_i - # note: optimize_urls cooks the src and data-small-upload further after this + # note: optimize_urls cooks the src further after this thumbnail = upload.thumbnail(w, h) if thumbnail && thumbnail.filesize.to_i < upload.filesize img["src"] = thumbnail.url @@ -248,8 +238,8 @@ class CookedPostProcessor img["src"] = upload.url end - if small_upload = loading_image(upload) - img["data-small-upload"] = small_upload.url + if !@disable_dominant_color && (color = upload.dominant_color(calculate_if_missing: true).presence) + img["data-dominant-color"] = color end end @@ -329,7 +319,7 @@ class CookedPostProcessor end end - %w{src data-small-upload}.each do |selector| + %w{src}.each do |selector| @doc.css("img[#{selector}]").each do |img| custom_emoji = img["class"]&.include?("emoji-custom") && Emoji.custom?(img["title"]) img[selector] = UrlHelper.cook_url( diff --git a/lib/discourse.rb b/lib/discourse.rb index 3abaafaa7b3..9bd00a2e640 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -109,6 +109,16 @@ module Discourse nil end + class CommandError < RuntimeError + attr_reader :status, :stdout, :stderr + def initialize(message, status: nil, stdout: nil, stderr: nil) + super(message) + @status = status + @stdout = stdout + @stderr = stderr + end + end + private class CommandRunner @@ -145,7 +155,12 @@ module Discourse if !status.exited? || !success_status_codes.include?(status.exitstatus) failure_message = "#{failure_message}\n" if !failure_message.blank? - raise "#{caller[0]}: #{failure_message}#{stderr}" + raise CommandError.new( + "#{caller[0]}: #{failure_message}#{stderr}", + stdout: stdout, + stderr: stderr, + status: status + ) end stdout diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 5aa02520a20..8b5655c4b3b 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -168,6 +168,7 @@ class UploadCreator @upload.thumbnail_width, @upload.thumbnail_height = ImageSizer.resize(w, h) @upload.width, @upload.height = w, h @upload.animated = animated? + @upload.calculate_dominant_color!(@file.path) end add_metadata! diff --git a/spec/fabricators/upload_fabricator.rb b/spec/fabricators/upload_fabricator.rb index b7b8b90d669..5ccd0b96118 100644 --- a/spec/fabricators/upload_fabricator.rb +++ b/spec/fabricators/upload_fabricator.rb @@ -22,9 +22,11 @@ Fabricator(:upload) do end Fabricator(:image_upload, from: :upload) do - after_create do |upload| + transient color: "white" + + after_create do |upload, transients| file = Tempfile.new(['fabricated', '.png']) - `convert -size #{upload.width}x#{upload.height} xc:white "#{file.path}"` + `convert -size #{upload.width}x#{upload.height} xc:#{transients[:color]} "#{file.path}"` upload.url = Discourse.store.store_upload(file, upload) upload.sha1 = Upload.generate_digest(file.path) diff --git a/spec/lib/cooked_post_processor_spec.rb b/spec/lib/cooked_post_processor_spec.rb index a4515487e08..e808b0116cc 100644 --- a/spec/lib/cooked_post_processor_spec.rb +++ b/spec/lib/cooked_post_processor_spec.rb @@ -14,7 +14,7 @@ RSpec.describe CookedPostProcessor do RAW end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } let(:post_process) { sequence("post_process") } it "post process in sequence" do @@ -258,7 +258,7 @@ RSpec.describe CookedPostProcessor do before { SiteSetting.responsive_post_image_sizes = "1|1.5|3" } it "includes responsive images on demand" do - upload.update!(width: 2000, height: 1500, filesize: 10000) + upload.update!(width: 2000, height: 1500, filesize: 10000, dominant_color: "FFFFFF") post = Fabricate(:post, raw: "hello ") # fake some optimized images @@ -284,17 +284,6 @@ RSpec.describe CookedPostProcessor do filesize: 800 ) - # Fake a loading image - _optimized_image = OptimizedImage.create!( - url: "/#{upload_path}/10x10.png", - width: CookedPostProcessor::LOADING_SIZE, - height: CookedPostProcessor::LOADING_SIZE, - upload_id: upload.id, - sha1: SecureRandom.hex, - extension: '.png', - filesize: 123 - ) - cpp = CookedPostProcessor.new(post) cpp.add_to_size_cache(upload.url, 2000, 1500) @@ -302,7 +291,7 @@ RSpec.describe CookedPostProcessor do html = cpp.html - expect(html).to include(%Q|data-small-upload="//test.localhost/#{upload_path}/10x10.png"|) + expect(html).to include(%Q|data-dominant-color="FFFFFF"|) # 1.5x is skipped cause we have a missing thumb expect(html).to include("srcset=\"//test.localhost/#{upload_path}/666x500.jpg, //test.localhost/#{upload_path}/1998x1500.jpg 3x\"") expect(html).to include("src=\"//test.localhost/#{upload_path}/666x500.jpg\"") @@ -316,7 +305,7 @@ RSpec.describe CookedPostProcessor do html = cpp.html - expect(html).to include(%Q|data-small-upload="//cdn.localhost/#{upload_path}/10x10.png"|) + expect(html).to include(%Q|data-dominant-color="FFFFFF"|) expect(html).to include("srcset=\"//cdn.localhost/#{upload_path}/666x500.jpg, //cdn.localhost/#{upload_path}/1998x1500.jpg 3x\"") expect(html).to include("src=\"//cdn.localhost/#{upload_path}/666x500.jpg\"") end @@ -416,7 +405,7 @@ RSpec.describe CookedPostProcessor do HTML end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } before do SiteSetting.max_image_height = 2000 @@ -556,7 +545,7 @@ RSpec.describe CookedPostProcessor do HTML end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } before do SiteSetting.create_thumbnails = true @@ -580,7 +569,7 @@ RSpec.describe CookedPostProcessor do HTML end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } before do SiteSetting.create_thumbnails = true @@ -604,7 +593,7 @@ RSpec.describe CookedPostProcessor do HTML end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } before do SiteSetting.create_thumbnails = true @@ -631,7 +620,7 @@ RSpec.describe CookedPostProcessor do HTML end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } before do set_subfolder "/subfolder" @@ -671,7 +660,7 @@ RSpec.describe CookedPostProcessor do HTML end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } before do SiteSetting.max_image_height = 2000 @@ -699,7 +688,7 @@ RSpec.describe CookedPostProcessor do HTML end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } before do SiteSetting.max_image_height = 2000 @@ -727,7 +716,7 @@ RSpec.describe CookedPostProcessor do HTML end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } before do SiteSetting.max_image_height = 2000 @@ -817,7 +806,7 @@ RSpec.describe CookedPostProcessor do ![alttext|1750x2000|thumbnail](#{upload2.url}) MD - CookedPostProcessor.new(post, disable_loading_image: true).post_process + CookedPostProcessor.new(post, disable_dominant_color: true).post_process expect(post.reload.image_upload_id).to eq(upload2.id) end @@ -959,7 +948,7 @@ RSpec.describe CookedPostProcessor do it "adds lightbox and optimizes images" do post = Fabricate(:post, raw: "![image|1024x768, 50%](#{upload.short_url})") - cpp = CookedPostProcessor.new(post, disable_loading_image: true) + cpp = CookedPostProcessor.new(post, disable_dominant_color: true) cpp.post_process doc = Nokogiri::HTML5::fragment(cpp.html) @@ -974,7 +963,7 @@ RSpec.describe CookedPostProcessor do upload.update!(animated: true) post = Fabricate(:post, raw: "![image|1024x768, 50%](#{upload.short_url})") - cpp = CookedPostProcessor.new(post, disable_loading_image: true) + cpp = CookedPostProcessor.new(post, disable_dominant_color: true) cpp.post_process doc = Nokogiri::HTML5::fragment(cpp.html) @@ -992,7 +981,7 @@ RSpec.describe CookedPostProcessor do it "marks giphy images as animated" do post = Fabricate(:post, raw: "![tennis-gif|311x280](https://media2.giphy.com/media/7Oifk90VrCdNe/giphy.webp)") - cpp = CookedPostProcessor.new(post, disable_loading_image: true) + cpp = CookedPostProcessor.new(post, disable_dominant_color: true) cpp.post_process doc = Nokogiri::HTML5::fragment(cpp.html) @@ -1001,7 +990,7 @@ RSpec.describe CookedPostProcessor do it "marks giphy images as animated" do post = Fabricate(:post, raw: "![cat](https://media1.tenor.com/images/20c7ddd5e84c7427954f430439c5209d/tenor.gif)") - cpp = CookedPostProcessor.new(post, disable_loading_image: true) + cpp = CookedPostProcessor.new(post, disable_dominant_color: true) cpp.post_process doc = Nokogiri::HTML5::fragment(cpp.html) @@ -1016,7 +1005,7 @@ RSpec.describe CookedPostProcessor do [/quote] MD - cpp = CookedPostProcessor.new(post, disable_loading_image: true) + cpp = CookedPostProcessor.new(post, disable_dominant_color: true) cpp.post_process doc = Nokogiri::HTML5::fragment(cpp.html) @@ -1031,7 +1020,7 @@ RSpec.describe CookedPostProcessor do post = Fabricate(:post, raw: "https://discourse.org") - cpp = CookedPostProcessor.new(post, disable_loading_image: true) + cpp = CookedPostProcessor.new(post, disable_dominant_color: true) cpp.post_process doc = Nokogiri::HTML5::fragment(cpp.html) @@ -1591,7 +1580,7 @@ RSpec.describe CookedPostProcessor do RAW end - let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) } + let(:cpp) { CookedPostProcessor.new(post, disable_dominant_color: true) } it "does remove user ids" do cpp.remove_user_ids diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index cb20d184efd..6bcc49b1aac 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -618,4 +618,76 @@ RSpec.describe Upload do expect(Upload.secure_media_url?(url)).to eq(false) end end + + describe "#dominant_color" do + let(:white_image) { Fabricate(:image_upload, color: "white") } + let(:red_image) { Fabricate(:image_upload, color: "red") } + let(:not_an_image) { + upload = Fabricate(:upload) + + file = Tempfile.new(['invalid', '.txt']) + file << "Not really an image" + file.rewind + + upload.update(url: Discourse.store.store_upload(file, upload), extension: "txt") + upload + } + let(:invalid_image) { + upload = Fabricate(:upload) + + file = Tempfile.new(['invalid', '.png']) + file << "Not really an image" + file.rewind + + upload.update(url: Discourse.store.store_upload(file, upload)) + upload + } + + it "correctly identifies and stores an image's dominant color" do + expect(white_image.dominant_color).to eq(nil) + expect(white_image.dominant_color(calculate_if_missing: true)).to eq("FFFFFF") + expect(white_image.dominant_color).to eq("FFFFFF") + + expect(red_image.dominant_color).to eq(nil) + expect(red_image.dominant_color(calculate_if_missing: true)).to eq("FF0000") + expect(red_image.dominant_color).to eq("FF0000") + end + + it "can be backfilled" do + expect(white_image.dominant_color).to eq(nil) + expect(red_image.dominant_color).to eq(nil) + + Upload.backfill_dominant_colors!(5) + + white_image.reload + red_image.reload + + expect(white_image.dominant_color).to eq("FFFFFF") + expect(red_image.dominant_color).to eq("FF0000") + end + + it "stores an empty string for non-image uploads" do + expect(not_an_image.dominant_color).to eq(nil) + expect(not_an_image.dominant_color(calculate_if_missing: true)).to eq("") + expect(not_an_image.dominant_color).to eq("") + end + + it "correctly handles invalid image files" do + expect(invalid_image.dominant_color).to eq(nil) + expect(invalid_image.dominant_color(calculate_if_missing: true)).to eq("") + expect(invalid_image.dominant_color).to eq("") + end + + it "correctly handles unparsable ImageMagick output" do + Discourse::Utils.stubs(:execute_command).returns('someinvalidoutput') + + expect(invalid_image.dominant_color).to eq(nil) + + expect { + invalid_image.dominant_color(calculate_if_missing: true) + }.to raise_error(/Calculated dominant color but unable to parse output/) + + expect(invalid_image.dominant_color).to eq(nil) + end + end end diff --git a/spec/requests/api/schemas/json/upload_create_response.json b/spec/requests/api/schemas/json/upload_create_response.json index 05555c7b9e3..32f92c5d7d9 100644 --- a/spec/requests/api/schemas/json/upload_create_response.json +++ b/spec/requests/api/schemas/json/upload_create_response.json @@ -39,6 +39,9 @@ }, "human_filesize": { "type": "string" + }, + "dominant_color": { + "type": ["string", "null"] } }, "required": [