From 3c43821a22995cf26bfe495f2d3555c972886669 Mon Sep 17 00:00:00 2001 From: Charlotte Van Petegem Date: Wed, 9 Oct 2024 17:50:40 +0200 Subject: [PATCH] Precache actual files instead of storing lengths (#644) Co-authored-by: Charlotte Van Petegem --- README.md | 3 +- app/controllers/tracks_controller.rb | 23 ++---- app/jobs/calculate_content_length_job.rb | 7 -- app/jobs/convert_transcode_job.rb | 7 -- app/jobs/create_transcoded_item_job.rb | 22 ++++++ ...anscoded_items_for_codec_conversion_job.rb | 14 ++++ app/jobs/recalculate_content_lengths_job.rb | 13 ---- app/jobs/transcode_cache_clean_job.rb | 7 -- app/models/audio_file.rb | 32 ++------ app/models/codec_conversion.rb | 24 ++++-- app/models/content_length.rb | 17 ----- app/models/transcoded_item.rb | 43 ++++++----- config/application.rb | 3 +- config/brakeman.ignore | 75 ++++++++++++++++++- config/environments/development.rb | 2 +- config/environments/production.rb | 2 +- config/environments/test.rb | 2 +- config/initializers/good_job.rb | 13 ---- .../20241004203216_delete_content_lengths.rb | 15 ++++ ..._remove_last_used_from_transcoded_items.rb | 22 ++++++ db/schema.rb | 18 +---- lib/tasks/ffmpeg.rake | 36 --------- test/controllers/tracks_controller_test.rb | 39 +++++----- test/factories/transcoded_items.rb | 6 +- .../jobs/calculate_content_length_job_test.rb | 7 -- test/jobs/convert_transcode_job_test.rb | 7 -- test/jobs/create_transcoded_item_job_test.rb | 45 +++++++++++ ...ed_items_for_codec_conversion_job_test.rb} | 19 ++--- test/jobs/transcode_cache_clean_job_test.rb | 13 ---- test/models/audio_file_test.rb | 21 ++---- test/models/codec_conversion_test.rb | 22 ++++++ test/models/transcoded_item_test.rb | 3 +- 32 files changed, 310 insertions(+), 272 deletions(-) delete mode 100644 app/jobs/calculate_content_length_job.rb delete mode 100644 app/jobs/convert_transcode_job.rb create mode 100644 app/jobs/create_transcoded_item_job.rb create mode 100644 app/jobs/queue_transcoded_items_for_codec_conversion_job.rb delete mode 100644 app/jobs/recalculate_content_lengths_job.rb delete mode 100644 app/jobs/transcode_cache_clean_job.rb delete mode 100644 app/models/content_length.rb delete mode 100644 config/initializers/good_job.rb create mode 100644 db/migrate/20241004203216_delete_content_lengths.rb create mode 100644 db/migrate/20241004230124_remove_last_used_from_transcoded_items.rb delete mode 100644 lib/tasks/ffmpeg.rake delete mode 100644 test/jobs/calculate_content_length_job_test.rb delete mode 100644 test/jobs/convert_transcode_job_test.rb create mode 100644 test/jobs/create_transcoded_item_job_test.rb rename test/jobs/{recalculate_content_lengths_job_test.rb => queue_transcoded_items_for_codec_conversion_job_test.rb} (68%) delete mode 100644 test/jobs/transcode_cache_clean_job_test.rb diff --git a/README.md b/README.md index 51a9db38..7c0f00ea 100644 --- a/README.md +++ b/README.md @@ -62,8 +62,7 @@ you want. * FFMPEG_LOG_LOCATION * RAILS_STORAGE_PATH - * FFMPEG_VERSION_LOCATION - * RAILS_TRANSCODE_CACHE + * RAILS_TRANSCODES_PATH * BOOTSNAP_CACHE_DIR * PIDFILE * RAILS_LOG_TO_STDOUT diff --git a/app/controllers/tracks_controller.rb b/app/controllers/tracks_controller.rb index 945ee087..81014019 100644 --- a/app/controllers/tracks_controller.rb +++ b/app/controllers/tracks_controller.rb @@ -57,22 +57,15 @@ def audio raise ActiveRecord::RecordNotFound.new('codec_conversion does not exist', 'codec_conversion') if conversion.nil? && params[:codec_conversion_id].present? if conversion.present? - item = TranscodedItem.find_by(audio_file:, codec_conversion: conversion) - if item.present? && File.exist?(item.path) - item.update(last_used: Time.current) - audio_with_file(item.path, item.codec_conversion.resulting_codec.mimetype) - else - if item.present? - # Maybe the file was lost, maybe the transcode just hadn't finished - # yet. Anyway, doing the transcode again doesn't really hurt. - ConvertTranscodeJob.perform_later(item) - else - TranscodedItem.create(audio_file:, codec_conversion: conversion) - end - # AudioFile will only do the conversion if the `ContentLength` doesn't exist yet. - content_length = audio_file.calc_audio_length(conversion) - audio_with_stream(audio_file.convert(conversion), conversion.resulting_codec.mimetype, content_length.length) + transcoded_item = TranscodedItem.find_by(audio_file:, codec_conversion: conversion) + unless transcoded_item.present? && File.exist?(transcoded_item.path) + transcoded_item.destroy! if transcoded_item.present? # The file was lost. This shouldn't happen, so delete the item + + # This does the conversion inline + CreateTranscodedItemJob.perform_now(audio_file, conversion) + transcoded_item = TranscodedItem.find_by(audio_file:, codec_conversion: conversion) end + audio_with_file(transcoded_item.path, transcoded_item.mimetype) else audio_with_file(audio_file.full_path, audio_file.codec.mimetype) end diff --git a/app/jobs/calculate_content_length_job.rb b/app/jobs/calculate_content_length_job.rb deleted file mode 100644 index 0fc4a0ad..00000000 --- a/app/jobs/calculate_content_length_job.rb +++ /dev/null @@ -1,7 +0,0 @@ -class CalculateContentLengthJob < ApplicationJob - queue_as :within_5_minutes - - def perform(audio_file, codec_conversion) - audio_file.calc_audio_length(codec_conversion) - end -end diff --git a/app/jobs/convert_transcode_job.rb b/app/jobs/convert_transcode_job.rb deleted file mode 100644 index f1e34655..00000000 --- a/app/jobs/convert_transcode_job.rb +++ /dev/null @@ -1,7 +0,0 @@ -class ConvertTranscodeJob < ApplicationJob - queue_as :within_30_seconds - - def perform(transcode) - transcode.do_conversion - end -end diff --git a/app/jobs/create_transcoded_item_job.rb b/app/jobs/create_transcoded_item_job.rb new file mode 100644 index 00000000..ce7821d2 --- /dev/null +++ b/app/jobs/create_transcoded_item_job.rb @@ -0,0 +1,22 @@ +class CreateTranscodedItemJob < ApplicationJob + queue_as :within_30_seconds + + def perform(audio_file, codec_conversion) + # Check that this transcoded_item was not created while this job was in the queue + return if TranscodedItem.find_by(audio_file:, codec_conversion:).present? + + uuid = TranscodedItem.uuid_for(codec_conversion) + path = TranscodedItem.path_for(codec_conversion, uuid) + FileUtils.mkdir_p Pathname.new(path).parent + audio_file.convert(codec_conversion, path) + + TranscodedItem.transaction do + # Check that the transcoded item was not created while we were executing + if TranscodedItem.find_by(audio_file:, codec_conversion:).present? + FileUtils.rm_f path + else + TranscodedItem.create!(audio_file:, codec_conversion:, uuid:) + end + end + end +end diff --git a/app/jobs/queue_transcoded_items_for_codec_conversion_job.rb b/app/jobs/queue_transcoded_items_for_codec_conversion_job.rb new file mode 100644 index 00000000..3f59b1f9 --- /dev/null +++ b/app/jobs/queue_transcoded_items_for_codec_conversion_job.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class QueueTranscodedItemsForCodecConversionJob < ApplicationJob + queue_as :within_30_seconds + + def perform(codec_conversion) + AudioFile.find_in_batches do |batch| + jobs = batch.filter_map do |af| + CreateTranscodedItemJob.new(af, codec_conversion) if Rails.configuration.queue_create_transcoded_item_if.call(af) + end + ActiveJob.perform_all_later(jobs) + end + end +end diff --git a/app/jobs/recalculate_content_lengths_job.rb b/app/jobs/recalculate_content_lengths_job.rb deleted file mode 100644 index caea72e4..00000000 --- a/app/jobs/recalculate_content_lengths_job.rb +++ /dev/null @@ -1,13 +0,0 @@ -class RecalculateContentLengthsJob < ApplicationJob - queue_as :within_5_minutes - - def perform - AudioFile.find_each do |af| - next unless Rails.application.config.recalculate_content_length_if.call af - - CodecConversion.find_each do |cc| - CalculateContentLengthJob.set(queue: :whenever).perform_later(af, cc) - end - end - end -end diff --git a/app/jobs/transcode_cache_clean_job.rb b/app/jobs/transcode_cache_clean_job.rb deleted file mode 100644 index 65441993..00000000 --- a/app/jobs/transcode_cache_clean_job.rb +++ /dev/null @@ -1,7 +0,0 @@ -class TranscodeCacheCleanJob < ApplicationJob - queue_as :within_30_minutes - - def perform - TranscodedItem.where.not(last_used: (Rails.configuration.transcode_cache_expiry.call..)).destroy_all - end -end diff --git a/app/models/audio_file.rb b/app/models/audio_file.rb index fccb1f24..56bacd47 100644 --- a/app/models/audio_file.rb +++ b/app/models/audio_file.rb @@ -18,7 +18,6 @@ class AudioFile < ApplicationRecord belongs_to :location belongs_to :codec has_one :track, dependent: :nullify - has_many :content_lengths, dependent: :destroy has_many :transcoded_items, dependent: :destroy validates :filename, presence: true, uniqueness: { scope: :location } @@ -27,7 +26,7 @@ class AudioFile < ApplicationRecord validates :sample_rate, presence: true validates :bit_depth, presence: true - after_save :queue_content_length_calculations + after_create :queue_create_transcoded_items def check_self return true if File.exist?(full_path) @@ -36,44 +35,27 @@ def check_self false end - def convert(codec_conversion) + def convert(codec_conversion, out_file_name) parameters = codec_conversion.ffmpeg_params.split - stdin, stdout, = Open3.popen2( + Open3.popen2( 'ffmpeg', '-i', full_path, '-f', codec_conversion.resulting_codec.extension, *parameters, '-map_metadata', '-1', - '-map', 'a', '-', + '-map', 'a', + out_file_name, err: [Rails.configuration.ffmpeg_log_location, 'a'] ) - stdin.close - stdout - end - - def calc_audio_length(codec_conversion) - existing = ContentLength.find_by(audio_file: self, codec_conversion:) - return existing if existing.present? - - stdout = convert(codec_conversion) - length = 0 - while (bytes = stdout.read(16.kilobytes)) - length += bytes.length - end - - ContentLength.find_or_create_by(audio_file: self, codec_conversion:) do |cl| - cl.length = length - end end def full_path File.join(location.path, filename) end - def queue_content_length_calculations - ContentLength.where(audio_file: self).destroy_all + def queue_create_transcoded_items CodecConversion.find_each do |cc| - CalculateContentLengthJob.perform_later(self, cc) + CreateTranscodedItemJob.perform_later(self, cc) end end end diff --git a/app/models/codec_conversion.rb b/app/models/codec_conversion.rb index b63968fe..132bdc79 100644 --- a/app/models/codec_conversion.rb +++ b/app/models/codec_conversion.rb @@ -10,20 +10,28 @@ class CodecConversion < ApplicationRecord belongs_to :resulting_codec, class_name: 'Codec' - has_many :content_lengths, dependent: :destroy - has_many :transcoded_items, dependent: :destroy + + has_many :transcoded_items, dependent: :delete_all validates :name, presence: true, uniqueness: true validates :ffmpeg_params, presence: true scope :by_codec, ->(codec) { where(resulting_codec: codec) } - after_save :queue_content_length_calculations + before_destroy :delete_transcoded_item_files + after_save :reset_transcoded_items + + delegate :mimetype, to: :resulting_codec + + def reset_transcoded_items + transcoded_items.delete_all + delete_transcoded_item_files + QueueTranscodedItemsForCodecConversionJob.perform_later(self) + end + + private - def queue_content_length_calculations - ContentLength.where(codec_conversion: self).destroy_all - AudioFile.find_each do |af| - CalculateContentLengthJob.perform_later(af, self) - end + def delete_transcoded_item_files + FileUtils.rm_rf TranscodedItem.codec_conversion_base_path(self) end end diff --git a/app/models/content_length.rb b/app/models/content_length.rb deleted file mode 100644 index 23e5f8b5..00000000 --- a/app/models/content_length.rb +++ /dev/null @@ -1,17 +0,0 @@ -# == Schema Information -# -# Table name: content_lengths -# -# id :bigint not null, primary key -# length :integer not null -# audio_file_id :bigint not null -# codec_conversion_id :bigint not null -# - -class ContentLength < ApplicationRecord - belongs_to :audio_file - belongs_to :codec_conversion - - validates :audio_file, uniqueness: { scope: :codec_conversion } - validates :length, presence: true -end diff --git a/app/models/transcoded_item.rb b/app/models/transcoded_item.rb index 81fd037c..68bc0a98 100644 --- a/app/models/transcoded_item.rb +++ b/app/models/transcoded_item.rb @@ -3,44 +3,49 @@ # Table name: transcoded_items # # id :bigint not null, primary key -# last_used :datetime not null -# path :string not null +# uuid :string not null # created_at :datetime not null # updated_at :datetime not null # audio_file_id :bigint not null # codec_conversion_id :bigint not null # class TranscodedItem < ApplicationRecord - BASE_PATH = Rails.configuration.transcode_cache_path + BASE_PATH = Rails.configuration.transcode_storage_path belongs_to :audio_file belongs_to :codec_conversion - validates :path, presence: true, uniqueness: true + validates :audio_file_id, uniqueness: { scope: :codec_conversion_id } + validates :uuid, presence: true, uniqueness: { scope: :codec_conversion_id } - after_create -> { ConvertTranscodeJob.perform_later(self) } - after_destroy :delete_file + # Be careful when adding destroy callbacks! For performance reasons + # CodecConversion#destroy/CodecConversion#update ignore these (and take care + # of deleting the relevant files themselves). + after_commit :delete_file, on: :destroy - def initialize(params) - super - self.path = random_path + delegate :mimetype, to: :codec_conversion + + def path + self.class.path_for(codec_conversion, uuid) end - def do_conversion - tmppath = random_path - FileUtils.mkdir_p Pathname.new(tmppath).parent - File.open(tmppath, 'w') { |f| IO.copy_stream(audio_file.convert(codec_conversion), f) } - FileUtils.mkdir_p Pathname.new(path).parent - FileUtils.mv(tmppath, path) + def self.uuid_for(codec_conversion) + loop do + uuid = SecureRandom.uuid + return uuid unless TranscodedItem.exists?(codec_conversion:, uuid:) + end end - private + def self.codec_conversion_base_path(codec_conversion) + File.join(BASE_PATH, codec_conversion.id.to_s) + end - def random_path - uuid = SecureRandom.uuid - File.join(BASE_PATH, uuid[0..1], uuid[2..3], uuid) + def self.path_for(codec_conversion, uuid) + File.join(codec_conversion_base_path(codec_conversion), uuid[0..1], uuid[2..3], uuid) end + private + def delete_file FileUtils.rm_f path end diff --git a/config/application.rb b/config/application.rb index f0f6efea..3ac11386 100644 --- a/config/application.rb +++ b/config/application.rb @@ -40,7 +40,6 @@ class Application < Rails::Application config.active_job.queue_adapter = :good_job config.active_job.default_queue_name = :within_5_minutes - config.transcode_cache_expiry = -> { 1.day.ago } - config.recalculate_content_length_if = ->(af) { af.length > 299 || af.track.created_at.after?(1.month.ago) } + config.queue_create_transcoded_item_if = ->(af) { af.length > 299 || af.track.created_at.after?(1.month.ago) } end end diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 159363b7..52204900 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -1,5 +1,28 @@ { "ignored_warnings": [ + { + "warning_type": "File Access", + "warning_code": 16, + "fingerprint": "1234149990ced2f918f21be30d850427bfdd49bae163b4bc966e370a1a1df78f", + "check_name": "FileAccess", + "message": "Model attribute used in file name", + "file": "app/models/codec_conversion.rb", + "line": 35, + "link": "https://brakemanscanner.org/docs/warning_types/file_access/", + "code": "FileUtils.rm_rf(TranscodedItem.codec_conversion_base_path(self))", + "render_path": null, + "location": { + "type": "method", + "class": "CodecConversion", + "method": "delete_transcoded_item_files" + }, + "user_input": "TranscodedItem.codec_conversion_base_path(self)", + "confidence": "Medium", + "cwe_id": [ + 22 + ], + "note": "Attribute is never set by user" + }, { "warning_type": "File Access", "warning_code": 16, @@ -7,7 +30,7 @@ "check_name": "SendFile", "message": "Model attribute used in file name", "file": "app/controllers/tracks_controller.rb", - "line": 85, + "line": 78, "link": "https://brakemanscanner.org/docs/warning_types/file_access/", "code": "send_file(Track.find(params[:id]).audio_file.full_path)", "render_path": null, @@ -22,8 +45,54 @@ 22 ], "note": "The attribute is not configurable by users, but rather set by the scan job" + }, + { + "warning_type": "File Access", + "warning_code": 16, + "fingerprint": "59e792a2f517205c25eb262cdac551caaed445b8bda3f581f575edd599afdd17", + "check_name": "FileAccess", + "message": "Model attribute used in file name", + "file": "app/jobs/create_transcoded_item_job.rb", + "line": 10, + "link": "https://brakemanscanner.org/docs/warning_types/file_access/", + "code": "FileUtils.mkdir_p(Pathname.new(TranscodedItem.path_for(codec_conversion, TranscodedItem.uuid_for(codec_conversion))).parent)", + "render_path": null, + "location": { + "type": "method", + "class": "CreateTranscodedItemJob", + "method": "perform" + }, + "user_input": "TranscodedItem.path_for(codec_conversion, TranscodedItem.uuid_for(codec_conversion))", + "confidence": "Weak", + "cwe_id": [ + 22 + ], + "note": "This is never set by a user" + }, + { + "warning_type": "File Access", + "warning_code": 16, + "fingerprint": "8feeb482e779274a005699dc3b1686e6d497d928be7c6508c58fcac32ccdab6f", + "check_name": "FileAccess", + "message": "Model attribute used in file name", + "file": "app/jobs/create_transcoded_item_job.rb", + "line": 16, + "link": "https://brakemanscanner.org/docs/warning_types/file_access/", + "code": "FileUtils.rm_f(TranscodedItem.path_for(codec_conversion, TranscodedItem.uuid_for(codec_conversion)))", + "render_path": null, + "location": { + "type": "method", + "class": "CreateTranscodedItemJob", + "method": "perform" + }, + "user_input": "TranscodedItem.path_for(codec_conversion, TranscodedItem.uuid_for(codec_conversion))", + "confidence": "Medium", + "cwe_id": [ + 22 + ], + "note": "This is never set by a user" } ], - "updated": "2024-08-10 11:14:43 +0200", - "brakeman_version": "6.1.2" + "updated": "2024-10-06 17:38:18 +0200", + "brakeman_version": "6.2.1" } diff --git a/config/environments/development.rb b/config/environments/development.rb index 7a9fe9ee..5af6e347 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -59,5 +59,5 @@ config.token_hash_rounds = 10 config.ffmpeg_log_location = Rails.root.join('log/ffmpeg.log').to_s - config.transcode_cache_path = Rails.root.join('storage/transcode_cache').to_s + config.transcode_storage_path = Rails.root.join('storage/transcodes').to_s end diff --git a/config/environments/production.rb b/config/environments/production.rb index b1f35801..ba749e50 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -77,5 +77,5 @@ config.token_hash_rounds = 10 config.ffmpeg_log_location = ENV.fetch('FFMPEG_LOG_LOCATION') { Rails.root.join('log/ffmpeg.log').to_s } - config.transcode_cache_path = ENV.fetch('RAILS_TRANSCODE_CACHE') { Rails.root.join('storage/transcode_cache').to_s } + config.transcode_storage_path = ENV.fetch('RAILS_TRANSCODES_PATH') { Rails.root.join('storage/transcodes').to_s } end diff --git a/config/environments/test.rb b/config/environments/test.rb index 22645337..d2a07bf9 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -64,5 +64,5 @@ config.token_hash_rounds = 1 config.ffmpeg_log_location = Rails.root.join('tmp/log/ffmpeg.log').to_s - config.transcode_cache_path = Rails.root.join('tmp/storage/transcode_cache').to_s + config.transcode_storage_path = Rails.root.join('tmp/storage/transcodes').to_s end diff --git a/config/initializers/good_job.rb b/config/initializers/good_job.rb deleted file mode 100644 index aca5ef25..00000000 --- a/config/initializers/good_job.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -Rails.application.configure do - # GoodJob recurring jobs - # See https://github.com/bensheldon/good_job#cron-style-repeatingrecurring-jobs - config.good_job.enable_cron = true - config.good_job.cron = { - clean_transcode_cache: { - cron: 'every 4 hours', - class: 'TranscodeCacheCleanJob' - } - } -end diff --git a/db/migrate/20241004203216_delete_content_lengths.rb b/db/migrate/20241004203216_delete_content_lengths.rb new file mode 100644 index 00000000..184139e6 --- /dev/null +++ b/db/migrate/20241004203216_delete_content_lengths.rb @@ -0,0 +1,15 @@ +class DeleteContentLengths < ActiveRecord::Migration[7.2] + def up + drop_table :content_lengths + end + + def down + create_table :content_lengths do |t| + t.references :audio_file, foreign_key: true, null: false + t.references :codec_conversion, foreign_key: true, null: false + t.integer :length, null: false + + t.index [:audio_file_id, :codec_conversion_id], unique: true + end + end +end diff --git a/db/migrate/20241004230124_remove_last_used_from_transcoded_items.rb b/db/migrate/20241004230124_remove_last_used_from_transcoded_items.rb new file mode 100644 index 00000000..e4330405 --- /dev/null +++ b/db/migrate/20241004230124_remove_last_used_from_transcoded_items.rb @@ -0,0 +1,22 @@ +class RemoveLastUsedFromTranscodedItems < ActiveRecord::Migration[7.2] + def up + # Delete all old transcoded items, since these were transcoded with artifacts + TranscodedItem.delete_all + + change_table :transcoded_items do |t| + t.remove :last_used, :path + t.string :uuid, null: false + t.index [:codec_conversion_id, :uuid], unique: true + end + + CodecConversion.find_each(&:reset_transcoded_items) + end + + def down + change_table :transcoded_items do |t| + t.remove :uuid + t.timestamp :last_used, null: false, default: -> { 'CURRENT_TIMESTAMP' } + t.string :path, null: false, unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 39dfc831..0b5d9a64 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2024_09_21_132403) do +ActiveRecord::Schema[7.2].define(version: 2024_10_04_230124) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -126,15 +126,6 @@ t.index ["extension"], name: "index_codecs_on_extension", unique: true end - create_table "content_lengths", force: :cascade do |t| - t.bigint "audio_file_id", null: false - t.bigint "codec_conversion_id", null: false - t.integer "length", null: false - t.index ["audio_file_id", "codec_conversion_id"], name: "index_content_lengths_on_audio_file_id_and_codec_conversion_id", unique: true - t.index ["audio_file_id"], name: "index_content_lengths_on_audio_file_id" - t.index ["codec_conversion_id"], name: "index_content_lengths_on_codec_conversion_id" - end - create_table "cover_filenames", force: :cascade do |t| t.string "filename", null: false t.index ["filename"], name: "index_cover_filenames_on_filename", unique: true @@ -336,16 +327,15 @@ end create_table "transcoded_items", force: :cascade do |t| - t.string "path", null: false - t.datetime "last_used", precision: nil, default: -> { "CURRENT_TIMESTAMP" }, null: false t.bigint "audio_file_id", null: false t.bigint "codec_conversion_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "uuid", null: false t.index ["audio_file_id", "codec_conversion_id"], name: "index_transcoded_items_on_audio_file_id_and_codec_conversion_id", unique: true t.index ["audio_file_id"], name: "index_transcoded_items_on_audio_file_id" + t.index ["codec_conversion_id", "uuid"], name: "index_transcoded_items_on_codec_conversion_id_and_uuid", unique: true t.index ["codec_conversion_id"], name: "index_transcoded_items_on_codec_conversion_id" - t.index ["path"], name: "index_transcoded_items_on_path", unique: true end create_table "users", force: :cascade do |t| @@ -367,8 +357,6 @@ add_foreign_key "audio_files", "locations" add_foreign_key "auth_tokens", "users" add_foreign_key "codec_conversions", "codecs", column: "resulting_codec_id" - add_foreign_key "content_lengths", "audio_files" - add_foreign_key "content_lengths", "codec_conversions" add_foreign_key "genres_tracks", "genres" add_foreign_key "genres_tracks", "tracks" add_foreign_key "images", "image_types" diff --git a/lib/tasks/ffmpeg.rake b/lib/tasks/ffmpeg.rake deleted file mode 100644 index 0c5d8f72..00000000 --- a/lib/tasks/ffmpeg.rake +++ /dev/null @@ -1,36 +0,0 @@ -require 'open3' - -# ffmpeg versions can be either x.x.x or x.x -FFMPEG_REGEX = /ffmpeg version (\d+\.\d+\.?\d*)/ - -namespace :ffmpeg do - task check_version: :environment do - path = ENV.fetch('FFMPEG_VERSION_LOCATION') { Rails.root.join('log/ffmpeg_version.txt').to_s } - prev_version = begin - File.read(path) - rescue StandardError - '0.0.0' - end - - stdin, stdout, = Open3.popen2('ffmpeg -version') - stdin.close - new_version = stdout.gets.match(FFMPEG_REGEX)[1] - - exit if prev_version == new_version - - ContentLength.destroy_all - RecalculateContentLengthsJob.perform_later - - File.write(path, new_version) - end - - task init: :environment do - path = ENV.fetch('FFMPEG_VERSION_LOCATION') { Rails.root.join('log/ffmpeg_version.txt') } - abort("The ffmpeg version file already exists: #{path}") if File.exist?(path) - - stdin, stdout, = Open3.popen2('ffmpeg -version') - stdin.close - version = stdout.gets.match(FFMPEG_REGEX)[1] - File.write(path, version) - end -end diff --git a/test/controllers/tracks_controller_test.rb b/test/controllers/tracks_controller_test.rb index 159c9b6a..53493d34 100644 --- a/test/controllers/tracks_controller_test.rb +++ b/test/controllers/tracks_controller_test.rb @@ -272,6 +272,12 @@ class TracksControllerTest < ActionDispatch::IntegrationTest class TracksControllerAudioTest < ActionDispatch::IntegrationTest setup do sign_in_as(create(:user)) + AudioFile.alias_method :old_convert, :convert + AudioFile.define_method :convert, ->(_codec_conversion, out_file_name) { FileUtils.cp full_path, out_file_name } + end + + teardown do + AudioFile.alias_method :convert, :old_convert end test 'should return not_found if codec_conversion does not exit ' do @@ -285,51 +291,46 @@ class TracksControllerAudioTest < ActionDispatch::IntegrationTest end test 'should create transcoded_item if codec_conversion is present' do - io = StringIO.new Rails.root.join('test/files/base.flac').read - AudioFile.any_instance.stubs(:convert).returns(io) mp3 = Codec.create(mimetype: 'audio/mpeg', extension: 'mp3') codec_conversion = CodecConversion.create(name: 'MP3 (V0)', ffmpeg_params: '-acodec mp3 -q:a 0', resulting_codec: mp3) location = Location.create(path: Rails.root.join('test/files')) flac = Codec.create(mimetype: 'audio/flac', extension: 'flac') audio_file = create(:audio_file, location:, filename: '/base.flac', codec: flac) perform_enqueued_jobs - length = audio_file.content_lengths.find_by(codec_conversion:).length + TranscodedItem.destroy_all track = create(:track, audio_file:) assert_difference('TranscodedItem.count', 1) do get audio_track_url(track, codec_conversion_id: codec_conversion.id) end - assert_equal length.to_s, response.headers['content-length'] + transcoded_item = TranscodedItem.find_by(audio_file:, codec_conversion:) + + assert_equal File.open(transcoded_item.path).size.to_s, response.headers['content-length'] assert_match 'audio/mpeg', response.headers['content-type'] assert_response :success end - test 'should not create transcoded_item if it already exists but should queue if file is gone' do - io = StringIO.new Rails.root.join('test/files/base.flac').read - AudioFile.any_instance.stubs(:convert).returns(io) + test 'should create transcoded_item if it already exists but file is gone' do codec_conversion = create(:codec_conversion) location = Location.create(path: Rails.root.join('test/files')) flac = Codec.create(mimetype: 'audio/flac', extension: 'flac') audio_file = create(:audio_file, location:, filename: '/base.flac', codec: flac) track = create(:track, audio_file:) - transcode = TranscodedItem.create(audio_file:, codec_conversion:) perform_enqueued_jobs + transcode = TranscodedItem.find_by(audio_file:, codec_conversion:) File.delete(transcode.path) - assert_enqueued_jobs 1 do - assert_difference('TranscodedItem.count', 0) do - get audio_track_url(track, codec_conversion_id: codec_conversion.id) - end + assert_difference('TranscodedItem.count', 0) do + get audio_track_url(track, codec_conversion_id: codec_conversion.id) end + assert_nil TranscodedItem.find_by(id: transcode.id) assert_response :success end test 'should not create transcoded_item if it already exists' do - io = StringIO.new Rails.root.join('test/files/base.flac').read - AudioFile.any_instance.stubs(:convert).returns(io) codec_conversion = create(:codec_conversion) location = Location.create(path: Rails.root.join('test/files')) flac = Codec.create(mimetype: 'audio/flac', extension: 'flac') @@ -345,15 +346,14 @@ class TracksControllerAudioTest < ActionDispatch::IntegrationTest end test 'should return correct headers for range request' do - io = StringIO.new Rails.root.join('test/files/base.flac').read - AudioFile.any_instance.stubs(:convert).returns(io) mp3 = Codec.create(mimetype: 'audio/mpeg', extension: 'mp3') codec_conversion = CodecConversion.create(name: 'MP3 (V0)', ffmpeg_params: '-acodec mp3 -q:a 0', resulting_codec: mp3) location = Location.create(path: Rails.root.join('test/files')) flac = Codec.create(mimetype: 'audio/flac', extension: 'flac') audio_file = create(:audio_file, location:, filename: '/base.flac', codec: flac) perform_enqueued_jobs - length = audio_file.content_lengths.find_by(codec_conversion:).length + transcoded_item = TranscodedItem.find_by(audio_file:, codec_conversion:) + length = File.open(transcoded_item.path).size track = create(:track, audio_file:) get(audio_track_url(track, codec_conversion_id: codec_conversion.id), headers: { range: 'bytes=150-500' }) @@ -366,15 +366,14 @@ class TracksControllerAudioTest < ActionDispatch::IntegrationTest end test 'accepts range request without end' do - io = StringIO.new Rails.root.join('test/files/base.flac').read - AudioFile.any_instance.stubs(:convert).returns(io) mp3 = Codec.create(mimetype: 'audio/mpeg', extension: 'mp3') codec_conversion = CodecConversion.create(name: 'MP3 (V0)', ffmpeg_params: '-acodec mp3 -q:a 0', resulting_codec: mp3) location = Location.create(path: Rails.root.join('test/files')) flac = Codec.create(mimetype: 'audio/flac', extension: 'flac') audio_file = create(:audio_file, location:, filename: '/base.flac', codec: flac) perform_enqueued_jobs - length = audio_file.content_lengths.find_by(codec_conversion:).length + transcoded_item = TranscodedItem.find_by(audio_file:, codec_conversion:) + length = File.open(transcoded_item.path).size track = create(:track, audio_file:) get(audio_track_url(track, codec_conversion_id: codec_conversion.id), headers: { range: 'bytes=150-' }) diff --git a/test/factories/transcoded_items.rb b/test/factories/transcoded_items.rb index b5018590..11a83178 100644 --- a/test/factories/transcoded_items.rb +++ b/test/factories/transcoded_items.rb @@ -3,8 +3,7 @@ # Table name: transcoded_items # # id :bigint not null, primary key -# last_used :datetime not null -# path :string not null +# uuid :string not null # created_at :datetime not null # updated_at :datetime not null # audio_file_id :bigint not null @@ -12,8 +11,7 @@ # FactoryBot.define do factory :transcoded_item do - path { Rails.root.join('tmp/storage', SecureRandom.uuid) } - last_used { '2020-07-20 09:33:51' } + uuid { SecureRandom.uuid } audio_file codec_conversion end diff --git a/test/jobs/calculate_content_length_job_test.rb b/test/jobs/calculate_content_length_job_test.rb deleted file mode 100644 index f66edc81..00000000 --- a/test/jobs/calculate_content_length_job_test.rb +++ /dev/null @@ -1,7 +0,0 @@ -require 'test_helper' - -class CalculateContentLengthJobTest < ActiveJob::TestCase - # test "the truth" do - # assert true - # end -end diff --git a/test/jobs/convert_transcode_job_test.rb b/test/jobs/convert_transcode_job_test.rb deleted file mode 100644 index 737398ac..00000000 --- a/test/jobs/convert_transcode_job_test.rb +++ /dev/null @@ -1,7 +0,0 @@ -require 'test_helper' - -class ConvertTranscodeJobTest < ActiveJob::TestCase - # test "the truth" do - # assert true - # end -end diff --git a/test/jobs/create_transcoded_item_job_test.rb b/test/jobs/create_transcoded_item_job_test.rb new file mode 100644 index 00000000..c439f80f --- /dev/null +++ b/test/jobs/create_transcoded_item_job_test.rb @@ -0,0 +1,45 @@ +require 'test_helper' + +class CreateTranscodedItemJobTest < ActiveJob::TestCase + setup do + location = Location.create(path: Rails.root.join('test/files')) + flac = Codec.create(mimetype: 'audio/flac', extension: 'flac') + @audio_file = create(:audio_file, location:, filename: '/base.flac', codec: flac) + @codec_conversion = create(:codec_conversion) + + AudioFile.alias_method :old_convert, :convert + AudioFile.define_method :convert, ->(_codec_conversion, out_file_name) { FileUtils.cp full_path, out_file_name } + end + + teardown do + AudioFile.alias_method :convert, :old_convert + end + + test 'should create a new TranscodedItem with existing file' do + assert_difference('TranscodedItem.count', 1) do + CreateTranscodedItemJob.perform_now(@audio_file, @codec_conversion) + end + assert_path_exists TranscodedItem.find_by(audio_file: @audio_file, codec_conversion: @codec_conversion).path + end + + test 'should abort when TranscodedItem was created while job was waiting' do + CreateTranscodedItemJob.perform_later(@audio_file, @codec_conversion) + + AudioFile.stubs(:convert).never + TranscodedItem.create!(audio_file: @audio_file, codec_conversion: @codec_conversion, uuid: '0000-0000-0000') + + perform_enqueued_jobs + end + + test 'should abort when TranscodedItem was created while converting' do + audio_file = @audio_file + codec_conversion = @codec_conversion + AudioFile.define_method :convert, lambda { |_codec_conversion, _out_file_name| + TranscodedItem.create!(audio_file:, codec_conversion:, uuid: '0000-0000-0000') + } + + FileUtils.stubs(:rm_f).once + + CreateTranscodedItemJob.perform_now(@audio_file, @codec_conversion) + end +end diff --git a/test/jobs/recalculate_content_lengths_job_test.rb b/test/jobs/queue_transcoded_items_for_codec_conversion_job_test.rb similarity index 68% rename from test/jobs/recalculate_content_lengths_job_test.rb rename to test/jobs/queue_transcoded_items_for_codec_conversion_job_test.rb index 8e1a4659..ed5a49c5 100644 --- a/test/jobs/recalculate_content_lengths_job_test.rb +++ b/test/jobs/queue_transcoded_items_for_codec_conversion_job_test.rb @@ -1,15 +1,10 @@ require 'test_helper' -class RecalculateContentLengthsJobTest < ActiveJob::TestCase - def setup - # ContentLengths are automatically created when we create an AudioFile and CodecConversion. Manually creating one would result in an error due to uniqueness contraints. - io = StringIO.new Rails.root.join('test/files/base.flac').read - AudioFile.any_instance.stubs(:convert).returns(io) - @audio_file = create(:audio_file) +class QueueTranscodedItemsForCodecConversionJobTest < ActiveJob::TestCase + setup do + @audio_file = create(:audio_file, location: create(:location), filename: '/base.flac', codec: create(:codec)) + create(:track, audio_file: @audio_file) @codec_conversion = create(:codec_conversion) - @track = create(:track, audio_file: @audio_file) - - ContentLength.destroy_all end test 'should not enqueue job if audio is shorter than config and track is older than config' do @@ -20,7 +15,7 @@ def setup # rubocop:enable Rails/SkipsModelValidations assert_no_enqueued_jobs do - RecalculateContentLengthsJob.perform_now + QueueTranscodedItemsForCodecConversionJob.perform_now(@codec_conversion) end end @@ -32,7 +27,7 @@ def setup # rubocop:enable Rails/SkipsModelValidations assert_enqueued_jobs 1 do - RecalculateContentLengthsJob.perform_now + QueueTranscodedItemsForCodecConversionJob.perform_now(@codec_conversion) end end @@ -44,7 +39,7 @@ def setup # rubocop:enable Rails/SkipsModelValidations assert_enqueued_jobs 1 do - RecalculateContentLengthsJob.perform_now + QueueTranscodedItemsForCodecConversionJob.perform_now(@codec_conversion) end end end diff --git a/test/jobs/transcode_cache_clean_job_test.rb b/test/jobs/transcode_cache_clean_job_test.rb deleted file mode 100644 index 8adcce11..00000000 --- a/test/jobs/transcode_cache_clean_job_test.rb +++ /dev/null @@ -1,13 +0,0 @@ -require 'test_helper' - -class TranscodeCacheCleanJobTest < ActiveJob::TestCase - test 'Transcode cache clean should remove old transcodes' do - io = StringIO.new Rails.root.join('test/files/base.flac').read - AudioFile.any_instance.stubs(:convert).returns(io) - 5.times { create(:transcoded_item, last_used: 2.days.ago) } - 3.times { create(:transcoded_item, last_used: 1.hour.ago) } - assert_difference('TranscodedItem.count', -5) do - TranscodeCacheCleanJob.perform_now - end - end -end diff --git a/test/models/audio_file_test.rb b/test/models/audio_file_test.rb index 7f3026e8..53ff74e3 100644 --- a/test/models/audio_file_test.rb +++ b/test/models/audio_file_test.rb @@ -22,23 +22,14 @@ def setup end test 'convert should not crash' do - stdin = StringIO.new - stdout = StringIO.new Rails.root.join('test/files/base.flac').read - Open3.stubs(:popen2).returns([stdin, stdout, 0]) - ret_out = @audio_file.convert(create(:codec_conversion)) + codec_conversion = create(:codec_conversion) - assert_predicate stdin, :closed? - assert_equal stdout, ret_out - end + path = TranscodedItem.path_for(codec_conversion, SecureRandom.uuid) - test 'convert should not write to stdin' do - stdin = StringIO.new - stdout = StringIO.new Rails.root.join('test/files/base.flac').read - Open3.stubs(:popen2).returns([stdin, stdout, 0]) - ret_out = @audio_file.convert(create(:codec_conversion)) + Open3.stubs(:popen2).once.with do |*args, **kwargs| + args.include?('ffmpeg') && args.include?(@audio_file.full_path) && args[-1] == path && kwargs.key?(:err) + end - assert_predicate stdin, :closed? - assert_equal '', stdin.string - assert_equal stdout, ret_out + @audio_file.convert(codec_conversion, path) end end diff --git a/test/models/codec_conversion_test.rb b/test/models/codec_conversion_test.rb index cf5f18ed..938d3264 100644 --- a/test/models/codec_conversion_test.rb +++ b/test/models/codec_conversion_test.rb @@ -11,4 +11,26 @@ require 'test_helper' class CodecConversionTest < ActiveSupport::TestCase + setup do + @codec_conversion = create(:codec_conversion) + create_list(:transcoded_item, 5, codec_conversion: @codec_conversion) + end + + test 'destroying a codec conversion deletes all files and transcoded_items' do + FileUtils.stubs(:rm_rf).once.with(File.join(TranscodedItem::BASE_PATH, @codec_conversion.id.to_s)) + + assert_difference('TranscodedItem.count', -5) do + @codec_conversion.destroy + end + end + + test 'changing a codec conversion deletes all files and transcoded_items and schedules a new job' do + FileUtils.stubs(:rm_rf).once.with(File.join(TranscodedItem::BASE_PATH, @codec_conversion.id.to_s)) + + assert_enqueued_jobs 1 do + assert_difference('TranscodedItem.count', -5) do + @codec_conversion.update!(ffmpeg_params: '-acodec mp3') + end + end + end end diff --git a/test/models/transcoded_item_test.rb b/test/models/transcoded_item_test.rb index 663aabc7..01339f79 100644 --- a/test/models/transcoded_item_test.rb +++ b/test/models/transcoded_item_test.rb @@ -3,8 +3,7 @@ # Table name: transcoded_items # # id :bigint not null, primary key -# last_used :datetime not null -# path :string not null +# uuid :string not null # created_at :datetime not null # updated_at :datetime not null # audio_file_id :bigint not null