From a6529eafeed3cb07d975be90e76e65bf84a5b833 Mon Sep 17 00:00:00 2001 From: Robert May Date: Wed, 14 Aug 2013 09:41:57 +0100 Subject: [PATCH 1/4] Hotfix for support email address --- app/views/shared/_anon_nav.html.slim | 2 +- app/views/shared/_nav.html.slim | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/shared/_anon_nav.html.slim b/app/views/shared/_anon_nav.html.slim index 2c1c8c8c..93665c65 100644 --- a/app/views/shared/_anon_nav.html.slim +++ b/app/views/shared/_anon_nav.html.slim @@ -58,7 +58,7 @@ nav id="layout-nav" class="top-bar" active_wrapper: :li li= link_to t("nav.blog"), blog_path - li= mail_to t("nav.support"), ISO[:support_email] + li= link_to t("nav.support"), "mailto:#{ISO[:support_email]}" ul class="right" = link_to t("nav.sign_in"), new_user_session_path, active_on: true, \ diff --git a/app/views/shared/_nav.html.slim b/app/views/shared/_nav.html.slim index 394d2f03..0c079c7a 100644 --- a/app/views/shared/_nav.html.slim +++ b/app/views/shared/_nav.html.slim @@ -58,7 +58,7 @@ nav id="layout-nav" class="top-bar" active_wrapper: :li li= link_to t("nav.blog"), blog_path - li= mail_to t("nav.support"), ISO[:support_email] + li= link_to t("nav.support"), "mailto:#{ISO[:support_email]}" ul class="right" - if user_signed_in? From 1c6e83d6d70371f4cb2648f087697d25c804d0f4 Mon Sep 17 00:00:00 2001 From: Robert May Date: Sat, 17 Aug 2013 17:52:35 +0100 Subject: [PATCH 2/4] Parse rotation field on metadata --- app/models/metadata.rb | 17 +++++++++++++++ spec/models/metadata_spec.rb | 42 ++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/app/models/metadata.rb b/app/models/metadata.rb index d088f49e..041fd2eb 100644 --- a/app/models/metadata.rb +++ b/app/models/metadata.rb @@ -178,6 +178,23 @@ def square? format == 'square' end + def rotate? + camera.present? && camera['camera_orientation'].present? + end + + def rotate_by + if rotate? + match = camera['camera_orientation'].match(/Rotate (\d+) (CW|CCW)/) + + if match + degrees = match[1] + direction = match[2] + + direction == "CCW" ? "-#{degrees}".to_i : degrees.to_i + end + end + end + def can_edit?(key) self.respond_to?("#{key}=") && EDITABLE_KEYS.include?(key.to_sym) end diff --git a/spec/models/metadata_spec.rb b/spec/models/metadata_spec.rb index e6fd17af..86b50c48 100644 --- a/spec/models/metadata_spec.rb +++ b/spec/models/metadata_spec.rb @@ -52,5 +52,47 @@ }) end end + + describe "#rotate?" do + context "rotate" do + before { metadata.stub(:camera) { { 'camera_orientation' => 'Rotate 90 CW' } } } + + it "returns true" do + metadata.rotate?.should be true + end + end + + context "don't rotate" do + it "returns false" do + metadata.rotate?.should be false + end + end + end + + describe "#rotate_by" do + context "clockwise" do + before { metadata.stub(:camera) { { 'camera_orientation' => 'Rotate 90 CW' } } } + + it "returns a positive number" do + metadata.rotate_by.should eq(90) + end + end + + context "counter-clockwise" do + before { metadata.stub(:camera) { { 'camera_orientation' => 'Rotate 90 CCW' } } } + + it "returns a negative number" do + metadata.rotate_by.should eq(-90) + end + end + + context "not a rotation command" do + before { metadata.stub(:camera) { { 'camera_orientation' => 'Horizontal (normal)' } } } + + it "returns nil" do + metadata.rotate_by.should be nil + end + end + end end end From bfb519b4fe19b1274463d2776798a85d97e4c4e9 Mon Sep 17 00:00:00 2001 From: Robert May Date: Sat, 17 Aug 2013 18:39:49 +0100 Subject: [PATCH 3/4] Consolidating processing into single job This should help reduce problems likely caused by constantly fetching the photos from AWS, as well as enabling support for affecting the images based on the metadata, i.e. rotation. --- app/models/metadata.rb | 8 +- app/workers/photo_expansion_worker.rb | 85 ++++++++++++++++----- spec/workers/photo_expansion_worker_spec.rb | 56 ++++++++++++++ 3 files changed, 127 insertions(+), 22 deletions(-) create mode 100644 spec/workers/photo_expansion_worker_spec.rb diff --git a/app/models/metadata.rb b/app/models/metadata.rb index 041fd2eb..41793696 100644 --- a/app/models/metadata.rb +++ b/app/models/metadata.rb @@ -83,11 +83,6 @@ def set_defaults self.creator ||= {} self.image ||= {} end - - after_commit :enqueue_extraction, on: :create - def enqueue_extraction - MetadataWorker.perform_async(id) - end def extract_from_photograph begin @@ -106,6 +101,9 @@ def extract_from_photograph convert_lat_lng set_format + + self.processing = false + return true rescue ArgumentError => ex # Prevent UTF-8 bug from stopping photo upload self.camera ||= {} diff --git a/app/workers/photo_expansion_worker.rb b/app/workers/photo_expansion_worker.rb index 026d1e4a..11693d8d 100644 --- a/app/workers/photo_expansion_worker.rb +++ b/app/workers/photo_expansion_worker.rb @@ -1,26 +1,77 @@ class PhotoExpansionWorker + class MetadataRecordMissing < StandardError; end + include Sidekiq::Worker include Timeout sidekiq_options queue: :photos def perform(photograph_id) - timeout(120) do - photo = Photograph.find(photograph_id) - photo.standard_image = photo.image.thumb("3000x3000>") - photo.save! - - if photo.standard_image.width > photo.standard_image.height - ImageWorker.perform_async(photo.id, :standard_image, :homepage_image, "2000x", "-quality 80") - ImageWorker.perform_async(photo.id, :standard_image, :large_image, "1500x", "-quality 80") - elsif photo.standard_image.height > photo.standard_image.width - ImageWorker.perform_async(photo.id, :standard_image, :homepage_image, "2000x1400#", "-quality 80") - ImageWorker.perform_async(photo.id, :standard_image, :large_image, "x1000", "-quality 80") - else - ImageWorker.perform_async(photo.id, :standard_image, :homepage_image, "2000x1400#", "-quality 80") - ImageWorker.perform_async(photo.id, :standard_image, :large_image, "1500x1500>", "-quality 80") - end - - ImageWorker.perform_async(photo.id, :standard_image, :thumbnail_image, "500x500>", "-quality 70") + timeout(300) do + @photo = Photograph.find(photograph_id) + + # Extract the metadata and save it + extract_metadata + + # Generate all the other image sizes + generate_images + + @photo.save! + end + end + + private + + def extract_metadata + metadata = @photo.metadata + + # It's possible that Sidekiq could hit this before Postgres catches up + raise MetadataRecordMissing if metadata.nil? + + # Extract the metadata first to allow us to work off that data + metadata.extract_from_photograph + metadata.save! + end + + def generate_standard_image + # Create a standard image for generating the smaller sizes + standard_image = @photo.image.thumb("3000x3000>") + + # Rotate the image if the metadata says so + if @photo.metadata.rotate? + standard_image = standard_image.process(:rotate, @photo.metadata.rotate_by) + end + + # Set the standard image and save + @photo.standard_image = standard_image + end + + def generate_images + # Generate a sensible base size first + generate_standard_image + + # Generate other sizes based on dimensions + case + when @photo.landscape? + generate_image(:standard_image, :homepage_image, "2000x", "-quality 80") + generate_image(:standard_image, :large_image, "1500x", "-quality 80") + when @photo.portrait? + generate_image(:standard_image, :homepage_image, "2000x1400#", "-quality 80") + generate_image(:standard_image, :large_image, "x1000", "-quality 80") + else + generate_image(:standard_image, :homepage_image, "2000x1400#", "-quality 80") + generate_image(:standard_image, :large_image, "1500x1500>", "-quality 80") + end + + generate_image(:standard_image, :thumbnail_image, "500x500>", "-quality 70") + end + + def generate_image(source, target, size, encode_opts) + source = @photo.send(source) + if source.present? + image = source.thumb(size).encode(:jpg, encode_opts) + @photo.send("#{target}=", image) + else + raise "Source doesn't exist yet" end end end diff --git a/spec/workers/photo_expansion_worker_spec.rb b/spec/workers/photo_expansion_worker_spec.rb new file mode 100644 index 00000000..92677352 --- /dev/null +++ b/spec/workers/photo_expansion_worker_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe PhotoExpansionWorker do + subject { PhotoExpansionWorker.new } + let(:photograph) { Photograph.make(metadata: metadata) } + let(:metadata) { Metadata.make } + + before { Photograph.stub(:find) { photograph } } + before { Metadata.stub(:find) { metadata } } + before { photograph.stub(:save!) { true } } + + describe "metadata" do + before { metadata.stub(:extract_from_photograph) { true } } + before { metadata.stub(:save!) { true } } + after { subject.perform(1) } + + it "calls extract_metadata" do + subject.should_receive(:extract_metadata) + end + + it "calls extract_from_photograph on the metadata" do + metadata.should_receive(:extract_from_photograph) + end + + it "saves the metadata" do + metadata.should_receive(:save!) + end + end + + describe "image generation" do + before { subject.instance_variable_set('@photo', photograph) } + + describe "#generate_standard_image" do + let(:image) { double('image').as_null_object } + before { photograph.stub_chain(:image, :thumb) { image } } + after { subject.send(:generate_standard_image) } + + context "metadata.rotate? is true" do + before { metadata.stub(:rotate?) { true } } + before { metadata.stub(:rotate_by) { 90 } } + + it "rotates the image" do + image.should_receive(:process).with(:rotate, 90) + end + end + + context "metadata.rotate? is false" do + before { metadata.stub(:rotate?) { false } } + + it "doesn't rotate the image" do + image.should_not_receive(:process) + end + end + end + end +end From 2c286a433afb1221080ef15fc4ee460f24116b2f Mon Sep 17 00:00:00 2001 From: Robert May Date: Sat, 17 Aug 2013 20:12:38 +0100 Subject: [PATCH 4/4] Cache S3 data fetches in memcached to reduce generation time This implements our own wrapper around the Dragonfly S3 store, caching retrieval for 5 minutes (currently) so that processing multiple image sizes doesn't require multiple fetches. --- app/workers/photo_expansion_worker.rb | 61 ++++++++++++--------- config/initializers/dragonfly.rb | 3 +- lib/extensions/caching_s3_data_store.rb | 21 +++++++ spec/workers/photo_expansion_worker_spec.rb | 17 ++++++ 4 files changed, 74 insertions(+), 28 deletions(-) create mode 100644 lib/extensions/caching_s3_data_store.rb diff --git a/app/workers/photo_expansion_worker.rb b/app/workers/photo_expansion_worker.rb index 11693d8d..a44fb8a1 100644 --- a/app/workers/photo_expansion_worker.rb +++ b/app/workers/photo_expansion_worker.rb @@ -22,27 +22,32 @@ def perform(photograph_id) private def extract_metadata - metadata = @photo.metadata + Benchmark.measure "Extracting metadata" do + metadata = @photo.metadata - # It's possible that Sidekiq could hit this before Postgres catches up - raise MetadataRecordMissing if metadata.nil? + # It's possible that Sidekiq could hit this before Postgres catches up + raise MetadataRecordMissing if metadata.nil? - # Extract the metadata first to allow us to work off that data - metadata.extract_from_photograph - metadata.save! + # Extract the metadata first to allow us to work off that data + metadata.extract_from_photograph + metadata.save! + end end def generate_standard_image - # Create a standard image for generating the smaller sizes - standard_image = @photo.image.thumb("3000x3000>") + Benchmark.measure "Generating standard image" do + # Create a standard image for generating the smaller sizes + standard_image = @photo.image.thumb("3000x3000>") - # Rotate the image if the metadata says so - if @photo.metadata.rotate? - standard_image = standard_image.process(:rotate, @photo.metadata.rotate_by) - end + # Rotate the image if the metadata says so + if @photo.metadata.rotate? + standard_image = standard_image.process(:rotate, @photo.metadata.rotate_by) + end - # Set the standard image and save - @photo.standard_image = standard_image + # Set the standard image and save + @photo.standard_image = standard_image + @photo.save! + end end def generate_images @@ -52,26 +57,28 @@ def generate_images # Generate other sizes based on dimensions case when @photo.landscape? - generate_image(:standard_image, :homepage_image, "2000x", "-quality 80") - generate_image(:standard_image, :large_image, "1500x", "-quality 80") + ImageWorker.perform_async(@photo.id, :standard_image, :homepage_image, "2000x", "-quality 80") + ImageWorker.perform_async(@photo.id, :standard_image, :large_image, "1500x", "-quality 80") when @photo.portrait? - generate_image(:standard_image, :homepage_image, "2000x1400#", "-quality 80") - generate_image(:standard_image, :large_image, "x1000", "-quality 80") + ImageWorker.perform_async(@photo.id, :standard_image, :homepage_image, "2000x1400#", "-quality 80") + ImageWorker.perform_async(@photo.id, :standard_image, :large_image, "x1000", "-quality 80") else - generate_image(:standard_image, :homepage_image, "2000x1400#", "-quality 80") - generate_image(:standard_image, :large_image, "1500x1500>", "-quality 80") + ImageWorker.perform_async(@photo.id, :standard_image, :homepage_image, "2000x1400#", "-quality 80") + ImageWorker.perform_async(@photo.id, :standard_image, :large_image, "1500x1500>", "-quality 80") end - generate_image(:standard_image, :thumbnail_image, "500x500>", "-quality 70") + ImageWorker.perform_async(@photo.id, :standard_image, :thumbnail_image, "500x500>", "-quality 70") end def generate_image(source, target, size, encode_opts) - source = @photo.send(source) - if source.present? - image = source.thumb(size).encode(:jpg, encode_opts) - @photo.send("#{target}=", image) - else - raise "Source doesn't exist yet" + Benchmark.measure "Generating #{target} image from #{source}" do + source = @photo.send(source) + if source.present? + image = source.thumb(size).encode(:jpg, encode_opts) + @photo.send("#{target}=", image) + else + raise "Source doesn't exist yet" + end end end end diff --git a/config/initializers/dragonfly.rb b/config/initializers/dragonfly.rb index 38649b5a..f7b2e3dc 100644 --- a/config/initializers/dragonfly.rb +++ b/config/initializers/dragonfly.rb @@ -1,7 +1,8 @@ require 'dragonfly' require 'rack/cache' +require Rails.root.join("lib/extensions/caching_s3_data_store") -datastore = Dragonfly::DataStorage::S3DataStore.new( +datastore = Dragonfly::DataStorage::CachingS3DataStore.new( :region => ENV['S3_REGION'], :bucket_name => ENV['S3_BUCKET'], :access_key_id => ENV['S3_KEY'], diff --git a/lib/extensions/caching_s3_data_store.rb b/lib/extensions/caching_s3_data_store.rb new file mode 100644 index 00000000..0b6ef80f --- /dev/null +++ b/lib/extensions/caching_s3_data_store.rb @@ -0,0 +1,21 @@ +module Dragonfly + module DataStorage + class CachingS3DataStore < S3DataStore + def retrieve(uid) + cache.fetch(cache_key_for(uid), expires_in: 5.minutes) do + super(uid) + end + end + + private + + def cache + @cache ||= Dalli::Client.new + end + + def cache_key_for(uid) + "dragonfly-#{uid}" + end + end + end +end diff --git a/spec/workers/photo_expansion_worker_spec.rb b/spec/workers/photo_expansion_worker_spec.rb index 92677352..0b71a1b7 100644 --- a/spec/workers/photo_expansion_worker_spec.rb +++ b/spec/workers/photo_expansion_worker_spec.rb @@ -51,6 +51,23 @@ image.should_not_receive(:process) end end + + it "sets the standard image" do + photograph.should_receive(:standard_image=) + end + end + + describe "#generate_images" do + before { subject.stub(:generate_image) { true } } + after { subject.send(:generate_images) } + + it "generates the standard image" do + subject.should_receive(:generate_standard_image) + end + + it "generates 3 smaller images" do + + end end end end