PERF: stop downloading images from post processor and lean on uploads

Previously we would unconditionally fetch all images via HTTP to grab
original sizing from cooked post processor in 2 different spots.

This was wasteful as we already calculate and cache this info in upload records.

This also simplifies some specs and reduces use of mocks.
This commit is contained in:
Sam 2022-11-25 12:40:31 +11:00 committed by GitHub
parent 45f3e9f19e
commit 755ca0fcbb
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 39 additions and 27 deletions

View File

@ -153,13 +153,20 @@ class CookedPostProcessor
src = img["src"] src = img["src"]
return if src.blank? || is_a_hyperlink?(img) || is_svg?(img) return if src.blank? || is_a_hyperlink?(img) || is_svg?(img)
upload = Upload.get_from_url(src)
original_width, original_height = nil
if (upload.present?)
original_width = upload.width || 0
original_height = upload.height || 0
else
original_width, original_height = (get_size(src) || [0, 0]).map(&:to_i) original_width, original_height = (get_size(src) || [0, 0]).map(&:to_i)
if original_width == 0 || original_height == 0 if original_width == 0 || original_height == 0
Rails.logger.info "Can't reach '#{src}' to get its dimension." Rails.logger.info "Can't reach '#{src}' to get its dimension."
return return
end end
end
upload = Upload.get_from_url(src)
if (upload.present? && upload.animated?) || src.match?(GIF_SOURCES_REGEXP) if (upload.present? && upload.animated?) || src.match?(GIF_SOURCES_REGEXP)
img.add_class("animated") img.add_class("animated")

View File

@ -45,10 +45,7 @@ module CookedProcessorMixin
oneboxed_images.each do |img| oneboxed_images.each do |img|
next if img["src"].blank? next if img["src"].blank?
src = img["src"].sub(/^https?:/i, "")
parent = img.parent parent = img.parent
img_classes = (img["class"] || "").split(" ")
link_classes = ((parent&.name == "a" && parent["class"]) || "").split(" ")
if respond_to?(:process_hotlinked_image, true) if respond_to?(:process_hotlinked_image, true)
still_an_image = process_hotlinked_image(img) still_an_image = process_hotlinked_image(img)
@ -183,7 +180,13 @@ module CookedProcessorMixin
return unless is_valid_image_url?(absolute_url) return unless is_valid_image_url?(absolute_url)
upload = Upload.get_from_url(absolute_url)
if upload && upload.width && upload.width > 0
@size_cache[url] = [upload.width, upload.height]
else
@size_cache[url] = FastImage.size(absolute_url) @size_cache[url] = FastImage.size(absolute_url)
end
rescue Zlib::BufError, URI::Error, OpenSSL::SSL::SSLError rescue Zlib::BufError, URI::Error, OpenSSL::SSL::SSLError
# FastImage.size raises BufError for some gifs, leave it. # FastImage.size raises BufError for some gifs, leave it.
end end

View File

@ -21,6 +21,16 @@ Fabricator(:upload) do
extension "png" extension "png"
end end
Fabricator(:large_image_upload, from: :upload) do
width 2000
height 2000
after_create do |upload, _transients|
file = file_from_fixtures("2000x2000.png")
upload.url = Discourse.store.store_upload(file, upload)
upload.sha1 = Upload.generate_digest(file)
end
end
Fabricator(:image_upload, from: :upload) do Fabricator(:image_upload, from: :upload) do
transient color: "white" transient color: "white"

BIN
spec/fixtures/images/2000x2000.png vendored Normal file

Binary file not shown.

After

Width:  |  Height:  |  Size: 2.4 KiB

View File

@ -271,7 +271,6 @@ RSpec.describe Jobs::PullHotlinkedImages do
post = Fabricate(:post, raw: "<img src='#{url}'>") post = Fabricate(:post, raw: "<img src='#{url}'>")
upload.update(access_control_post: Fabricate(:post)) upload.update(access_control_post: Fabricate(:post))
FileStore::S3Store.any_instance.stubs(:store_upload).returns(upload.url) FileStore::S3Store.any_instance.stubs(:store_upload).returns(upload.url)
FastImage.expects(:size).returns([100, 100]).at_least_once
expect { Jobs::PullHotlinkedImages.new.execute(post_id: post.id) } expect { Jobs::PullHotlinkedImages.new.execute(post_id: post.id) }
.to change { Upload.count }.by(1) .to change { Upload.count }.by(1)
@ -374,7 +373,7 @@ RSpec.describe Jobs::PullHotlinkedImages do
post.rebake! post.rebake!
post.reload post.reload
expect(post.cooked).to match(/<img src=.*\/uploads/) expect(post.cooked).to match(/<img src=.*\/uploads.*\ class="thumbnail/)
expect(post.upload_references.count).to eq(1) expect(post.upload_references.count).to eq(1)
end end
@ -418,7 +417,7 @@ RSpec.describe Jobs::PullHotlinkedImages do
MD MD
expect(post.cooked).to match(/<p><img src=.*\/uploads/) expect(post.cooked).to match(/<p><img src=.*\/uploads/)
expect(post.cooked).to match(/<img src=.*\/uploads.*\ class="thumbnail"/) expect(post.cooked).to match(/<img src=.*\/uploads.*\ class="thumbnail/)
expect(post.cooked).to match(/<span class="broken-image/) expect(post.cooked).to match(/<span class="broken-image/)
expect(post.cooked).to match(/<div class="large-image-placeholder">/) expect(post.cooked).to match(/<div class="large-image-placeholder">/)
end end
@ -542,12 +541,10 @@ RSpec.describe Jobs::PullHotlinkedImages do
end end
describe "with a lightboxed image" do describe "with a lightboxed image" do
fab!(:upload) { Fabricate(:upload) } fab!(:upload) { Fabricate(:large_image_upload) }
fab!(:user) { Fabricate(:user) } fab!(:user) { Fabricate(:user) }
before do before do
FastImage.expects(:size).returns([1750, 2000]).at_least_once
OptimizedImage.stubs(:resize).returns(true)
Jobs.run_immediately! Jobs.run_immediately!
end end

View File

@ -5,6 +5,7 @@ require "file_store/s3_store"
RSpec.describe CookedPostProcessor do RSpec.describe CookedPostProcessor do
fab!(:upload) { Fabricate(:upload) } fab!(:upload) { Fabricate(:upload) }
fab!(:large_image_upload) { Fabricate(:large_image_upload) }
let(:upload_path) { Discourse.store.upload_path } let(:upload_path) { Discourse.store.upload_path }
describe "#post_process" do describe "#post_process" do
@ -764,7 +765,6 @@ RSpec.describe CookedPostProcessor do
end end
it "won't remove the original image if another post doesn't have an image" do it "won't remove the original image if another post doesn't have an image" do
FastImage.stubs(:size)
topic = post.topic topic = post.topic
cpp.post_process cpp.post_process
@ -781,8 +781,7 @@ RSpec.describe CookedPostProcessor do
end end
it "generates thumbnails correctly" do it "generates thumbnails correctly" do
FastImage.expects(:size).returns([1750, 2000]) # image size in cooked is 1500*2000
topic = post.topic topic = post.topic
cpp.post_process cpp.post_process
topic.reload topic.reload
@ -941,17 +940,13 @@ RSpec.describe CookedPostProcessor do
describe "#convert_to_link" do describe "#convert_to_link" do
fab!(:thumbnail) { Fabricate(:optimized_image, upload: upload, width: 512, height: 384) } fab!(:thumbnail) { Fabricate(:optimized_image, upload: upload, width: 512, height: 384) }
before do
CookedPostProcessor.any_instance.stubs(:get_size).with(upload.url).returns([1024, 768])
end
it "adds lightbox and optimizes images" do it "adds lightbox and optimizes images" do
post = Fabricate(:post, raw: "![image|1024x768, 50%](#{upload.short_url})") post = Fabricate(:post, raw: "![image|1024x768, 50%](#{large_image_upload.short_url})")
cpp = CookedPostProcessor.new(post, disable_dominant_color: true) cpp = CookedPostProcessor.new(post, disable_dominant_color: true)
cpp.post_process cpp.post_process
doc = Nokogiri::HTML5::fragment(cpp.html) doc = Nokogiri::HTML5::fragment(cpp.html)
expect(doc.css('.lightbox-wrapper').size).to eq(1) expect(doc.css('.lightbox-wrapper').size).to eq(1)
expect(doc.css('img').first['srcset']).to_not eq(nil) expect(doc.css('img').first['srcset']).to_not eq(nil)
end end
@ -1001,7 +996,7 @@ RSpec.describe CookedPostProcessor do
it "optimizes images in quotes" do it "optimizes images in quotes" do
post = Fabricate(:post, raw: <<~MD) post = Fabricate(:post, raw: <<~MD)
[quote] [quote]
![image|1024x768, 50%](#{upload.short_url}) ![image|1024x768, 50%](#{large_image_upload.short_url})
[/quote] [/quote]
MD MD
@ -1016,7 +1011,7 @@ RSpec.describe CookedPostProcessor do
it "optimizes images in Onebox" do it "optimizes images in Onebox" do
Oneboxer.expects(:onebox) Oneboxer.expects(:onebox)
.with("https://discourse.org", anything) .with("https://discourse.org", anything)
.returns("<aside class='onebox'><img src='#{upload.url}' width='512' height='384'></aside>") .returns("<aside class='onebox'><img src='#{large_image_upload.url}' width='512' height='384'></aside>")
post = Fabricate(:post, raw: "https://discourse.org") post = Fabricate(:post, raw: "https://discourse.org")