From f38ea44deb23033e7aac5d8dbce9c2a2502f05f3 Mon Sep 17 00:00:00 2001 From: Ben Pennell Date: Mon, 18 Sep 2023 10:58:33 -0400 Subject: [PATCH 1/3] Adding configurable support for using graphicsmagick in place of imagemagick (implemented by Max Kadel) --- .circleci/config.yml | 2 +- lib/hydra/derivatives.rb | 1 + lib/hydra/derivatives/processors/image.rb | 30 ++- .../derivatives/services/image_service.rb | 48 ++++ spec/processors/image_spec.rb | 234 +++++++++++------- 5 files changed, 218 insertions(+), 97 deletions(-) create mode 100644 lib/hydra/derivatives/services/image_service.rb diff --git a/.circleci/config.yml b/.circleci/config.yml index 1b48504..1ea9d9b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -28,7 +28,7 @@ jobs: name: Install dependencies command: | sudo apt-get update - sudo apt-get install ghostscript libpng-dev imagemagick ffmpeg libreoffice dcraw + sudo apt-get install ghostscript libpng-dev imagemagick graphicsmagick ffmpeg libreoffice dcraw - restore_cache: name: Restore Kakadu Cache keys: diff --git a/lib/hydra/derivatives.rb b/lib/hydra/derivatives.rb index 7509fed..86dff10 100644 --- a/lib/hydra/derivatives.rb +++ b/lib/hydra/derivatives.rb @@ -38,6 +38,7 @@ module Derivatives autoload :PersistExternalFileOutputFileService autoload :TempfileService autoload :MimeTypeService + autoload :ImageService end # Raised if the timout elapses diff --git a/lib/hydra/derivatives/processors/image.rb b/lib/hydra/derivatives/processors/image.rb index d319e08..24998f5 100644 --- a/lib/hydra/derivatives/processors/image.rb +++ b/lib/hydra/derivatives/processors/image.rb @@ -20,16 +20,38 @@ def process_with_timeout # When resizing images, it is necessary to flatten any layers, otherwise the background # may be completely black. This happens especially with PDFs. See #110 def create_resized_image - create_image do |xfrm| + if Hydra::Derivatives::ImageService.processor == :graphicsmagick + create_resized_image_with_graphicsmagick + else + create_resized_image_with_imagemagick + end + end + + def create_resized_image_with_graphicsmagick + Hydra::Derivatives::Logger.debug('[ImageProcessor] Using GraphicsMagick image resize method') + create_image do |temp_file| if size - xfrm.combine_options do |i| - i.flatten - i.resize(size) + # remove layers and resize using convert instead of mogrify + MiniMagick::Tool::Convert.new do |cmd| + cmd << temp_file.path # input + cmd.flatten + cmd.resize(size) + cmd << temp_file.path # output end end end end + def create_resized_image_with_imagemagick + Hydra::Derivatives::Logger.debug('[ImageProcessor] Using ImageMagick image resize method') + create_image do |temp_file| + if size + temp_file.flatten + temp_file.resize(size) + end + end + end + def create_image xfrm = selected_layers(load_image_transformer) yield(xfrm) if block_given? diff --git a/lib/hydra/derivatives/services/image_service.rb b/lib/hydra/derivatives/services/image_service.rb new file mode 100644 index 0000000..d7421e6 --- /dev/null +++ b/lib/hydra/derivatives/services/image_service.rb @@ -0,0 +1,48 @@ +module Hydra::Derivatives + module ImageService + def self.default_processor + :imagemagick + end + + def self.processor + case ENV['IMAGE_PROCESSOR'] + when 'imagemagick' + Hydra::Derivatives::Logger.debug('[ImageProcessor] Using ImageMagick as image processor') + :imagemagick + when 'graphicsmagick' + Hydra::Derivatives::Logger.debug('[ImageProcessor] Using GraphicsMagick as image processor') + :graphicsmagick + else + Hydra::Derivatives::Logger.debug("[ImageProcessor] The environment variable IMAGE_PROCESSOR should be set to either 'imagemagick' or 'graphicsmagick'. It is currently set to: #{ENV['IMAGE_PROCESSOR']}. Defaulting to using #{default_processor}") + default_processor + end + end + + def self.cli + case processor + when :graphicsmagick + :graphicsmagick + when :imagemagick + :imagemagick + end + end + + def self.external_convert_command + case processor + when :graphicsmagick + 'gm convert' + when :imagemagick + 'convert' + end + end + + def self.external_identify_command + case processor + when :graphicsmagick + 'gm identify' + when :imagemagick + 'identify' + end + end + end +end \ No newline at end of file diff --git a/spec/processors/image_spec.rb b/spec/processors/image_spec.rb index c06bcf2..9f71cc2 100644 --- a/spec/processors/image_spec.rb +++ b/spec/processors/image_spec.rb @@ -4,134 +4,184 @@ describe Hydra::Derivatives::Processors::Image do subject { described_class.new(file_name, directives) } - let(:file_name) { "file_name" } + let(:file_name) { 'file_name' } - context "when arguments are passed as a hash" do - before { allow(subject).to receive(:load_image_transformer).and_return(mock_image) } + context 'using ImageMagick as the image processor' do + before do + allow(MiniMagick).to receive(:cli).and_return(:imagemagick) + end - context "with a multi-page pdf source file" do - let(:first_page) { instance_double("MockPage") } - let(:second_page) { instance_double("MockPage") } - let(:mock_image) { instance_double("MockImageOfPdf", layers: [first_page, second_page]) } + around do |example| + cached_image_processor = ENV['IMAGE_PROCESSOR'] + ENV['IMAGE_PROCESSOR'] = 'imagemagick' + example.run + ENV['IMAGE_PROCESSOR'] = cached_image_processor + end + context 'when arguments are passed as a hash' do before do - allow(mock_image).to receive(:type).and_return("PDF") - allow(first_page).to receive(:combine_options) { |&block| block.call(first_page) } - allow(second_page).to receive(:combine_options) { |&block| block.call(second_page) } - allow(mock_image).to receive(:combine_options) { |&block| block.call(mock_image) } + allow(subject).to receive(:load_image_transformer).and_return(mock_image) end - context "when default" do - let(:directives) { { label: :thumb, size: "200x300>", format: 'png', quality: 75 } } - - it "uses the first page" do - expect(first_page).to receive(:flatten) - expect(second_page).not_to receive(:flatten) - expect(first_page).to receive(:resize).with("200x300>") - expect(second_page).not_to receive(:resize) - expect(first_page).to receive(:format).with("png") - expect(second_page).not_to receive(:format) - expect(first_page).to receive(:quality).with("75") - expect(second_page).not_to receive(:quality) - expect(subject).to receive(:write_image).with(first_page) - subject.process + context 'with a multi-page pdf source file' do + let(:first_page) { instance_double('MockPage') } + let(:second_page) { instance_double('MockPage') } + let(:mock_image) { instance_double('MockImageOfPdf', layers: [first_page, second_page]) } + + before do + allow(mock_image).to receive(:type).and_return('PDF') + allow(first_page).to receive(:combine_options) { |&block| block.call(first_page) } + allow(second_page).to receive(:combine_options) { |&block| block.call(second_page) } + allow(mock_image).to receive(:combine_options) { |&block| block.call(mock_image) } + end + + context 'when default' do + let(:directives) { { label: :thumb, size: '200x300>', format: 'png', quality: 75 } } + + it 'uses the first page' do + expect(first_page).to receive(:flatten) + expect(second_page).not_to receive(:flatten) + expect(first_page).to receive(:resize).with('200x300>') + expect(second_page).not_to receive(:resize) + expect(first_page).to receive(:format).with('png') + expect(second_page).not_to receive(:format) + expect(first_page).to receive(:quality).with('75') + expect(second_page).not_to receive(:quality) + expect(subject).to receive(:write_image).with(first_page) + subject.process + end + end + + context 'when specifying a layer' do + let(:directives) { { label: :thumb, size: '200x300>', format: 'png', quality: 75, layer: 1 } } + + it 'uses the second page' do + expect(second_page).to receive(:flatten) + expect(first_page).not_to receive(:flatten) + expect(second_page).to receive(:resize).with('200x300>') + expect(first_page).not_to receive(:resize) + expect(second_page).to receive(:format).with('png') + expect(first_page).not_to receive(:format) + expect(second_page).to receive(:quality).with('75') + expect(first_page).not_to receive(:quality) + expect(subject).to receive(:write_image).with(second_page) + subject.process + end end end - context "when specifying a layer" do - let(:directives) { { label: :thumb, size: "200x300>", format: 'png', quality: 75, layer: 1 } } - - it "uses the second page" do - expect(second_page).to receive(:flatten) - expect(first_page).not_to receive(:flatten) - expect(second_page).to receive(:resize).with("200x300>") - expect(first_page).not_to receive(:resize) - expect(second_page).to receive(:format).with("png") - expect(first_page).not_to receive(:format) - expect(second_page).to receive(:quality).with("75") - expect(first_page).not_to receive(:quality) - expect(subject).to receive(:write_image).with(second_page) - subject.process + context 'with an image source file' do + before { allow(mock_image).to receive(:type).and_return('JPEG') } + + context 'when default' do + let(:mock_image) { instance_double('MockImage') } + let(:directives) { { label: :thumb, size: '200x300>', format: 'png', quality: 75 } } + + before do + allow(mock_image).to receive(:combine_options) { |&block| block.call(mock_image) } + end + + it 'uses the image file' do + expect(mock_image).not_to receive(:layers) + expect(mock_image).to receive(:flatten) + expect(mock_image).to receive(:resize).with('200x300>') + expect(mock_image).to receive(:format).with('png') + expect(mock_image).to receive(:quality).with('75') + expect(subject).to receive(:write_image).with(mock_image) + subject.process + end + end + + context 'when specifying a layer' do + let(:first_layer) { instance_double('MockPage') } + let(:second_layer) { instance_double('MockPage') } + let(:mock_image) { instance_double('MockImage', layers: [first_layer, second_layer]) } + let(:directives) { { label: :thumb, size: '200x300>', format: 'png', quality: 75, layer: 1 } } + + before do + allow(first_layer).to receive(:combine_options) { |&block| block.call(first_layer) } + allow(second_layer).to receive(:combine_options) { |&block| block.call(second_layer) } + allow(mock_image).to receive(:combine_options) { |&block| block.call(mock_image) } + end + + it 'uses the layer' do + expect(second_layer).to receive(:flatten) + expect(first_layer).not_to receive(:flatten) + expect(second_layer).to receive(:resize).with('200x300>') + expect(first_layer).not_to receive(:resize) + expect(second_layer).to receive(:format).with('png') + expect(first_layer).not_to receive(:format) + expect(second_layer).to receive(:quality).with('75') + expect(first_layer).not_to receive(:quality) + expect(subject).to receive(:write_image).with(second_layer) + subject.process + end end end end - context "with an image source file" do - before { allow(mock_image).to receive(:type).and_return("JPEG") } - - context "when default" do - let(:mock_image) { instance_double("MockImage") } - let(:directives) { { label: :thumb, size: "200x300>", format: 'png', quality: 75 } } + describe '#process' do + let(:directives) { { size: '100x100>', format: 'png' } } + context 'when a timeout is set' do before do - allow(mock_image).to receive(:combine_options) { |&block| block.call(mock_image) } + subject.timeout = 0.1 + allow(subject).to receive(:create_resized_image) { sleep 0.2 } + end + it 'raises a timeout exception' do + expect { subject.process }.to raise_error Hydra::Derivatives::TimeoutError end + end - it "uses the image file" do - expect(mock_image).not_to receive(:layers) - expect(mock_image).to receive(:flatten) - expect(mock_image).to receive(:resize).with("200x300>") - expect(mock_image).to receive(:format).with("png") - expect(mock_image).to receive(:quality).with("75") - expect(subject).to receive(:write_image).with(mock_image) + context 'when not set' do + before { subject.timeout = nil } + it 'processes without a timeout' do + expect(subject).to receive(:process_with_timeout).never + expect(subject).to receive(:create_resized_image).once subject.process end end - context "when specifying a layer" do - let(:first_layer) { instance_double("MockPage") } - let(:second_layer) { instance_double("MockPage") } - let(:mock_image) { instance_double("MockImage", layers: [first_layer, second_layer]) } - let(:directives) { { label: :thumb, size: "200x300>", format: 'png', quality: 75, layer: 1 } } + context 'when running the complete command', requires_imagemagick: true do + let(:file_name) { File.join(fixture_path, "test.tif") } - before do - allow(first_layer).to receive(:combine_options) { |&block| block.call(first_layer) } - allow(second_layer).to receive(:combine_options) { |&block| block.call(second_layer) } - allow(mock_image).to receive(:combine_options) { |&block| block.call(mock_image) } + it 'calls the ImageMagick version of create_resized_image' do + expect(subject).to receive(:create_resized_image_with_imagemagick) + subject.process end - it "uses the layer" do - expect(second_layer).to receive(:flatten) - expect(first_layer).not_to receive(:flatten) - expect(second_layer).to receive(:resize).with("200x300>") - expect(first_layer).not_to receive(:resize) - expect(second_layer).to receive(:format).with("png") - expect(first_layer).not_to receive(:format) - expect(second_layer).to receive(:quality).with("75") - expect(first_layer).not_to receive(:quality) - expect(subject).to receive(:write_image).with(second_layer) + it 'converts the image' do + expect(Hydra::Derivatives::PersistBasicContainedOutputFileService).to receive(:call).with(kind_of(StringIO), directives) subject.process end end end end - describe "#process" do - let(:directives) { { size: "100x100>", format: "png" } } + context 'using GraphicsMagick' do + let(:directives) { { size: '100x100>', format: 'png' } } + let(:file_name) { File.join(fixture_path, "test.tif") } - context "when a timeout is set" do - before do - subject.timeout = 0.1 - allow(subject).to receive(:create_resized_image) { sleep 0.2 } - end - it "raises a timeout exception" do - expect { subject.process }.to raise_error Hydra::Derivatives::TimeoutError - end + before do + allow(MiniMagick).to receive(:cli).and_return(:graphicsmagick) end - context "when not set" do - before { subject.timeout = nil } - it "processes without a timeout" do - expect(subject).to receive(:process_with_timeout).never - expect(subject).to receive(:create_resized_image).once - subject.process - end + around do |example| + cached_image_processor = ENV['IMAGE_PROCESSOR'] + ENV['IMAGE_PROCESSOR'] = 'graphicsmagick' + example.run + ENV['IMAGE_PROCESSOR'] = cached_image_processor + end + + it 'calls the GraphicsMagick version of create_resized_image' do + expect(subject).to receive(:create_resized_image_with_graphicsmagick) + subject.process end - context "when running the complete command", requires_imagemagick: true do - let(:file_name) { File.join(fixture_path, "test.tif") } + context 'when running the complete command' do + let(:file_name) { File.join(fixture_path, 'test.tif') } - it "converts the image" do + it 'converts the image' do expect(Hydra::Derivatives::PersistBasicContainedOutputFileService).to receive(:call).with(kind_of(StringIO), directives) subject.process end From 8874ae1917767c130c64bf3e6956c796eafa0c88 Mon Sep 17 00:00:00 2001 From: Ben Pennell Date: Mon, 18 Sep 2023 11:07:12 -0400 Subject: [PATCH 2/3] Rubocop --- lib/hydra/derivatives/services/image_service.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/hydra/derivatives/services/image_service.rb b/lib/hydra/derivatives/services/image_service.rb index d7421e6..15bcded 100644 --- a/lib/hydra/derivatives/services/image_service.rb +++ b/lib/hydra/derivatives/services/image_service.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true module Hydra::Derivatives module ImageService def self.default_processor @@ -45,4 +46,4 @@ def self.external_identify_command end end end -end \ No newline at end of file +end From 52f0b144d202c41ca81367b5f3524f779c21a4ba Mon Sep 17 00:00:00 2001 From: Ben Pennell Date: Mon, 18 Sep 2023 11:22:47 -0400 Subject: [PATCH 3/3] Remove unused methods, add default processor test --- .../derivatives/services/image_service.rb | 27 ------------ spec/processors/image_spec.rb | 42 +++++++++++++++++++ 2 files changed, 42 insertions(+), 27 deletions(-) diff --git a/lib/hydra/derivatives/services/image_service.rb b/lib/hydra/derivatives/services/image_service.rb index 15bcded..6cc7757 100644 --- a/lib/hydra/derivatives/services/image_service.rb +++ b/lib/hydra/derivatives/services/image_service.rb @@ -18,32 +18,5 @@ def self.processor default_processor end end - - def self.cli - case processor - when :graphicsmagick - :graphicsmagick - when :imagemagick - :imagemagick - end - end - - def self.external_convert_command - case processor - when :graphicsmagick - 'gm convert' - when :imagemagick - 'convert' - end - end - - def self.external_identify_command - case processor - when :graphicsmagick - 'gm identify' - when :imagemagick - 'identify' - end - end end end diff --git a/spec/processors/image_spec.rb b/spec/processors/image_spec.rb index 9f71cc2..73728d9 100644 --- a/spec/processors/image_spec.rb +++ b/spec/processors/image_spec.rb @@ -187,4 +187,46 @@ end end end + + context 'using default processor (imagemagick)' do + before do + allow(MiniMagick).to receive(:cli).and_return(:imagemagick) + end + + around do |example| + cached_image_processor = ENV['IMAGE_PROCESSOR'] + ENV['IMAGE_PROCESSOR'] = nil + example.run + ENV['IMAGE_PROCESSOR'] = cached_image_processor + end + + context 'when arguments are passed as a hash' do + before do + allow(subject).to receive(:load_image_transformer).and_return(mock_image) + end + + context 'with an image source file' do + before { allow(mock_image).to receive(:type).and_return('JPEG') } + + context 'when default' do + let(:mock_image) { instance_double('MockImage') } + let(:directives) { { label: :thumb, size: '200x300>', format: 'png', quality: 75 } } + + before do + allow(mock_image).to receive(:combine_options) { |&block| block.call(mock_image) } + end + + it 'uses the image file' do + expect(mock_image).not_to receive(:layers) + expect(mock_image).to receive(:flatten) + expect(mock_image).to receive(:resize).with('200x300>') + expect(mock_image).to receive(:format).with('png') + expect(mock_image).to receive(:quality).with('75') + expect(subject).to receive(:write_image).with(mock_image) + subject.process + end + end + end + end + end end