diff --git a/CHANGES.md b/CHANGES.md index ad094cea..ab4e81bc 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -14,6 +14,10 @@ * Kithe derivatves store created at in shrine metadata for derivative, as UTC iso8601 string https://github.com/sciencehistory/kithe/pull/190 +* `Attacher.define_derivative` block can now take an optional `add_metadata` keyword arg with a hash that can be mutated to set metadata on derivative. https://github.com/sciencehistory/kithe/pull/193 https://github.com/sciencehistory/kithe/blob/master/guides/derivatives.md#specifying-metadata-for-defined-derivatives + +* Built-in derivative transformers will supply some creation tooling metadata if given the `add_metadata` argument. If you are using these, you might have to change the signatures on your `define_derivative` calls to take advantage of new functionality. https://github.com/sciencehistory/kithe/pull/193 https://github.com/sciencehistory/kithe/blob/master/guides/derivatives.md#kithe-provided-derivative-creation-tools + ### Fixed * Use up-to-date vips CLI thumbnail recommendations, without deprecated options Should be no change to thumbnails generated, just differnet form of args. https://github.com/sciencehistory/kithe/pull/192 https://github.com/sciencehistory/kithe/pull/194 diff --git a/app/derivative_transformers/kithe/ffmpeg_extract_jpg.rb b/app/derivative_transformers/kithe/ffmpeg_extract_jpg.rb index f9d92623..a64c3da3 100644 --- a/app/derivative_transformers/kithe/ffmpeg_extract_jpg.rb +++ b/app/derivative_transformers/kithe/ffmpeg_extract_jpg.rb @@ -8,6 +8,10 @@ module Kithe # @example tempfile = FfmpegExtractJpg.new.call(url) # @example tempfile = FfmpegExtractJpg.new(start_seconds: 60).call(shrine_uploaded_file) # @example tempfile = FfmpegExtractJpg.new(start_seconds: 10, width_pixels: 420).call(shrine_uploaded_file) + # + # @example you can also provide a Hash which will be mutated with metadata relevant to + # the derivative created, ffmpeg version and args: + # @example tempfile = FfmpegExtractJpg.new(start_seconds: 10, width_pixels: 420).call(shrine_uploaded_file, add_metadata: my_hash) class FfmpegExtractJpg class_attribute :ffmpeg_command, default: "ffmpeg" attr_reader :start_seconds, :frame_sample_size, :width_pixels @@ -43,19 +47,19 @@ def initialize(start_seconds: 0, frame_sample_size: false, width_pixels: nil) # Most efficient is if we have a remote URL to give ffmpeg, one way or another! # # @returns [Tempfile] jpg extracted from movie - def call(input_arg) + def call(input_arg, add_metadata:nil) if input_arg.kind_of?(Shrine::UploadedFile) if input_arg.respond_to?(:url) && input_arg.url&.start_with?(/https?\:/) - _call(input_arg.url) + _call(input_arg.url, add_metadata: add_metadata) else Shrine.with_file(input_arg) do |local_file| - _call(local_file.path) + _call(local_file.path, add_metadata: add_metadata) end end elsif input_arg.respond_to?(:path) - _call(input_arg.path) + _call(input_arg.path, add_metadata: add_metadata) else - _call(input_arg.to_s) + _call(input_arg.to_s, add_metadata: add_metadata) end end @@ -67,14 +71,26 @@ def call(input_arg) # @param ffmpeg_source_arg [String] filepath or URL. ffmpeg can take urls, which # can be very efficient. # + # @param add_metadata [Hash], optional, if provided will be filled out with metadata + # relevant to the derivative created -- ffmpeg version and args. + # # @returns Tempfile pointing to a thumbnail - def _call(ffmpeg_source_arg) + def _call(ffmpeg_source_arg, add_metadata: nil) tempfile = Tempfile.new(['temp_deriv', ".jpg"]) ffmpeg_args = produce_ffmpeg_args(input_arg: ffmpeg_source_arg, output_path: tempfile.path) TTY::Command.new(printer: :null).run(*ffmpeg_args) + if add_metadata + add_metadata[:ffmpeg_command] = ffmpeg_args.join(" ") + + `#{ffmpeg_command} -version` =~ /ffmpeg version (\d+\.\d+.*) Copyright/ + if $1 + add_metadata[:ffmpeg_version] = $1 + end + end + return tempfile rescue StandardError => e tempfile.unlink if tempfile diff --git a/app/derivative_transformers/kithe/ffmpeg_transformer.rb b/app/derivative_transformers/kithe/ffmpeg_transformer.rb index 6f170042..b198f6c1 100644 --- a/app/derivative_transformers/kithe/ffmpeg_transformer.rb +++ b/app/derivative_transformers/kithe/ffmpeg_transformer.rb @@ -39,13 +39,23 @@ def transform_arguments end # Will raise TTY::Command::ExitError if the ffmpeg returns non-null. - def call(original_file) + def call(original_file, add_metadata: nil) tempfile = Tempfile.new(['temp_deriv', ".#{@output_suffix}"]) # -y tells ffmpeg to overwrite the abovementioned tempfile (still empty) # with the output of ffmpeg. ffmpeg_args = [ffmpeg_command, "-y", "-i", original_file.path] ffmpeg_args += transform_arguments + [tempfile.path] - TTY::Command.new(printer: :null).run(*ffmpeg_args) + out, err = TTY::Command.new(printer: :null).run(*ffmpeg_args) + + if add_metadata + add_metadata[:ffmpeg_command] = ffmpeg_args.join(" ") + + `#{ffmpeg_command} -version` =~ /ffmpeg version (\d+\.\d+.*) Copyright/ + if $1 + add_metadata[:ffmpeg_version] = $1 + end + end + return tempfile end end diff --git a/app/derivative_transformers/kithe/vips_cli_image_to_jpeg.rb b/app/derivative_transformers/kithe/vips_cli_image_to_jpeg.rb index 2d4d101b..afddb638 100644 --- a/app/derivative_transformers/kithe/vips_cli_image_to_jpeg.rb +++ b/app/derivative_transformers/kithe/vips_cli_image_to_jpeg.rb @@ -19,8 +19,8 @@ module Kithe # built for use with kithe derivatives transformations, eg: # # class Asset < KitheAsset - # define_derivative(thumb) do |original_file| - # Kithe::VipsCliImageToJpeg.new(max_width: 100, thumbnail_mode: true).call(original_file) + # Attacher.define_derivative(:thumb) do |original_file, add_metadata:| + # Kithe::VipsCliImageToJpeg.new(max_width: 100, thumbnail_mode: true).call(original_file, add_metadata: add_metadata) # end # end # @@ -50,7 +50,7 @@ def initialize(max_width:nil, jpeg_q: 85, thumbnail_mode: false) end # Will raise TTY::Command::ExitError if the external Vips command returns non-null. - def call(original_file) + def call(original_file, add_metadata: nil) tempfile = Tempfile.new(["kithe_vips_cli_image_to_jpeg", ".jpg"]) vips_args = [] @@ -62,7 +62,7 @@ def call(original_file) # really huge one million pixels so it should not come into play, and # we're constraining proportionally by width. # https://github.com/jcupitt/libvips/issues/781 - vips_args.concat [vips_thumbnail_command, original_file.path] + vips_args.concat [vips_thumbnail_command, "--version", original_file.path] vips_args.concat maybe_profile_normalization_args vips_args.concat ["--size", "#{max_width}x65500"] vips_args.concat ["-o", "#{tempfile.path}#{vips_jpg_params}"] @@ -75,7 +75,16 @@ def call(original_file) vips_args.concat ["#{tempfile.path}#{vips_jpg_params}"] end - TTY::Command.new(printer: :null).run(*vips_args) + out, err = TTY::Command.new(printer: :null).run(*vips_args) + + if add_metadata + add_metadata[:vips_command] = vips_args.join(" ") + + out =~ /vips[ \-](\d+\.\d+\.\d+.*$)/ + if $1 + add_metadata[:vips_version] = $1 + end + end return tempfile end diff --git a/app/models/kithe/asset/derivative_definition.rb b/app/models/kithe/asset/derivative_definition.rb index b9bc96aa..66b4e274 100644 --- a/app/models/kithe/asset/derivative_definition.rb +++ b/app/models/kithe/asset/derivative_definition.rb @@ -9,12 +9,32 @@ def initialize(key:, proc:, content_type: nil, default_create: true) @proc = proc end + # @return [Hash] add_metadata hash of metadata to add to derivative on storage def call(original_file:,attacher:) + add_metadata = {} + kwargs = {} + if proc_accepts_keyword?(:attacher) - proc.call(original_file, attacher: attacher) + kwargs[:attacher] = attacher + end + + if proc_accepts_keyword?(:add_metadata) + kwargs[:add_metadata] = add_metadata + end + + return_val = if kwargs.present? + proc.call(original_file, **kwargs) else proc.call(original_file) end + + # Save in context to later write to actual stored derivative metadata + if add_metadata.present? + attacher.context[:add_metadata] ||= {} + attacher.context[:add_metadata][key] = add_metadata + end + + return_val end # Do content-type restrictions defined for this definition match a given asset? diff --git a/guides/derivatives.md b/guides/derivatives.md index 895825e3..3866fefc 100644 --- a/guides/derivatives.md +++ b/guides/derivatives.md @@ -103,6 +103,19 @@ class Asset < Kithe::Asset end ``` +### Specifying metadata for defined derivatives + +You can also optionally provide an `add_metadata` keyword arg to your block. It will be given a mutable hash that you can mutate to specify JSON-compatible metadata that will be stored with the derivative. + +```ruby +class MyAssetUploader < Kithe::AssetUploader + Attacher.define_derivative(:thumb_small) do |original_file, add_metadata:| + add_metadata[:my_key] = "my value" + + anything_that_returns_io_like_object(original_file) + end +end +``` ### Kithe-provided derivative-creation tools @@ -115,8 +128,8 @@ Which can create a resized JPG from an image input, using a shell out to the `vi ```ruby class Asset < Kithe::Asset - define_derivative(:download_small) do |original_file| - Kithe::VipsCliImageToJpeg.new(max_width: 500).call(original_file) + define_derivative(:download_small) do |original_file, add_metadata:| + Kithe::VipsCliImageToJpeg.new(max_width: 500).call(original_file, add_metadata: add_metadata) end end ``` @@ -125,8 +138,8 @@ If you pass `thumbnail_mode: true` when instantiating Kithe::VipsCliImageToJpeg, ```ruby class Asset < Kithe::Asset - define_derivative(:thumb_small, content_type: "image") do |original_file| - Kithe::VipsCliImageToJpeg.new(max_width: 500, thumbnail_mode: true).call(original_file) + define_derivative(:thumb_small, content_type: "image") do |original_file, add_metadata:| + Kithe::VipsCliImageToJpeg.new(max_width: 500, thumbnail_mode: true).call(original_file, add_metadata: add_metadata) end end ``` @@ -138,18 +151,18 @@ Which creates audio files from any audio file original, using a shell out to the ```ruby # Create a stereo 128k mp3 derivative. output_suffix is the only mandatory argument. -define_derivative('mp3', content_type: "audio") do |original_file| - Kithe::FfmpegTransformer.new(bitrate: '128k', output_suffix: 'mp3').call(original_file) +define_derivative('mp3', content_type: "audio") do |original_file, add_metadata:| + Kithe::FfmpegTransformer.new(bitrate: '128k', output_suffix: 'mp3').call(original_file, add_metadata: add_metadata) end # A mono webm file at only 64k: -define_derivative('webm', content_type: "audio") do |original_file| - Kithe::FfmpegTransformer.new(bitrate: '64k', force_mono: true, output_suffix: 'webm').call(original_file) +define_derivative('webm', content_type: "audio") do |original_file, add_metadata:| + Kithe::FfmpegTransformer.new(bitrate: '64k', force_mono: true, output_suffix: 'webm').call(original_file, add_metadata: add_metadata) end # libopus is used by default for webm conversions, but you could specify another codec: -define_derivative('webm', content_type: "audio") do |original_file| - Kithe::FfmpegTransformer.new(output_suffix: 'webm', audio_codec: 'libopencore-amrwb').call(original_file) +define_derivative('webm', content_type: "audio") do |original_file, add_metadata:| + Kithe::FfmpegTransformer.new(output_suffix: 'webm', audio_codec: 'libopencore-amrwb').call(original_file, add_metadata: add_metadata) end ``` @@ -163,7 +176,7 @@ However, to take advantage of this feature, avoiding a download, you'd have to w complex code. Same if you want to produce multiple resolution thumbnails. This isn't yet fully documented, but be aware of the existence of this service. ```ruby -image_tmp_file = Kithe::FfmpegExtractJpg.new(start_seconds: start_seconds).call(original) +image_tmp_file = Kithe::FfmpegExtractJpg.new(start_seconds: start_seconds).call(original, add_metadata: add_metadata) ``` ## Manually triggering creaton of derivatives from definitions diff --git a/lib/shrine/plugins/kithe_derivatives.rb b/lib/shrine/plugins/kithe_derivatives.rb index 9c2c91a1..5c5a5145 100644 --- a/lib/shrine/plugins/kithe_derivatives.rb +++ b/lib/shrine/plugins/kithe_derivatives.rb @@ -32,9 +32,13 @@ def self.load_dependencies(uploader, *) module InstanceMethods - # Override to fix "filename" metadata to be something reasonable, regardless + # 1. Override to fix "filename" metadata to be something reasonable, regardless # of what if anything was the filename of the IO being attached. shrine S3 will - # insist on setting a default content-disposition with this filename. + # insist on setting a default content-disposition with this filename, so we + # can use that. + # + # 2. Set any specified derivative metadata on derivative. Specified add_metadata + # found in context. def extract_metadata(io, derivative:nil, **context) result = super @@ -43,9 +47,13 @@ def extract_metadata(io, derivative:nil, **context) result["filename"] = "#{context[:record].friendlier_id}_#{derivative}.#{extension}" end - # Add timestamp for derivatives please + # If derivative, add timestamp and any specified extra data if derivative result["created_at"] ||= Time.current.utc.iso8601.to_s + + if (metadata = context.dig(:add_metadata, derivative)).present? + result.merge!(metadata.stringify_keys) + end end result diff --git a/spec/derivative_transformers/ffmpeg_extract_jpg_spec.rb b/spec/derivative_transformers/ffmpeg_extract_jpg_spec.rb index 7d61a9d0..52135b41 100644 --- a/spec/derivative_transformers/ffmpeg_extract_jpg_spec.rb +++ b/spec/derivative_transformers/ffmpeg_extract_jpg_spec.rb @@ -20,6 +20,16 @@ expect(result).to be_kind_of(Tempfile) expect(Marcel::MimeType.for(StringIO.new(result.read))).to eq "image/jpeg" end + + it "sets add_metadata" do + add_metadata = {} + + extractor = Kithe::FfmpegExtractJpg.new(frame_sample_size: 300) + extractor.call(video_path, add_metadata: add_metadata) + + expect(add_metadata[:ffmpeg_version]).to match /\d+\.\d+/ + expect(add_metadata[:ffmpeg_command]).to match /ffmpeg -y -i .*\.jpg/ + end end describe "from File" do @@ -32,6 +42,15 @@ expect(result).to be_kind_of(Tempfile) expect(Marcel::MimeType.for(StringIO.new(result.read))).to eq "image/jpeg" end + + it "sets add_metadata" do + add_metadata = {} + + Kithe::FfmpegExtractJpg.new.call(video_file, add_metadata: add_metadata) + + expect(add_metadata[:ffmpeg_version]).to match /\d+\.\d+/ + expect(add_metadata[:ffmpeg_command]).to match /ffmpeg -y -i .*\.jpg/ + end end describe "from Shrine::UploadedFile without good url" do @@ -52,6 +71,15 @@ expect(result).to be_kind_of(Tempfile) expect(Marcel::MimeType.for(StringIO.new(result.read))).to eq "image/jpeg" end + + it "sets add_metadata" do + add_metadata = {} + + Kithe::FfmpegExtractJpg.new.call(uploaded_file_obj, add_metadata: add_metadata) + + expect(add_metadata[:ffmpeg_version]).to match /\d+\.\d+/ + expect(add_metadata[:ffmpeg_command]).to match /ffmpeg -y -i .*\.jpg/ + end end describe "from Shrine::UploadedFile with good url" do @@ -80,6 +108,19 @@ result = Kithe::FfmpegExtractJpg.new.call(uploaded_file_obj) end + + it "sets add_metadata" do + add_metadata = {} + + expect_any_instance_of(TTY::Command).to receive(:run) do |instance, *args| + expect(args[0..3]).to eq ["ffmpeg", "-y", "-i", url] + end + + Kithe::FfmpegExtractJpg.new.call(uploaded_file_obj, add_metadata: add_metadata) + + expect(add_metadata[:ffmpeg_version]).to match /\d+\.\d+/ + expect(add_metadata[:ffmpeg_command]).to match /ffmpeg -y -i .*\.jpg/ + end end @@ -95,5 +136,18 @@ result = Kithe::FfmpegExtractJpg.new.call(url) end + + it "sets add_metadata" do + add_metadata = {} + + expect_any_instance_of(TTY::Command).to receive(:run) do |instance, *args| + expect(args[0..3]).to eq ["ffmpeg", "-y", "-i", url] + end + + Kithe::FfmpegExtractJpg.new.call(url, add_metadata: add_metadata) + + expect(add_metadata[:ffmpeg_version]).to match /\d+\.\d+/ + expect(add_metadata[:ffmpeg_command]).to match /ffmpeg -y -i .*\.jpg/ + end end end diff --git a/spec/derivative_transformers/ffmpeg_transformer_spec.rb b/spec/derivative_transformers/ffmpeg_transformer_spec.rb index c88ca38a..79769b7a 100644 --- a/spec/derivative_transformers/ffmpeg_transformer_spec.rb +++ b/spec/derivative_transformers/ffmpeg_transformer_spec.rb @@ -85,4 +85,16 @@ output.close! end end + + describe "ffmpeg command metadata" do + it "is provided when arg is given" do + add_metadata = {} + + instance = described_class.new(output_suffix: 'mp3', force_mono: true, bitrate: '32k') + output = instance.call(input_file, add_metadata: add_metadata) + + expect(add_metadata[:ffmpeg_version]).to match /\d+\.\d+/ + expect(add_metadata[:ffmpeg_command]).to match /ffmpeg -y -i .*\.mp3/ + end + end end diff --git a/spec/derivative_transformers/vips_cli_image_to_jpeg_spec.rb b/spec/derivative_transformers/vips_cli_image_to_jpeg_spec.rb index 5651b5d3..561fa683 100644 --- a/spec/derivative_transformers/vips_cli_image_to_jpeg_spec.rb +++ b/spec/derivative_transformers/vips_cli_image_to_jpeg_spec.rb @@ -26,6 +26,16 @@ output.close! end + + describe "with add_metadata" do + it "adds metadata" do + add_metadata = {} + + Kithe::VipsCliImageToJpeg.new(thumbnail_mode: true, max_width: width).call(input_file, add_metadata: add_metadata) + expect(add_metadata[:vips_version]).to match /\d+\.\d+\.\d+/ + expect(add_metadata[:vips_command]).to match /vipsthumbnail .*\.jpg\[Q=85,interlace,optimize_coding,keep=none\]/ + end + end end end @@ -53,6 +63,17 @@ output.close! end + + describe "with add_metadata" do + it "adds metadata" do + add_metadata = {} + + Kithe::VipsCliImageToJpeg.new(thumbnail_mode: false, max_width: width).call(input_file, add_metadata: add_metadata) + expect(add_metadata[:vips_version]).to match /\d+\.\d+\.\d+/ + expect(add_metadata[:vips_command]).to match /vipsthumbnail .*\.jpg\[Q=85,interlace,optimize_coding\]/ + end + end + end end end diff --git a/spec/models/kithe/asset/asset_derivatives_spec.rb b/spec/models/kithe/asset/asset_derivatives_spec.rb index 086a8b58..da151f27 100644 --- a/spec/models/kithe/asset/asset_derivatives_spec.rb +++ b/spec/models/kithe/asset/asset_derivatives_spec.rb @@ -171,5 +171,34 @@ end end end + + describe "with specified addiitonal metadata" do + temporary_class("CustomUploader2") do + call_fakeio = method(:fakeio) # weird closure issue + deriv_path = derivative_file_path + Class.new(Kithe::AssetUploader) do + self::Attacher.define_derivative(:fixed_with_metadata) do |io, add_metadata:| + add_metadata[:added_metadata] = "set value" + call_fakeio.(File.binread(deriv_path)) + end + end + end + + temporary_class("CustomAsset2") do + Class.new(Kithe::Asset) do + set_shrine_uploader(CustomUploader2) + end + end + + it "automatically sets up derivative properly" do + asset = CustomAsset2.create!(title: "test", file: File.open(original_file_path)) + # happened in BG job, so have to reload to see it. + asset.reload + + derivative = asset.file_derivatives[:fixed_with_metadata] + expect(derivative).to be_present + expect(derivative.metadata["added_metadata"]).to eq "set value" + end + end end end